Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

heap-buffer-overflow in FeedbackPsRembPacket() constructor #246

Closed
ibc opened this issue Dec 15, 2018 · 4 comments
Closed

heap-buffer-overflow in FeedbackPsRembPacket() constructor #246

ibc opened this issue Dec 15, 2018 · 4 comments
Assignees
@ibc @jmillan
Labels
bug fuzzer
Milestone
v2 updates

Comments

@ibc
Copy link
Member

ibc commented Dec 15, 2018 

=================================================================
==16445==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000002960 at pc 0x000000ac3b89 bp 0x7ffd1c9c7750 sp 0x7ffd1c9c7748
READ of size 1 at 0x602000002960 thread T0
    #0 0xac3b88 in RTC::RTCP::FeedbackPsRembPacket::FeedbackPsRembPacket(RTC::RTCP::Packet::CommonHeader*) /mediasoup/worker/out/../src/RTC/RTCP/FeedbackPsRemb.cpp:53:23
    #1 0xac32ae in RTC::RTCP::FeedbackPsRembPacket::Parse(unsigned char const*, unsigned long) /mediasoup/worker/out/../src/RTC/RTCP/FeedbackPsRemb.cpp:32:53
    #2 0xac129b in RTC::RTCP::FeedbackPsAfbPacket::Parse(unsigned char const*, unsigned long) /mediasoup/worker/out/../src/RTC/RTCP/FeedbackPsAfb.cpp:34:18
    #3 0xa4dddb in RTC::RTCP::FeedbackPacket<RTC::RTCP::FeedbackPs>::Parse(unsigned char const*, unsigned long) /mediasoup/worker/out/../src/RTC/RTCP/Feedback.cpp:171:15
    #4 0xa2746b in RTC::RTCP::Packet::Parse(unsigned char const*, unsigned long) /mediasoup/worker/out/../src/RTC/RTCP/Packet.cpp:127:17
    #5 0xb4a8fe in fuzz(unsigned char const*, unsigned long) /mediasoup/worker/out/../fuzzer/fuzzer.cpp:44:26
    #6 0xb4aaea in LLVMFuzzerTestOneInput /mediasoup/worker/out/../fuzzer/fuzzer.cpp:63:2
    #7 0x45f47a in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:576:15
    #8 0x45eb95 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:485:3
    #9 0x4616ce in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__Fuzzer::vector<std::__Fuzzer::basic_string<char, std::__Fuzzer::char_traits<char>, std::__Fuzzer::allocator<char> >, fuzzer::fuzzer_allocator<std::__Fuzzer::basic_string<char, std::__Fuzzer::char_traits<char>, std::__Fuzzer::allocator<char> > > > const&) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:788:7
    #10 0x461a35 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<std::__Fuzzer::basic_string<char, std::__Fuzzer::char_traits<char>, std::__Fuzzer::allocator<char> >, fuzzer::fuzzer_allocator<std::__Fuzzer::basic_string<char, std::__Fuzzer::char_traits<char>, std::__Fuzzer::allocator<char> > > > const&) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:811:3
    #11 0x4584f3 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:765:6
    #12 0x4801d2 in main /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #13 0x7f4392cc882f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #14 0x434a28 in _start (/mediasoup/worker/out/Release/mediasoup-worker-fuzzer+0x434a28)

0x602000002960 is located 0 bytes to the right of 16-byte region [0x602000002950,0x602000002960)
allocated by thread T0 here:
    #0 0x5951b2 in operator new[](unsigned long) /tmp/final/llvm.src/projects/compiler-rt/lib/asan/asan_new_delete.cc:109:3
    #1 0x45f361 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:563:23
    #2 0x45eb95 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:485:3
    #3 0x4616ce in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__Fuzzer::vector<std::__Fuzzer::basic_string<char, std::__Fuzzer::char_traits<char>, std::__Fuzzer::allocator<char> >, fuzzer::fuzzer_allocator<std::__Fuzzer::basic_string<char, std::__Fuzzer::char_traits<char>, std::__Fuzzer::allocator<char> > > > const&) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:788:7
    #4 0x461a35 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<std::__Fuzzer::basic_string<char, std::__Fuzzer::char_traits<char>, std::__Fuzzer::allocator<char> >, fuzzer::fuzzer_allocator<std::__Fuzzer::basic_string<char, std::__Fuzzer::char_traits<char>, std::__Fuzzer::allocator<char> > > > const&) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:811:3
    #5 0x4584f3 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:765:6
    #6 0x4801d2 in main /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #7 0x7f4392cc882f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

