Bug 1317936 (CVE-2016-9904)

Fix cross-origin information leak from shared atoms

RESOLVED FIXED in Firefox -esr45

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

({csectype-sop, sec-high})

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

Firefox Tracking Flags

(firefox-esr4550+ fixed, firefox50+ fixed, firefox51+ fixed, firefox52+ fixed, firefox53+ 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

2 years 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

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

Er, bug 1312001 comment 18.
(Assignee)

Updated

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

Comment 2

2 years 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

2 years ago
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 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

2 years 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

2 years ago
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

2 years ago
Attachment #8811253 - Attachment description: Patch → Part 1 - Add hash code to atoms and use it for Map/Set
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

2 years 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

2 years 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)

Updated

2 years ago
Flags: needinfo?(jdemooij)
(Assignee)

Comment 15

2 years 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: 2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Comment 16

2 years 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

2 years 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+
Tracking 53- since this is resolved fixed.
tracking-firefox53: ? → -
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')
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?
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: ? → +
tracking-firefox50: ? → +
tracking-firefox52: ? → +
tracking-firefox-esr45: ? → 51+
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.
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

2 years 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

2 years 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

2 years ago
Flags: needinfo?(jdemooij)
Flags: needinfo?(dveditz)
(Assignee)

Updated

2 years 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.