Closed Bug 1400912 Opened 3 years ago Closed 3 years ago
Sanitizer: stack-use-after-scope when plugging in webcam (regression)
I can repro. The references in this `LockAndDispatch` class look shady. I found this using ASAN. https://pastebin.mozilla.org/9032634
Paul, does the build you use contain the patch for bug 1399395?
Assignee: nobody → 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
I have no idea about all that, but I can file yeah. I was just fixing the weirdness ASAN reported.
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.  Hard to see how being able to modify these values would give anyone an edge.  http://searchfox.org/mozilla-central/rev/15ce5cb2db0c85abbabe39a962b0e697c9ef098f/dom/media/systemservices/CamerasChild.cpp#219
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.
These two parameters aFailureValue and aSuccessValue  may be written after LockAndDispatch() ctor inits mFailureValue and mSuccessValue. Take example of CamerasChild::NumberOfCaptureDevices. mReplyInteger will be written in CamerasChild::RecvReplyNumberOfCaptureDevices()  before CamerasChild::NumberOfCaptureDevices() calls dispatcher.ReturnValue();  http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/dom/media/systemservices/CamerasChild.cpp#219  http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/dom/media/systemservices/CamerasChild.cpp#327
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: nobody → 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.
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)
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.
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).
Whiteboard: [adv-main58+] → [adv-main58+][post-critsmash-triage]
You need to log in before you can comment on or make changes to this bug.