SUMMARY: AddressSanitizer: heap-buffer-overflow /mediasoup/worker/out/../src/RTC/RTCP/FeedbackPsRemb.cpp:53:23 in RTC::RTCP::FeedbackPsRembPacket::FeedbackPsRembPacket(RTC::RTCP::Packet::CommonHeader*)
Shadow bytes around the buggy address:
  0x0c047fff84d0: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
  0x0c047fff84e0: fa fa fd fd fa fa fd fa fa fa fd fa fa fa fd fd
  0x0c047fff84f0: fa fa fd fd fa fa 00 00 fa fa fd fd fa fa fd fd
  0x0c047fff8500: fa fa fd fd fa fa fd fa fa fa fd fd fa fa fd fd
  0x0c047fff8510: fa fa fd fa fa fa fd fd fa fa 00 00 fa fa fd fd
=>0x0c047fff8520: fa fa fd fd fa fa 00 00 fa fa 00 00[fa]fa fa fa
  0x0c047fff8530: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8540: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8550: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8560: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8570: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==16445==ABORTING
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0x8f,0xce,0x0,0x3,0x12,0x34,0x56,0x78,0x0,0x0,0x0,0x0,0x52,0x45,0x4d,0x42,
\x8f\xce\x00\x03\x124Vx\x00\x00\x00\x00REMB
artifact_prefix='fuzzer/reports/'; Test unit written to fuzzer/reports/crash-458003b01372ea8ae6456f86da40d3b1d32d905d
Base64: j84AAxI0VngAAAAAUkVNQg==

The issue is that the code is reading memory out of the given buffer. However it's hard to figure out how to fix it since the constructor does not receive any len argument, so no idea how to check it.

For you @jmillan.
@ibc ibc added this to the v2 updates milestone Dec 15, 2018
@ibc
Copy link
Member Author

ibc commented Dec 15, 2018 •
edited
Loading

@jmillan may I understand this +1 in the FeedbackPsRembPacket(CommonHeader* commonHeader) constructor?

auto* data = reinterpret_cast<uint8_t*>(commonHeader + 1);

UPDATE: Ok, it seems to make data point to the SSRC of packet sender 4 bytes length of the REMB packet.

Wouldn't it be better like this?:

auto* data = reinterpret_cast<uint8_t*>(commonHeader) + sizeof(CommonHeader);

@ibc
Copy link
Member Author

ibc commented Dec 15, 2018 •
edited
Loading

Also, can be rewrite this:

if (len != sizeof(CommonHeader) + sizeof(FeedbackPacket::Header) + sizeof(Header) + (numSsrcs * sizeof(uint32_t)))

to this?

if (len != sizeof(CommonHeader) + sizeof(FeedbackPacket::Header) + sizeof(FeedbackPacket::Header) + (numSsrcs * sizeof(uint32_t)))

Well, honestly no idea what the initial sizeof(Header) means.

@ibc
Copy link
Member Author

ibc commented Dec 15, 2018

What I see is that FeedbackPsRembPacket::FeedbackPsRembPacket() constructor is calculating the packet len by itself:

size_t len = static_cast<size_t>(ntohs(commonHeader->length) + 1) * 4;

However, it's never tested whether such a len matches the len above in FeedbackPsRembPacket* FeedbackPsRembPacket::Parse(const uint8_t* data, size_t len). Shouldn't both match and shouldn't that be verified?

ibc added a commit that referenced this issue Dec 15, 2018
@ibc
RTCP/FeedbackPsRemb: it seems to fix issue #246 but not 100% sure sue…
c84399c 
… to #243
@ibc ibc self-assigned this Dec 15, 2018
@ibc
Copy link
Member Author

ibc commented Dec 15, 2018

Well, I think I may have fixed it.

ibc added a commit that referenced this issue Dec 15, 2018
@ibc
Related to #246: verify that len in FeedbackPsRembPacket::Parse() mat…
37f20b2 
…ches the total REMB packet len
@ibc ibc closed this as completed Dec 16, 2018
lavarsicious pushed a commit to lavarsicious/mediasoup that referenced this issue Feb 5, 2019
@ibc @lavarsicious
RTCP/FeedbackPsRemb: it seems to fix issue versatica#246 but not 100%…
39c7d5b 
… sure sue to versatica#243
lavarsicious pushed a commit to lavarsicious/mediasoup that referenced this issue Feb 5, 2019
@ibc @lavarsicious
Related to versatica#246: verify that len in FeedbackPsRembPacket::Pa…
623ca0c 
…rse() matches the total REMB packet len
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ibc ibc

@jmillan jmillan

Labels
bug fuzzer
Milestone
v2 updates
Development

No branches or pull requests
2 participants
@ibc @jmillan