Closed Bug 1438556 Opened 3 years ago Closed 3 years ago

Avoid non-wrapper cross-compartment edges in ICs

Categories

(Core :: JavaScript Engine: JIT, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
thunderbird_esr52 --- unaffected
firefox-esr52 --- unaffected
firefox-esr60 61+ fixed
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 + fixed
firefox62 + fixed

People

(Reporter: tcampbell, Assigned: mgaudet)

References

Details

(Keywords: sec-high, Whiteboard: [adv-main61+][adv-esr60.1+][post-critsmash-triage])

Attachments

(7 files, 21 obsolete files)

1.11 KB, text/plain
Details
1.35 KB, text/plain
Details
671 bytes, application/x-javascript
Details
9.77 KB, patch
mgaudet
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
2.90 KB, patch
mgaudet
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
4.08 KB, patch
mgaudet
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
16.09 KB, patch
Details | Diff | Splinter Review
Attached file Testcase (obsolete) —
While stumbling over my own bugs in Bug 1438086, I discovered that under certain cases we can fill an IC stub field with a cross-compartment edge. This trips assertion [1].

While my specific failure was caused by me accidentally adding guards we didn't have before, I can force the same behavior on m-c.

The direct fix is to not attach a CCW IC if anything in chain is marked as uncacheable prototype. This will avoid direct prototype guards storing an cross-compartment object in stub. With the cleanup in Bug 1438086, we could simplify the check to only |holder->hasUncacheableProto()|.

[1] https://searchfox.org/mozilla-central/source/js/src/jsgc.cpp#4048-4050
Jan, can you weight in on the severity of this? We wind up with stub field pointing into another compartment (without any wrappers).
Flags: needinfo?(jdemooij)
(In reply to Ted Campbell [:tcampbell] from comment #2)
> Jan, can you weight in on the severity of this? We wind up with stub field
> pointing into another compartment (without any wrappers).

Is it the same zone? In that case we should still mark the target object as GC happens per zone. I'm not sure if it's possible for the other compartment to become dead. Still, this violates an important invariant so it's probably best to track as sec-high...
Flags: needinfo?(jdemooij)
Keywords: sec-high
Yes, we do check that it is same zone. I think the other compartment is able to be nuked from under us, though.
I'll put together a fix in the next week.
Flags: needinfo?(tcampbell)
Assignee: nobody → tcampbell
Flags: needinfo?(tcampbell)
Priority: -- → P1
Flags: needinfo?(tcampbell)
Group: core-security → javascript-core-security
This is a rehash of Bug 1342261 it seems and security assessment there remains the same. I'm trying to figure out how to untangle this from my work in Bug 1438086 which is making this bug more obvious in my local testing.
I see that this issue is covered by Bug 1353351. The argument made there is that since the global is well help, the compartment won't be blown away and same-zone edges (which these are) don't actually break the GC. Will move discussion to there.
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(tcampbell)
Resolution: --- → DUPLICATE
Duplicate of bug: 1353351
Reopening since it makes more sense to fix the IC here rather than allow edges in Bug 1353351.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Assignee: tcampbell → mgaudet
Status: REOPENED → ASSIGNED
When the property isn't on the object, and we are doing a cross-compartment
wrapper load, we can get into a situation where a cross-compartment pointer is
baked into the IC slot field.

This disallows creating an IC in cases where the property is found further up
the prototype chain, by restricting CCW ICs to those where the property is
not found (holder == nullptr) or the holder is the object we have directly.
This patch unshares some of the property access code between regular and
cross-compartment cases, allowing us to use a series of shape-guards on the
protochain instead of the previous shape-teleporting checks, that would
end up baking in a cross-compartment pointer.

This is a more complete fix than the patch attached on comment 10, however
it is more complex, and I have yet to entirely convince myself of its 
correctness.
Comment on attachment 8967053 [details] [diff] [review]
Use shape guards on the prototype chain for cross-compartment ICs

Review of attachment 8967053 [details] [diff] [review]:
-----------------------------------------------------------------

I wonder if this can be unified with ShapeGuardProtoChain

::: js/src/jit/CacheIR.cpp
@@ +763,5 @@
> +
> +        // Can't successfully guard here as we would need to
> +        // save the proto (creating a cross-compartment pointer)
> +        if (obj->hasUncacheableProto())
> +            return false;

In general for CacheIR, we want to hoist these checks before emitting anything to |writer| so alternate ICs don't have garbage in them.
Duplicate of this bug: 1353351
Changing to sec-other as we did for Bug 1353351 since same-zoneness makes this not a security problem. This is still ugly and a footgun so we will fix it.

Matthew, be sure to look at the test case on the other bug too.
Keywords: sec-highsec-other
As discussed with Matthew, this is two related problems to be fixed together.

1) If property is not found, the UNCACHEABLEPROTO flag will cause us to add an cross-compartment edge to prototype object.
2) If property is found on prototype chain (but not object itself), we will add a cross-compartment edge to 'holder' object.
Summary: Uncacheable prototypes can cause non-wrapper cross-compartment edges in ICs → Avoid non-wrapper cross-compartment edges in ICs
This patch unshares some of the property access code between regular and
cross-compartment cases, allowing us to use a series of shape-guards on the
protochain instead of the previous shape-teleporting checks, that would
end up baking in a cross-compartment pointer.
Attachment #8967053 - Attachment is obsolete: true
Attachment #8967052 - Attachment is obsolete: true
Attachment #8967524 - Attachment is obsolete: true
This patch unshares some of the property access code between regular and
cross-compartment cases, allowing us to use a series of shape-guards on the
protochain instead of the previous shape-teleporting checks, that would
end up baking in a cross-compartment pointer.
Attachment #8967712 - Flags: review?(tcampbell)
Attachment #8967051 - Attachment is obsolete: true
Comment on attachment 8967712 [details] [diff] [review]
Use shape guards on the prototype chain for cross-compartment ICs

