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

RESOLVED FIXED in Firefox 27

Status

()

--
critical
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: gkw, Assigned: sunfish)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla28
All
Mac OS X
crash, regression, sec-critical, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox26 unaffected, firefox27+ fixed, firefox28+ fixed, firefox-esr24 unaffected, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 unaffected)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Created attachment 829652 [details]
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)
(Assignee)

Comment 1

5 years ago
Created attachment 829747 [details] [diff] [review]
range-unsigned-compare.patch

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]
range-unsigned-compare.patch

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.
status-firefox26: --- → unaffected
status-firefox27: --- → affected
status-firefox28: --- → affected
tracking-firefox27: --- → ?
tracking-firefox28: --- → ?
(Assignee)

Comment 4

5 years ago
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
(Reporter)

Comment 5

5 years ago
Please don't forget to first request for sec-approval, since this is also in Aurora.
Blocks: 918607
(Assignee)

Comment 6

5 years ago
Comment on attachment 829747 [details] [diff] [review]
range-unsigned-compare.patch

[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]
range-unsigned-compare.patch

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+

Updated

5 years ago
tracking-firefox27: ? → +
tracking-firefox28: ? → +
https://hg.mozilla.org/mozilla-central/rev/f410dbf98d0f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
status-firefox28: affected → fixed
(Assignee)

Comment 11

5 years ago
(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.
status-b2g18: --- → unaffected
status-b2g-v1.1hd: --- → unaffected
status-b2g-v1.2: --- → unaffected
status-firefox-esr24: --- → unaffected

Updated

5 years ago
Whiteboard: [qa-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.