Closed Bug 1174971 Opened 9 years ago Closed 9 years ago

compartment mismatch when accessing lastIndex over Xrays

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

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)

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.
Attached patch Tests. v1Splinter Review
This test triggers the compartment mismatch.
Attachment #8622807 - Flags: review?(arai.unmht)
Attachment #8622808 - Flags: review?(gkrizsanits)
Attachment #8622808 - Flags: review?(arai.unmht)
[Tracking Requested - why for this release]: security bug we haven't quite shipped yet
Assignee: nobody → bobbyholley
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?
Sylvestre, is it too late to take this on 39?
Flags: needinfo?(sledru)
Attachment #8622808 - Flags: sec-approval? → sec-approval+
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 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 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+
n-i moved to Liz as she is the owner of 39.
Flags: needinfo?(sledru) → needinfo?(lhenry)
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)
Flags: in-testsuite?
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 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 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+
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?
https://hg.mozilla.org/mozilla-central/rev/67be1324fc24
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.