WebRTC crash [@webrtc::Trace::Add]

RESOLVED FIXED in Firefox 19

Status

()

defect
--
critical
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: posidron, Unassigned)

Tracking

(Blocks 1 bug, {crash, sec-low, testcase})

Trunk
mozilla19
x86_64
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox18 disabled, firefox19 verified, firefox-esr10 unaffected, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [asan][qa-][adv-main19-])

Attachments

(3 attachments)

Reporter

Description

7 years ago
Posted file testcase
No description provided.
Reporter

Comment 1

7 years ago
Posted file callstack
Bug is in upstream code at video_capture_qtkit.mm:154

        WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceVideoCapture, _id,
                     "Failed to set capture device %s (unique ID %s) even "
                     "though it was a valid return from "
                     "VideoCaptureMacQTKitInfo");

Note the two %s's but no parameters for them.

Mild security risk (leak of info from random memory).  log routine uses vsnprintf() so it won't copy past the end of the buffer.  sec-low I'd think.
Group: core-security
Reporter

Comment 4

7 years ago
My internal voice said look at the printf() function but then only noticed 0x000000000070 and figured this won't be also a format string issue in 2012.

Shouldn't this have been popped up in a static code analyzer?
Reporter

Comment 5

7 years ago
@decoder: There are some ObjC files present in WebRTC and I am not sure how your MacOS situation is - perhaps we might miss some bugs here.
Attachment #674989 - Flags: review?(tterribe)
Comment on attachment 674989 [details] [diff] [review]
make sure we pass values for formatted log messages

This problem also still exists upstream. Please file an issue with them.
Attachment #674989 - Flags: review?(tterribe) → review+
Issue 1002 in their tracker, soon to be made hidden
Comment on attachment 674989 [details] [diff] [review]
make sure we pass values for formatted log messages

[Security approval request comment]
How easily can the security issue be deduced from the patch?
Trivially, though the testcase may not be obvious.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes

Which older supported branches are affected by this flaw?
17, 18 - though webrtc is preffed off by default in both.

If not all supported branches, which bug introduced the flaw?
WebRTC checkin and turning the build pref on.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
No special backport needed, trivial to apply.
Only is an issue if the user turns on the pref, and you can force the device open to fail (which might just be possible; see the testcase).

How likely is this patch to cause regressions; how much testing does it need?
Nil likelihood of regressions.
Attachment #674989 - Flags: sec-approval?
Christoph, the testcase you provided here is it a shutdown crash for you?
Reporter

Comment 10

7 years ago
(In reply to Henrik Skupin (:whimboo) from comment #9)
> Christoph, the testcase you provided here is it a shutdown crash for you?

Do you mean if the crash happens while you shutdown Firefox? No. 
Although it takes a few seconds till you get the crash.
I also wasn't able to reproduce it without the onconnection event handlers.

Can not reproduce this anymore with m-c changeset: 111360:58c8080a1a7c
(In reply to Randell Jesup [:jesup] from comment #3)
> Mild security risk (leak of info from random memory).  log routine uses
> vsnprintf() so it won't copy past the end of the buffer.  sec-low I'd think.

The log isn't available to web content, is it? If not then it's not even a potential memory leak. sec-approval+
Attachment #674989 - Flags: sec-approval? → sec-approval+
(In reply to Christoph Diehl [:cdiehl] from comment #10)
> Do you mean if the crash happens while you shutdown Firefox? No. 
> Although it takes a few seconds till you get the crash.
> I also wasn't able to reproduce it without the onconnection event handlers.
> 
> Can not reproduce this anymore with m-c changeset: 111360:58c8080a1a7c

For me it only crashed on shutdown. I let the testcase run for a couple of minutes before. Interesting.
Just tested again and the browser crashes on shutdown after I run the testcase once.
https://hg.mozilla.org/mozilla-central/rev/8e03750d5434
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Keywords: verifyme
Once verified this should be uplifted to Aurora/18
Verified on trunk 11/1 by running the test case attached a few times to make sure no crash happens.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Flags: in-testsuite?
Tried to reproduce the crash to get a crashtest for this on the 10/24 build - I'm not getting a crash.
Flags: in-testsuite?
Reporter

Comment 19

7 years ago
Are you using an ASan enabled build?
(In reply to Christoph Diehl [:cdiehl] from comment #19)
> Are you using an ASan enabled build?

Ah, that would explain why I can't reproduce this. Are our mochitests ran against asan builds?
Whiteboard: [asan]
No; so far as I know, we have no good way to automate asan tests (and due to CPU use we might not even if the were possible).
(In reply to Jason Smith [:jsmith] from comment #20)

> 
> Ah, that would explain why I can't reproduce this. Are our mochitests ran
> against asan builds?

If your mochitests are part of the regular mozilla-central tree, then they are pushed to try at least once a day on Linux 64 bit with ASan enabled.
Flags: in-testsuite?
Status: VERIFIED → RESOLVED
Closed: 7 years ago7 years ago
Whiteboard: [asan] → [asan][qa-]
How is status 18 "affected" when this is pref'd off? We mark these as "disabled" for that status.
Whiteboard: [asan][qa-] → [asan][qa-][adv-main19-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.