1312001 - (CVE-2017-5378) ASLR leak and cross-frame oracle via pointer scrambling in Map/Set

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jann Horn]

Attachment: firefox-aslr-break.tar

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.143 Safari/537.36

Steps to reproduce:

In Nightly (non-debug build) on x86-64, open the file attack.html from the attached tarball and wait around 15 seconds (depending on your CPU) until the displayed result is green or red.

For the second way to abuse the behavior described here, unpack leak_interned.html, an attacker, and leak_interned_victim.html, a victim page. This attack is less reliable. Open leak_interned.html in a fresh browser instance, click one of the three name buttons, click the "tell me what I clicked button", and then wait a couple seconds (I think you have to wait for a GC run to happen or so).


Actual results:

In the first PoC, something like this will be shown:

bucket access delay calibration: hit: 0.014999999999993463, miss: 0, limit: 0.0074999999999967315
current char code: 41
number of possible results: 1
result: 0x01f00028

The displayed result (0x01f00028) is the pointer to the atomized string corresponding to '\0', truncated to 32 bits. On Linux, the corresponding mapping is visible in /proc/<plugin-container-pid>/maps:

7f3001400000-7f3002000000 rw-p 00000000 00:00 0 


The basic idea here is to perform a hash DoS attack, but with a goal other than Denial of Service. After a single hashtable bucket has been filled with lots of entries, insertions and negative lookups on the hashtable will be much slower for keys that map to the filled hashmap bucket. This means that a timing measurement can be used to determine whether a given key maps to a specific hashtable bucket as follows:
 - Fill the hashtable bucket with data.
 - Measure the time needed to look up the key in the hashtable.

Filling a specific hashtable bucket with data is easy because:
 - int32 numbers are simply mapped with ScrambleHashCode(number),
   no unknown values are involved
 - the inverse of ScrambleHashCode() is just a modular multiplication with
   the multiplicative inverse of the constant scrambling factor, so it is possible
   to compute a series of int32 numbers that map to adjacent hash codes.

The reason this can be used to break ASLR is that, for strings and objects, 32-bit truncated pointers are scrambled with ScrambleHashCode() and then used to derive hashmap bucket indexes. Therefore, by measuring lookup times for strings/objects in a hashmap with a known filled bucket, the range of possible pointers can be constrained.

Strings are converted to pointers by atomizing (interning) them, then taking the pointer to the atom. For some strings, atoms are precreated; in particular, atoms exist for single-character strings with charcodes 0-255. Because these are allocated in a loop during runtime initialization (StaticStrings::init()), they form a sequence in memory, so the distance between the atomized strings for String.fromCharCode(i) and String.fromCharCode(i+1) (for i in range(0,255)) is always 0x18.

The algorithm used in the PoC works roughly as follows (this is just the basic idea, the implementation in the exploit is optimized a bit):

find_bucket_precise() attempts to find the bucket index at which a given value will be placed in a Set of size 2^22.
This is done as follows:
 - Locate the value in a Set with 64 buckets (find_bucket64()) by testing with 64 different
   Set instances that have different filled buckets and checking with which Set the operation is slow
 - Repeatedly create new Sets with doubled bucket counts (128, 256, 512, ...) and test
   whether the access at the doubled old bucket index is slow. Basically, determine the 22-bit bucket index
   bit by bit.

The main code attempts to find a single candidate for the address of the '\0' atom:
 - Compute the bucket index of '\0' with find_bucket_precise().
 - Let candidates_set be the result of deriving 32-bit truncated pointer candidates
   from the bucket index of '\0' using the inverse of ScrambleHashCode().
 - For characters '\1', '\2', '\3', ...:
   - Compute the character's bucket index with find_bucket_precise().
   - Derive 32-bit truncated pointer candidates from the character's bucket index.
   - Let current_0candidates be the result of subtracting character_code*0x18
     from the candidates for the current character.
   - Let candidates_set be the values that are contained in both candidates_set
     and current_0candidates.
 - Assert that candidates_set contains exactly one value.
 - Return the value in candidates_set.



This is not just usable to break ASLR, as demonstrated by the second PoC. Another issue is that, because atoms are shared across frames, the ability to determine bucket indexes can be used to determine whether another frame is using an atom with specific contents (because an atom can only move in memory if nobody is using it). I'm not entirely sure, but I think string literals are always atomized? So this can e.g. be used to leak information like usernames embedded in JavaScript code across websites. The PoC determines which one out of a set of possible names was selected in the other frame across origins.


