New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 851662 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 4
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 2
Type: Bug-Security



Sign in to add a comment

Security: WebRTC: Unchecked Optional Access in Updating timestamp after RED packet

Project Member Reported by natashenka@google.com, Jun 11 2018

Issue description

The attached file causes an unchecked access to an Optional object when processing rtp packets. To reproduce the issue, run:


neteq_rtpplay --g722 9 --pcmu 0 --pcma 8 --isac 103 --isac 104 --avt 110 --opus 111 --avt 112 --avt 113 --cn_nb 105 --avt 126 unknown soob.pcm

on the attached file.

This issue is probably not exploitable, but I'm filing it under the security component just in case.
 
unknown
71 bytes View Download

Comment 1 by palmer@chromium.org, Jun 11 2018

Cc: phoglund@chromium.org mcasas@chromium.org tommi@chromium.org guidou@chromium.org henrika@chromium.org
Labels: OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows

Comment 2 by wfh@chromium.org, Jun 12 2018

Hmm I'm not able to repro this. I get this output:

$ out/asan64/neteq_rtpplay --g722 9 --pcmu 0 --pcma 8 --isac 103 --isac 104 --avt 110 --opus 111 --avt 112 --avt 113 --cn_nb 105 --avt 126 unknown soob.pcm
Input file: unknown
Found valid packet with payload type 117 and SSRC 0x312e3020
Output file: soob.pcm
[0612/133923.708213:ERROR:neteq_impl.cc(117)] Sample rate 0 Hz not supported. Changing to 8000 Hz.


#
# Fatal error in ../../third_party/webrtc/modules/audio_coding/neteq/tools/neteq_test.cc, line 122
# last system error: 0
# Check failed: neteq_->RegisterPayloadType(c.second.first, c.second.second, c.first) == NetEq::kOK (-1 vs. 0)
# Cannot register ilbc to payload type 102
#
Aborted

--

I had to patch my local webrtc (under chromium) checkout to get the neteq_rtpplay target compiling. The patches I used are here: https://webrtc-review.googlesource.com/#/c/src/+/83260

I am on rev 9dce71b9 of webrtc.

Comment 3 by wfh@chromium.org, Jun 12 2018

ah, this is because chromium overrides webrtc and disables ilbc:

https://cs.chromium.org/chromium/src/.gn?type=cs&l=65

now I get a crash, but it doesn't symbolize yet.

Comment 4 by wfh@chromium.org, Jun 12 2018

This is a NULL Ptr deref at:

#0  0x00000000005c8f35 in InsertPacketInternal() () at ../../third_party/webrtc/modules/audio_coding/neteq/neteq_impl.cc:681
#1  0x00000000005c7d68 in InsertPacket() () at ../../third_party/webrtc/modules/audio_coding/neteq/neteq_impl.cc:145
#2  0x00000000005121af in Run() () at ../../third_party/webrtc/modules/audio_coding/neteq/tools/neteq_test.cc:61
#3  0x0000000000488328 in RunTest() () at ../../third_party/webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:474
#4  0x00007ffff59e02b1 in __libc_start_main () at /lib/x86_64-linux-gnu/libc.so.6
#5  0x00000000003ac02a in _start ()

looks like a NULL deref here:

https://cs.chromium.org/chromium/src/third_party/webrtc/modules/audio_coding/neteq/neteq_impl.cc?l=681

I agree I don't think this is exploitable - is this crash the same one you are getting, natashenka?
No, there's an OOB asan crash ... are you testing with ASAN? 

Comment 6 by wfh@chromium.org, Jun 12 2018

yes this is an asan build but it's a chromium checkout, perhaps I need to try with a webrtc checkout.
I used a WebRTC checkout to test this, that's probably the easiest.

Comment 8 by guidou@chromium.org, Jun 14 2018

Components: -Blink>WebRTC Blink>WebRTC>Audio

Comment 9 by wfh@chromium.org, Jun 14 2018

