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

RESOLVED FIXED in Firefox -esr52
(NeedInfo from)

Status

()

defect
P1
normal
Rank:
15
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: drno, Assigned: drno, NeedInfo)

Tracking

(Blocks 1 bug, {csectype-bounds, sec-high})

Trunk
mozilla56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 wontfix, firefox-esr5255+ fixed, firefox54 wontfix, firefox55+ fixed, firefox56+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main55-][adv-esr52.3-] don't disclose until upstream agrees to disclose)

Attachments

(3 attachments, 1 obsolete attachment)

./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
Rank: 15
Posted patch bug1372383.patch (obsolete) — Splinter Review
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
Attachment #8876970 - Flags: review?(docfaraday)
Summary: [Libfuzzer] Heap-buffer-overflow in sdp_parse_attr_fmtp → [Libfuzzer] Heap-buffer-overflow in sdp_parse_attr_fmtp when parsing annex p attribute
(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 on attachment 8876970 [details] [diff] [review]
bug1372383.patch

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

Let's get that loop check you mentioned also.
Attachment #8876970 - Flags: review?(docfaraday) → review+
Blocks: 792125
Group: core-security → media-core-security
I'm going to mark this sec-high. Feel free to adjust if it can't be triggered from web content.
(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.
This is originally external code. I notified upstream to access the impact.
We need sec-approval so we can land this!  and probably uplift to beta
(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 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.
Attachment #8876970 - Flags: sec-approval?
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).
Attachment #8876970 - Flags: sec-approval? → sec-approval+
Upstream has confirmed to me that they are affected by this problem.
This has had sec-approval+ since July 5. Is this going to land soon?
Flags: needinfo?(drno)
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
Attachment #8876970 - Attachment is obsolete: true
Flags: needinfo?(drno)
Same patch as attachment 8888790 [details] [diff] [review] but for the ESR52 branch.
Attachment #8888791 - Flags: review+
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
Attachment #8888790 - Flags: review+
Attachment #8888790 - Flags: approval-mozilla-beta?
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.
Attachment #8888791 - Flags: approval-mozilla-esr52?
Please don't disclose this until we have the okay from upstream to disclose, as they have products which are affected by this.
Keywords: checkin-needed
Whiteboard: don't disclose until upstream agrees to disclose
https://hg.mozilla.org/mozilla-central/rev/c4788415e461
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment on attachment 8888790 [details] [diff] [review]
bug1372383.patch

sec-high, beta55+
Attachment #8888790 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8888791 [details] [diff] [review]
bug1372383_esr52.patch

webrtc sec fix, esr52.3+
Attachment #8888791 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Reminder to self to add unit test once disclosed.
Flags: needinfo?(drno)
What's the timeline to upstream fix and disclosure?
Whiteboard: don't disclose until upstream agrees to disclose → [adv-main55-][adv-esr52.3-] don't disclose until upstream agrees to disclose
Group: media-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [adv-main55-][adv-esr52.3-] don't disclose until upstream agrees to disclose → [post-critsmash-triage][adv-main55-][adv-esr52.3-] don't disclose until upstream agrees to disclose
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.