Closed
Bug 1174971
Opened 9 years ago
Closed 9 years ago
compartment mismatch when accessing lastIndex over Xrays
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox38 | --- | unaffected |
firefox39 | + | fixed |
firefox38.0.5 | --- | unaffected |
firefox40 | + | fixed |
firefox41 | + | fixed |
firefox-esr31 | --- | unaffected |
firefox-esr38 | --- | unaffected |
b2g-v2.0 | --- | unaffected |
b2g-v2.0M | --- | unaffected |
b2g-v2.1 | --- | unaffected |
b2g-v2.1S | --- | unaffected |
b2g-v2.2 | --- | unaffected |
b2g-master | --- | fixed |
People
(Reporter: bholley, Assigned: bholley)
Details
(Keywords: regression, sec-high)
Attachments
(2 files, 1 obsolete file)
1.18 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
6.99 KB,
patch
|
bholley
:
review+
gkrizsanits
:
feedback+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
This is a regression from bug 1079919. Affects 39 (currently on beta) and up. The problem is that getOwnPropertyFromTargetIfSafe is kind of unintuitive, and operates in the target compartment. This means that callers need to rewrap it, which the newly-added RegExp code doesn't do. This is kind of a footgun. I'll work up a patch.
Assignee | ||
Comment 1•9 years ago
|
||
This test triggers the compartment mismatch.
Attachment #8622807 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8622808 -
Flags: review?(gkrizsanits)
Attachment #8622808 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 3•9 years ago
|
||
[Tracking Requested - why for this release]: security bug we haven't quite shipped yet
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bobbyholley
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8622808 [details] [diff] [review] Introduce two variants of getOwnPropertyFromTargetIfSafe. v1 [Security approval request comment] How easily could an exploit be constructed based on the patch? It's a pretty easy compartment mismatch to trigger, but only if you find chrome code that touches .lastIndex, which may be rare. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The patch is clear, yes. Which older supported branches are affected by this flaw? 39 and up. If not all supported branches, which bug introduced the flaw? bug 1079919. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should be straightforward. How likely is this patch to cause regressions; how much testing does it need? Low risk.
Attachment #8622808 -
Flags: sec-approval?
Assignee | ||
Comment 5•9 years ago
|
||
Sylvestre, is it too late to take this on 39?
Flags: needinfo?(sledru)
Updated•9 years ago
|
Attachment #8622808 -
Flags: sec-approval? → sec-approval+
Updated•9 years ago
|
status-firefox38:
--- → unaffected
status-firefox38.0.5:
--- → unaffected
status-firefox39:
--- → affected
status-firefox40:
--- → affected
status-firefox41:
--- → affected
status-firefox-esr31:
--- → unaffected
status-firefox-esr38:
--- → unaffected
Comment 6•9 years ago
|
||
Comment on attachment 8622807 [details] [diff] [review] Tests. v1 Review of attachment 8622807 [details] [diff] [review]: ----------------------------------------------------------------- Confirmed that the testcase causes compartment mismatch in JSOP_GETPROP lastIndex. I'll look into cpp part shortly.
Attachment #8622807 -
Flags: review?(arai.unmht) → review+
Comment 7•9 years ago
|
||
Comment on attachment 8622808 [details] [diff] [review] Introduce two variants of getOwnPropertyFromTargetIfSafe. v1 Review of attachment 8622808 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for fixing this :D ::: js/xpconnect/wrappers/XrayWrapper.cpp @@ +711,5 @@ > return false; > for (size_t i = 0; i < targetProps.length(); ++i) { > Rooted<JSPropertyDescriptor> desc(cx); > RootedId id(cx, targetProps[i]); > + if (!getOwnPropertyFromTargetIfSafeInTargetCompartment(cx, target, wrapper, id, &desc)) It would be better to wrap this line into 100-chars.
Attachment #8622808 -
Flags: review?(arai.unmht) → review+
Comment 8•9 years ago
|
||
Comment on attachment 8622808 [details] [diff] [review] Introduce two variants of getOwnPropertyFromTargetIfSafe. v1 Review of attachment 8622808 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/wrappers/XrayWrapper.h @@ +317,1 @@ > static bool getOwnPropertyFromTargetIfSafe(JSContext* cx, I understand your logic but I have to admit that I've got confused 3 times because of these names while I was reviewing this patch. This comment would be more clear if you said something like: "auto enters the target's compartment", while the other "assumes that cx is in the target compartment already" or something similar. Because after reading this comment seeing that the first line what this function does is: JSAutoCompartment ac(cx, target); can be confusing. To me the name getOwnPropertyFromTarget already suggests that it assumes that cx has to be in the targets compartment, and the other function is the one that is doing extra work (it could be getOwnPropertyFromTargetEnsureCompartment). But the last thing I want to do is to start a name debate over an urgent sec bug, so I'm fine with your version just please adjust the comments a bit accordingly.
Attachment #8622808 -
Flags: review?(gkrizsanits) → review+
Comment 9•9 years ago
|
||
n-i moved to Liz as she is the owner of 39.
Flags: needinfo?(sledru) → needinfo?(lhenry)
Assignee | ||
Comment 10•9 years ago
|
||
I tweaked the naming a bit - is this better gabor?
Attachment #8622808 -
Attachment is obsolete: true
Attachment #8623101 -
Flags: review+
Attachment #8623101 -
Flags: feedback?(gkrizsanits)
Assignee | ||
Updated•9 years ago
|
Flags: in-testsuite?
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8623101 [details] [diff] [review] Introduce two variants of getOwnPropertyFromTargetIfSafe. r=gabor,r=arai Approval Request Comment [Feature/regressing bug #]: bug 1079919 [User impact if declined]: sec-high [Describe test coverage new/current, TreeHerder]: automated test, but not checked in for security reasons. [Risks and why]: Low risk - we'd assert like crazy if we screwed this up. [String/UUID change made/needed]: None.
Attachment #8623101 -
Flags: approval-mozilla-beta?
Attachment #8623101 -
Flags: approval-mozilla-aurora?
Comment 12•9 years ago
|
||
Comment on attachment 8623101 [details] [diff] [review] Introduce two variants of getOwnPropertyFromTargetIfSafe. r=gabor,r=arai Review of attachment 8623101 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this looks perfect!
Attachment #8623101 -
Flags: feedback?(gkrizsanits) → feedback+
Comment 13•9 years ago
|
||
Comment on attachment 8623101 [details] [diff] [review] Introduce two variants of getOwnPropertyFromTargetIfSafe. r=gabor,r=arai Approved for uplift to aurora and beta since this is rated sec-high.
Flags: needinfo?(lhenry)
Attachment #8623101 -
Flags: approval-mozilla-esr38?
Attachment #8623101 -
Flags: approval-mozilla-beta?
Attachment #8623101 -
Flags: approval-mozilla-beta+
Attachment #8623101 -
Flags: approval-mozilla-aurora?
Attachment #8623101 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 14•9 years ago
|
||
Comment on attachment 8623101 [details] [diff] [review] Introduce two variants of getOwnPropertyFromTargetIfSafe. r=gabor,r=arai 38 is unaffected, no?
Attachment #8623101 -
Flags: approval-mozilla-esr38?
Updated•9 years ago
|
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.0M:
--- → unaffected
status-b2g-v2.1:
--- → unaffected
status-b2g-v2.1S:
--- → unaffected
status-b2g-v2.2:
--- → unaffected
status-b2g-master:
--- → affected
https://hg.mozilla.org/mozilla-central/rev/67be1324fc24
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment 16•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/eeca7a1394e3 https://hg.mozilla.org/releases/mozilla-beta/rev/b493f95649f4
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•