Closed Bug 1388045 Opened 7 years ago Closed 7 years ago

Branch Pruning suggests to optimize away observable operands.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 + fixed
firefox57 + fixed

People

(Reporter: nbp, Assigned: nbp)

References

Details

(Keywords: sec-moderate, Whiteboard: [adv-main56+][post-critsmash-triage])

Attachments

(1 file, 1 obsolete file)

Branch Pruning checks if operands are observable or not, and if not we might not flag these operands as having removed uses.

The problem here comes from the fact that we should check the slot corresponding to the frame encoded by the resume point, and the aliasing made in the second loop of Bug 1342016 breaks this rule by using the compile info of the inner most frame for flagging the outer-most one.

as highlighted here: http://searchfox.org/mozilla-central/rev/b52285fffc13f36eca6b47de735d4e4403b3859e/js/src/jit/IonAnalysis.cpp#198,228,232

This can lead to bugs where the operands are replaced by OptimizedOut magic values, instead of objects, or arguments which might be expected. (function / this / …)
Assignee: nobody → nicolas.b.pierron
Comment on attachment 8894503 [details] [diff] [review]
Branch Pruning: Check the compile info associated with the resume point.

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

Good catch. This is a real problem. I would suggest adding the following assertion above the other |info.isObservableSlot()|:

> MOZ_ASSERT(&(rp->block()->info()) == &info);

::: js/src/jit/IonAnalysis.cpp
@@ +224,5 @@
>          if (mir->shouldCancel("FlagAllOperandsAsHavingRemovedUses loop 2"))
>              return false;
>  
>          for (size_t i = 0, e = rp->numOperands(); i < e; i++) {
> +            if (rp->isObservableOperand(i))

Instead of using `isObservableOperand()`, let's store `rp->block()->info()` into a temporary variable outside of the inner loop, and then use the same `isObservableSlot(i)`.

Alternatively, it would be acceptable to move `isObservableOperand()` out of MIR.cpp and into MIR.h, so it can be inlined.

Actually, that's probably not such a bad idea. If you wanted to land this bug without painting a target on the issue, you could just phrase it as a patch making the `isObserableOperand()` variants and `isRecoverableOperand()` inlineable.
Attachment #8894503 - Flags: review?(sstangl) → review+
Comment on attachment 8894965 [details] [diff] [review]
Branch Pruning: Check the compile info associated with the resume point.

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

Getting all branch pruning requirements should be easy.
Getting all the rest of the compiler requirements might be hard.

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

Yes, which is why I am asking for approval, even if I rated it as sec-moderate.

> Which older supported branches are affected by this flaw?

Since Firefox 55.

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

Bug 1342016

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

The same patch should apply cleanly, or this should be an easy fix.

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

In the worst case, this would be a performance regession on benchmark, as was attempted to be fixed by Bug 1342016.  If any, it would be fixed by Bug 1377710.
Attachment #8894965 - Flags: sec-approval?
Let's check this into trunk on 8/28, three weeks into the current cycle. At that point, we'll want a beta patch for 56 nominated as well to go in after this lands on trunk.
Whiteboard: [checkin on 8/28]
Attachment #8894965 - Flags: sec-approval? → sec-approval+
Per comment #5, track 56+/57+.
Attachment #8894503 - Attachment is obsolete: true
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bf6d4f8f985
Branch Pruning: Check the compile info associated with the resume point. r=sstangl
Whiteboard: [checkin on 8/28]
https://hg.mozilla.org/mozilla-central/rev/2bf6d4f8f985

Please nominate this patch for Beta approval when you get a chance. It grafts cleanly.
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(nicolas.b.pierron)
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Group: javascript-core-security → core-security-release
Comment on attachment 8894965 [details] [diff] [review]
Branch Pruning: Check the compile info associated with the resume point.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1342016
[User impact if declined]: Optimized out values where an object is expected.
[Is this code covered by automated tests?]: Yes.
[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]: {}
[Is the change risky?]: Unlikely.
[Why is the change risky/not risky?]: The change is small, and can be proof read again and again, and … again.
[String changes made/needed]: None
Flags: needinfo?(nicolas.b.pierron)
Attachment #8894965 - Flags: approval-mozilla-beta?
Comment on attachment 8894965 [details] [diff] [review]
Branch Pruning: Check the compile info associated with the resume point.

Fix a security issue. Beta56+.
Attachment #8894965 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [adv-main56+]
Flags: qe-verify-
Whiteboard: [adv-main56+] → [adv-main56+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.