Assertion failure: !has(reg), at jit/RegisterSets.h

RESOLVED FIXED in Firefox 42, Firefox OS v2.2

Status

()

Core
JavaScript Engine: JIT
--
critical
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: gkw, Assigned: nbp)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla44
ARM
All
assertion, regression, sec-high, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 wontfix, firefox41 wontfix, firefox42+ fixed, firefox43+ fixed, firefox44+ fixed, firefox-esr31 unaffected, firefox-esr3842+ fixed, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 wontfix, b2g-v2.1S wontfix, b2g-v2.2 fixed, b2g-v2.2r fixed, b2g-master fixed, fennec42+)

Details

(Whiteboard: [jsbugmon:update][post-critsmash-triage][adv-main42+][adv-esr38.4+])

Attachments

(4 attachments)

(Reporter)

Description

2 years ago
var x = [];
var y = [];
for (var i = 0; i < 3; i++) {
    y.push(x);
}

asserts js 32-bit debug ARM-simulator shell on m-c changeset fba4b0cd3823 with --fuzzing-safe --no-threads --ion-eager --unboxed-arrays at Assertion failure: !has(reg), at jit/RegisterSets.h

Configure options:

LD=ld CROSS_COMPILE=1 CC="clang -Qunused-arguments -msse2 -mfpmath=sse -arch i386" RANLIB=ranlib CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments -msse2 -mfpmath=sse" AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 HOST_CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse" sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-arm-simulator --enable-simulator=arm --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--32 --enable-debug --enable-nspr-build --enable-more-deterministic --enable-simulator=arm" -r fba4b0cd3823

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/8b68a9f000b3
user:        Nicolas B. Pierron
date:        Wed Aug 26 20:48:05 2015 +0200
summary:     Bug 1190446 - Update Coverage information in Baseline. r=jandem

Nicolas, is bug 1190446 a likely regressor?
Flags: needinfo?(nicolas.b.pierron)
(Reporter)

Comment 1

2 years ago
Created attachment 8661001 [details]
stack

(lldb) bt 5
* thread #1: tid = 0x5882e3, 0x00764ecd js-dbg-32-dm-nsprBuild-armSim-darwin-fba4b0cd3823`js::jit::AutoGenericRegisterScope<js::jit::Register>::AutoGenericRegisterScope(js::jit::MacroAssembler&, js::jit::Register) + 64 at RegisterSets.h:860, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00764ecd js-dbg-32-dm-nsprBuild-armSim-darwin-fba4b0cd3823`js::jit::AutoGenericRegisterScope<js::jit::Register>::AutoGenericRegisterScope(js::jit::MacroAssembler&, js::jit::Register) + 64 at RegisterSets.h:860
    frame #1: 0x00764e8d js-dbg-32-dm-nsprBuild-armSim-darwin-fba4b0cd3823`js::jit::AutoGenericRegisterScope<js::jit::Register>::AutoGenericRegisterScope(this=0xbfffe500, masm=<unavailable>, reg=(reg_ = r12)) + 61 at MacroAssembler.cpp:2898
    frame #2: 0x00829e28 js-dbg-32-dm-nsprBuild-armSim-darwin-fba4b0cd3823`js::jit::MacroAssemblerARMCompat::store32(js::jit::Register, js::jit::BaseIndex const&) [inlined] js::jit::ScratchRegisterScope::ScratchRegisterScope(this=<unavailable>, masm=<unavailable>) + 72 at Assembler-arm.h:55
    frame #3: 0x00829e09 js-dbg-32-dm-nsprBuild-armSim-darwin-fba4b0cd3823`js::jit::MacroAssemblerARMCompat::store32(js::jit::Register, js::jit::BaseIndex const&) [inlined] js::jit::ScratchRegisterScope::ScratchRegisterScope(masm=<unavailable>) at Assembler-arm.h:55
    frame #4: 0x00829e09 js-dbg-32-dm-nsprBuild-armSim-darwin-fba4b0cd3823`js::jit::MacroAssemblerARMCompat::store32(this=<unavailable>, src=(reg_ = r12), dest=0xbfffe578) + 41 at MacroAssembler-arm.cpp:2343
(lldb)
(Assignee)

Updated

2 years ago
Group: javascript-core-security
Keywords: sec-want
(Assignee)

Comment 2

2 years ago
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0)
> Nicolas, is bug 1190446 a likely regressor?

No.

This particular test case should fail since Bug 1165392 & Bug 1165463, but the underlying issue is present in the JS Engine since Bug 976446.

This issue does the following: when storing a constant value in an array, instead of pushing the value of the constant, we push the address of the element in the Array element.

