1372383 - [Libfuzzer] Heap-buffer-overflow in sdp_parse_attr_fmtp when parsing annex p attribute

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P1)

Description

[Nils Ohlmeier [:drno]]

Attachment: crash-1420d107bd85adcad9c80f8c24ebe447a521a49d

./firefox crash-1420d107bd85adcad9c80f8c24ebe447a521a49d
WARNING: The binary has too many instrumented PCs.
         You may want to reduce the size of the binary
         for more efficient fuzzing and precise coverage data
[14392] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /home/nohlmeier/src/mozilla-central/xpcom/base/nsTraceRefcnt.cpp, line 172
[14392] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /home/nohlmeier/src/mozilla-central/xpcom/base/nsTraceRefcnt.cpp, line 172
[14392] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /home/nohlmeier/src/mozilla-central/xpcom/base/nsTraceRefcnt.cpp, line 172
Running LibFuzzer tests...
[14392] WARNING: NS_ENSURE_TRUE(greBinD) failed: file /home/nohlmeier/src/mozilla-central/tools/fuzzing/libfuzzer/harness/LibFuzzerTestHarness.h, line 227
Warning: MOZILLA_FIVE_HOME not set.
[14392] WARNING: NS_ENSURE_TRUE(greD) failed: file /home/nohlmeier/src/mozilla-central/tools/fuzzing/libfuzzer/harness/LibFuzzerTestHarness.h, line 220
INFO: Seed: 3452789901
INFO: Loaded 12 modules (2369097 guards): [0x5d96d0, 0x5dbce0), [0x7f30fb261140, 0x7f30fb267870), [0x7f30fe901640, 0x7f30fe901bec), [0x7f30fe8f4560, 0x7f30fe8f478c), [0x7f30fb0d6660, 0x7f30fb0d81ec), [0x7f30fb063ea0, 0x7f30fb0672cc), [0x7f30faf91740, 0x7f30fafa2130), [0x7f30fad83f40, 0x7f30fad88904), [0x7f30face1100, 0x7f30facf99b8), [0x7f30fa82a8a0, 0x7f30fa834f04), [0x7f30fe8bd140, 0x7f30fe8bd148), [0x7f30f0ff7060, 0x7f30f18ba1dc), 
./firefox: Running 1 inputs 1 time(s) each.
Running: crash-1420d107bd85adcad9c80f8c24ebe447a521a49d
=================================================================
==14392==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60400007e3fc at pc 0x7f30e6d58e3a bp 0x7ffdadf49680 sp 0x7ffdadf49678
READ of size 1 at 0x60400007e3fc thread T0
    #0 0x7f30e6d58e39  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/gtest/libxul.so+0x10794e39)
    #1 0x7f30e6d58437  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/gtest/libxul.so+0x10794437)
    #2 0x7f30e6d0a5f3  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/gtest/libxul.so+0x107465f3)
    #3 0x7f30e6d076cc  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/gtest/libxul.so+0x107436cc)
    #4 0x7f30e6d4a00f  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/gtest/libxul.so+0x1078600f)
    #5 0x7f30e6f2173f  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/gtest/libxul.so+0x1095d73f)
    #6 0x7f30e7999f41  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/gtest/libxul.so+0x113d5f41)
    #7 0x574d5c  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/firefox+0x574d5c)
    #8 0x574f4f  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/firefox+0x574f4f)
    #9 0x56d1bf  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/firefox+0x56d1bf)
    #10 0x56fe01  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/firefox+0x56fe01)
    #11 0x7f30e6c68da5  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/gtest/libxul.so+0x106a4da5)
    #12 0x7f30e6b461d3  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/gtest/libxul.so+0x105821d3)
    #13 0x7f30e6b54f44  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/gtest/libxul.so+0x10590f44)
    #14 0x7f30e6b566b9  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/gtest/libxul.so+0x105926b9)
    #15 0x51d5af  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/firefox+0x51d5af)
    #16 0x7f30fd5de82f  (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #17 0x426d48  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/firefox+0x426d48)

