Last Comment Bug 531222 - Using SSM off main thread
: Using SSM off main thread
Status: RESOLVED FIXED
[sg:critical?]
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
:
Mentors:
Depends on:
Blocks: CVE-2010-0160
  Show dependency treegraph
 
Reported: 2009-11-25 20:00 PST by Ben Turner (not reading bugmail, use the needinfo flag!)
Modified: 2010-06-20 10:08 PDT (History)
10 users (show)
jonas: blocking1.9.2-
jonas: wanted1.9.2+
dveditz: wanted1.9.0.x-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final-fixed
.8+
.8-fixed


Attachments
Stack (2.74 KB, text/plain)
2009-11-25 20:00 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
no flags Details
Proposed fix (2.46 KB, patch)
2009-11-30 14:35 PST, Blake Kaplan (:mrbkap)
bent.mozilla: review+
jst: superreview+
jst: approval1.9.2+
dveditz: approval1.9.1.8+
Details | Diff | Splinter Review

Description Ben Turner (not reading bugmail, use the needinfo flag!) 2009-11-25 20:00:28 PST
Created attachment 414676 [details]
Stack

See attached stack.
Comment 1 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-25 20:42:01 PST
This will likely lots of nice crashes as soon as some popular site starts using workers.
Comment 2 Mike Beltzner [:beltzner, not reading bugmail] 2009-11-27 07:13:00 PST
Do we expect the fix to be particularly invasive? I'd rather not block on a speculative fix and focus on some of the known problems at hand, even though I feel a little dirty for saying so.

Some additional information would really help with a blocking information, here. I'm not capable of inferring impact or risk from that stack :(
Comment 3 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-27 07:31:01 PST
Blake or Jst, could you asses what needs to be done here. It looks like the bad call here happens somewhere deep in XPConnect-land.
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-27 07:32:16 PST
For what it's worth I suspect we could take this in a dot-release instead. But I guess that depends on how invasive the patch will be.
Comment 5 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-11-27 20:03:54 PST
And I think that we can only trigger this with workers or an extension, so maybe average users are still fine.
Comment 6 Boris Zbarsky [:bz] 2009-11-27 22:08:12 PST
Hmm.  Why would XPCNativeWrapper be even useful in a worker?  Can we just make those not exit there?  That would at least prevent the attached stack from happening...

The extension situation is a bit more interesting, but if it's trying to use XPCNativeWrapper off the main thread it loses anyway (since the underlying object is not threadsafe).

The other obvious weirdness here is the fact that XPCNativeWrapper::AttachNewConstructorObject calls into EnsureLegalActivity at all.  Can we just make XPC_NW_Enumerate detect when it's called on XPCNativeWrapper.prototype (as here) and bail out?  That's what XPC_NW_Call does, for example (or at least claims to; the code in XPC_NW_toString makes me think it screws it up)...  Though there are other EnsureLegalActivity callsites here that I think we should avoid making on the proto object (toString, say!).
Comment 7 Brandon Sterne (:bsterne) 2009-11-30 09:40:39 PST
Trying to assign severity here, and I'm not SSM-literate enough to get much from the stack.  I assume this a critical bug?
Comment 8 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-30 09:55:33 PST
The fact that it's the SSM that's being used off the main thread I think is unlikely to cause problems extra security problems. This is likely your "average" we-can-probably-crash-in-exploitable-ways bug. Though possibly harder to hit since we'll only crash under certain race conditions.
Comment 9 Blake Kaplan (:mrbkap) 2009-11-30 10:26:20 PST
(In reply to comment #6)
> Hmm.  Why would XPCNativeWrapper be even useful in a worker?  Can we just make
> those not exit there?  That would at least prevent the attached stack from
> happening...

We have to do this (and with XPCSafeJSObjectWrapper). The security wrappers all use the script security manager and therefore can't be used off the main thread. Surprisingly, this is actually safe because workers don't share any objects.

(Also, the objects that security wrappers usually protect are usually DOM objects which are main-thread-only).

> The extension situation is a bit more interesting, but if it's trying to use
> XPCNativeWrapper off the main thread it loses anyway (since the underlying
> object is not threadsafe).

Yep.


> The other obvious weirdness here is the fact that
> XPCNativeWrapper::AttachNewConstructorObject calls into EnsureLegalActivity at
> all.  Can we just make XPC_NW_Enumerate detect when it's called on
> XPCNativeWrapper.prototype (as here) and bail out?

IMO we should do this in another bug. I'm not entirely sure why we bothered with JS_InitClass and decided to have an XPCNativeWrapper.prototype at all. But this is separate because exposing the constructor at all means that a worker could do new XPCNativeWrapper(some XPConnected objected) and hit the SSM that way.
Comment 10 Blake Kaplan (:mrbkap) 2009-11-30 14:35:42 PST
Created attachment 415254 [details] [diff] [review]
Proposed fix

As things are now, this patch is sufficient to fix workers because they don't share objects and so we won't implicitly create wrappers for them off the main thread.
Comment 11 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-30 14:56:35 PST
Given that this seems like a low-risk patch (right?) I'd say we can take this in a dot-release if need be. Please renominate if you disagree.

Definitely think we should approve the patch if it passes reviews though.
Comment 12 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-11-30 15:20:17 PST
Comment on attachment 415254 [details] [diff] [review]
Proposed fix

Great!
Comment 13 Johnny Stenback (:jst, jst@mozilla.com) 2009-11-30 17:09:22 PST
Comment on attachment 415254 [details] [diff] [review]
Proposed fix

I think we should take this for 1.9.2, shipping w/o this patch is IMO riskier than taking the safe and simple patch.
Comment 14 Blake Kaplan (:mrbkap) 2009-12-02 12:20:25 PST
http://hg.mozilla.org/mozilla-central/rev/009b0693775c
Comment 15 Jonas Sicking (:sicking) PTO Until July 5th 2009-12-09 10:14:44 PST
Blake, are you landing this on the 1.9.2 branch? It's approved and ready to go.

Ben says we'll also want this on the 1.9.1 branch.
Comment 16 Blake Kaplan (:mrbkap) 2009-12-09 11:49:10 PST
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/13ec995460e3
Comment 17 Daniel Veditz [:dveditz] 2009-12-14 15:01:12 PST
Comment on attachment 415254 [details] [diff] [review]
Proposed fix

Approved for 1.9.1.7, a=dveditz for release-drivers
Comment 18 Blake Kaplan (:mrbkap) 2009-12-14 18:26:13 PST
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a8cee81f470a
Comment 19 Al Billings [:abillings] 2010-01-15 17:20:06 PST
Are there any test cases for this running or any manual steps to verify the problem and the fix?
Comment 20 Ben Turner (not reading bugmail, use the needinfo flag!) 2010-01-15 19:53:51 PST
I think that I hit this the first time testing the POC in bug 533000. Not 100% sure though.
Comment 21 Henrik Skupin (:whimboo) 2010-01-28 07:11:23 PST
(In reply to comment #20)
> I think that I hit this the first time testing the POC in bug 533000. Not 100%
> sure though.

But that means that the POC would be only one specific example. I was able to verify the fix on bug 533000 but do not know how we could proceed with testing here. If it would be even possible for us without any testcase.

Note You need to log in before you can comment on or make changes to this bug.