Closed Bug 1400912 Opened 3 years ago Closed 3 years ago

AddressSanitizer: stack-use-after-scope when plugging in webcam (regression)


(Core :: WebRTC: Audio/Video, defect)

Not set



Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 + wontfix
firefox58 + fixed


(Reporter: padenot, Assigned: jib)


(Blocks 1 open bug)


(Keywords: csectype-other, sec-moderate, Whiteboard: [adv-main58+][post-critsmash-triage])


(1 file, 2 obsolete files)

I can repro. The references in this `LockAndDispatch` class look shady. I found this using ASAN.
Group: core-security → media-core-security
Paul, does the build you use contain the patch for bug 1399395?
Flags: needinfo?(padenot)
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → padenot
Flags: needinfo?(padenot)
Attachment #8909766 - Flags: review?(jib)
While it's probably ok, I'm not in love with the use of raw ptrs in GetCaptureDevice/LockAndDispatch/DispatchToParent.
I'd prefer it to pass the references down, using already_AddRefed<> and ....forget() (or nsCOMPtr& and a final .forget()), such that there's clear ownership of the runnable, especially in failure cases.  As I said, it's probably safe (the final Dispatch() will add a ref). 

I'd also not store the runnable in LockAndDispatch(), and instead give is as a parameter to Dispatch()/DispatchToParent()... Plus I'm not sure that the private Dispatch() has any utility over just putting DispatchToParent in LockAndDispatch().  However, this is all cleanup/nits.

The cleanup could be done later; can you file a bug on that?  That can be public
Flags: needinfo?(padenot)
I have no idea about all that, but I can file yeah. I was just fixing the weirdness ASAN reported.
Flags: needinfo?(padenot)
Attachment #8909766 - Flags: review?(jib) → review+
I fail to see how this is a sec bug. The bug appears to be that LockAndDispatch's mSuccessValue and mFailureValue are int& and by default initialized to function argument default values. [1]

Hard to see how being able to modify these values would give anyone an edge.

They're never written to.
I haven't thought too much about that, it was a bit late when I opened the bug. ASAN issue = sec-bug by default, in my mind. It's certainly better to have something wrongly restricted than wrongly opened.

In any case, I'm landing this.
This is an ASAN trigger, but it's not a UAF, it's a use-after-scope.

Use-after-scope can be a sec issue if it's written to after later, deeper calls are made, but mostly for writes (though reads could in theory disclose parts of the stack, or cause incorrect action).  Reducing severity in this case.
This has been backed out for breaking a bunch of tests. It's really scary that my patch changes the behavior. jib, can you investigate? I don't know this code, and I don't have time anyways.
Flags: needinfo?(jib)
These two parameters aFailureValue and aSuccessValue [1] may be written after LockAndDispatch() ctor inits mFailureValue and mSuccessValue.

Take example of CamerasChild::NumberOfCaptureDevices. mReplyInteger will be written in CamerasChild::RecvReplyNumberOfCaptureDevices() [2] before 
CamerasChild::NumberOfCaptureDevices() calls dispatcher.ReturnValue();


Crazy. Please refactor this back to safety ASAP. Currently the code looks ok-ish (it's plain wrong, but not harmful), but we change code all the time, and the next person to change this will not know about this undocumented invariant.

Also, this is in code that is not tested by unit or integration tests, which makes it even a higher priority. If possible, we should add plugging and unplugging webcam devices to a manual test suite, and then mock the webcam platform layer and write tests.

It was by luck that I found this one: I grabbed this webcam and plugged it into my Linux machine with an ASAN build because this webcam has a microphone and I needed an audio input to test something else. I don't want us to rely on luck for stability and security.
Assignee: padenot → nobody
Assignee: nobody → jib
Flags: needinfo?(jib)
This is a minimal patch for uplift (as in not code we want going forward).

Doing more than this requires refactoring the LockAndDispatch() API to not rely on this arguably weird late-init-by-reference mechanism.
Attachment #8909766 - Attachment is obsolete: true
Attachment #8912325 - Flags: review?(padenot)
This appears to have regressed a while back, in bug 1239384.
OS: Unspecified → All
Hardware: Unspecified → All
Summary: AddressSanitizer: stack-use-after-scope when plugging in webcam on (at least) linux → AddressSanitizer: stack-use-after-scope when plugging in webcam (regression)
s/bool/int/ typo
Attachment #8912325 - Attachment is obsolete: true
Attachment #8912325 - Flags: review?(padenot)
Attachment #8913664 - Flags: review?(padenot)
Attachment #8913664 - Flags: review?(padenot) → review+
Tracked in 57 for now, if the uplift is risky/too late, we'll have to live with a 57 wontfix.
This is a safe fix. We just misunderstood how the references were used in our first attempt. Should be solid now FWIW.
Did you want to land this on trunk and request uplift? 
It could still make the 57 beta 8 build later this week.
Flags: needinfo?(jib)
Flags: needinfo?(jib)
Keywords: checkin-needed
This was found with ubsan (Undefined-Behaviour SANitized) by Paul. We've discussed and agreed none of our current code paths make this an issue.

Assuming we make no changes in this area for 57, then it's safe not to uplift this.
Minor correction: it was found by ASAN (though would be found by either).
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Group: media-core-security → core-security-release
Whiteboard: [adv-main58+]
Flags: qe-verify-
Whiteboard: [adv-main58+] → [adv-main58+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.