Closed Bug 1127206 Opened 5 years ago Closed 5 years ago

Crash when using certain File() constructors on workers

Categories

(Core :: DOM: Core & HTML, defect)

35 Branch
x86_64
Linux
defect
Not set

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)

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?
Flags: needinfo?(bzbarsky)
How did you ever get a wrapped nsIFile into a chrome worker?
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)
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.
Oh, and the testcase here is to do this in a worker:

  new File({});
Also, can we get rid of the USVString ctor for File?  It's web-observably wrong behavior...
Not sure about landing the testcase with this, since it makes it _very_ clear what the problem is
Attachment #8556615 - Flags: review?(peterv)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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)
Attachment #8556615 - Attachment is obsolete: true
Attachment #8556615 - Flags: review?(peterv)
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.
Blocks: 1127501
Attachment #8556642 - Flags: review?(peterv) → review+
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?
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.
Version: 33 Branch → 35 Branch
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.
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+
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?
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.
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.
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?
Attachment #8556642 - Flags: approval-mozilla-beta?
Attachment #8556642 - Flags: approval-mozilla-beta+
Attachment #8556642 - Flags: approval-mozilla-aurora?
Attachment #8556642 - Flags: approval-mozilla-aurora+
(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)
https://hg.mozilla.org/mozilla-central/rev/568af40e5e0d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Whiteboard: [adv-main36+]
Group: core-security → core-security-release
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.