0x60400007e3fc is located 0 bytes to the right of 44-byte region [0x60400007e3d0,0x60400007e3fc)
allocated by thread T0 here:
    #0 0x4df3d8  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/firefox+0x4df3d8)
    #1 0x51f0ab  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/firefox+0x51f0ab)
    #2 0x7f30e6cfd763  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/gtest/libxul.so+0x10739763)
    #3 0x7f30e6d09a45  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/gtest/libxul.so+0x10745a45)
    #4 0x7f30e6d076cc  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/gtest/libxul.so+0x107436cc)
    #5 0x7f30e6d4a00f  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/gtest/libxul.so+0x1078600f)
    #6 0x7f30e6f2173f  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/gtest/libxul.so+0x1095d73f)
    #7 0x7f30e7999f41  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/gtest/libxul.so+0x113d5f41)
    #8 0x574d5c  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/firefox+0x574d5c)
    #9 0x574f4f  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/firefox+0x574f4f)
    #10 0x56d1bf  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/firefox+0x56d1bf)
    #11 0x56fe01  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/firefox+0x56fe01)
    #12 0x7f30e6c68da5  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/gtest/libxul.so+0x106a4da5)
    #13 0x7f30e6b461d3  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/gtest/libxul.so+0x105821d3)
    #14 0x7f30e6b54f44  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/gtest/libxul.so+0x10590f44)
    #15 0x7f30e6b566b9  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/gtest/libxul.so+0x105926b9)
    #16 0x51d5af  (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/firefox+0x51d5af)
    #17 0x7f30fd5de82f  (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

SUMMARY: AddressSanitizer: heap-buffer-overflow (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/gtest/libxul.so+0x10794e39) 
Shadow bytes around the buggy address:
  0x0c0880007c20: fa fa 00 00 00 00 00 07 fa fa 00 00 00 00 00 07
  0x0c0880007c30: fa fa 00 00 00 00 00 07 fa fa 00 00 00 00 00 07
  0x0c0880007c40: fa fa 00 00 00 00 00 07 fa fa fd fd fd fd fd fd
  0x0c0880007c50: fa fa 00 00 00 00 00 07 fa fa 00 00 00 00 00 07
  0x0c0880007c60: fa fa 00 00 00 00 00 00 fa fa fd fd fd fd fd fd
=>0x0c0880007c70: fa fa 00 00 00 00 00 fa fa fa 00 00 00 00 00[04]
  0x0c0880007c80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0880007c90: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0880007ca0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0880007cb0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0880007cc0: 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
==14392==ABORTING

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: bug1372383.patch

This patch ensures that an encountering 'p=\n', so an empty Annex P token, the parser errors out instead of continuing to parse the remainder of the string.

Question is if we should additionally check if fmtp_ptr has or is running over the length of the buffer as an exit criteria for the while loop here: http://searchfox.org/mozilla-central/rev/d840ebd5858a61dbc1622487c1fab74ecf235e03/media/webrtc/signaling/src/sdp/sipcc/sdp_attr.c#656

Comment 2

[Byron Campen [:bwc]]