I tried with a webrtc checkout and still get the same failure at the same place. Do I need a real hardware device (I don't have that).

Comment 10 by wfh@chromium.org, Jun 14 2018

perhaps it would be easier if you just posted the asan stack?

Comment 11 by wfh@chromium.org, Jun 14 2018

Labels: Security_Severity-Low Needs-Feedback Security_Impact-Stable Pri-1
Owner: guidou@chromium.org
Status: Assigned (was: Unconfirmed)
Needs a reliable repro case to proceed here. Assigning to guidou@ to follow up with the reporter.

Based on the comment in #0 I'm triaging as Low, but once we see the asan stack, we can reassess if needed.
This reproduces on a WebRTC checkout built on Linux for me. I'm OOO, I'll post the ASAN stack when I'm back on Tuesday.
Will wait for the stack trace before reassigning to someone from the audio team.
Project Member

Comment 14 by sheriffbot@chromium.org, Jun 15 2018

Labels: -Pri-1 Pri-2
The asan dump I'm seeing is:

==81313==ERROR: AddressSanitizer: unknown-crash on address 0x6100000011ec at pc 0x00000062000f bp 0x7fff89f2ec50 sp 0x7fff89f2ec48
READ of size 4 at 0x6100000011ec thread T0
    #0 0x62000e in SampleRateHz modules/audio_coding/neteq/decoder_database.h:79:64
    #1 0x62000e in webrtc::TimestampScaler::ToInternal(unsigned int, unsigned char) modules/audio_coding/neteq/timestamp_scaler.cc:46
    #2 0x61fa74 in webrtc::TimestampScaler::ToInternal(webrtc::Packet*) modules/audio_coding/neteq/timestamp_scaler.cc:26:23
    #3 0x61fb7c in webrtc::TimestampScaler::ToInternal(std::__1::list<webrtc::Packet, std::__1::allocator<webrtc::Packet> >*) modules/audio_coding/neteq/timestamp_scaler.cc:32:5
    #4 0x5c8c72 in webrtc::NetEqImpl::InsertPacketInternal(webrtc::RTPHeader const&, rtc::ArrayView<unsigned char const, -4711l>, unsigned int) modules/audio_coding/neteq/neteq_impl.cc:645:24
    #5 0x5c7e8d in webrtc::NetEqImpl::InsertPacket(webrtc::RTPHeader const&, rtc::ArrayView<unsigned char const, -4711l>, unsigned int) modules/audio_coding/neteq/neteq_impl.cc:145:7
    #6 0x4f146e in webrtc::test::NetEqTest::Run() modules/audio_coding/neteq/tools/neteq_test.cc:61:27
    #7 0x452668 in webrtc::test::(anonymous namespace)::RunTest(int, char**) modules/audio_coding/neteq/tools/neteq_rtpplay.cc:474:35
    #8 0x7f369c7c12b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)

0x6100000011ec is located 172 bytes inside of 184-byte region [0x610000001140,0x6100000011f8)
allocated by thread T0 here:
    #0 0x447f72 in operator new(unsigned long) /b/build/slave/linux_upload_clang/build/src/third_party/llvm/compiler-rt/lib/asan/asan_new_delete.cc:93:3
    #1 0x5f6986 in __libcpp_allocate buildtools/third_party/libc++/trunk/include/new:259:10
    #2 0x5f6986 in allocate buildtools/third_party/libc++/trunk/include/memory:1799
    #3 0x5f6986 in allocate buildtools/third_party/libc++/trunk/include/memory:1548
    #4 0x5f6986 in __construct_node<std::__1::pair<unsigned char, webrtc::DecoderDatabase::DecoderInfo> > buildtools/third_party/libc++/trunk/include/__tree:2191
    #5 0x5f6986 in std::__1::pair<std::__1::__tree_iterator<std::__1::__value_type<unsigned char, webrtc::DecoderDatabase::DecoderInfo>, std::__1::__tree_node<std::__1::__value_type<unsigned char, webrtc::DecoderDatabase::DecoderInfo>, void*>*, long>, bool> std::__1::__tree<std::__1::__value_type<unsigned char, webrtc::DecoderDatabase::DecoderInfo>, std::__1::__map_value_compare<unsigned char, std::__1::__value_type<unsigned char, webrtc::DecoderDatabase::DecoderInfo>, std::__1::less<unsigned char>, true>, std::__1::allocator<std::__1::__value_type<unsigned char, webrtc::DecoderDatabase::DecoderInfo> > >::__emplace_unique_key_args<unsigned char, std::__1::pair<unsigned char, webrtc::DecoderDatabase::DecoderInfo> >(unsigned char const&, std::__1::pair<unsigned char, webrtc::DecoderDatabase::DecoderInfo>&&) buildtools/third_party/libc++/trunk/include/__tree:2137
    #6 0x5f361f in __emplace_unique_extract_key<std::__1::pair<unsigned char, webrtc::DecoderDatabase::DecoderInfo> > buildtools/third_party/libc++/trunk/include/__tree:1214:14
    #7 0x5f361f in __emplace_unique<std::__1::pair<unsigned char, webrtc::DecoderDatabase::DecoderInfo> > buildtools/third_party/libc++/trunk/include/__tree:1176
    #8 0x5f361f in __insert_unique<std::__1::pair<unsigned char, webrtc::DecoderDatabase::DecoderInfo>, void> buildtools/third_party/libc++/trunk/include/__tree:1304
    #9 0x5f361f in insert<std::__1::pair<unsigned char, webrtc::DecoderDatabase::DecoderInfo>, void> buildtools/third_party/libc++/trunk/include/map:1054
    #10 0x5f361f in webrtc::DecoderDatabase::RegisterPayload(unsigned char, webrtc::NetEqDecoder, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) modules/audio_coding/neteq/decoder_database.cc:198
    #11 0x5cfdcd in webrtc::NetEqImpl::RegisterPayloadType(webrtc::NetEqDecoder, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, unsigned char) modules/audio_coding/neteq/neteq_impl.cc:240:26
    #12 0x4f02c7 in webrtc::test::NetEqTest::RegisterDecoders(std::__1::map<int, std::__1::pair<webrtc::NetEqDecoder, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::less<int>, std::__1::allocator<std::__1::pair<int const, std::__1::pair<webrtc::NetEqDecoder, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > > > const&) modules/audio_coding/neteq/tools/neteq_test.cc:120:5
    #13 0x4efe8a in webrtc::test::NetEqTest::NetEqTest(webrtc::NetEq::Config const&, std::__1::map<int, std::__1::pair<webrtc::NetEqDecoder, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::less<int>, std::__1::allocator<std::__1::pair<int const, std::__1::pair<webrtc::NetEqDecoder, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > > > const&, std::__1::map<int, webrtc::test::NetEqTest::ExternalDecoderInfo, std::__1::less<int>, std::__1::allocator<std::__1::pair<int const, webrtc::test::NetEqTest::ExternalDecoderInfo> > > const&, std::__1::unique_ptr<webrtc::test::NetEqInput, std::__1::default_delete<webrtc::test::NetEqInput> >, std::__1::unique_ptr<webrtc::test::AudioSink, std::__1::default_delete<webrtc::test::AudioSink> >, webrtc::test::NetEqTest::Callbacks) modules/audio_coding/neteq/tools/neteq_test.cc:45:3
    #14 0x452576 in webrtc::test::(anonymous namespace)::RunTest(int, char**) modules/audio_coding/neteq/tools/neteq_rtpplay.cc:471:13
    #15 0x7f369c7c12b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)

SUMMARY: AddressSanitizer: unknown-crash modules/audio_coding/neteq/decoder_database.h:79:64 in SampleRateHz
Shadow bytes around the buggy address:
  0x0c207fff81e0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c207fff81f0: 00 00 00 00 00 00 00 00 00 f7 00 00 00 04 00 fa
  0x0c207fff8200: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c207fff8210: 00 00 00 00 00 00 00 00 00 f7 00 00 00 04 00 fa
  0x0c207fff8220: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
=>0x0c207fff8230: 00 00 00 00 00 00 00 00 00 f7 00 00 00[04]00 fa
  0x0c207fff8240: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c207fff8250: 00 00 00 00 00 00 00 00 00 f7 00 00 00 04 00 fa
  0x0c207fff8260: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c207fff8270: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c207fff8280: 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
==81313==ABORTING

The root cause here is that in modules/audio_coding/neteq/decoder_database.h there's the following code:

const AudioDecoder* decoder = GetDecoder();                  
RTC_DCHECK_EQ(1, !!decoder + !!cng_decoder_);              
return decoder ? decoder->SampleRateHz() : cng_decoder_->sample_rate_hz
}

