gcslice(5); function f(y) { return (-8 < (Math.abs(Math.tan())) ? y : y) } f(0); f(0); asserts js debug shell on m-c changeset c049cb230d77 with --ion-eager --ion-check-range-analysis at Assertion failure: Double input should be equal or higher than Lowerbound., at jit/IonMacroAssembler.cpp s-s because range analysis bugs can sometimes be s-s, so locking pending analysis. My configure flags are: CC="clang -Qunused-arguments" AR=ar CXX="clang++ -Qunused-arguments" sh ./configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --with-ccache --enable-threadsafe <other NSPR options> === Tinderbox Build Bisection Results === Last "good" changeset has timestamp "20131209104147" and the hash "a79294ed7ebe" First "bad" changeset has timestamp "20131209134603" and the hash "de6ebd46e7d0" Likely regression window: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a79294ed7ebe&tochange=de6ebd46e7d0 Guessing related to bug 943303, so setting needinfo? from :sunfish.

=== Tinderbox Build Bisection Results === Last "good" changeset has timestamp "20131209051803" and the hash "d1e1af5e908d" First "bad" changeset has timestamp "20131209055701" and the hash "fb26d5645076" Likely regression window: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d1e1af5e908d&tochange=fb26d5645076 I re-ran this on mozilla-inbound, and it seems that Nicolas' patches there may also have played a role.

Range::intersect is intersecting [?, -8] and [0, ?] and declaring the intersection to be empty, when in fact the intersection contains NaN. The bug appears to have been introduced in bug 765119. It's made more visible by recent patches because they cause the optimizer to do more things when empty ranges are detected. An exploit is at least theoretically possible, but I haven't investigated it. Attached is a patch which fixes the bug.

Comment on attachment 8347773 [details] [diff] [review] intersect-gets-nan.patch Review of attachment 8347773 [details] [diff] [review]: ----------------------------------------------------------------- Nice catch! The problem is that a True-branch might be taken while we should have taken the false-branch. This is made visible by the removal of the condition and of the false-branch in case of a NaN. Which means that a NaN can flow in the true-branch where it is unexpected. The assertion here is reported by the RangeAssert added for the MBeta instruction at the beginning of the block. This bug should be considered as sec-critical and I think it is exploitable since Bug 943303 landed.

Comment on attachment 8347773 [details] [diff] [review] intersect-gets-nan.patch [Security approval request comment] How easily could an exploit be constructed based on the patch? Given an understanding of the range-based optimizations, it probably wouldn't be prohibitively hard. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? They make the actual bug quite clear, but they don't demonstrate how to exploit it. Which older supported branches are affected by this flaw? All branches, though it's not currently known whether the flaw is exploitable before bug 943303. If not all supported branches, which bug introduced the flaw? Bug 765119. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Backports would be trivial to create for any branch desired. They would be very low risk. How likely is this patch to cause regressions; how much testing does it need? It's very unlikely to cause regressions. It's a very small change, and it's just disabling an optimization.

