1317936 - (CVE-2016-9904) Fix cross-origin information leak from shared atoms

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

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.

Comment 1

[Jan de Mooij [:jandem]]

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

Er, bug 1312001 comment 18.

Comment 2

[Jan de Mooij [:jandem]]

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.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: 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 :/

Comment 4

[Jon Coppeard (:jonco)]

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?

Comment 5

[Jan de Mooij [:jandem]]

(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 6

[Jason Orendorff [:jorendorff]]

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.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: 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.

Comment 8

[Jon Coppeard (:jonco)]

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.

Comment 10

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: sec-high with disclosure date around the release date of Fx51.

Comment 11

[Daniel Veditz [:dveditz]]

It's OK to land this one now. Thanks for asking.

Comment 12

[Jan de Mooij [:jandem]]

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.

Comment 13

[Gerry Chang [:gchang]]

Track 51+ as sec-high.

Comment 14

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be48744b160459666cbf97f065a97cdad695092f
https://hg.mozilla.org/integration/mozilla-inbound/rev/b92dcbb89ffcf96216a9cfdacfc3b2d652606028

Comment 15

[Jan de Mooij [:jandem]]

This was merged a few days ago:

https://hg.mozilla.org/mozilla-central/rev/be48744b160459666cbf97f065a97cdad695092f
https://hg.mozilla.org/mozilla-central/rev/b92dcbb89ffcf96216a9cfdacfc3b2d652606028

Comment 16

[Jan de Mooij [:jandem]]

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.

Comment 17

[Jan de Mooij [:jandem]]

These approval requests apply to both patches.

Comment 18

[Gerry Chang [:gchang]]

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.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/69b1f1cd3ae8
https://hg.mozilla.org/releases/mozilla-aurora/rev/ea3f6566462f

Comment 20

[Marcia Knous [:marcia]]

Tracking 53- since this is resolved fixed.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c82318b2897c
https://hg.mozilla.org/releases/mozilla-beta/rev/1345c10edd89

Comment 22

[Carsten Book [:Tomcat]]

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')

Comment 23

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/edd9158508d92d1d99229fa6079b90d1b0e16293
https://hg.mozilla.org/releases/mozilla-esr45/rev/222bdae65a115b06057fd1029f3fd38e26bfaecd

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/8ae673f34a5b
https://hg.mozilla.org/releases/mozilla-esr45/rev/409c23c144fe

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 28

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for mozilla-release.

Comment 30

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-release/rev/1624e795be20cd01401bfca4aae60278360242c4
https://hg.mozilla.org/releases/mozilla-release/rev/1a8bbd6f7e2198f2ef47b603af80db405590aed4

Comment 31

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 32

[Al Billings [:abillings - ex-MoCo]]

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?

Comment 33

[Jan de Mooij [:jandem]]

(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?

Comment 34

[Al Billings [:abillings - ex-MoCo]]

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.