If both decoder and cng_decoder_ are not defined, the DCHECK fires, but in release, the next line executes, and since decoder is NULL, 

cng_decoder_->sample_rate_hz

is executed. cng_decoder_ is an empty optional, so this reads out of bounds. 
                                  
Owner: hlundin@chromium.org
natashenka@: Thanks a lot for the trace.
hlundin@: Please take a look or triage further.
Status: Started (was: Assigned)
The problem with the input file is that the "Block PT" (see https://tools.ietf.org/html/rfc2198.html#section-3) is 117, which maps to RED. That is, the RED packet's first subpart is itself tagged as a RED packet. NetEq crashes on this, which it should of course not. I will fix that.

For reference, this issue can be reproduced without ASAN. Simply set dcheck_always_on = true in GN arguments, compile neteq_rtpplay and run the command in the OP.
Cc: ivoc@chromium.org
Fix is in review now: https://webrtc-review.googlesource.com/c/src/+/86824
Project Member

Comment 19 by bugdroid1@chromium.org, Jul 3

The following revision refers to this bug:
  https://webrtc.googlesource.com/src.git/+/defa7a80491c8598d74ba2895bdb73d7d1e674f3

commit defa7a80491c8598d74ba2895bdb73d7d1e674f3
Author: Henrik Lundin <henrik.lundin@webrtc.org>
Date: Tue Jul 03 20:27:57 2018

NetEq: Handle nested RED packets

This CL makes NetEq handle nested RED packets without crashing. Nested
RED packets mean that the block PT (see
https://tools.ietf.org/html/rfc2198.html#section-3) in the RED packet
is also set to the RED PT. This implies a nested RED packet, which is
not supported. Instead, all payloads in a RED packet that have the RED
PT will be discarded.

Bug:  chromium:851662 
Change-Id: I86ec257e60fb8076e3574ac5a4a1ca50196f6b34
Reviewed-on: https://webrtc-review.googlesource.com/86824
Commit-Queue: Henrik Lundin <henrik.lundin@webrtc.org>
Reviewed-by: Ivo Creusen <ivoc@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#23825}
[modify] https://crrev.com/defa7a80491c8598d74ba2895bdb73d7d1e674f3/modules/audio_coding/neteq/mock/mock_red_payload_splitter.h
[modify] https://crrev.com/defa7a80491c8598d74ba2895bdb73d7d1e674f3/modules/audio_coding/neteq/neteq_impl.cc
[modify] https://crrev.com/defa7a80491c8598d74ba2895bdb73d7d1e674f3/modules/audio_coding/neteq/red_payload_splitter.cc
[modify] https://crrev.com/defa7a80491c8598d74ba2895bdb73d7d1e674f3/modules/audio_coding/neteq/red_payload_splitter.h
[modify] https://crrev.com/defa7a80491c8598d74ba2895bdb73d7d1e674f3/modules/audio_coding/neteq/red_payload_splitter_unittest.cc

Project Member

Comment 20 by bugdroid1@chromium.org, Jul 4

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/47f7073d7af4edcefd0c554d514bc5e50f83726e

commit 47f7073d7af4edcefd0c554d514bc5e50f83726e
Author: webrtc-chromium-autoroll <webrtc-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Wed Jul 04 01:24:04 2018

Roll src/third_party/webrtc d000b0a32ee7..57900cb93360 (6 commits)

https://webrtc.googlesource.com/src.git/+log/d000b0a32ee7..57900cb93360


git log d000b0a32ee7..57900cb93360 --date=short --no-merges --format='%ad %ae %s'
2018-07-03 buildbot@webrtc.org Roll chromium_revision 6df2efa531..79cbcdf6fb (572277:572378)
2018-07-03 shampson@webrtc.org Adding shampson (me) as an owner to pc/ & api/.
2018-07-03 henrik.lundin@webrtc.org NetEq: Handle nested RED packets
2018-07-03 shampson@webrtc.org Adding ICE configurations to the PC perf test.
2018-07-03 buildbot@webrtc.org Roll chromium_revision ce19c6d80b..6df2efa531 (572160:572277)
2018-07-03 srte@webrtc.org Adds debug printing for congestion controllers.


Created with:
  gclient setdep -r src/third_party/webrtc@57900cb93360

The AutoRoll server is located here: https://webrtc-chromium-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:linux_chromium_archive_rel_ng;master.tryserver.chromium.mac:mac_chromium_archive_rel_ng

BUG=chromium:None,chromium:None,chromium:851662,chromium:None
TBR=webrtc-chromium-sheriffs-robots@google.com

Change-Id: Id33803de30fd9ae7b5196b0337c6740324a3a3ad
Reviewed-on: https://chromium-review.googlesource.com/1125419
Reviewed-by: webrtc-chromium-autoroll <webrtc-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: webrtc-chromium-autoroll <webrtc-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#572430}
[modify] https://crrev.com/47f7073d7af4edcefd0c554d514bc5e50f83726e/DEPS

Status: Fixed (was: Started)
natashenka@, thanks for reporting! I believe I have fixed the issue with the above CL. If you can verify, I would appreciate your help.

Project Member

Comment 22 by sheriffbot@chromium.org, Jul 4

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member

Comment 23 by sheriffbot@chromium.org, Oct 11

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment