Closed Bug 994444 Opened 7 years ago Closed 7 years ago

Fix !resumeAfter for JSOP_LOOPENTRY in Ion-to-Baseline bailout

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla31
Tracking Status
firefox28 - wontfix
firefox29 - wontfix
firefox30 - wontfix
firefox31 - verified
firefox-esr24 - wontfix
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- wontfix
b2g-v1.3 --- wontfix
b2g-v1.3T --- wontfix
b2g-v1.4 --- wontfix
b2g-v2.0 --- fixed

People

(Reporter: shu, Assigned: shu)

Details

(Keywords: csectype-dos, sec-low, Whiteboard: [adv-main31+])

Attachments

(1 file, 1 obsolete file)

We try to find the next non-LOOPENTRY/NOP/LOOPHEAD/GOTO pc when bailing out on a !resumeAfter loopentry. If the loop body is empty, we get caught in an infinite loop.
We need to do the general cycle detection instead of just pattern matching on empty loops because we can nest them:

js> dis(function() { for(;;) for(;;); })
flags: LAMBDA
loc     op
-----   --
main:
00000:  nop
00001:  loophead
00002:  loopentry 129
00004:  nop
00005:  loophead
00006:  loopentry 130
00008:  goto 5 (-3)
00013:  goto 1 (-12)
00018:  retrval
Assignee: nobody → shu
Status: NEW → ASSIGNED
Attachment #8404381 - Flags: review?(kvijayan)
Oops, the comment should read "Cycles can cause ..."
Also, the test should be run with --ion-parallel-compile=off --ion-eager
Group: core-security, javascript-core-security
Comment on attachment 8404381 [details] [diff] [review]
0001-Fix-resumeAfter-case-in-baseline-bailout.patch

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

This patch needs to be backported to all versions where Ion is enabled by default, which basically means everything except b2g18 and b2g1.1.

::: js/src/jit/BaselineBailouts.cpp
@@ +792,5 @@
>      size_t endOfBaselineJSFrameStack = builder.framePushed();
>  
>      // If we are resuming at a LOOPENTRY op, resume at the next op to avoid
>      // a bailout -> enter Ion -> bailout loop with --ion-eager. See also
>      // ThunkToInterpreter.

Kannan, would we still have this issue with Baseline?
Do we still need this hack that I added back in the day of ThunkToInterpreter?

@@ +809,5 @@
> +            // Advance the pc.
> +            jsbytecode *oldPc = pc;
> +            pc = GetNextNonLoopEntryPc(pc);
> +            if (oldPc == pc)
> +                break;

This condition is not needed, as the following condition will catch this case if this is a cycle of 1 instruction.

@@ +815,5 @@
> +            // Advance fasterPc twice as fast.
> +            fasterPc = GetNextNonLoopEntryPc(GetNextNonLoopEntryPc(fasterPc));
> +
> +            // Break on cycles.
> +            if (fasterPc == pc)