(In reply to Nils Ohlmeier [:drno] from comment #1)
> Created attachment 8876970 [details] [diff] [review]
> bug1372383.patch
> 
> This patch ensures that an encountering 'p=\n', so an empty Annex P token,
> the parser errors out instead of continuing to parse the remainder of the
> string.
> 
> Question is if we should additionally check if fmtp_ptr has or is running
> over the length of the buffer as an exit criteria for the while loop here:
> http://searchfox.org/mozilla-central/rev/
> d840ebd5858a61dbc1622487c1fab74ecf235e03/media/webrtc/signaling/src/sdp/
> sipcc/sdp_attr.c#656

I would like to see that loop check.

Comment 3

[Byron Campen [:bwc]]

Comment on attachment 8876970 [details] [diff] [review]
bug1372383.patch

Review of attachment 8876970 [details] [diff] [review]:
-----------------------------------------------------------------

Let's get that loop check you mentioned also.

Comment 4

[Andrew McCreight [:mccr8]]

I'm going to mark this sec-high. Feel free to adjust if it can't be triggered from web content.

Comment 5

[Nils Ohlmeier [:drno]]

(In reply to Andrew McCreight [:mccr8] from comment #4)
> I'm going to mark this sec-high. Feel free to adjust if it can't be
> triggered from web content.

Same here: easily accessible from content.

Comment 6

[Nils Ohlmeier [:drno]]

This is originally external code. I notified upstream to access the impact.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

We need sec-approval so we can land this!  and probably uplift to beta

Comment 8

[Nils Ohlmeier [:drno]]

(In reply to Randell Jesup [:jesup] from comment #7)
> We need sec-approval so we can land this!  and probably uplift to beta

Still waiting for upstream to access, but I can get the sec-approval going.

Comment 9

[Nils Ohlmeier [:drno]]

Comment on attachment 8876970 [details] [diff] [review]
bug1372383.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not too hard as the expected behavior is documented in a public spec.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No tests or check-in comments. We might want to take out the link to the spec from the patch set to reduce the risk.

Which older supported branches are affected by this flaw? All branches up to esr-45 are affected by this.

If not all supported branches, which bug introduced the flaw? The behavior got probably bug 792188.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The patch applies cleanly up to release, though not to ESR45. But it would be easy to backport it to ESR45 as well if needed.

How likely is this patch to cause regressions; how much testing does it need? It is highly unlikely to cause regression as this in a part of the SDP parser which is not actively used by WebRTC. No testing needed.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

ESR45 isn't supported anymore.

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8876970 [details] [diff] [review]
bug1372383.patch

sec-approval+ for trunk.
If the patch applies cleanly, please nominate it for Beta and ESR52 (or make new patches and nominate those).

Comment 12

[Nils Ohlmeier [:drno]]

Upstream has confirmed to me that they are affected by this problem.

Comment 13

[Al Billings [:abillings - ex-MoCo]]

This has had sec-approval+ since July 5. Is this going to land soon?

Comment 14

[Nils Ohlmeier [:drno]]

Attachment: bug1372383.patch

Rebased, remove comment pointing to the spec for the parser and made the commit message less obvious.

Carrying forward r+bwc and sec-approval:abillings

Comment 15

[Nils Ohlmeier [:drno]]

Attachment: bug1372383_esr52.patch

Same patch as attachment 8888790 [details] [diff] [review] but for the ESR52 branch.

Comment 16

[Nils Ohlmeier [:drno]]

Comment on attachment 8888790 [details] [diff] [review]
bug1372383.patch

Approval Request Comment
[Feature/Bug causing the regression]: When WebRTC support landed in Firefox
[User impact if declined]: Browser or content process crashes
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes manually
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: No
[Why is the change risky/not risky?]: In a regular WebRTC call the H263 part of the SDP parser (which this patch is for) will never be used. 
[String changes made/needed]: N/A

Comment 17

[Nils Ohlmeier [:drno]]

Comment on attachment 8888791 [details] [diff] [review]
bug1372383_esr52.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: It is sec-high.
User impact if declined: Crashes
Fix Landed on Version: No yet landed.
Risk to taking this patch (and alternatives if risky): Really low, as this part of the parser is not used in a regular WebRTC call.
String or UUID changes made by this patch: N/A

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 18

[Nils Ohlmeier [:drno]]

Please don't disclose this until we have the okay from upstream to disclose, as they have products which are affected by this.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c4788415e46117f43700b7fc64cad9971542985e

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c4788415e461

Comment 21

[Julien Cristau [:jcristau]]

Comment on attachment 8888790 [details] [diff] [review]
bug1372383.patch

sec-high, beta55+

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/160b4883a3f5

Comment 23

[Julien Cristau [:jcristau]]

Comment on attachment 8888791 [details] [diff] [review]
bug1372383_esr52.patch

webrtc sec fix, esr52.3+

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/56349462ff47

Comment 25

[Nils Ohlmeier [:drno]]

Reminder to self to add unit test once disclosed.

Comment 26

[Al Billings [:abillings - ex-MoCo]]

What's the timeline to upstream fix and disclosure?