(In reply to Dan Gohman [:sunfish] from comment #4) > Which older supported branches are affected by this flaw? > > All branches, though it's not currently known whether the flaw is > exploitable before bug 943303. > > If not all supported branches, which bug introduced the flaw? > > Bug 765119. Before Bug 943303, the only difference is that in one case we are reading a value which default to null [1], while in the other case we are reading a value which is reset to null [2]. So I do not see any reason to fix this bug above Gecko 29. The code might be wrong, but there is no consequence associated with setting eagerly the emptyRange flag. [1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h#326 [2] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/RangeAnalysis.cpp#1034

This needs to get a security rating before it can be approved for checkin. How serious is this? If it isn't sec-critical or sec-high, it doesn't need approval either.

MBeta::computeRange sets the unreachable_ flag, formerly called the earlyAbort_ flag. That flag is used by MPhi::computeRange. If that flag is true when it shouldn't be, the phi's range might exclude NaN when it shouldn't. In theory, that's exploitable. So to be on the conservative side, we should assume this is exploitable in all versions.

Comment on attachment 8347773 [details] [diff] [review] intersect-gets-nan.patch sec-approval+ for trunk. We should get branch patches too.

Attached is a patch for the mozilla-beta tree. It also applies to the mozilla-esr24 tree. The original patch applies cleanly on the mozilla-aurora tree.

Attached is a patch for the mozilla-b2g18 branch.

Patch landed on mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/c93234bf8a97

landed on central https://hg.mozilla.org/mozilla-central/rev/c93234bf8a97

Comment on attachment 8347773 [details] [diff] [review] intersect-gets-nan.patch [Approval Request Comment] Bug caused by (feature/regressing bug #): 765119 User impact if declined: This potential security vulnerability goes unfixed. Testing completed (on m-c, etc.): Tested on m-c. Risk to taking this patch (and alternatives if risky): Very low. String or IDL/UUID changes made by this patch: None.

Comment on attachment 8348557 [details] [diff] [review] intersect-gets-nan-beta.patch [Approval Request Comment] Bug caused by (feature/regressing bug #): 765119 User impact if declined: This potential security vulnerability goes unfixed. Testing completed (on m-c, etc.): Tested on m-c. Risk to taking this patch (and alternatives if risky): Very low. String or IDL/UUID changes made by this patch: None.

Comment on attachment 8348561 [details] [diff] [review] intersect-gets-nan-b2g18.patch NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): 765119 User impact if declined: This potential security vulnerability goes unfixed. Testing completed: Risk to taking this patch (and alternatives if risky): Tested on m-c. String or UUID changes made by this patch: None.

Comment on attachment 8348557 [details] [diff] [review] intersect-gets-nan-beta.patch [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: It is sec-critical. User impact if declined: This potential security vulnerability goes unfixed. Fix Landed on Version: 29 Risk to taking this patch (and alternatives if risky): Very low. String or UUID changes made by this patch: None. See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment on attachment 8348557 [details] [diff] [review] intersect-gets-nan-beta.patch Need release management to approve for ESR24.

https://hg.mozilla.org/releases/mozilla-aurora/rev/facabfc24d51 https://hg.mozilla.org/releases/mozilla-beta/rev/dbac310ca225

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/e7751fc22245 https://hg.mozilla.org/releases/mozilla-b2g18/rev/0a2e7ed17611 https://hg.mozilla.org/releases/mozilla-esr24/rev/c3097274b33c We still need to get this approved for esr24.

Well that went well. Backed out for bustage across the board. https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/991786b983f5 https://hg.mozilla.org/releases/mozilla-b2g18/rev/37829373c384 https://hg.mozilla.org/releases/mozilla-esr24/rev/d76c5eec6cec esr24/b2g26 bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=32974345&tree=Mozilla-B2g26-v1.2 b2g18 bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=32974177&tree=Mozilla-B2g18

Here's a new b2g18 patch which fixes that error. I don't currently have a way to test b2g18 builds, though I have now confirmed that RangeAnalysis.cpp compiles if I compile it manually.

For the esr24/b2g26 bustage, it looks like you took the intersect-gets-nan.patch patch. The intersect-gets-nan-beta.patch patch was the one intended for esr24/b2g26.

(In reply to Dan Gohman [:sunfish] from comment #21) > Created attachment 8359991 [details] [diff] [review] > intersect-gets-nan-b2g18.patch > > Here's a new b2g18 patch which fixes that error. I don't currently have a > way to test b2g18 builds, though I have now confirmed that RangeAnalysis.cpp > compiles if I compile it manually. I am not sure to understand what "tracking-b2g18: ? → 27+" means. AFAIK, 1.1 was the last branch based on b2g18, and it should be replaced soonish by 1.2 which is based on Gecko 26.

Until v1.1 EOL, security fixes are still being backported in case of a v1.1.x security release.

https://hg.mozilla.org/releases/mozilla-esr24/rev/0a9219e71d17 https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/231eef816a22 https://hg.mozilla.org/releases/mozilla-b2g18/rev/f8e43936dd12

Confirmed assertion on m-c 2013-12-14. Verified fixed on 24esr, 2014-01-19. Verified fixed on FF27, 2014-01-20. Verified fixed on FF28 and FF29, 2014-01-21.

JSBugMon: This bug has been automatically verified fixed.