Closed
Bug 1127206
Opened 10 years ago
Closed 10 years ago
Crash when using certain File() constructors on workers
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
| Tracking | Status | |
|---|---|---|
| firefox35 | --- | wontfix |
| firefox36 | + | fixed |
| firefox37 | + | fixed |
| firefox38 | + | fixed |
| firefox-esr31 | --- | unaffected |
| b2g-v1.4 | --- | unaffected |
| b2g-v2.0 | --- | unaffected |
| b2g-v2.0M | --- | unaffected |
| b2g-v2.1 | --- | unaffected |
| b2g-v2.1S | --- | unaffected |
| b2g-v2.2 | --- | fixed |
| b2g-master | --- | fixed |
People
(Reporter: nsm, Assigned: bzbarsky)
References
Details
(Keywords: csectype-race, sec-high, Whiteboard: [adv-main36+])
Attachments
(1 file, 1 obsolete file)
|
5.16 KB,
patch
|
peterv
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
The 1 and 2 argument forms of the File constructor match the "chrome only" (not tagged [ChromeOnly]) constructor which accepts an nsIFile as the first argument. Using these on a worker attempts to unwrap an nsIFile off main thread and crashes:
#0 nsXPCWrappedJS::GetNewOrUsed (jsObj=..., aIID=..., wrapperResult=0x7fffbd4f8590) at /home/nikhil/mozilla-central-fetch/js/xpconnect/src/XPCWrappedJS.cpp:330
#1 0x00007fffef6c84c7 in mozilla::dom::UnwrapArgImpl (src=..., iid=..., ppArg=0x7fffbd4f9568) at /home/nikhil/mozilla-central-fetch/dom/bindings/BindingUtils.cpp:2740
#2 0x00007fffeee222b7 in mozilla::dom::UnwrapArg<nsIFile> (src=..., ppArg=0x7fffbd4f9568) at ../../dist/include/mozilla/dom/BindingUtils.h:64
#3 0x00007fffeee01dd3 in mozilla::dom::FileBinding::_constructor (cx=0x7fffc71395d0, argc=2, vp=0x7fffbfc651b8) at ./FileBinding.cpp:748
#4 0x00007ffff2a32d94 in js::CallJSNative (cx=0x7fffc71395d0, native=0x7fffeee009b0 <mozilla::dom::FileBinding::_constructor(JSContext*, unsigned int, JS::Value*)>, args=...)
at /home/nikhil/mozilla-central-fetch/js/src/jscntxtinlines.h:226
#5 0x00007ffff2a33021 in js::CallJSNativeConstructor (cx=0x7fffc71395d0, native=0x7fffeee009b0 <mozilla::dom::FileBinding::_constructor(JSContext*, unsigned int, JS::Value*)>, args=...)
at /home/nikhil/mozilla-central-fetch/js/src/jscntxtinlines.h:259
#6 0x00007ffff2a061c7 in js::InvokeConstructor (cx=0x7fffc71395d0, args=...) at /home/nikhil/mozilla-central-fetch/js/src/vm/Interpreter.cpp:595
#7 0x00007ffff29fa9c6 in Interpret (cx=0x7fffc71395d0, state=...) at /home/nikhil/mozilla-central-fetch/js/src/vm/Interpreter.cpp:2558
#8 0x00007ffff29ee10a in js::RunScript (cx=0x7fffc71395d0, state=...) at /home/nikhil/mozilla-central-fetch/js/src/vm/Interpreter.cpp:448
We could introduce non-ctor, static "File.fromnsIFile()" and change all call sites, unless there is a way to [ChromeOnly] a constructor or codegen some checks on the worker. Boris, any suggestions?
| Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(bzbarsky)
How did you ever get a wrapped nsIFile into a chrome worker?
| Assignee | ||
Comment 2•10 years ago
|
||
You don't.
The problem is that nsIFile is an external interface, so we use UnwrapArg<nsIFile>. This calls UnwrapArgImpl, which proceeds to do nsXPCWrappedJS::GetNewOrUsed if the passed in object is not a reflector. I think this is a security bug, actually, until Bobby convinces me otherwise.
Here are the things we use as the template argument for UnwrapArg:
imgINotificationObserver
imgIRequest
mozilla::dom::EventTarget
mozilla::dom::IndirectlyImplementedInterface
mozilla::dom::TestExternalInterface
nsIBrowserDOMWindow
nsIChannel
nsIDOMCSSRule
nsIDOMDataChannel
nsIDOMMozMmsMessage
nsIDOMMozSmsMessage
nsIDOMMozWakeLockListener
nsIDOMWindow
nsIFile
nsIFrameRequestCallback
nsIInputStream
nsIInputStreamCallback
nsIJSID
nsIMenuBuilder
nsIObserver
nsIOutputStream
nsIPrincipal
nsISelectionListener
nsISupports
nsITreeView
nsIURI
All of these look clearly mainthread only except mozilla::dom::EventTarget, which is only relevant here for non-webidl-binding event targets, of which there is only one, and that one is mainthread-only.
So we should just make UnwrapArgImpl return error on workers.
Group: core-security
Component: DOM: Workers → DOM
Flags: needinfo?(bzbarsky)
| Assignee | ||
Comment 3•10 years ago
|
||
Looks like this is basically a regression from bug 1047483 in practice...
[Tracking Requested - why for this release]: We should fix this. The tracking for 35 is in case we want to take a ridealong or something, but I assume this is basically wontfix for 35.
Blocks: 1047483
status-firefox-esr31:
--- → unaffected
tracking-firefox35:
--- → ?
tracking-firefox36:
--- → ?
tracking-firefox37:
--- → ?
tracking-firefox38:
--- → ?
| Assignee | ||
Comment 4•10 years ago
|
||
Oh, and the testcase here is to do this in a worker:
new File({});
| Assignee | ||
Comment 5•10 years ago
|
||
Also, can we get rid of the USVString ctor for File? It's web-observably wrong behavior...
| Assignee | ||
Comment 6•10 years ago
|
||
Not sure about landing the testcase with this, since it makes it _very_ clear what the problem is
Attachment #8556615 -
Flags: review?(peterv)
| Assignee | ||
Updated•10 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
| Assignee | ||
Comment 7•10 years ago
|
||
Also, we clearly have no test coverage for these "chromeonly" ctors on workers. Are they actually OK off-main-thread, esp. the string one?
Flags: needinfo?(amarchesini)
| Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8556642 -
Flags: review?(peterv)
| Assignee | ||
Updated•10 years ago
|
Attachment #8556615 -
Attachment is obsolete: true
Attachment #8556615 -
Flags: review?(peterv)
| Assignee | ||
Comment 9•10 years ago
|
||
Actually, I just realized that if we do this then we can assume external interfaces are not exposed in workers too. I filed bug 1127501 on that.
Updated•10 years ago
|
Updated•10 years ago
|
Attachment #8556642 -
Flags: review?(peterv) → review+
| Assignee | ||
Comment 10•10 years ago
|
||
Comment on attachment 8556642 [details] [diff] [review]
Don't try to do binding UnwrapArgImpl on worker threads. It can't do anything useful there
[Security approval request comment]
How easily could an exploit be constructed based on the patch? Probably not very easily. This bug allows touching the xpconnect data structures in a racy way from a worker thread. Then you still have to actually race against something in practice and then parlay the race into an exploit (e.g. by triggering causing a refcount to be off and the main thread to then work with a deleted object or something; I'm not sure whether that's even possible offhand).
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The tests are very clear: they test the things that would race without this patch. On the other hand, the code in the patch makes it clear that the issue is threadsafety, and that it's threadsafety of the File constructor. It even makes it clear-ish what sorts of arguments to the constructor could cause the threadsafety violation. The only part that's not necessarily obvious is how to pass those arguments from JS on a background thread, but I assume anyone familiar with the web will think of workers for the latter, and anyone familiar with Gecko would know to use {} for the former...
In any case, we could hold off on landing the test if we have to, but I don't really think it adds that much to what you can learn from the patch code.
Which older supported branches are affected by this flaw? 35, 36, 37
If not all supported branches, which bug introduced the flaw? Bug 1047483.
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? This patch should either apply as-is or be trivial to backport.
How likely is this patch to cause regressions; how much testing does it need? I don't think this is particularly regression-prone.
Attachment #8556642 -
Flags: sec-approval?
Updated•10 years ago
|
status-firefox35:
--- → wontfix
status-firefox36:
--- → affected
status-firefox37:
--- → affected
status-firefox38:
--- → affected
Comment 11•10 years ago
|
||
As you say, this sounds very hard to exploit in practice, but I'm marking in sec-high to be conservative. Feel free to adjust as desired.
Keywords: csectype-race,
sec-high
Version: 33 Branch → 35 Branch
Comment 12•10 years ago
|
||
I wonder how expensive it would be to crash if we're not on the main thread in IsCallerChrome(). Maybe this isn't that common.
Updated•10 years ago
|
Comment 13•10 years ago
|
||
Comment on attachment 8556642 [details] [diff] [review]
Don't try to do binding UnwrapArgImpl on worker threads. It can't do anything useful there
sec-approval+ for trunk. I'd like to get this on Aurora and Beta as well.
Can we check in the tests after we ship the fix, as we normally do for security issues, or is there an overriding reason to check them in now?
Attachment #8556642 -
Flags: sec-approval? → sec-approval+
| Assignee | ||
Comment 14•10 years ago
|
||
For a lot of security issues we just land the tests with the fix...
We could check in the tests later. I just always worry about forgetting to do that. If we have an automated reminder system for it, how do I use it?
Comment 15•10 years ago
|
||
I think we just set the in‑testsuite? flag and then scan for bugs assigned to self with that but different devs have a different process. There is no automated system.
I'm neutral about this. I just worry about our tests pwning us. Since we're mid-cycle, it probably isn't a big deal.
| Assignee | ||
Comment 16•10 years ago
|
||
OK. Basically, I would rather not poll on landing the tests.... and I doubt anyone else would like to do it either. We should add some notification for this sort of thing. :(
I think the risk of being pwned by this test is pretty low, so will just land it.
| Assignee | ||
Comment 17•10 years ago
|
||
Comment on attachment 8556642 [details] [diff] [review]
Don't try to do binding UnwrapArgImpl on worker threads. It can't do anything useful there
Approval Request Comment
[Feature/regressing bug #]: Bug 1047483
[User impact if declined]: Crashes caused by trying to do
nsXPCWrappedJS::GetNewOrUsed on a worker thread.
[Describe test coverage new/current, TreeHerder]: Has automated tests in the
patch.
[Risks and why]: Risk should be pretty low.
[String/UUID change made/needed]: None.
Attachment #8556642 -
Flags: approval-mozilla-beta?
Attachment #8556642 -
Flags: approval-mozilla-aurora?
| Assignee | ||
Comment 18•10 years ago
|
||
Updated•10 years ago
|
Attachment #8556642 -
Flags: approval-mozilla-beta?
Attachment #8556642 -
Flags: approval-mozilla-beta+
Attachment #8556642 -
Flags: approval-mozilla-aurora?
Attachment #8556642 -
Flags: approval-mozilla-aurora+
Comment 19•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #7)
> Also, we clearly have no test coverage for these "chromeonly" ctors on
> workers. Are they actually OK off-main-thread, esp. the string one?
Sorry to answer so late. We have 3 chrome-only ctors:
1. nsIFile => MainThread only.
2. Blob => This should be thread-safe
3. String => This should be thread-safe too. I'll try to see if we can get rid of this.
Flags: needinfo?(amarchesini)
Comment 20•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment 21•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/550f58e63d9a
https://hg.mozilla.org/releases/mozilla-beta/rev/63e9ce6a9a45
status-b2g-v1.4:
--- → unaffected
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.0M:
--- → unaffected
status-b2g-v2.1:
--- → unaffected
status-b2g-v2.1S:
--- → unaffected
status-b2g-v2.2:
--- → affected
status-b2g-master:
--- → fixed
Comment 22•10 years ago
|
||
Updated•10 years ago
|
Whiteboard: [adv-main36+]
Updated•10 years ago
|
Group: core-security → core-security-release
Updated•9 years ago
|
Group: core-security-release
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•