Closed
Bug 994444
Opened 10 years ago
Closed 10 years ago
Fix !resumeAfter for JSOP_LOOPENTRY in Ion-to-Baseline bailout
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
VERIFIED
FIXED
mozilla31
People
(Reporter: shu, Assigned: shu)
Details
(Keywords: csectype-dos, sec-low, Whiteboard: [adv-main31+])
Attachments
(1 file, 1 obsolete file)
3.20 KB,
patch
|
shu
:
review+
Sylvestre
:
approval-mozilla-aurora-
Sylvestre
:
approval-mozilla-beta-
Sylvestre
:
approval-mozilla-release-
Sylvestre
:
approval-mozilla-esr24-
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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 | ||
Comment 2•10 years ago
|
||
Oops, the comment should read "Cycles can cause ..."
Assignee | ||
Comment 3•10 years ago
|
||
Also, the test should be run with --ion-parallel-compile=off --ion-eager
Updated•10 years ago
|
Group: core-security, javascript-core-security
Comment 4•10 years ago
|
||
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+
Comment 5•10 years ago
|
||
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.
status-b2g18:
--- → unaffected
status-b2g-v1.2:
--- → affected
status-b2g-v1.3:
--- → affected
status-b2g-v1.3T:
--- → affected
status-b2g-v1.4:
--- → affected
status-b2g-v2.0:
--- → affected
status-firefox28:
--- → affected
status-firefox29:
--- → affected
status-firefox30:
--- → affected
status-firefox31:
--- → affected
status-firefox-esr24:
--- → affected
tracking-b2g18:
--- → ?
tracking-b2g-v1.2:
--- → ?
tracking-b2g-v1.3:
--- → ?
tracking-firefox28:
--- → ?
tracking-firefox29:
--- → ?
tracking-firefox30:
--- → ?
tracking-firefox31:
--- → ?
tracking-firefox-esr24:
--- → ?
Updated•10 years ago
|
Keywords: csectype-dos
Comment 7•10 years ago
|
||
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.
Updated•10 years ago
|
Assignee | ||
Comment 8•10 years ago
|
||
(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.
Assignee | ||
Comment 9•10 years ago
|
||
Addressed review comments.
Assignee | ||
Comment 10•10 years ago
|
||
Comment on attachment 8404906 [details] [diff] [review] fix Review of attachment 8404906 [details] [diff] [review]: ----------------------------------------------------------------- Carrying r=nbp
Attachment #8404906 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8404381 -
Attachment is obsolete: true
Attachment #8404381 -
Flags: review?(kvijayan)
Assignee | ||
Comment 11•10 years ago
|
||
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?
Assignee | ||
Comment 12•10 years ago
|
||
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?
Comment 13•10 years ago
|
||
(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.
Assignee | ||
Comment 14•10 years ago
|
||
(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.
Comment 15•10 years ago
|
||
(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.
Comment 16•10 years ago
|
||
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.
Assignee | ||
Comment 17•10 years ago
|
||
(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 18•10 years ago
|
||
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 19•10 years ago
|
||
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?
Updated•10 years ago
|
Assignee | ||
Comment 20•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a71e790f9652
Comment 21•10 years ago
|
||
landed on central https://hg.mozilla.org/mozilla-central/rev/a71e790f9652
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Updated•10 years ago
|
status-b2g-v1.1hd:
--- → unaffected
tracking-b2g18:
? → ---
tracking-b2g-v1.2:
? → ---
tracking-b2g-v1.3:
? → ---
Updated•10 years ago
|
Group: javascript-core-security
Comment 22•10 years ago
|
||
Confirmed DoS on Fx31 from 2014-04-08. Verified fixed on Fx31 from 2014-04-16.
Status: RESOLVED → VERIFIED
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
•