Bug 1317936 (CVE-2016-9904)

Fix cross-origin information leak from shared atoms

RESOLVED FIXED in Firefox 50

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 months ago
4 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

({csectype-sop, sec-high})

unspecified
mozilla53
csectype-sop, sec-high
Points:
---

Firefox Tracking Flags

(firefox50+ fixed, firefox51+ fixed, firefox52+ fixed, firefox53+ fixed, firefox-esr4550+ fixed)

Details

(Whiteboard: [adv-main50.1+][adv-ESR45.6+] Disclosure date ~Jan 21 2017 (note Fx51 scheduled for Jan 24))

Attachments

(2 attachments)

(Assignee)

Description

7 months ago
JS code can use a Map/Set timing attack to determine whether an atom is used by another compartment/zone. See bug 1312001.

The plan (bug 1312001 comment 13) is to add a hash code to atoms, to avoid leaking information about the atom's address.
(Assignee)

Comment 1

7 months ago
(In reply to Jan de Mooij [:jandem] from comment #0)
> The plan (bug 1312001 comment 13)

Er, bug 1312001 comment 18.
(Assignee)

Updated

7 months ago
Summary: Fix cross-origin information leak exposed by shared atoms → Fix cross-origin information leak from shared atoms
(Assignee)

Comment 2

7 months ago
Unfortunately this will add 8 bytes to all atoms, due to GC alignment restrictions. In theory we could use the 4 unused bytes as extra inline storage, but that will complicate things tremendously. Another option is to store the hash code only for atoms with length > X and calculate it lazily for short strings. Maybe we can do that as a follow-up.
(Assignee)

Comment 3

7 months ago
Created attachment 8811253 [details] [diff] [review]
Part 1 - Add hash code to atoms and use it for Map/Set

This adds a hash to all atoms and uses it in HashableValue::hash().

I think we have to fix jsid hashing to use this hash as well, since ShapeTables likely have a similar issue :/
Attachment #8811253 - Flags: review?(jorendorff)
Attachment #8811253 - Flags: review?(jcoppeard)

Comment 4

7 months ago
Comment on attachment 8811253 [details] [diff] [review]
Part 1 - Add hash code to atoms and use it for Map/Set

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

This is a nice clean patch.

Not for this bug, but now that we have separate thing kinds for atoms would it be possible to get rid of the ATOM_BIT flag and the process of morphing strings into atoms?
Attachment #8811253 - Flags: review?(jcoppeard) → review+
(Assignee)

Comment 5

7 months ago
(In reply to Jon Coppeard (:jonco) from comment #4)
> Not for this bug, but now that we have separate thing kinds for atoms would
> it be possible to get rid of the ATOM_BIT flag and the process of morphing
> strings into atoms?

Nice idea. I think it would hurt perf though as isAtom() is queried on some very hot paths (including JIT code we generate for comparing strings).
Comment on attachment 8811253 [details] [diff] [review]
Part 1 - Add hash code to atoms and use it for Map/Set

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

::: js/src/builtin/MapObject.cpp
@@ +69,4 @@
>  {
>      // HashableValue::setValue normalizes values so that the SameValue relation
>      // on HashableValues is the same as the == relationship on
>      // value.data.asBits.

Comment needs to be updated.
Attachment #8811253 - Flags: review?(jorendorff) → review+
Keywords: csectype-sop, sec-high
(Assignee)

Comment 7

7 months ago
Created attachment 8812732 [details] [diff] [review]
Part 2 - Fix jsid hashing

This changes jsid hashing to use atom->hash() if the id is an atom, to prevent a similar attack on shape tables.

I had to move DefaultHasher<jsid> to an internal header, a bit unfortunate but there are no consumers outside SpiderMonkey atm. The alternative is to move HashId out of line but I'd like to avoid the perf overhead of that for now.

When this is on all branches I'll post another patch to add comments.
Attachment #8812732 - Flags: review?(jcoppeard)
(Assignee)

Updated

7 months ago
Attachment #8811253 - Attachment description: Patch → Part 1 - Add hash code to atoms and use it for Map/Set

Comment 8

7 months ago
Comment on attachment 8812732 [details] [diff] [review]
Part 2 - Fix jsid hashing

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

Looks good.

::: js/src/jsiter.cpp
@@ -75,5 @@
> -    }
> -    static bool match(jsid id1, jsid id2) {
> -        return id1 == id2;
> -    }
> -};

Thanks for removing this.
Attachment #8812732 - Flags: review?(jcoppeard) → review+
(Assignee)

Updated

7 months ago
Flags: needinfo?(dveditz)
[Tracking Requested - why for this release]: sec-high with disclosure date around the release date of Fx51.
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
Whiteboard: Disclosure date ~Jan 21 2017 (note Fx51 scheduled for Jan 24)
It's OK to land this one now. Thanks for asking.
Flags: needinfo?(dveditz)
(Assignee)

Comment 12

7 months ago
Comment on attachment 8811253 [details] [diff] [review]
Part 1 - Add hash code to atoms and use it for Map/Set

Requesting approval for both patches.


[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not trivial. 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?
No.

> Which older supported branches are affected by this flaw?
All.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be possible to backport this. There is some risk but this has to be in 51.

> How likely is this patch to cause regressions; how much testing does it need?
Perf and correctness regressions are certainly possible, but having this code in a few Nightlies before uplifting should be sufficient I think.
Attachment #8811253 - Flags: sec-approval?
Attachment #8811253 - Flags: sec-approval? → sec-approval+
Track 51+ as sec-high.
tracking-firefox51: ? → +
(Assignee)

Comment 14

7 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/be48744b160459666cbf97f065a97cdad695092f
https://hg.mozilla.org/integration/mozilla-inbound/rev/b92dcbb89ffcf96216a9cfdacfc3b2d652606028
(Assignee)

Updated

7 months ago
Flags: needinfo?(jdemooij)
(Assignee)

Comment 15

7 months ago
This was merged a few days ago:

https://hg.mozilla.org/mozilla-central/rev/be48744b160459666cbf97f065a97cdad695092f
https://hg.mozilla.org/mozilla-central/rev/b92dcbb89ffcf96216a9cfdacfc3b2d652606028
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Comment 16

7 months ago
Comment on attachment 8811253 [details] [diff] [review]
Part 1 - Add hash code to atoms and use it for Map/Set

Approval Request Comment
[Feature/regressing bug #]: Old bug.
[User impact if declined]: Security issues.
[Describe test coverage new/current, TreeHerder]: This code is exercised by pretty much all tests we have.
[Risks and why]: These patches are not trivial so there is some risk. But still, this code is exercised a lot and Nightly testing didn't reveal any issues so far AFAIK.
[String/UUID change made/needed]: None.
Attachment #8811253 - Flags: approval-mozilla-esr45?
Attachment #8811253 - Flags: approval-mozilla-beta?
Attachment #8811253 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 17

7 months ago
These approval requests apply to both patches.
Comment on attachment 8811253 [details] [diff] [review]
Part 1 - Add hash code to atoms and use it for Map/Set

Fix a sec-high. Beta51+, Aurora52+, and ESR45+. Should be in 51 beta 4.
Attachment #8811253 - Flags: approval-mozilla-esr45?
Attachment #8811253 - Flags: approval-mozilla-esr45+
Attachment #8811253 - Flags: approval-mozilla-beta?
Attachment #8811253 - Flags: approval-mozilla-beta+
Attachment #8811253 - Flags: approval-mozilla-aurora?
Attachment #8811253 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/69b1f1cd3ae8
https://hg.mozilla.org/releases/mozilla-aurora/rev/ea3f6566462f
status-firefox52: affected → fixed
Tracking 53- since this is resolved fixed.
tracking-firefox53: ? → -
https://hg.mozilla.org/releases/mozilla-beta/rev/c82318b2897c
https://hg.mozilla.org/releases/mozilla-beta/rev/1345c10edd89
status-firefox51: affected → fixed
Group: javascript-core-security → core-security-release
jan, this has problems to apply to esr45

grafting 377427:c82318b2897c "Bug 1317936 part 1 - Add hash code to atoms. r=jonco,jorendorff a=gchang"
merging js/src/builtin/MapObject.cpp
merging js/src/gc/Allocator.cpp
merging js/src/gc/Heap.h
merging js/src/jit/VMFunctions.cpp
merging js/src/jsatom.cpp
merging js/src/jsatominlines.h
merging js/src/jscompartment.cpp
merging js/src/jscompartment.h
merging js/src/jsgc.cpp
merging js/src/jsgc.h
merging js/src/vm/Runtime.cpp
merging js/src/vm/String-inl.h
merging js/src/vm/String.cpp
merging js/src/vm/String.h
warning: conflicts while merging js/src/gc/Allocator.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging js/src/gc/Heap.h! (edit, then use 'hg resolve --mark')
warning: conflicts while merging js/src/jscompartment.h! (edit, then use 'hg resolve --mark')
warning: conflicts while merging js/src/jsgc.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging js/src/vm/String.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
(Assignee)

Comment 23

7 months ago
https://hg.mozilla.org/releases/mozilla-esr45/rev/edd9158508d92d1d99229fa6079b90d1b0e16293
https://hg.mozilla.org/releases/mozilla-esr45/rev/222bdae65a115b06057fd1029f3fd38e26bfaecd
status-firefox-esr45: --- → fixed
Temporarily reverted from esr45 for reasons. No action needed on your part, this'll be relanded at the appropriate time.

https://hg.mozilla.org/releases/mozilla-esr45/rev/29961c35f618
https://hg.mozilla.org/releases/mozilla-esr45/rev/45f5aa933cfa
status-firefox-esr45: fixed → affected
tracking-firefox50: --- → ?
tracking-firefox-esr45: --- → ?
Comment on attachment 8811253 [details] [diff] [review]
Part 1 - Add hash code to atoms and use it for Map/Set

This was already landed on ESR45 for the 45.6 release, so I think that makes this more important to ship with 50.1 as well.
Attachment #8811253 - Flags: approval-mozilla-release?
https://hg.mozilla.org/releases/mozilla-esr45/rev/8ae673f34a5b
https://hg.mozilla.org/releases/mozilla-esr45/rev/409c23c144fe
status-firefox-esr45: affected → fixed
Setting tracking- for fixed bugs seems to conflate bug status with severity and is a dangerous practice in general (What happens if a bug gets backed out? Are we going to rely on luck that it ends up back on RelMan's radar at that point?). Especially for security bugs, can we please stop doing this?
tracking-firefox53: - → ?
tracking-firefox53: ? → +

Updated

7 months ago
tracking-firefox50: ? → +

Updated

7 months ago
tracking-firefox52: ? → +
tracking-firefox-esr45: ? → 51+

Comment 28

7 months ago
Comment on attachment 8811253 [details] [diff] [review]
Part 1 - Add hash code to atoms and use it for Map/Set

Sec-high, meets the triage bar for inclusion in 50.1.0
Attachment #8811253 - Flags: approval-mozilla-release? → approval-mozilla-release+
Needs rebasing for mozilla-release.
(Assignee)

Comment 30

7 months ago
https://hg.mozilla.org/releases/mozilla-release/rev/1624e795be20cd01401bfca4aae60278360242c4
https://hg.mozilla.org/releases/mozilla-release/rev/1a8bbd6f7e2198f2ef47b603af80db405590aed4
status-firefox50: affected → fixed
Can't write an advisory for this issue without 0day'ing ESR45 because the fix in ESR45 doesn't ship until January. This should not have gone into 50.1.
Flags: needinfo?(abillings)
Ok. There is an ESR 45.6 release going out with 50.1. This should be in the advisories for both unless the disclosure precludes that. Jan?
Flags: needinfo?(abillings)
Whiteboard: Disclosure date ~Jan 21 2017 (note Fx51 scheduled for Jan 24) → [adv-main50.1+][adv-ESR45.6+] Disclosure date ~Jan 21 2017 (note Fx51 scheduled for Jan 24)
(Assignee)

Comment 33

7 months ago
(In reply to Al Billings [:abillings] from comment #32)
> Ok. There is an ESR 45.6 release going out with 50.1. This should be in the
> advisories for both unless the disclosure precludes that. Jan?

Al, just to be sure, did you mean to ask Jann, the bug reporter?
Flags: needinfo?(jdemooij) → needinfo?(abillings)
(Assignee)

Updated

7 months ago
Flags: needinfo?(jdemooij)
I was trying to understand the whiteboard, "Disclosure date ~Jan 21 2017 (note Fx51 scheduled for Jan 24)".

Dan Veditz says that it shouldn't limit us writing an advisory as it is just indicating when the reporter will be disclosing per his own policy.
Flags: needinfo?(jdemooij)
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
Alias: CVE-2016-9904
(Assignee)

Updated

7 months ago
Flags: needinfo?(jdemooij)
Flags: needinfo?(dveditz)
(Assignee)

Updated

5 months ago
Flags: needinfo?(jdemooij)
tracking-firefox-esr45: 51+ → 50+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.