Closed Bug 531222 Opened 15 years ago Closed 15 years ago

Using SSM off main thread

Categories

(Core :: XPConnect, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- final-fixed
blocking1.9.1 --- .8+
status1.9.1 --- .8-fixed

People

(Reporter: bent.mozilla, Assigned: mrbkap)

References

Details

(Whiteboard: [sg:critical?])

Attachments

(2 files)

Attached file Stack
See attached stack.
Flags: blocking1.9.2?
This will likely lots of nice crashes as soon as some popular site starts using workers.
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 :(
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.
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.
And I think that we can only trigger this with workers or an extension, so maybe average users are still fine.
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!).
Trying to assign severity here, and I'm not SSM-literate enough to get much from the stack.  I assume this a critical bug?
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.
Whiteboard: [sg:critical?]
(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.
Attached patch Proposed fixSplinter Review
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.
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #415254 - Flags: review?(bent.mozilla)
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.
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Comment on attachment 415254 [details] [diff] [review]
Proposed fix

Great!
Attachment #415254 - Flags: review?(bent.mozilla) → review+
Attachment #415254 - Flags: superreview?(jst)
Attachment #415254 - Flags: superreview?(jst) → superreview+
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.
Attachment #415254 - Flags: approval1.9.2+
http://hg.mozilla.org/mozilla-central/rev/009b0693775c
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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.
blocking1.9.1: --- → ?
Attachment #415254 - Flags: approval1.9.1.7?
blocking1.9.1: ? → .7+
Comment on attachment 415254 [details] [diff] [review]
Proposed fix

Approved for 1.9.1.7, a=dveditz for release-drivers
Attachment #415254 - Flags: approval1.9.1.7? → approval1.9.1.7+
Are there any test cases for this running or any manual steps to verify the problem and the fix?
I think that I hit this the first time testing the POC in bug 533000. Not 100% sure though.
(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.
Flags: wanted1.9.0.x-
Group: core-security
You need to log in before you can comment on or make changes to this bug.