Thus, this issue leak a pointer to a JS Array, which can later be used to map the entire memory in conjunction with another issue which allow to update the length of the Array.
Assignee: nobody → nicolas.b.pierron
Blocks: 976446
No longer blocks: 1190446
tracking-fennec: --- → ?
status-firefox40: --- → affected
status-firefox41: --- → affected
status-firefox42: --- → affected
status-firefox-esr31: --- → unaffected
status-firefox-esr38: --- → affected
Flags: needinfo?(nicolas.b.pierron)
Keywords: sec-want → sec-high
OS: Mac OS X → All
Hardware: x86 → ARM
status-b2g-v2.0: --- → wontfix
status-b2g-v2.0M: --- → wontfix
status-b2g-v2.1: --- → wontfix
status-b2g-v2.1S: --- → affected
status-b2g-v2.2: --- → affected
status-b2g-v2.2r: --- → affected
status-b2g-master: --- → affected
(Assignee)

Comment 3

2 years ago
Created attachment 8661230 [details] [diff] [review]
part 0 - Annotate the assertion with a recommended security rating.
Attachment #8661230 - Flags: review?(gary)
(Assignee)

Comment 4

2 years ago
Created attachment 8661287 [details] [diff] [review]
part 1 - ARM: Use a different scratch register for store32.

This patch change store32(Register, BaseIndex) to use the second scratch
register, such that this does not conflict with the scratch register used in
storePtr(ImmWord, T).

This patch also change the store32(Imm32, BaseIndex) to use the first
scratch register instead of the second, such that this does not introduce
any conflict.
Attachment #8661287 - Flags: review?(sstangl)
Comment on attachment 8661230 [details] [diff] [review]
part 0 - Annotate the assertion with a recommended security rating.

