Last Comment Bug 1317936 - (CVE-2016-9904) Fix cross-origin information leak from shared atoms
(CVE-2016-9904)
: Fix cross-origin information leak from shared atoms
Status: RESOLVED FIXED
[adv-main50.1+][adv-ESR45.6+] Disclos...
: csectype-sop, sec-high
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: Unspecified Unspecified
-- normal (vote)
: mozilla53
Assigned To: Jan de Mooij [:jandem]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 1312001
  Show dependency treegraph
 
Reported: 2016-11-16 01:28 PST by Jan de Mooij [:jandem]
Modified: 2017-02-09 08:03 PST (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
+
fixed
+
fixed
50+
fixed


Attachments
Part 1 - Add hash code to atoms and use it for Map/Set (31.24 KB, patch)
2016-11-16 06:55 PST, Jan de Mooij [:jandem]
jorendorff: review+
jcoppeard: review+
gchang: approval‑mozilla‑aurora+
gchang: approval‑mozilla‑beta+
rkothari: approval‑mozilla‑release+
gchang: approval‑mozilla‑esr45+
dveditz: sec‑approval+
Details | Diff | Splinter Review
Part 2 - Fix jsid hashing (5.12 KB, patch)
2016-11-21 03:30 PST, Jan de Mooij [:jandem]
jcoppeard: review+
Details | Diff | Splinter Review

Description User image Jan de Mooij [:jandem] 2016-11-16 01:28:22 PST
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 User image Jan de Mooij [:jandem] 2016-11-16 01:30:51 PST
(In reply to Jan de Mooij [:jandem] from comment #0)
> The plan (bug 1312001 comment 13)

Er, bug 1312001 comment 18.
Comment 2 User image Jan de Mooij [:jandem] 2016-11-16 05:46:33 PST
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 User image Jan de Mooij [:jandem] 2016-11-16 06:55:14 PST
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 :/
Comment 4 User image Jon Coppeard (:jonco) 2016-11-17 03:00:01 PST
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 User image Jan de Mooij [:jandem] 2016-11-17 03:27:06 PST
(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 User image Jason Orendorff [:jorendorff] 2016-11-17 07:39:35 PST
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 User image Jan de Mooij [:jandem] 2016-11-21 03:30:47 PST
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.
Comment 8 User image Jon Coppeard (:jonco) 2016-11-21 03:56:01 PST
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 User image Andrew McCreight [:mccr8] 2016-11-21 07:54:13 PST
[Tracking Requested - why for this release]: sec-high with disclosure date around the release date of Fx51.
Comment 11 User image Daniel Veditz [:dveditz] 2016-11-21 11:16:02 PST
It's OK to land this one now. Thanks for asking.
Comment 12 User image Jan de Mooij [:jandem] 2016-11-22 00:07:53 PST
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 User image Gerry Chang [:gchang] 2016-11-23 23:05:00 PST
Track 51+ as sec-high.
Comment 16 User image Jan de Mooij [:jandem] 2016-11-26 05:26:24 PST
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 User image Jan de Mooij [:jandem] 2016-11-26 05:27:00 PST
These approval requests apply to both patches.
Comment 18 User image Gerry Chang [:gchang] 2016-11-27 19:45:05 PST
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 20 User image Marcia Knous [:marcia - use ni] 2016-11-28 07:41:46 PST
Tracking 53- since this is resolved fixed.
Comment 22 User image Carsten Book [:Tomcat] 2016-11-29 07:41:29 PST
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 24 User image Ryan VanderMeulen [:RyanVM] 2016-11-29 18:05:54 PST
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 User image Ryan VanderMeulen [:RyanVM] 2016-11-30 06:08:13 PST
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 27 User image Ryan VanderMeulen [:RyanVM] 2016-11-30 20:40:53 PST
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 User image Ritu Kothari (:ritu) 2016-12-05 10:17:02 PST
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 User image Ryan VanderMeulen [:RyanVM] 2016-12-05 15:06:12 PST
Needs rebasing for mozilla-release.
Comment 31 User image Al Billings [:abillings] 2016-12-07 12:11:03 PST
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 User image Al Billings [:abillings] 2016-12-07 12:39:10 PST
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 User image Jan de Mooij [:jandem] 2016-12-07 14:13:33 PST
(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 User image Al Billings [:abillings] 2016-12-07 15:15:10 PST
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.

Note You need to log in before you can comment on or make changes to this bug.