Closed Bug 1073350 Opened 5 years ago Closed 5 years ago

WebRTC: heap-buffer-overflow [@webrtc::ExtractBuffer]

Categories

(Core :: WebRTC, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla35
Tracking Status
firefox32 --- unaffected
firefox33 + fixed
firefox34 + fixed
firefox35 + fixed
firefox-esr31 --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: posidron, Assigned: jesup)

References

(Blocks 2 open bugs)

Details

(4 keywords)

Attachments

(3 files, 1 obsolete file)

This happened while opening https://gist.github.com/anonymous/a709930b5d604512cce8 in a build with '--enable-ipc-fuzzer' enabled.

Run Faulty this way to reproduce the issue:

export MOZ_IPC_MESSAGE_LOG=0
export FAULTY_ENABLE_LOGGING=1
export FAULTY_PROBABILITY=3500
export FAULTY_SEED=201095
export FAULTY_PICKLE=1
export FAULTY_PARENT=0
export FAULTY_CHILDREN=1
$HOME/dev/repos/mozilla/mozilla-ipc/obj-ff64-asan-opt/dist/NightlyDebug.app/Contents/MacOS/firefox -P fuzzing -no-remote $1 2>&1 | tee ~/Desktop/faulty-session.txt
I don't get anything like this output, running on Linux ASAN with --enable-ipc-fuzzer - no indication that fuzzing is occurring
Also, I see no hint of "--enable-ipc-fuzzing" in configure.in.  My assumption is that it's in a private repo somewhere.  What do I need to do to reproduce this?
Flags: needinfo?(cdiehl)
You need to apply v9 of the patch from here https://bugzilla.mozilla.org/show_bug.cgi?id=777067
Flags: needinfo?(cdiehl)
Keywords: sec-high
I suspect if NeedsShmem or their replies are getting tweaked that this is the same as bug 1073345
[Faulty] pickle field {int} of value: 6 changed to: 7

This caused it to check the Shmem ID for one of the messages, causing it to use the "wrong" data (which may have been too small, causing us to read outside it).  If we get past that, we can end up with a duplicate in the Shmem cache, which is very bad also (though not really a sec issue).

This would imply changing the Shmem for a smaller one would cause it to read past the end, getting data from whatever follows the Shmem.  (guard data on debug, but not in release)

If that's the case, we may want to really fix the deserializer to check who owns the buffer.

See bug 1073345
Comment on attachment 8498441 [details] [diff] [review]
Validate that returned decoded Shmems have enough data

Probably add a log to warn about bad frames after if (!CheckFrameData())...

Note: this is (minor) protect against DoS and (major) protect against the unlikely case of a too-small shmem being returned for a plane which is copied to memory which happens to be immediately followed (i.e. no crash) by sensitive memory, causing it to appear in the frame, which could be dumped to a canvas and then exported.
Attachment #8498441 - Flags: review?(cpearce)
Comment on attachment 8498441 [details] [diff] [review]
Validate that returned decoded Shmems have enough data

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

::: content/media/gmp/GMPVideoi420FrameImpl.cpp
@@ +72,5 @@
> +  int32_t half_width = (aFrameData.mWidth() + 1) / 2;
> +  if ((aFrameData.mYPlane().mStride() < aFrameData.mWidth()) ||
> +      (aFrameData.mUPlane().mStride() < half_width) ||
> +      (aFrameData.mVPlane().mStride() < half_width) ||
> +      (aFrameData.mYPlane().mSize() < aFrameData.mYPlane().mStride() * aFrameData.mHeight()	) ||

Extra spaces between aFrameData.mHeight() and ) near end of this line.

We should probably reject frames with planes with negative strides and negative sizes too.

