Closed
Bug 909494
Opened 10 years ago
Closed 10 years ago
Crash with SIGTRAP with ParallelArray and --ion-check-range-analysis
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla26
Tracking | Status | |
---|---|---|
firefox23 | --- | wontfix |
firefox24 | + | fixed |
firefox25 | + | fixed |
firefox26 | + | fixed |
firefox-esr17 | --- | unaffected |
b2g18 | --- | unaffected |
People
(Reporter: decoder, Assigned: sunfish)
References
Details
(4 keywords, Whiteboard: [jsbugmon:][adv-main24+])
Attachments
(4 files)
2.32 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
1.20 KB,
patch
|
bajaj
:
approval-mozilla-aurora+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
913 bytes,
patch
|
bajaj
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1000 bytes,
patch
|
abillings
:
approval-mozilla-release-
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision 4887845b1142 (threadsafe build, run with --fuzzing-safe --ion-check-range-analysis): assertArraySeqParResultsEq(range(0, 1024), "filter", function(e, i) { return (i % (1.1)) != 0; }); function range(n, m) { var result = []; for (var i = n; i < m; i++) result.push(i); return result; } function assertArraySeqParResultsEq(arr, op, func) { arr[op].apply(arr, [func]); }
Reporter | ||
Updated•10 years ago
|
Whiteboard: [jsbugmon:update,bisect]
Assignee | ||
Comment 1•10 years ago
|
||
This bug is very similar to bug 908867. In this case, it's the rhs being "decimal" that's causing the problem. Both will be fixed in the same patch, as soon as I get approval to submit it.
Reporter | ||
Updated•10 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
Reporter | ||
Comment 2•10 years ago
|
||
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Reporter | ||
Updated•10 years ago
|
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
Assignee | ||
Comment 4•10 years ago
|
||
The "foo" function in the testcase doesn't actually reproduce the bug yet, but it will once some other range analysis enhancements land.
Attachment #797333 -
Flags: review?(nicolas.b.pierron)
Updated•10 years ago
|
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Comment 6•10 years ago
|
||
Inherit tracking flags from Bug 908867.
status-firefox25:
--- → unaffected
status-firefox26:
--- → affected
status-firefox-esr17:
--- → unaffected
Comment 7•10 years ago
|
||
Comment on attachment 797333 [details] [diff] [review] bug908867.patch Review of attachment 797333 [details] [diff] [review]: ----------------------------------------------------------------- Be aware that this is changing a line introduce in Gecko 25, as part of Bug 897747, as opposed to what the tracking flags inherited from Bug 908867 mention. As you will have to backport you should land the test case separately once the patch landed on both inbound & aurora.
Attachment #797333 -
Flags: review?(nicolas.b.pierron) → review+
Updated•10 years ago
|
Comment 8•10 years ago
|
||
In fact, I think the bug might even come from Bug 841666 which accepted the specialization of MMod compute range with doubles without paying attention to the bounds of the range previously made on Integers.
Assignee | ||
Comment 9•10 years ago
|
||
Fix on trunk: https://hg.mozilla.org/integration/mozilla-inbound/rev/b6412dc95e7f Testcase on trunk: https://hg.mozilla.org/integration/mozilla-inbound/rev/c0e80bbe50fb
Comment 10•10 years ago
|
||
This should not have been landed with a security rating followed by sec-approval since this affects multiple branches. Please see https://wiki.mozilla.org/Security/Bug_Approval_Process for the process for security bugs. We definitely shouldn't have landed any tests at this time.
![]() |
||
Comment 11•10 years ago
|
||
Bug 841666 landed on Firefox 22, so this likely affects 23 and 24 too.
Assignee | ||
Comment 12•10 years ago
|
||
I apologize for my mistake. I will take more care to follow the process in the future. Attached is a patch which contains the fix for aurora. I'll backport the fix to firefox23 and firefox24 next and attach the patches to this bug. I'll wait for further instructions before checking anything in.
Comment 13•10 years ago
|
||
The risk seems to be that we could have a off-by-less-than-one which can be scale to any magnitude with a multiplication and later truncated to get an offset. Hopefully, (I really feel released by reading the code carefully) there is no way that we would remove a bound check around an array if we have a modulo as an operand of the array. So this bug cannot be sec-critical. Marty: Can you confirm that I read the code correctly?
Flags: needinfo?(mrosenberg)
Keywords: sec-high
Assignee | ||
Comment 14•10 years ago
|
||
The patch that introduced the --ion-check-range-analysis flag also included fixes for several pre-existing range analysis bugs, at least one of which goes back to 6688ede89a36 (bug 699883). Is it important to backport these other fixes as well?
Flags: needinfo?
Assignee | ||
Comment 15•10 years ago
|
||
This is the backport to mozilla-beta. I also fixed the "1-a" expression, to make the code work more like the code on trunk.
Assignee | ||
Comment 16•10 years ago
|
||
Here's the mozilla-release patch. It's the same as the mozilla-beta patch except that in mozilla-release, the "ion" directory has not yet been renamed to "jit", and some of the patch context is different.
Comment 17•10 years ago
|
||
(In reply to Dan Gohman [:sunfish] from comment #14) > The patch that introduced the --ion-check-range-analysis flag also included > fixes for several pre-existing range analysis bugs, at least one of which > goes back to 6688ede89a36 (bug 699883). Is it important to backport these > other fixes as well? I think we should consider any patch which manipulate operations which can be interpreted in a LinearSum, as they are used for removing bound checks. So probably any fix on range analysis of MAdd, MSub and MMul.
Assignee | ||
Comment 18•10 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #17) > (In reply to Dan Gohman [:sunfish] from comment #14) > > The patch that introduced the --ion-check-range-analysis flag also included > > fixes for several pre-existing range analysis bugs, at least one of which > > goes back to 6688ede89a36 (bug 699883). Is it important to backport these > > other fixes as well? > > I think we should consider any patch which manipulate operations which can > be interpreted in a LinearSum, as they are used for removing bound checks. > > So probably any fix on range analysis of MAdd, MSub and MMul. The ranges of MAdd, MSub, and MMul are computed in terms of the ranges of their operands, which could be any operator, so if the range analysis for those operators matters, doesn't that mean that any operator matters?
Comment 19•10 years ago
|
||
(In reply to Dan Gohman [:sunfish] from comment #18) > (In reply to Nicolas B. Pierron [:nbp] from comment #17) > > (In reply to Dan Gohman [:sunfish] from comment #14) > > > The patch that introduced the --ion-check-range-analysis flag also included > > > fixes for several pre-existing range analysis bugs, at least one of which > > > goes back to 6688ede89a36 (bug 699883). Is it important to backport these > > > other fixes as well? > > > > I think we should consider any patch which manipulate operations which can > > be interpreted in a LinearSum, as they are used for removing bound checks. > > > > So probably any fix on range analysis of MAdd, MSub and MMul. > > The ranges of MAdd, MSub, and MMul are computed in terms of the ranges of > their operands, which could be any operator, so if the range analysis for > those operators matters, doesn't that mean that any operator matters? The only case that I see so far would be to get through the lowering of MBoundsCheckLower [0] which is created by tryHoistBoundsCheck[1], which use jit::ExtractLinearSum [2], which expects to see MAdd, MSub and constants (not even MMul). [0] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/Lowering.cpp#l2001 http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.cpp#l2213 [1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/RangeAnalysis.cpp#l1530 [2] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonAnalysis.cpp#l1105
Assignee | ||
Comment 20•10 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #19) > The only case that I see so far would be to get through the lowering of > MBoundsCheckLower [0] which is created by tryHoistBoundsCheck[1], which use > jit::ExtractLinearSum [2], which expects to see MAdd, MSub and constants > (not even MMul). Ah, and it doesn't appear to actually use the range() from the MAdd or MSub; it just uses the symbolicLower() and symbolicUpper(). And the "range()->lower() < minimum_" check in MBoundsCheckLower::fallible() [0] is actually unreachable, because MBoundsCheckLower objects are never assigned a range. Tricky. However, mozilla-aurora for example has the MinMaxD optimization which disables a NaN check if range analysis says the operands aren't NaN [1]. Looking at that code with this bug in mind, it seems like it could theoretically be subverted to significant ends. [0] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.cpp#l2213 [1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/shared/CodeGenerator-x86-shared.cpp#l385 I'm not familiar with all of the ways range analysis is used, so I don't know that there aren't other problems too. Consequently, I went and created backported patches of the dynamic range checking (--ion-check-range-analysis) to mozilla-aurora, mozilla-beta, and mozilla-release, and patches backporting all the fixes for bugs that the checking turns up on jit_tests.py and jstests.py, for each of those trees. Let me know if you'd like me to attach these patches to this bug.
Comment 21•10 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #13) > The risk seems to be that we could have a off-by-less-than-one which can be > scale to any magnitude with a multiplication and later truncated to get an > offset. > > Hopefully, (I really feel released by reading the code carefully) there is > no way that we would remove a bound check around an array if we have a > modulo as an operand of the array. So this bug cannot be sec-critical. > > Marty: Can you confirm that I read the code correctly? Yes, we can get an arbitrary amount above the range that we've calculated, but we can only get a double range. Won't we need to have an explicit conversion to integer? I don't see any range calculation for a double -> integer conversion.
Comment 22•10 years ago
|
||
(In reply to Marty Rosenberg [:mjrosenb] from comment #21) > (In reply to Nicolas B. Pierron [:nbp] from comment #13) > > The risk seems to be that we could have a off-by-less-than-one which can be > > scale to any magnitude with a multiplication and later truncated to get an > > offset. > > > > Hopefully, (I really feel released by reading the code carefully) there is > > no way that we would remove a bound check around an array if we have a > > modulo as an operand of the array. So this bug cannot be sec-critical. > > > > Marty: Can you confirm that I read the code correctly? > > Yes, we can get an arbitrary amount above the range that we've calculated, > but we can only get a double range. Won't we need to have an explicit > conversion to integer? I don't see any range calculation for a double -> > integer conversion. Yes, we would need a truncate before writing to / reading from any array. My worry is about removing bounds check. If we are ever able to remove a bound check, then we can overflow or underflow the buffer (which would be a way easier exploit). One of the thing I was thinking of something like: arr[((-1 % 0.5) * 2 * 8) | 0] = 0x7fffffff; // map all memory in this array But hopefully this case is not supported as described in comment 19.
Comment 23•10 years ago
|
||
(In reply to Dan Gohman [:sunfish] from comment #15) > Created attachment 797580 [details] [diff] [review] > beta.patch If your patches are ready, you should ask for approval. Look at the attachement details.
Comment 24•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c0e80bbe50fb https://hg.mozilla.org/mozilla-central/rev/b6412dc95e7f
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Flags: needinfo?
QA Contact: general → sunfish
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Assignee | ||
Comment 25•10 years ago
|
||
Comment on attachment 797549 [details] [diff] [review] aurora.patch [Approval Request Comment] Bug caused by (feature/regressing bug #): 909494 User impact if declined: See bug 909494 Testing completed (on m-c, etc.): The patch is green on m-c. Risk to taking this patch (and alternatives if risky): It seems low. The patch is small and simple, and there are no known complications. String or IDL/UUID changes made by this patch: None.
Attachment #797549 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 26•10 years ago
|
||
Comment on attachment 797580 [details] [diff] [review] beta.patch [Approval Request Comment] Bug caused by (feature/regressing bug #): 909494 User impact if declined: See bug 909494 Testing completed (on m-c, etc.): The patch is green on m-c. Risk to taking this patch (and alternatives if risky): It seems low. The patch is small and simple, and there are no known complications. String or IDL/UUID changes made by this patch: None.
Attachment #797580 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 27•10 years ago
|
||
Comment on attachment 797586 [details] [diff] [review] release.patch [Approval Request Comment] Regression caused by (bug #): 909494 User impact if declined: See bug 909494 Testing completed (on m-c, etc.): The patch is green on m-c. Risk to taking this patch (and alternatives if risky): It seems low. The patch is small and simple, and there are no known complications. String or IDL/UUID changes made by this patch: None.
Attachment #797586 -
Flags: approval-mozilla-release?
Comment 28•10 years ago
|
||
This is a security bug that is rated "sec-high" that affects more than trunk. Before branches can be approved, someone needs to mark this sec-approval? and fill out the template. Just because this got checked in incorrectly without sec-approval doesn't mean we shouldn't still answer the questions. It helps us determine whether to take it on branches (though, since it is public, effectively, I'm inclined to do so).
Comment 29•10 years ago
|
||
Comment on attachment 797586 [details] [diff] [review] release.patch We're not going to take this on release branch. The question right now is Aurora and Beta.
Attachment #797586 -
Flags: approval-mozilla-release? → approval-mozilla-release-
Assignee | ||
Comment 30•10 years ago
|
||
Comment on attachment 797549 [details] [diff] [review] aurora.patch [Security approval request comment] How easily could an exploit be constructed based on the patch? It's not trivial. The available testcases don't trigger the bug in release builds, as they rely on the --ion-check-range-analysis debug flag. A few different people have looked at this bug and have not been able to construct a testcase which actually exploits the bug. That doesn't necessarily mean it's impossible, of course. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The bug itself is pretty obvious in the patch, but it takes some creativity to connect the bug with the right circumstances to make this security-relevant. Which older supported branches are affected by this flaw? Releases back to Firefox 23 contain the bug. If not all supported branches, which bug introduced the flaw? The flaw was introduced in bug 841666. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Yes. Attached to bug 909494 are backports for all affected branches. How likely is this patch to cause regressions; how much testing does it need? It is very unlikely to cause regressions. The patch is small and uncomplicated, and probably does not need any testing beyond what would be done for any patch.
Attachment #797549 -
Flags: sec-approval?
Assignee | ||
Comment 31•10 years ago
|
||
I created bug 911320 to track the patches which backport --ion-check-range-analysis and associated bug fixes to all the various branches.
Comment 32•10 years ago
|
||
Thanks. We should probably take this on Beta and Aurora.
Updated•10 years ago
|
Attachment #797549 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 33•10 years ago
|
||
Do you want me to check the patch into Beta and Aurora now, or should I wait for the branch approvals?
Flags: needinfo?(abillings)
Updated•10 years ago
|
Flags: needinfo?(release-mgmt)
Updated•10 years ago
|
Comment 35•10 years ago
|
||
Comment on attachment 797580 [details] [diff] [review] beta.patch Given the cat is out the bag already, and the looks safe and contained. Also discussed with :nbp and confirmed this area has enough tests, so approving on branches.If there is anyways QA can help in additional verification please add verifyme with more details.
Attachment #797580 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•10 years ago
|
Attachment #797549 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
Flags: needinfo?(release-mgmt)
Comment 36•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/9dd5f0e7f716 https://hg.mozilla.org/releases/mozilla-beta/rev/2a52a96cb0c6
Updated•10 years ago
|
Assignee: general → sunfish
Blocks: 841666
status-b2g18:
--- → unaffected
Keywords: regression
Whiteboard: [jsbugmon:] → [jsbugmon:][adv-main24+]
Updated•8 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•