Closed Bug 936737 Opened 6 years ago Closed 6 years ago

Crash with SIGTRAP involving --ion-check-range-analysis


(Core :: JavaScript Engine: JIT, defect, critical)

Not set



Tracking Status
firefox26 --- unaffected
firefox27 + fixed
firefox28 + fixed
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected


(Reporter: gkw, Assigned: sunfish)


(Blocks 1 open bug)


(4 keywords, Whiteboard: [qa-])


(2 files)

Attached file lldb stack
x = String
x.valueOf = (function(stdlib, foreign) {
    "use asm"
    var ff = foreign.ff
    function f() {
        (6 > (((ff() | 0) + 0x9bfc8dab) >>> 0)) ? 0 : 0
    return f
})(this, {
    ff: decodeURIComponent
x + (function() {})

crashes 32-bit and 64-bit js debug (and opt, I think) shell on m-c changeset a07aebef20e7 with --ion-check-range-analysis at SIGTRAP.

s-s because this seems to be a range analysis problem and I don't know how bad this is. Dan, thoughts?

My configure flags are:

LD=ld CROSS_COMPILE=1 RANLIB=ranlib CC="clang -Qunused-arguments -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments" HOST_CXX="clang++ -Qunused-arguments" sh ./configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --with-ccache --disable-threadsafe
Flags: needinfo?(sunfish)
Confirmed. The regression was introduced in 42a20a0d4269 for bug 918607

It appears I misunderstood unsigned comparisons. The simplest and safest fix is to just revert the part of that patch which enabled beta nodes for unsigned comparisons. Attached is a patch which does just that.
Assignee: nobody → sunfish
Attachment #829747 - Flags: review?(nicolas.b.pierron)
Flags: needinfo?(sunfish)
Comment on attachment 829747 [details] [diff] [review]

Review of attachment 829747 [details] [diff] [review]:

I am surprised that range analysis did not get caught by this issue since Bug 871002 landed.

::: js/src/jit/RangeAnalysis.cpp
@@ +145,5 @@
>              continue;
>          MCompare *compare = test->getOperand(0)->toCompare();
> +
> +        // TODO: support unsigned comparisons

If you add a ":TODO: …" you should also add a bug number as part of the comment.
Attachment #829747 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Dan Gohman [:sunfish] from comment #1)
> Confirmed. The regression was introduced in 42a20a0d4269 for bug 918607

Setting the tracking flags accordingly.
I'm assessing this as sec-critical, since there are theoretically ways to go from incorrect range information to arbitrary pointer dereferences.
Keywords: sec-critical
Please don't forget to first request for sec-approval, since this is also in Aurora.
Blocks: 918607
Comment on attachment 829747 [details] [diff] [review]

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

It would take clever reasoning to construct a testcase which triggers the bug in a way that's actually exploitable.

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

The patch just disables a feature, so it doesn't specifically identify the bug. It does identify the feature though, which would tell someone where to start looking.

Which older supported branches are affected by this flaw?

mozilla-aurora also contains the flaw.

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

It was introduced for bug 918607.

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

The given patch is also suitable for mozilla-aurora.

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

Very unlikely. It's actually just reverting part of a patch to an earlier state, partially disabling the new feature that was added.
Attachment #829747 - Flags: sec-approval?
Comment on attachment 829747 [details] [diff] [review]

sec-approval+ for trunk. I've also given Aurora approval since you've said it will apply as is.
Attachment #829747 - Flags: sec-approval?
Attachment #829747 - Flags: sec-approval+
Attachment #829747 - Flags: approval-mozilla-aurora+
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
(In reply to {N/A until Nov. 18} Nicolas B. Pierron [:nbp] from comment #2)
> If you add a ":TODO: …" you should also add a bug number as part of the
> comment.

Now that the patch has landed, I filed bug 938176 to track fixing this. The TODO comment is actually the old comment restored as part of the revert of the buggy patch.
Whiteboard: [qa-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.