Expected results:

Firefox should not leak addresses or information about strings in other frames to JavaScript code.

In V8, these attacks won't work because:

Objects can have private, random 32-bit members ("hash_code_symbol") specifying their hashmap buckets. The member is set when the object is used in a hashtable the first time.

Strings are not interned for usage in hashtables; instead, when a string is used in a hashtable for the first time, a hash of its contents is computed and stored in the string by String::ComputeAndSetHash().

It might make sense to copy V8's behavior here.

This bug is subject to a 90 day disclosure deadline. If 90 days elapse without a broadly available patch, I will post it publicly.

Comment 1

[Jan de Mooij [:jandem]]

(In reply to Jann Horn from comment #0)
> Objects can have private, random 32-bit members ("hash_code_symbol")
> specifying their hashmap buckets. The member is set when the object is used
> in a hashtable the first time.

We do have a mechanism to assign unique IDs to cells (Zone::getUniqueId), we should probably use that here. We should be careful not to regress perf though...

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Jan de Mooij [:jandem] from comment #1)
> We do have a mechanism to assign unique IDs to cells (Zone::getUniqueId), we
> should probably use that here. We should be careful not to regress perf
> though...

I wrote a patch to do this and it was indeed a performance regression, being about twice as slow for adding objects to a map.

Comment 3

[Jann Horn]

(In reply to Jon Coppeard (:jonco) from comment #2)
> (In reply to Jan de Mooij [:jandem] from comment #1)
> > We do have a mechanism to assign unique IDs to cells (Zone::getUniqueId), we
> > should probably use that here. We should be careful not to regress perf
> > though...
> 
> I wrote a patch to do this and it was indeed a performance regression, being
> about twice as slow for adding objects to a map.

Zone::getUniqueId() doesn't look very fast. It uses a hashmap, and it uses js::gc::NextCellUniqueId(), which uses an atomic increment-and-return, which iirc implies a cmpxchg loop on x86, and it does some optimization stuff.
Can't you generate a random number using thread-local randomness state or so and cram it into the object directly?

Comment 4

[Jann Horn]

s/, and it does some optimization stuff//, I have no idea what I was trying to write there.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Jann Horn from comment #3)
> Can't you generate a random number using thread-local randomness state or so
> and cram it into the object directly?

Unfortunately it's not that easy, object layout is heavily optimized and we have different kinds of objects. We do want something more efficient than that hashtable lookup though. For strings, things are a bit easier because we have a flags word we can steal a number of bits from. I'll think about it more.

Comment 6

[Marcia Knous [:marcia]]

Tracking 52+ because this bug involves leaking addresses or information thru JS.

Comment 7

[Daniel Veditz [:dveditz]]

[Tracking Requested - why for this release]:

Jan: Can you take this on, or find someone who will?

Comment 8

[Jan de Mooij [:jandem]]

I'm not really familiar with the Map/Set code or with HashTable algorithms.

Jason/Jeff, can you take a look at this?

Comment 9

[Jason Orendorff [:jorendorff]]

I think we should fix this by making ScrambleHashCode impossible to invert offline. Store a different random integer "key" in each OrderedHashTable; use it as input to a new two-argument SecureScrambleHashCode(key, hash).

Pros:

- only costs 1 word per Map/Set
- relatively uninvasive patch
- dumb (no need to separately produce a secure hash code for each type)
- also prevents HashDoS attacks more generally

Downsides:

- It can't just multiply `key * hash` -- it'd be too easy to reverse-engineer the key. We need (at most) a secure hashing function that has a key; the one I know about is SipHash.

- Not sure about the speed. SipHash-1-3 is secure enough and should be faster than what comment 3 describes, at least. But what the slowdown will be, I don't know.

Comment 10

[Jann Horn]

> I think we should fix this by making ScrambleHashCode impossible
to invert offline.

I think that that would almost certainly fix the ASLR leak part. I'm wondering about the remaining impact of being able to measure random collisions though. If an attacker could detect random collisions with buckets of size around 3 to 5 or so, that would probably still be sufficient for executing the attack on interned strings. And with small bucket counts, finding small random collisions shouldn't be too hard - and as soon as you have a small collision, finding more elements mapping into the same bucket is relatively easy.

Comment 11

[Jann Horn]

(In reply to Jann Horn from comment #10)
> > I think we should fix this by making ScrambleHashCode impossible
> > to invert offline.
> 
> I think that that would almost certainly fix the ASLR leak part. I'm
> wondering about the remaining impact of being able to measure random
> collisions though. If an attacker could detect random collisions with
> buckets of size around 3 to 5 or so, that would probably still be sufficient
> for executing the attack on interned strings.

I guess the nicest fix for that would be to have separate sets of interned strings for cross-origin frames, with only the predefined names common to all frames (in case you need them to be common for access to properties that are accessible cross-origin?). Would that work?

Comment 12

[Jon Coppeard (:jonco)]

Attachment: bug1312001-use-unique-id-for-map

FWIW, here's a patch to make Map and Set use unique IDs for GC things stored in the hashtable.  This fixes the problem, but is not great for performance.

Comment 13

[Jon Coppeard (:jonco)]

(In reply to Jon Coppeard (:jonco) from comment #12)
...and thinking about it some more it doesn't fix the problem of string collisions either.

I think we probably want to go with something like Jason suggested above, and use a per-compartment table to intern strings rather than system-wide atomization.

Comment 14

[Jason Orendorff [:jorendorff]]

(In reply to Jann Horn from comment #10)
> > I think we should fix this by making ScrambleHashCode impossible
> > to invert offline.
> 
> I think that that would almost certainly fix the ASLR leak part. I'm
> wondering about the remaining impact of being able to measure random
> collisions though. If an attacker could detect random collisions with
> buckets of size around 3 to 5 or so, that would probably still be sufficient
> for executing the attack on interned strings. And with small bucket counts,
> finding small random collisions shouldn't be too hard - and as soon as you
> have a small collision, finding more elements mapping into the same bucket
> is relatively easy.

Yep. You are right about all of this.

(In reply to Jann Horn from comment #11)
> I guess the nicest fix for that would be to have separate sets of interned
> strings for cross-origin frames, with only the predefined names common to
> all frames (in case you need them to be common for access to properties that
> are accessible cross-origin?). Would that work?

Or we can copy V8's behavior and use the same hash code for HashableValue that we use for the atoms table. That's a hash of the characters in the string, not the interned string's address, so it wouldn't change when an atom is GC'd.

But this means either adding a word to JSString; or caching hash codes in the OrderedHashTable, which increases its size by 1/4 *and* requires computing the hash code on every string-key lookup, which we currently skip when the key is already atomized. I hate to say it, but maybe Map and Set have to pay this cost. The alternatives seem to be "make the whole system pay".

Comment 15

[Jan de Mooij [:jandem]]

(In reply to Jason Orendorff [:jorendorff] from comment #14)
> But this means either adding a word to JSString;

Maybe we only have to add this word to atoms. It's annoying, because we already have a different string size for fat inline strings, but not impossible I think.

Strings have 25 bits available in the flags word, so another option is to store the hash code in there. If that's too small, we could also use these bits to store an index into a Vector<HashNumber>. If we maintain the invariant that there's a 1:1 mapping between JSAtom* and entry into this Vector, it should be easy to compact this Vector when we GC the atoms zone. When there are more than 2 ** 25 atoms (rare), we could use a HashMap<JSAtom*, HashNumber> for the remaining atoms.

Comment 16

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

Tracked for 50.1.0

Comment 17

[Jason Orendorff [:jorendorff]]

(In reply to Jan de Mooij [:jandem] from comment #15)
> (In reply to Jason Orendorff [:jorendorff] from comment #14)
> > But this means either adding a word to JSString;
> 
> Maybe we only have to add this word to atoms. It's annoying, because we
> already have a different string size for fat inline strings, but not
> impossible I think.

Great, let's do this one. It's fine for the hash code to be stored differently depending on some other bit -- what are methods for?

> Strings have 25 bits available in the flags word, so another option is to
> store the hash code in there. [...]

Wow, this went downhill fast. :)  I like the other idea better.

Comment 18

[Jason Orendorff [:jorendorff]]

OK, the summary so far: We have two bugs.

1.  Cross-domain information leak: a page can tell whether or not another page is using an atom, by trying to observe a string's hash code changing. If it changes, that means the atom was GC'd and recreated (at a different address), so no other page in the runtime is using that atom.  Possible fixes:

    *   Use separate atoms per domain; or

    *   Add a hash code field to all atoms and use that for HashableValue (instead of the
        pointer-based hash code we're currently using).

    I like the latter.

2.  Pointer leak: a page can reconstitute the hash code for a key by measuring performance effects of hash code collisions. The hash code of an object is the object's address. Possible fixes:

    *   Use a random hash code for objects, storing it either in each object or in a side table; or

    *   Do what comment 9 says.

    I'm not as sure here but let's try the latter and test performance.

Jeff, can you take this?

Comment 19

[Jan de Mooij [:jandem]]

(In reply to Jason Orendorff [:jorendorff] from comment #18)
>     *   Add a hash code field to all atoms and use that for HashableValue
> (instead of the
>         pointer-based hash code we're currently using).
> 
>     I like the latter.

I can try to do this part, as I'm probably most familiar with the string representation. It scares me a bit to backport a patch like this, but let's see how it turns out. Spinning this off to a new bug.

Comment 20

[Jan de Mooij [:jandem]]

(In reply to Jason Orendorff [:jorendorff] from comment #9)
> I think we should fix this by making ScrambleHashCode impossible to invert
> offline.

We should do something similar for ShapeTable (it doesn't use js::HashMap), as that's another map with {int31, atom, symbol} keys and is probably also vulnerable.

Comment 21

[Jan de Mooij [:jandem]]

What's the status here? With Hawaii and the holidays coming up we really don't have a lot of time.

Comment 22

[Jason Orendorff [:jorendorff]]

Attachment: Scramble hash codes securely, to avoid leaking bits of object and string addresses

MozReview-Commit-ID: yR1cIjrlPP

Comment 23

[Jason Orendorff [:jorendorff]]

This patch costs about 15% speed on microbenchmarks that just do Map.prototype.get in a tight loop, and 10% on other stuff like inserts. Jan, if you think we can do better by finding room for a hash code in JSObject, tell me where to look and I'll give it a shot.

Otherwise I'll look at ShapeTable next.

Comment 24

[Jason Orendorff [:jorendorff]]

Attachment: LookupHitTest-speed.png

Comment 25

[Jason Orendorff [:jorendorff]]

Attachment: LookupMissTest-speed.png

Comment 26

[Jason Orendorff [:jorendorff]]

Oh, I should try to make up the perf difference by storing hash keys in every table entry. I'll try that first.

Comment 27

[Jason Orendorff [:jorendorff]]

OK, comment 26 was a dumb idea. Caching hash codes in the table doesn't eliminate any prepareHash calls. No point doing it (specially since equality testing is so cheap here).

Comment 28

[Jan de Mooij [:jandem]]

(In reply to Jason Orendorff [:jorendorff] from comment #23)
> Jan, if you think we can do better by finding room for a hash code in JSObject,
> tell me where to look and I'll give it a shot.

No idea, I think this is really difficult.

> Otherwise I'll look at ShapeTable next.

For ShapeTable, we have jsid (= int/atom/symbol) keys. Int and atom jsids now use deterministic hashing that does not involve any memory addresses, so one option is to do the same for JS::Symbol (it has an unused word for GC alignment reasons that we could use to store a hash code). Fixing it in ShapeTable might be nice anyway to avoid DoS attacks, but adding a stable hash code to JS::Symbol and using it in HashId would be sufficient to stop the ASLR issues. What do you think?

Comment 29

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #28)
> Fixing it in ShapeTable might be nice anyway to avoid DoS attacks

Nevermind this comment, I just read the comments in the patch and it's not meant to protect against DoS.

Comment 30

[Jan de Mooij [:jandem]]

Comment on attachment 8815939 [details] [diff] [review]
Scramble hash codes securely, to avoid leaking bits of object and string addresses

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

Very nice!

::: js/src/jsgc.cpp
@@ +246,5 @@
>  using mozilla::ArrayLength;
>  using mozilla::Get;
>  using mozilla::Maybe;
>  using mozilla::Swap;
> +using mozilla::HashCodeScrambler;

Nit: keep this sorted.

::: js/src/vm/Runtime.cpp
@@ +787,5 @@
> +    return randomKeyGenerator_.ref();
> +}
> +
> +mozilla::HashCodeScrambler
> +JSRuntime::randomHashCodeScrambler() {

Nit: { on next line.

::: mfbt/HashFunctions.h
@@ +346,5 @@
> +    uint64_t sipHash(uint64_t m) {
> +      // 2. Compression.
> +      v3 ^= m;
> +      sipRound();
> +      v0 ^= m;

The reference implementation (https://github.com/veorq/SipHash/blob/master/siphash24.c) does another sipRound() here, IIUC. Is that not needed because our input length is constant and we don't have any bytes left?

@@ +372,5 @@
> +      v1 ^= v2;
> +      v2 = RotateLeft(v2, 32);
> +    }
> +
> +    uint64_t v0, v1, v2, v3;

Nit: MFBT so should probably use mV0, mV1 etc here, and a* prefix for arguments...

Comment 31

[Jason Orendorff [:jorendorff]]

> The reference implementation
> (https://github.com/veorq/SipHash/blob/master/siphash24.c) does another
> sipRound() here, IIUC. Is that not needed because our input length is
> constant and we don't have any bytes left?

The algorithm is designed for a variable number of rounds. SipHash-2-4 has 2 rounds per word of input, and 4 rounds to finish. SipHash-1-3 is considered sufficient for hash tables, according to: https://github.com/rust-lang/rust/issues/29754

Comment 32

[Jason Orendorff [:jorendorff]]

Attachment: Scramble hash codes securely, to avoid leaking bits of object and string addresses

MozReview-Commit-ID: yR1cIjrlPP

Comment 33

[Jason Orendorff [:jorendorff]]

Comment on attachment 8817055 [details] [diff] [review]
Scramble hash codes securely, to avoid leaking bits of object and string addresses

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

Not RCE but quite easy.

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

Absolutely, yes.

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

No, but easy. This is medium-low risk.

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

It needs the usual CI testing. Regressions are possible, since I'm changing the implementation of JS Map and Set, but unlikely. I've run the JS unit tests and everything passes.

Comment 34

[Jan de Mooij [:jandem]]

(In reply to Jason Orendorff [:jorendorff] from comment #23)
> This patch costs about 15% speed on microbenchmarks that just do
> Map.prototype.get in a tight loop, and 10% on other stuff like inserts.

I was thinking, as a followup we could maybe do this slower-scrambling only for object/symbol values? All other Values can be hashed without leaking memory addresses.

Comment 35

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

Comment on attachment 8817055 [details] [diff] [review]
Scramble hash codes securely, to avoid leaking bits of object and string addresses

sec-approval+ for trunk. We'll need this on affected branches (not release) as well.

Comment 36

[Jason Orendorff [:jorendorff]]

> We'll need this on affected branches (not release) as well.

I'm confused -- all branches are affected, including ESR. It's an old bug.

Comment 37

[Jason Orendorff [:jorendorff]]

OK, I just need to think a bit more about comment 28 before landing. I'll do it tomorrow.

Separately, we need to extend bug 1317936's if-statement in HashValue() to cover "registry" Symbols (the kind created by Symbol.for(str)). I'll do that in this bug. Second round of sec-approval :-P

Comment 38

[Jason Orendorff [:jorendorff]]

Attachment: Scramble hash codes securely, to avoid leaking bits of object and symbol addresses

MozReview-Commit-ID: yR1cIjrlPP

Comment 39

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

(In reply to Jason Orendorff [:jorendorff] from comment #36)
> > We'll need this on affected branches (not release) as well.
> 
> I'm confused -- all branches are affected, including ESR. It's an old bug.

This means "I want to see a patch made and nominated for Aurora, Beta, and ESR45 after this lands on Trunk. Please make and nominate this patch." Sorry if that wasn't clear.

Comment 40

[Jan de Mooij [:jandem]]

Comment on attachment 8820073 [details] [diff] [review]
Scramble hash codes securely, to avoid leaking bits of object and symbol addresses

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

::: js/src/builtin/MapObject.cpp
@@ +70,4 @@
>  {
>      // HashableValue::setValue normalizes values so that the SameValue relation
>      // on HashableValues is the same as the == relationship on
> +    // value.asRawBits(). So why not just return that? Security.

Thanks. It was on my TODO list to fix this comment later, but with the atom bug already fixed on all branches it seems fine to do this now.

@@ +83,5 @@
> +        Symbol* sym = v.toSymbol();
> +        if (sym->isWellKnownSymbol())
> +            return HashNumber(sym->code());
> +        if (sym->code() == SymbolCode::InSymbolRegistry)
> +            return sym->description()->hash();

If we need something similar for HashId, we can factor it out to HashSymbol or Symbol::hash() probably.

@@ +92,1 @@
>      return v.asRawBits();

MOZ_ASSERT(!v.isMarkable()); here to avoid regressing this when a new GC type is added.

::: js/src/jit-test/tests/collections/Map-scale.js
@@ +1,5 @@
>  // Maps can hold at least 64K values.
>  
>  var N = 1 << 16;
>  var m = new Map;
> +for (var i = 0; i < N; i++) {

Nit: revert test change.

Comment 41

[Jason Orendorff [:jorendorff]]

Comment on attachment 8820073 [details] [diff] [review]
Scramble hash codes securely, to avoid leaking bits of object and symbol addresses

NOTE: Same answers as last time (see comment 33) except the last question.
It's a new patch for the same issue.

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

Not RCE but quite easy.

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

Absolutely, yes.

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

No, but easy. This is medium-low risk.

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

It needs the usual CI testing. Regressions are possible, since I'm changing the implementation of JS Map and Set. A bit more likely than last time, as the patch is a bit more complex. I've run the JS unit tests and everything passes.

Comment 42

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

Comment on attachment 8820073 [details] [diff] [review]
Scramble hash codes securely, to avoid leaking bits of object and symbol addresses

sec-approval+ for trunk. We'll want this everywhere affected if we can do it soon.

Comment 43

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

Jann, our release is on January 24. Is there any way to move your disclosure out a week from the January 21 so we can get our release out the door first? We did fix the other related issue in 50.1 in bug 1317936 .

Comment 44

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c484c1e7eeb61f4abd6d9e2352eacd52b1a47cbf
Bug 1312001 - Scramble hash codes securely, to avoid leaking bits of object and symbol addresses.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c484c1e7eeb6

Please request Aurora/Beta/ESR45 approval (and attached rebased patches if necessary) ASAP.

Comment 46

[Jan de Mooij [:jandem]]

We also still have to fix the ShapeTable thing, right?

Comment 47

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb9955655951

Comment 48

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6df545f5d0d1

Comment 49

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7ab702b2974

Comment 50

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3ab56005733

Comment 51

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4cf8a0c64b7

Comment 52

[Jason Orendorff [:jorendorff]]

Attachment: (aurora) Scramble hash codes securely, to avoid leaking bits of object and symbol addresses

MozReview-Commit-ID: yR1cIjrlPP

Comment 53

[Jason Orendorff [:jorendorff]]

Attachment: (beta) Scramble hash codes securely, to avoid leaking bits of object and symbol addresses

MozReview-Commit-ID: yR1cIjrlPP

Comment 54

[Jason Orendorff [:jorendorff]]

Attachment: (release) Scramble hash codes securely, to avoid leaking bits of object and symbol addresses

MozReview-Commit-ID: yR1cIjrlPP

Comment 55

[Jason Orendorff [:jorendorff]]

Comment on attachment 8825939 [details] [diff] [review]
(aurora) Scramble hash codes securely, to avoid leaking bits of object and symbol addresses

Requesting approval for aurora. For answers to sec-approval questions, see comment 41.

Comment 56

[Jason Orendorff [:jorendorff]]

Comment on attachment 8825943 [details] [diff] [review]
(beta) Scramble hash codes securely, to avoid leaking bits of object and symbol addresses

Requesting approval for beta. For answers to sec-approval questions, see comment 41.

Comment 57

[Jason Orendorff [:jorendorff]]

Comment on attachment 8825944 [details] [diff] [review]
(release) Scramble hash codes securely, to avoid leaking bits of object and symbol addresses

Requesting approval for release. For answers to sec-approval questions, see comment 41.

Comment 58

[Jason Orendorff [:jorendorff]]

Comment on attachment 8825939 [details] [diff] [review]
(aurora) Scramble hash codes securely, to avoid leaking bits of object and symbol addresses

Approval Request Comment
[Feature/Bug causing the regression]: 1312001
[User impact if declined]: sec-high security risk that will be disclosed this month.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: Unzip the tar from the original report. Visit the file: URL for the attack.html file. After a second or two, you should see "NO RESULT FOUND (CONTRADICTION)". If you see a hexadecimal number instead, that's bad.
[List of other uplifts needed for the feature/fix]: Everywhere.
[Is the change risky?]: Slightly.
[Why is the change risky/not risky?]: Risky because it's a largeish patch that affects JS Map/Set code; slight because that code is well tested.
[String changes made/needed]: none

Comment 59

[Jason Orendorff [:jorendorff]]

Comment on attachment 8825943 [details] [diff] [review]
(beta) Scramble hash codes securely, to avoid leaking bits of object and symbol addresses

See comment 58.

Comment 60

[Jason Orendorff [:jorendorff]]

Comment on attachment 8825944 [details] [diff] [review]
(release) Scramble hash codes securely, to avoid leaking bits of object and symbol addresses

See comment 58.

Comment 61

[Jason Orendorff [:jorendorff]]

This stuff backported pretty cleanly. There was a conflict with rev aeb98c845e96, easily fixed.

Comment 62

[Jason Orendorff [:jorendorff]]

Attachment: (esr45) Scramble hash codes securely, to avoid leaking bits of object and symbol addresses

MozReview-Commit-ID: yR1cIjrlPP

Comment 63

[Jason Orendorff [:jorendorff]]

Comment on attachment 8825939 [details] [diff] [review]
(aurora) Scramble hash codes securely, to avoid leaking bits of object and symbol addresses

(Ugh. hg bzexport marked this obsolete for me.)

See comment 58.

Comment 64

[Jason Orendorff [:jorendorff]]

Comment on attachment 8825943 [details] [diff] [review]
(beta) Scramble hash codes securely, to avoid leaking bits of object and symbol addresses

See comment 58.

Comment 65

[Jason Orendorff [:jorendorff]]

Comment on attachment 8825944 [details] [diff] [review]
(release) Scramble hash codes securely, to avoid leaking bits of object and symbol addresses

See comment 58.

Comment 66

[Jason Orendorff [:jorendorff]]

Comment on attachment 8825986 [details] [diff] [review]
(esr45) Scramble hash codes securely, to avoid leaking bits of object and symbol addresses

[Approval Request Comment]
User impact if declined: sec-high security risk
Fix Landed on Version: 50 (current Nightly)
Risk to taking this patch (and alternatives if risky): slight (see comment 58)
String or UUID changes made by this patch: none

Comment 67

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8171cab46d7f

Comment 68

[Jann Horn]

(In reply to Al Billings [:abillings] from comment #43)
> Jann, our release is on January 24. Is there any way to move your disclosure
> out a week from the January 21 so we can get our release out the door first?
> We did fix the other related issue in 50.1 in bug 1317936 .

Sorry about the late reply.

Yes, that's fine with me.

Comment 69

[Julien Cristau [:jcristau]]

Comment on attachment 8825944 [details] [diff] [review]
(release) Scramble hash codes securely, to avoid leaking bits of object and symbol addresses

there won't be a 50 dot release for this

Comment 70

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

sec-approval+ if that is needed here.

Comment 71

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8825943 [details] [diff] [review]
(beta) Scramble hash codes securely, to avoid leaking bits of object and symbol addresses

This should land for today's beta 14 build.

Comment 72

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a4996e963d0d
https://hg.mozilla.org/releases/mozilla-beta/rev/91be97c8b0ac

Talked this over with jandem and decided it was better to move the ShapeTable work to a follow-up bug since this one is already getting confusing enough to track.

Comment 73

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/c5e67d41bdd0

Comment 74

[Matt Wobensmith [:mwobensmith][:matt:]]

I was able to see the behavior on Windows and Linux using "attack.html" PoC. The Mac doesn't seem to exhibit the issue using this test case. Also, I was never able to get the other PoC to work on any platform. 

Confirmed bug on Fx50 and Fx53, using builds from 2016-11.
Verified fixed on Fx 51.0b14 and Fx 45.7.0 ESR.

Jann, have you tried our latest builds to make sure that we have fixed this issue?

Comment 75

[Jann Horn]

Both POCs stopped working for me in Nightly, and the patches look sane to me (but I'm not really familiar with the Firefox codebase).