Review of attachment 8661230 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/RegisterSets.h
@@ +748,5 @@
>      using Parent::has;
>  
>      using Parent::addUnchecked;
>      void add(RegType reg) {
> +        MOZ_ASSERT(!has(reg), "sec-citical: Potentially overwritten register");

I realize this isn't my review, but it's spelled "citical" instead of "critical" everywhere.
(Reporter)

Comment 6

2 years ago
Comment on attachment 8661230 [details] [diff] [review]
part 0 - Annotate the assertion with a recommended security rating.

Moving on to :sstangl for review. As he pointed out, "citical" should be spelled as "critical" instead.
Attachment #8661230 - Flags: review?(gary) → review?(sstangl)
Comment on attachment 8661230 [details] [diff] [review]
part 0 - Annotate the assertion with a recommended security rating.

Not strongly opposing, but I wonder if it's a good thing to sound a five-alarm blaze so obviously for anyone who can fuzz us.  I would rest slightly easier if this were maintained not in the tree, although I acknolwedge that has its own problems of the connection going stale, etc.
Comment on attachment 8661287 [details] [diff] [review]
part 1 - ARM: Use a different scratch register for store32.

Review of attachment 8661287 [details] [diff] [review]:
-----------------------------------------------------------------

While the patch looks fine to me, please keep in mind that this is yet again not exploitable, because the ScratchRegister is only overwritten inside store32(Register, BaseIndex) if (dest.offset != 0), and it is always zero. We very rarely use a BaseIndex with a constant offset.

If you really wanted, you could lessen the scope by just acquiring the ScratchRegisterScope within the specific branch, and duplicating the call to ma_str(). That may look nicer than Maybe<> and emplace().

The bug could possibly be S-S, but the test case given is not an example of something exploitable.
Attachment #8661287 - Flags: review?(sstangl) → review+
Comment on attachment 8661230 [details] [diff] [review]
part 0 - Annotate the assertion with a recommended security rating.

Review of attachment 8661230 [details] [diff] [review]:
-----------------------------------------------------------------

Aha, now it is my review! I agree with Jeff.
Attachment #8661230 - Flags: review?(sstangl) → review-
(Assignee)

Comment 10

2 years ago
Comment on attachment 8661287 [details] [diff] [review]
part 1 - ARM: Use a different scratch register for store32.

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

The patch highlights an issue with store32, looking at the chain of callers makes it obvious that we reuse the scratch register which is updated in this patch.

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

No.

> Which older supported branches are affected by this flaw?

All.

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

Bug 976446

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

I will attach the backports in a few minutes.  The backports do not rely on the ScratchRegisterScope which is used here to assert the correctness.

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

store32 is used by different caller, I should have covered this by looking carefully at the code, and running the test suite locally.  The test suite on treehereder is likely to catch remaining issues if any.
Attachment #8661287 - Flags: sec-approval?
Attachment #8661287 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 11

2 years ago
Comment on attachment 8661287 [details] [diff] [review]
part 1 - ARM: Use a different scratch register for store32.

Not requesting aurora uplift yet :/
Attachment #8661287 - Flags: approval-mozilla-aurora?
sec-approval for checkin on October 6, two weeks into the next cycle.
status-firefox40: affected → wontfix
status-firefox41: affected → wontfix
tracking-firefox42: --- → +
tracking-firefox43: --- → +
tracking-firefox-esr38: --- → 42+
Whiteboard: [jsbugmon:update] → [jsbugmon:update][checkin on 10/6]
Attachment #8661287 - Flags: sec-approval? → sec-approval+
This can be landed now.
Flags: needinfo?(nicolas.b.pierron)
status-firefox44: --- → affected
tracking-firefox44: --- → +
(Assignee)

Comment 14

2 years ago
Created attachment 8671496 [details] [diff] [review]
(beta) ARM: Use a different scratch register for store32.

This patch should do the same as the previous patches, but for some reasons
that I do not understand I cannot compile & test ARM on beta right now.

Should I hold the landing on inbound?
(Assignee)

Comment 15

2 years ago
(ni? for comment 14)
Flags: needinfo?(abillings)
I'd go ahead and land and watch it closely afterwards.
Flags: needinfo?(abillings)
(Assignee)

Comment 17

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/464e82856a55
Flags: needinfo?(nicolas.b.pierron)
(Reporter)

Comment 18

2 years ago
This landed on m-c:

http://hg.mozilla.org/mozilla-central/rev/464e82856a55

:Tomcat, not sure if you're aware, but some bugs have not been marked fixed when the patch has moved on to m-c. This is the second one I've noticed. Maybe it's because it is a security bug?
Flags: needinfo?(cbook)
(Reporter)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Whiteboard: [jsbugmon:update][checkin on 10/6] → [jsbugmon:update]
status-b2g-v2.1S: affected → wontfix
status-b2g-master: affected → fixed
Target Milestone: --- → mozilla44
(Assignee)

Comment 19

2 years ago
Comment on attachment 8661287 [details] [diff] [review]
part 1 - ARM: Use a different scratch register for store32.

Approval Request Comment
[Feature/regressing bug #]: Bug 976446
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]: inbound / m-c
[Risks and why]: see comment 2 and comment 10.
[String/UUID change made/needed]: N/A
Attachment #8661287 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 20

2 years ago
Comment on attachment 8671496 [details] [diff] [review]
(beta) ARM: Use a different scratch register for store32.

Approval Request Comment
(same as previous comment)
Attachment #8671496 - Flags: approval-mozilla-beta?
Group: javascript-core-security → core-security-release
Attachment #8661287 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8671496 [details] [diff] [review]
(beta) ARM: Use a different scratch register for store32.

Fix a sec high, taking it.
Should be in 42 beta 7.
Attachment #8671496 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-aurora/rev/b14beb29be00

gary: yeah seems something got wrong when marking this is on central, but fixed this
status-firefox43: affected → fixed
Flags: needinfo?(cbook)
https://hg.mozilla.org/releases/mozilla-beta/rev/ebafbc7c1a87
status-firefox42: affected → fixed
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/099fe0777201
status-b2g-v2.2: affected → fixed
tracking-fennec: ? → 42+
Whiteboard: [jsbugmon:update] → [jsbugmon:update][post-critsmash-triage]
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/099fe0777201
status-b2g-v2.2r: affected → fixed
This needs an ESR38 patch as a sec-high. Nicolas is out (not accepting needinfo? so I assume he's out).
Flags: needinfo?(rkothari)
(Assignee)

Comment 27

2 years ago
(In reply to Al Billings [:abillings] from comment #26)
> This needs an ESR38 patch as a sec-high. Nicolas is out (not accepting
> needinfo? so I assume he's out).

The beta patch should work for esr38.
Do we want to take this on ESR38?
Flags: needinfo?(sledru)
Ritu is in PTO too.

NBP, could you request the uplift to esr too? Merci
Flags: needinfo?(sledru)
Flags: needinfo?(rkothari)
Flags: needinfo?(nicolas.b.pierron)
(Assignee)

Comment 30

2 years ago
Comment on attachment 8671496 [details] [diff] [review]
(beta) ARM: Use a different scratch register for store32.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: None.
Fix Landed on Version: All except ESR.
Risk to taking this patch (and alternatives if risky): See comment 2 and comment 10
String or UUID changes made by this patch: None
Flags: needinfo?(nicolas.b.pierron)
Attachment #8671496 - Flags: approval-mozilla-esr38?
Comment on attachment 8671496 [details] [diff] [review]
(beta) ARM: Use a different scratch register for store32.

Approved for uplift to esr38 since this fixes a sec issue we're releasing in all other branches.
Attachment #8671496 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
This didn't land before build 1.
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-esr38/rev/794da8ad9e7e

Sorry for letting this slip until today.
status-firefox-esr38: affected → fixed
Whiteboard: [jsbugmon:update][post-critsmash-triage] → [jsbugmon:update][post-critsmash-triage][adv-main42+][adv-esr38.4+]

Updated

2 years ago
Keywords: checkin-needed
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.