@@ +73,5 @@
> +  if ((aFrameData.mYPlane().mStride() < aFrameData.mWidth()) ||
> +      (aFrameData.mUPlane().mStride() < half_width) ||
> +      (aFrameData.mVPlane().mStride() < half_width) ||
> +      (aFrameData.mYPlane().mSize() < aFrameData.mYPlane().mStride() * aFrameData.mHeight()	) ||
> +      (aFrameData.mUPlane().mSize() < half_width * ((aFrameData.mHeight()+1)/2)) ||

Shouldn't the stride of the U/V planes be correct; you're recalculating the stride here, but it seems to me the planes should store the correct one and we should validate it.
Comment on attachment 8498441 [details] [diff] [review]
Validate that returned decoded Shmems have enough data

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

We should add more checks here.
Attachment #8498441 - Flags: review?(cpearce) → review-
Attachment #8498441 - Attachment is obsolete: true
Comment on attachment 8498470 [details] [diff] [review]
Validate that returned decoded Shmems have enough data

checks checks checks... I forgot to add the LOGE() after the CheckFrameData call, BTW, so assume it's there.
Attachment #8498470 - Flags: review?(cpearce)
Comment on attachment 8498470 [details] [diff] [review]
Validate that returned decoded Shmems have enough data

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

Looks good. Thanks!
Attachment #8498470 - Flags: review?(cpearce) → review+
Comment on attachment 8498470 [details] [diff] [review]
Validate that returned decoded Shmems have enough data

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  
tough - they'd know there was something there, but trying to get it to be useful would be tough (and you need to break the plugin first as well)

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
A bit, though it isn't clear what getting it wrong allows.  We need this on beta asap anyways before Openh264 goes to release.  We could degrade the comments, but if it's going to aurora/beta ASAP it won't matter.

Which older supported branches are affected by this flaw?  Aurora and Beta

If not all supported branches, which bug introduced the flaw? GMP support

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  Should apply trivially

How likely is this patch to cause regressions; how much testing does it need? Very unlikely to regress; has been tested locally.
Attachment #8498470 - Flags: sec-approval?
Comment on attachment 8498470 [details] [diff] [review]
Validate that returned decoded Shmems have enough data

sec-approval+. We'll want Aurora and Beta patches made and nominated ASAP. As soon as this goes green on Trunk, we'll want this in the branches.

Does this need to go into ESR31?
Attachment #8498470 - Flags: sec-approval? → sec-approval+
Attachment #8498470 - Flags: approval-mozilla-beta?
Attachment #8498470 - Flags: approval-mozilla-aurora?
Attachment #8498470 - Flags: approval-mozilla-beta?
Attachment #8498470 - Flags: approval-mozilla-beta+
Attachment #8498470 - Flags: approval-mozilla-aurora?
Attachment #8498470 - Flags: approval-mozilla-aurora+
Backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66ad106fcb68

Sample log w/ build errors:
https://tbpl.mozilla.org/php/getParsedLog.php?id=49354395&tree=Mozilla-Inbound

The problems are:
 (1) You can't define a static function in one .cpp file, declare it in the header (as static), and then call it from another .cpp file.   Each compilation unit that includes the header will get its own  version of the static function. (all of them undefined, except for the one compilation unit with the definition).  This probably happens to work (Werror failures aside) because unified builds mean that the caller & the definition ended up in the same compilation unit; still, that's not something to depend on, and the Werror failure indicates that each #includer is getting its own version of the static function.

 (2) LOGE is apparently undefined.
Should have been class-static, not static function
Once more with feeling, with bustage fixes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/146eef5daadc
Assignee: nobody → rjesup
https://hg.mozilla.org/mozilla-central/rev/146eef5daadc
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(rjesup)
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Flags: needinfo?(rjesup)
Blocks: 1078105
Hi Christoph, repeating what I just typed in bug #1073345. We'd like to verify this bug fix, but won't be able to do so using your fuzzer before release. Are you able to verify that it's been fixed in Fx33? Thank you.
Flags: qe-verify-
Flags: needinfo?(cdiehl)
Yes, this seems fixed. Let Faulty run for an hour now and did not see this crash anymore.
Flags: needinfo?(cdiehl)
Status: RESOLVED → VERIFIED
Group: core-security
You need to log in before you can comment on or make changes to this bug.