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

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

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

Attachment: make sure we pass values for formatted log messages

Comment 3

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

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.

Comment 4

[Christoph Diehl [:posidron]]

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?

Comment 5

[Christoph Diehl [:posidron]]

@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.

Comment 6

[Timothy B. Terriberry (:derf)]

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.

Comment 7

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

Issue 1002 in their tracker, soon to be made hidden

Comment 8

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

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.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Christoph, the testcase you provided here is it a shutdown crash for you?

Comment 10

[Christoph Diehl [:posidron]]

(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

Comment 11

[Daniel Veditz [:dveditz]]

(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+

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Just tested again and the browser crashes on shutdown after I run the testcase once.

Comment 14

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e03750d5434

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8e03750d5434

Comment 16

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

Once verified this should be uplifted to Aurora/18

Comment 17

[Jason Smith [:jsmith]]

Verified on trunk 11/1 by running the test case attached a few times to make sure no crash happens.

Comment 18

[Jason Smith [:jsmith]]

Tried to reproduce the crash to get a crashtest for this on the 10/24 build - I'm not getting a crash.

Comment 19

[Christoph Diehl [:posidron]]

Are you using an ASan enabled build?

Comment 20

[Jason Smith [:jsmith]]

(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?

Comment 21

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

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).

Comment 22

[Christian Holler (:decoder)]

(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.

Comment 23

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

How is status 18 "affected" when this is pref'd off? We mark these as "disabled" for that status.