Closed Bug 1404636 Opened 6 years ago Closed 6 years ago

Differential Testing: Different output message involving typed arrays


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




Tracking Status
firefox-esr52 57+ fixed
firefox56 --- wontfix
firefox57 + fixed
firefox58 + fixed


(Reporter: gkw, Assigned: jandem)


(Keywords: sec-high, testcase, Whiteboard: [adv-main57+][adv-esr52.5+][post-critsmash-triage])


(2 files)

x = new Uint32Array(5);
try {
} catch (e) {}
x[3] = -1;

$ ./js-64-dm-linux-19f368b1267d --fuzzing-safe --no-threads --ion-eager testcase.js 

$ ./js-64-dm-linux-19f368b1267d --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js 

Tested this on m-c rev 19f368b1267d.

My configure flags are:

AR=ar sh ./configure --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/ -b "--enable-debug --enable-more-deterministic" -r 19f368b1267d

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
user:        Jan de Mooij
date:        Tue Oct 04 10:19:41 2016 +0200
summary:     Bug 1306626 - Don't attach an Ion GetDenseElement stub if we're not accessing a dense element. r=h4writer

Jan, is bug 1306626 a likely regressor?
Flags: needinfo?(jdemooij)
Actually, turning this s-s since this involves typed arrays.
Group: javascript-core-security
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0)
> Jan, is bug 1306626 a likely regressor?

No, but I'll take a look.
No longer blocks: 1306626
This is actually a pretty bad TI bug. Patches coming up.
Assignee: nobody → jdemooij
Keywords: sec-high
Attached patch Part 1 - FixSplinter Review
PropertyReadNeedsTypeBarrier was returning false for typed array elements. Then we added an IC and infallibly unboxed the return value to int32.
Flags: needinfo?(jdemooij)
Attachment #8914331 - Flags: review?(bhackett1024)
This emits debug checks in JIT code for infallible unbox operations. If we had done this sooner it would probably have caught this bug a long time ago.
Attachment #8914335 - Flags: review?(bhackett1024)
Attachment #8914331 - Flags: review?(bhackett1024) → review+
Attachment #8914335 - Flags: review?(bhackett1024) → review+
Priority: -- → P1
Comment on attachment 8914331 [details] [diff] [review]
Part 1 - Fix

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not too easy. Maybe with some work.

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

> Which older supported branches are affected by this flaw?

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

> How likely is this patch to cause regressions; how much testing does it need?
Attachment #8914331 - Flags: sec-approval?
Does this need to be lifted to FF57? If so let's get that approval asap.
Flags: needinfo?(jdemooij)
Sec-approval+ for trunk. This seems fairly safe to take on ESR52 and Beta. Can you please nominate patches for there as well? I don't think this is won't fix for 57 unless you think there is a reason not explained why we wouldn't take it there.
Attachment #8914331 - Flags: sec-approval? → sec-approval+
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Yes, this should be fine for 57 and ESR, please nominate patches for uplift (without tests). It can make it into tomorrow's beta 12 build if we get it landed by Thursday morning.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Comment on attachment 8914331 [details] [diff] [review]
Part 1 - Fix

Approval Request Comment
[Feature/Bug causing the regression]: Old bug.
[User impact if declined]: Security issues.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Some risk.
[Why is the change risky/not risky?]: Probably low risk but a bit hard to say.
[String changes made/needed]: None.
Attachment #8914331 - Flags: approval-mozilla-esr52?
Attachment #8914331 - Flags: approval-mozilla-beta?
Comment on attachment 8914331 [details] [diff] [review]
Part 1 - Fix

Fix for sec-high issue, let's uplift for beta 12 and for ESR.
Attachment #8914331 - Flags: approval-mozilla-esr52?
Attachment #8914331 - Flags: approval-mozilla-esr52+
Attachment #8914331 - Flags: approval-mozilla-beta?
Attachment #8914331 - Flags: approval-mozilla-beta+
NI myself to land part 2 later.
Flags: needinfo?(jdemooij)
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Group: javascript-core-security → core-security-release
Whiteboard: [adv-main57+][adv-esr52.5+]
Flags: qe-verify-
Whiteboard: [adv-main57+][adv-esr52.5+] → [adv-main57+][adv-esr52.5+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.