Review of attachment 8967712 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. For maintainability concerns, I would like to fold some of this functions though.

::: js/src/jit/CacheIR.cpp
@@ +771,5 @@
> +            MOZ_ASSERT(!holderOrNull);
> +            return true;
> +        }
> +
> +        if (obj->hasUncacheableProto())

Sorry for sending you chasing your tail on this bug, but since we are using loadProto, we actually don't care about hasUncacheableProto anymore and we should avoid this check to avoid confusion.

@@ +808,5 @@
> +        }
> +    }
> +}
> +
> +static bool

|static void|

@@ +809,5 @@
> +    }
> +}
> +
> +static bool
> +EmitCrossCompartmentReadSlotGuard(CacheIRWriter& writer, JSObject* obj, JSObject* holder,

I have a strong preference to combining with EmitReadSlotGuard to avoid divergence in future. Feel free to use a templated form if you want (<typename MaybeCrossCompartment = false> perhaps?).

The explicit ShapeGuardProtoChainForCrossCompartmentHolder helper makes sense still.

@@ +817,5 @@
> +    TestMatchingReceiver(writer, obj, objId, &expandoId);
> +
> +    if (obj != holder) {
> +        if (holder) {
> +            // Guard proto chain integrity.

// Guard proto chain integrity. We use a variant of guards that avoid baking in any cross-compartment object pointers.

@@ +884,4 @@
>  }
>  
>  static void
> +EmitCrossCompartmentReadSlotResult(CacheIRWriter& writer, JSObject* obj, JSObject* holder,

Probably fold this as well.
Attachment #8967712 - Flags: review?(tcampbell)
Oh right. At the end of this, we also need a same-compartment assert for CacheIR::loadObject.
Addressed feedback. I'm open to suggestions on improving the template code,
not my strongest suit.
Attachment #8967877 - Flags: feedback?(tcampbell)
Attachment #8967712 - Attachment is obsolete: true
Comment on attachment 8967877 [details] [diff] [review]
Use shape guards on the prototype chain for cross-compartment ICs

Review of attachment 8967877 [details] [diff] [review]:
-----------------------------------------------------------------

Great. Comments about templates inline. You can fold the EmitReadSlotGuard (and probably should).

::: js/src/jit/CacheIR.cpp
@@ +787,5 @@
> +    }
> +}
> +
> +static void
> +EmitReadSlotGuardMaybeCrossCompartment(CacheIRWriter& writer, JSObject* obj, JSObject* holder,

Make this the templated EmitReadSlotGuard

@@ +795,5 @@
>      TestMatchingReceiver(writer, obj, objId, &expandoId);
>  
>      if (obj != holder) {
>          if (holder) {
> +            if (maybeCrossCompartment) {

if (MaybeCrossCompartment == SlotReadType::CrossCompartment)

@@ +823,4 @@
>      }
>  }
>  
> +enum SlotReadType {

|enum class SlotReadType|
(See "Scoped enumerations" on http://en.cppreference.com/w/cpp/language/enum if you are not familiar)

@@ +843,5 @@
> +{
> +    EmitReadSlotGuardMaybeCrossCompartment(writer, obj, holder, objId, holderId, true);
> +}
> +
> +template <SlotReadType MaybeCrossCompartment = Normal>

Good
Attachment #8967877 - Flags: feedback?(tcampbell) → feedback+
This patch unshares some of the property access code between regular and
cross-compartment cases, allowing us to use a series of shape-guards on the
protochain instead of the previous shape-teleporting checks, that would
end up baking in a cross-compartment pointer.
Attachment #8970355 - Flags: review?(tcampbell)
This patch is probably ready to run past try (after anonymization) and then land. Not sure about if we want to hold back the test cases on this one.
Comment on attachment 8970355 [details] [diff] [review]
Use shape guards on the prototype chain for cross-compartment ICs

Review of attachment 8970355 [details] [diff] [review]:
-----------------------------------------------------------------

Great!
Attachment #8970355 - Flags: review?(tcampbell) → review+
Matthew, can you add a patch to sprinkle some |assertSameCompartmentDebugOnly| calls in the following:

js::jit::CacheIRWriter::guardXrayExpandoShapeAndDefaultProto
js::jit::CacheIRWriter::guardProto
js::jit::CacheIRWriter::guardSpecificObject
js::jit::CacheIRWriter::guardCompartment
js::jit::CacheIRWriter::loadObject

Some of the comments might need to be tweaked to be clear that wrappers are held if cross-compartment.
Flags: needinfo?(mgaudet)
Attachment #8967877 - Attachment is obsolete: true
Flags: needinfo?(mgaudet)
Attachment #8967765 - Attachment is obsolete: true
Attachment #8967711 - Attachment is obsolete: true
Comment on attachment 8970667 [details] [diff] [review]
Assert same compartment in a number of CacheIR helpers

Added the requested assertion. 

Have try-built these two patches as well: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ebad45a112a1d1ed913030e4c4bb8de08a71711 (cgc failure didn't reproduce on retrigger) 

One thing I found: If you assertSameCompartment(global) inside of guardCompartment [1], there are some test failures. I don't know enough about globals to say if that's expected? 


[1]: https://searchfox.org/mozilla-central/source/js/src/jit/CacheIR.h#678
Attachment #8970667 - Flags: review?(tcampbell)
Comment on attachment 8970667 [details] [diff] [review]
Assert same compartment in a number of CacheIR helpers

Review of attachment 8970667 [details] [diff] [review]:
-----------------------------------------------------------------

Great :)
Attachment #8970667 - Flags: review?(tcampbell) → review+
(In reply to Matthew Gaudet (he/him) [:mgaudet] (UTC-5) from comment #29)

> One thing I found: If you assertSameCompartment(global) inside of
> guardCompartment [1], there are some test failures. I don't know enough
> about globals to say if that's expected? 

Seems worth investigating..
As we discussed on IRC, [1] seems broken since it is finding global of wrapper, not of target so the whole mechanism of using a global to ensure compartment pointer is not stale is a lie. It also may be worthwhile to scope the AutoCompartment to smaller blocks that are easier to reason about.

[1] https://searchfox.org/mozilla-central/rev/36dec78aecc40539ecc8d78e91612e38810f963c/js/src/jit/CacheIR.cpp#1044-1045
Attached patch Correct global wrapping (obsolete) — Splinter Review
Attachment #8971014 - Flags: review?(tcampbell)
Attached patch Narrow compartment-change scope (obsolete) — Splinter Review
Attachment #8971015 - Flags: review?(tcampbell)
Attachment #8971015 - Attachment is obsolete: true
Attachment #8971015 - Flags: review?(tcampbell)
Attachment #8971020 - Flags: review?(tcampbell)
Comment on attachment 8971020 [details] [diff] [review]
Narrow compartment-change scope

I think the AutoComparment should be scoped to any calls that use cross-compartment objects, not just the minimal region for asserts. I'm thinking roughly:

 https://searchfox.org/mozilla-central/rev/36dec78aecc40539ecc8d78e91612e38810f963c/js/src/jit/CacheIR.cpp#1050-1076,1097-1098
Attachment #8971020 - Flags: review?(tcampbell)
Comment on attachment 8971014 [details] [diff] [review]
Correct global wrapping

Review of attachment 8971014 [details] [diff] [review]:
-----------------------------------------------------------------

Good find here! Let's also rename these things to something like wrappedTargetGlobal. Clearly the current naming hasn't helped clarity enough.

I'd also like this combined with the other AutoCompartment patch so clearing review for now.
Attachment #8971014 - Flags: review?(tcampbell)
Attachment #8971014 - Attachment is obsolete: true
Attachment #8970667 - Attachment is obsolete: true
Attachment #8971020 - Attachment is obsolete: true
Comment on attachment 8971316 [details] [diff] [review]
Correct compartment usage in CCW IC, and use correct global

Review of attachment 8971316 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/CacheIR.cpp
@@ +1093,4 @@
>      RootedShape shape(cx_);
>      RootedNativeObject holder(cx_);
> +    {
> +        // Switch compartment while manipulating raw objects

// Enter compartment of target since some checks have side-effects such as de-lazifying type info.

@@ +1098,5 @@
> +
> +
> +        // The first CCW for iframes is almost always wrapping another WindowProxy
> +        // so we optimize for that case as well.
> +        isWindowProxy= IsWindowProxy(unwrapped);

Nit: ...proxy = Is..

@@ +1145,5 @@
>          unwrappedId = writer.loadWrapperTarget(wrapperTargetId);
>      }
>  
> +    {
> +        AutoCompartment ac(cx_, unwrapped);

Sorry, I've gone back-and-forth on this and I think we don't actually want to do this so that we actually catch the original issue.
Attachment #8971316 - Flags: review?(tcampbell)
Nits addressed :)
Attachment #8971334 - Flags: review?(tcampbell)
Attachment #8971316 - Attachment is obsolete: true
Moving to sec-high since this cleanups are beginning to indicate that we may be susceptible to compartment confusion.

The stored global in IC is a wrapper so we will not currently detect the target global is nuked and as a result the baked in JSCompartment pointer may be reused to avoid security checks.
Keywords: sec-othersec-high
Looking at this again, [1] ensures there are no security check concerns. The remaining issue is only that the JSCompartment* may still be possibly nuked, at which point the IC is just storing a bare JSCompartment* with no indication it is still alive.

Some alternatives:
1) Dynamically check that the stored wrapper isn't a DeadProxyObject
2) Throw out all JIT code in Zone when nuking a compartment.
3) Is it possible to check Zone instead of compartment, or are there cases where some of the compartments are not valid here.

[1] https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/js/src/jit/CacheIR.cpp#1029,1080
Attachment #8971317 - Flags: review?(tcampbell) → review+
Comment on attachment 8971334 [details] [diff] [review]
Correct compartment usage in CCW IC, and use correct global

Review of attachment 8971334 [details] [diff] [review]:
-----------------------------------------------------------------

The AutoCompartment stuff seems sensible now. The nuked global case still seems quite suspect to me, so let's get some more input from others next week.
Attachment #8971334 - Flags: review?(tcampbell)
Attachment #8971997 - Flags: feedback?(tcampbell)
Comment on attachment 8971997 [details] [diff] [review]
Verify global wrapper not nuked

Jan, can you weigh in on this? I'd like to get more eyes on it since we keep missing details on the CCW IC.
Attachment #8971997 - Flags: feedback?(jdemooij)
Comment on attachment 8971997 [details] [diff] [review]
Verify global wrapper not nuked

Review of attachment 8971997 [details] [diff] [review]:
-----------------------------------------------------------------

This seems like it should solve our concerns.

The other approach would be to set a flag (on the Zone/JitZone?) that tells us to wipe out jit code when a compartment is nuked. This might be better in a follow-up that we can work on mozilla-central instead.
Attachment #8971997 - Flags: feedback?(tcampbell) → feedback+
Comment on attachment 8971997 [details] [diff] [review]
Verify global wrapper not nuked

Review of attachment 8971997 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense, good find.

It would be good to write a test for this (that we can land later) and verify with some logging that we hit the branch and it's working as expected - something like:

  var g = newGlobal();
  ... add an IC for some property of |g| ...
  g.eval("nukeAllCCWs()"); // or maybe nukeWrapper(g) works too
  ... IC GuardCompartment check should now fail when passed a wrapper...

::: js/src/jit/BaselineCacheIRCompiler.cpp
@@ +314,5 @@
> +    // it is pre-requisite for doing the compartment check.
> +    Address globalWrapper(stubAddress(reader.stubOffset()));
> +    masm.loadPtr(globalWrapper, scratch);
> +    Address handlerAddr(scratch, ProxyObject::offsetOfHandler());
> +    masm.branchPtr(Assembler::Equal, handlerAddr, ImmPtr(&DeadObjectProxy::singleton) , failure->label());

Uber nit: no space between ")" and "," and in the Ion code.
Attachment #8971997 - Flags: feedback?(jdemooij) → feedback+
This patch was used to verify, along with the test case in Comment 49, that the global check is in fact important. 

$ ./dist/bin/js --baseline-eager --no-ion ../jit-test/tests/cacheir/bug1438556.js
Bad Compartment
Nuking CCWs
Bad Global
Bad Global
Bad Global
Bad Global
Bad Global
Bad Global
Bad Global
(In reply to Matthew Gaudet (he/him) [:mgaudet] (UTC-5) from comment #50)
> This patch was used to verify, along with the test case in Comment 49, that
> the global check is in fact important. 

Great work validating this.
Attached patch Verify global wrapper not nuked (obsolete) — Splinter Review
Nits from Jan addressed. At this point, I think it's ready to go, and assuming r+ 
we'll need to talk about how to land this and where we want backports to.
Attachment #8972688 - Flags: review?(tcampbell)
Attachment #8971997 - Attachment is obsolete: true
Comment on attachment 8972688 [details] [diff] [review]
Verify global wrapper not nuked

Review of attachment 8972688 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: js/src/jit/IonCacheIRCompiler.cpp
@@ +738,1 @@
>      JSCompartment* compartment = compartmentStubField(reader.stubOffset());

Throw in an assert that the wrapper handler is CrossCompartmentWrapper::singleton.
Attachment #8972688 - Flags: review?(tcampbell) → review+
Comment on attachment 8970355 [details] [diff] [review]
Use shape guards on the prototype chain for cross-compartment ICs

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

It would be difficult. This patch fixes an issue that is by itself not exploitable we believe.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The comments on this patch describe the issue. 

Which older supported branches are affected by this flaw?

This was introduced in 54, so only beta and 60 appear to be covered. 

If not all supported branches, which bug introduced the flaw?

Bug 1319087. 

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I do not have backports prepared right now, but believe they won't be too hard. 

How likely is this patch to cause regressions; how much testing does it need?

It's unlikely that this patch will cause regressions, but it does need to run through at least a full try run.
Attachment #8970355 - Flags: sec-approval?
Comment on attachment 8971317 [details] [diff] [review]
Assert same compartment in a number of CacheIR helpers

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

It would be difficult. This patch simply tightens our assertions about things we believe to be true. 

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The comments on this patch describe the issue. 

Which older supported branches are affected by this flaw?

This was introduced in 54, so only beta and 60 appear to be covered. 

If not all supported branches, which bug introduced the flaw?

Bug 1319087. 

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I do not have backports prepared right now, but believe they won't be too hard. 

How likely is this patch to cause regressions; how much testing does it need?

It's unlikely that this patch will cause regressions, but it does need to run through at least a full try run.
Comment on attachment 8971317 [details] [diff] [review]
Assert same compartment in a number of CacheIR helpers

(See above comment)
Attachment #8971317 - Flags: sec-approval?
Comment on attachment 8972688 [details] [diff] [review]
Verify global wrapper not nuked

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

It would take effort, as the concern being addressed with this patch is the potential for compartment confusion after a cross compartment wrapper is nuked. 

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The comment perhaps highlights there's an issue, though not clearly what the concern is.

Which older supported branches are affected by this flaw?

Beta, 60. 

If not all supported branches, which bug introduced the flaw?

Bug 1319087

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I do not have backports prepared yet, but I believe they will be easy to do. 

How likely is this patch to cause regressions; how much testing does it need?

It's very unlikely this patch will cause regressions, but I would like to see this patch go through a try build.
Attachment #8972688 - Flags: sec-approval?
Sec-approval+ for checkin on May 22, two weeks into the new cycle (not before). 

Once it is on trunk, we should nominate patches for Beta and ESR60 branches so we can fix it everywhere (for the parts that affect multiple branches).
Attachment #8970355 - Flags: sec-approval? → sec-approval+
Attachment #8971317 - Flags: sec-approval? → sec-approval+
Attachment #8972688 - Flags: sec-approval? → sec-approval+
Comment on attachment 8971334 [details] [diff] [review]
Correct compartment usage in CCW IC, and use correct global

Requesting review on this; which has been looked at before with nits addressed. Apparently fell through the r? cracks.
Attachment #8971334 - Flags: review?(tcampbell)
Attachment #8972686 - Attachment is patch: false
Attachment #8972683 - Attachment is patch: false
Attached patch ESR-60-Rollup.patch (obsolete) — Splinter Review
Backported rollup patch for ESR60.
Comment on attachment 8971334 [details] [diff] [review]
Correct compartment usage in CCW IC, and use correct global

Review of attachment 8971334 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. We've answered the question about compartment nuking in the final patch.
Attachment #8971334 - Flags: review?(tcampbell) → review+
Attached file Testcase (updated)
Matthew, it looks like the property-not-found case runs into trouble still.

I'm thinking the best option here is to check for uncacheable prototypes and not attach CCWs at all. Doing the dynamic checks is a bit annoying and we need to also check the final proto link is null. If easier, disallow uncacheable prototypes in the found-property case as well since I can't imagine it comes up in practice.

(This is an alternate variation of failure on the sec-other portion of this bug. We should fix it at same time since it is related to the patch queue but it isn't a real sec concern).
Attachment #8951279 - Attachment is obsolete: true
Flags: needinfo?(mgaudet)
...If there's an uncachable proto on the prototype chain.
Attachment #8974713 - Flags: review?(tcampbell)
Flags: needinfo?(mgaudet)
Comment on attachment 8974713 [details] [diff] [review]
Don't attempt to attach a Cross Compartment IC for the missing element case

Review of attachment 8974713 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks. Let's fold this into "Use shape guards on the prototype chain for cross-compartment ICs" so we don't get too confused about all the pieces.

::: js/src/jit/CacheIR.cpp
@@ +1142,5 @@
>                  else
>                      preliminaryObjectAction_ = PreliminaryObjectAction::Unlink;
>              }
> +        } else {
> +            // We cannot generate shape guards for the not-found

// UNCACHEABLE_PROTO may result in guards against specific (cross-compartment) prototype objects, so don't try to attach IC if we see the flag at all.
Attachment #8974713 - Flags: review?(tcampbell) → review+
I'm also thinking it might be best to fold the two patches for the global wrapper into one since they are heavily coupled.
This patch unshares some of the property access code between regular and
cross-compartment cases, allowing us to use a series of shape-guards on the
protochain instead of the previous shape-teleporting checks, that would
end up baking in a cross-compartment pointer.

This also attempts to clarify the compartment membership situation while
doing the cross-compartment IC.
Attachment #8970355 - Attachment is obsolete: true
Attachment #8972688 - Attachment is obsolete: true
Attachment #8971317 - Attachment is obsolete: true
Comment on attachment 8975460 [details] [diff] [review]
[Part 1] Use shape guards on the prototype chain for cross-compartment ICs

This patch is a squash of previously approved patches (+ one extra patch relative to previous approval). Carrying Ted's previous r+

[Security approval request comment]

See Comment 54 for previous sec-approval justification, and Comment 58 for previous approval.
Attachment #8975460 - Flags: sec-approval?
Attachment #8975460 - Flags: review+
Comment on attachment 8975461 [details] [diff] [review]
[Part 2] Verify global wrapper not nuked

This patch is a squash of previously approved patches. Carrying Ted's previous r+

[Security approval request comment]

See Comment 55 for previous sec-approval justification, and Comment 58 for previous approval.
Attachment #8975461 - Flags: sec-approval?
Attachment #8975461 - Flags: review+
Comment on attachment 8975462 [details] [diff] [review]
[Part 3] Assert same compartment in a number of CacheIR helpers

This patch is a squash of previously approved patches. Carrying Ted's previous r+

[Security approval request comment]

See Comment 56 for previous sec-approval justification, and Comment 58 for previous approval.
Attachment #8975462 - Flags: sec-approval?
Attachment #8975462 - Flags: review+
Attachment #8974713 - Attachment is obsolete: true
Attachment #8974461 - Attachment is obsolete: true
Attachment #8971334 - Attachment is obsolete: true
Updated ESR 60 Rollup patch
These patches are due to land on May 22, but I will be on PTO. Ted Campbell has agreed to checkin and be the contact for them while I am gone.
Attachment #8975460 - Flags: sec-approval? → sec-approval+
Attachment #8975462 - Flags: sec-approval? → sec-approval+
Attachment #8975461 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/6ffbdf27a930
https://hg.mozilla.org/mozilla-central/rev/0302149bc169
https://hg.mozilla.org/mozilla-central/rev/2aec5918c12a
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Ted, can you please request Beta/ESR60 approval on the relevant patches when you get a chance? I assume that the original 3 patches as-attached are what need to land on Beta given your comment about rebasing for trunk.
Flags: needinfo?(tcampbell)
Whiteboard: [checkin on 5/22]
Attachment #8975467 - Attachment is patch: true
Attachment #8975467 - Attachment mime type: text/x-patch → text/plain
Comment on attachment 8975467 [details] [diff] [review]
ESR-60-Rollup.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
 sec-high bug
User impact if declined:
 Use-after-free by a motivated attacked.
Fix Landed on Version:
 Fixed in FF62
Risk to taking this patch (and alternatives if risky): 
 We've gone over this a number of times now and think we understand the problem reasonably well now, but it has taken multiple iterations to get to this point.
String or UUID changes made by this patch: 
 None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8975467 - Flags: approval-mozilla-esr60?
Comment on attachment 8975460 [details] [diff] [review]
[Part 1] Use shape guards on the prototype chain for cross-compartment ICs

Approval Request Comment
[Feature/Bug causing the regression]:
 Bug 1319087
[User impact if declined]:
 Sec-high. Use-after-free to a determined attacker.
[Is this code covered by automated tests?]:
 Landed on nightly with original tests. New test cases not landed do to sec-high.
[Has the fix been verified in Nightly?]:
 Yes
[Needs manual test from QE? If yes, steps to reproduce]: 
 No
[List of other uplifts needed for the feature/fix]:
 Part 1, 2, 3 of this bug.
[Why is the change risky/not risky?]:
 We've gone over this a number of times now and think we understand the problem reasonably well now, but it has taken multiple iterations to get to this point.
[String changes made/needed]:
 None
Flags: needinfo?(tcampbell)
Attachment #8975460 - Flags: approval-mozilla-beta?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #76)
> Ted, can you please request Beta/ESR60 approval on the relevant patches when
> you get a chance? I assume that the original 3 patches as-attached are what
> need to land on Beta given your comment about rebasing for trunk.

Yes, original 3 patches merge fine on Beta61.
Comment on attachment 8975460 [details] [diff] [review]
[Part 1] Use shape guards on the prototype chain for cross-compartment ICs

Fix for a sec-high. Approved for 61.0b9 and ESR 60.1.
Attachment #8975460 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8975467 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Whiteboard: [adv-main61+][adv-esr60.1+]
Flags: qe-verify-
Whiteboard: [adv-main61+][adv-esr60.1+] → [adv-main61+][adv-esr60.1+][post-critsmash-triage]
Depends on: 1483183
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.