nit: This is either breaking on cycles or at the end of the goto sequence.
Attachment #8404381 - Flags: review+
This issue can cause a DOS of Firefox if such malicious code is executed.  The interruption callback is not even honored (no slow script dialog), which means that there is no way to get back to the main thread.
Keywords: csectype-dos
Not sure why I didn't catch this. My DOM fuzzer doesn't *intentionally* create infinite loops, but that usually doesn't stop it from finding such simple bugs.
(In reply to Jesse Ruderman from comment #7)
> Not sure why I didn't catch this. My DOM fuzzer doesn't *intentionally*
> create infinite loops, but that usually doesn't stop it from finding such
> simple bugs.

The loops have to be infinite *and* have an empty body.
Attached patch fixSplinter Review
Addressed review comments.
Comment on attachment 8404906 [details] [diff] [review]
fix

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

Carrying r=nbp
Attachment #8404906 - Flags: review+
Attachment #8404381 - Attachment is obsolete: true
Attachment #8404381 - Flags: review?(kvijayan)
Comment on attachment 8404906 [details] [diff] [review]
fix

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

A DOS patch can be constructed pretty easily.

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

Comments and tests give you a DOS exploit, yes. The test can be removed, but I feel the comment is important for posterity.

Which older supported branches are affected by this flaw?

See tracking flags.

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

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

I don't, but patch is small and probably applies cleanly.

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

Unlikely to cause regressions.
Attachment #8404906 - Flags: sec-approval?
Comment on attachment 8404906 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 848679
User impact if declined: Possible DOS attacks
Testing completed (on m-c, etc.): None yet, waiting on sec-approval
Risk to taking this patch (and alternatives if risky): None
String or IDL/UUID changes made by this patch: None
Attachment #8404906 - Flags: approval-mozilla-release?
Attachment #8404906 - Flags: approval-mozilla-esr24?
Attachment #8404906 - Flags: approval-mozilla-beta?
Attachment #8404906 - Flags: approval-mozilla-aurora?
(In reply to Shu-yu Guo [:shu] from comment #11)
> [Security approval request comment]
> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?
> 
> Comments and tests give you a DOS exploit, yes. The test can be removed, but
> I feel the comment is important for posterity.

I think we can remove the comment as well as the test case from the original patch and land both the comment and the test case later.  If you still want some comment for posterity, we can replace it by

  // See Bug 994444 for a better comment.
(In reply to Nicolas B. Pierron [:nbp] from comment #13)
> (In reply to Shu-yu Guo [:shu] from comment #11)
> > [Security approval request comment]
> > Do comments in the patch, the check-in comment, or tests included in the
> > patch paint a bulls-eye on the security problem?
> > 
> > Comments and tests give you a DOS exploit, yes. The test can be removed, but
> > I feel the comment is important for posterity.
> 
> I think we can remove the comment as well as the test case from the original
> patch and land both the comment and the test case later.  If you still want
> some comment for posterity, we can replace it by
> 
>   // See Bug 994444 for a better comment.

Okay, I'll remove the test and comment and replace with a ref to this bug.
(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> Kannan, would we still have this issue with Baseline?
> Do we still need this hack that I added back in the day of
> ThunkToInterpreter?

Yes, I'm reasonably sure we do still need this.
This seems like a sec-low. Is there a reason that this DOS is higher than sec-low? If not, I'll clear the sec-approval? as you don't need approval to check in a low. That said, we're unlikely to take it on ESR24 or branches if it is low as well. It would ride the trains.
(In reply to Al Billings [:abillings] from comment #16)
> This seems like a sec-low. Is there a reason that this DOS is higher than
> sec-low? If not, I'll clear the sec-approval? as you don't need approval to
> check in a low. That said, we're unlikely to take it on ESR24 or branches if
> it is low as well. It would ride the trains.

I didn't close it, but I agree more or less. In fact, I left the bug open in the beginning. Aren't there a million ways to DOS the browser already?
Comment on attachment 8404906 [details] [diff] [review]
fix

Taking in account Al comment #16, we are going to let the patch ride the train.
FYI, We no longer accept "None" as a risk evaluation.
Attachment #8404906 - Flags: approval-mozilla-release?
Attachment #8404906 - Flags: approval-mozilla-release-
Attachment #8404906 - Flags: approval-mozilla-esr24?
Attachment #8404906 - Flags: approval-mozilla-esr24-
Attachment #8404906 - Flags: approval-mozilla-beta?
Attachment #8404906 - Flags: approval-mozilla-beta-
Attachment #8404906 - Flags: approval-mozilla-aurora?
Attachment #8404906 - Flags: approval-mozilla-aurora-
Comment on attachment 8404906 [details] [diff] [review]
fix

Clearing sec-approval?. You can check this into trunk whenever you want.
Attachment #8404906 - Flags: sec-approval?
landed on central https://hg.mozilla.org/mozilla-central/rev/a71e790f9652
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Group: javascript-core-security
Keywords: sec-low
Whiteboard: [adv-main31+]
Confirmed DoS on Fx31 from 2014-04-08.
Verified fixed on Fx31 from 2014-04-16.
Status: RESOLVED → VERIFIED
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.