Closed
Bug 531222
Opened 15 years ago
Closed 15 years ago
Using SSM off main thread
Categories
(Core :: XPConnect, defect)
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)
2.74 KB,
text/plain
|
Details | |
2.46 KB,
patch
|
bent.mozilla
:
review+
jst
:
superreview+
jst
:
approval1.9.2+
dveditz
:
approval1.9.1.8+
|
Details | Diff | Splinter Review |
See attached stack.
Flags: blocking1.9.2?
This will likely lots of nice crashes as soon as some popular site starts using workers.
Comment 2•15 years ago
|
||
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.
Reporter | ||
Comment 5•15 years ago
|
||
And I think that we can only trigger this with workers or an extension, so maybe average users are still fine.
Comment 6•15 years ago
|
||
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•15 years ago
|
||
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?]
Assignee | ||
Comment 9•15 years ago
|
||
(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.
Assignee | ||
Comment 10•15 years ago
|
||
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.
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-
Reporter | ||
Comment 12•15 years ago
|
||
Comment on attachment 415254 [details] [diff] [review] Proposed fix Great!
Attachment #415254 -
Flags: review?(bent.mozilla) → review+
Reporter | ||
Updated•15 years ago
|
Attachment #415254 -
Flags: superreview?(jst)
Updated•15 years ago
|
Attachment #415254 -
Flags: superreview?(jst) → superreview+
Comment 13•15 years ago
|
||
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+
Assignee | ||
Comment 14•15 years ago
|
||
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: --- → ?
Assignee | ||
Comment 16•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/13ec995460e3
status1.9.2:
--- → final-fixed
Assignee | ||
Updated•15 years ago
|
Attachment #415254 -
Flags: approval1.9.1.7?
Updated•15 years ago
|
blocking1.9.1: ? → .7+
status1.9.1:
--- → wanted
Comment 17•15 years ago
|
||
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+
Assignee | ||
Comment 18•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a8cee81f470a
Comment 19•14 years ago
|
||
Are there any test cases for this running or any manual steps to verify the problem and the fix?
Reporter | ||
Comment 20•14 years ago
|
||
I think that I hit this the first time testing the POC in bug 533000. Not 100% sure though.
Comment 21•14 years ago
|
||
(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.
Updated•14 years ago
|
Flags: wanted1.9.0.x-
Updated•14 years ago
|
Blocks: CVE-2010-0160
Updated•14 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•