475585 - (CVE-2008-5913) Re-seed Math.random() for each window/frame/context

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Johnathan Nightingale [:johnath]]

After discussion today, we agreed that it would be nice if each context got its own Math.random() seed, to reduce the risk of things like cross-site tracking based on randomness seed.

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

The attached patch makes the RNG context-local and seeds it when the context is created. In addition to time the context pointer and its successor pointer are used for the seed, in case we bring up a number of contexts at the same time.

Comment 3

[Andreas Gal :gal]

Fly-by reviews welcome too.

Comment 4

[Andreas Gal :gal]

dmandelin suggested seeding with the context pointer. dolske suggested making sure we don't just seed with one pointer.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 400900 [details] [diff] [review]
patch

>+js_InitRandom(JSContext *cx)
> {
>-    /* Finally, set the seed from current time. */
>-    random_setSeed(data, PRMJ_Now() / 1000);
>+    /*
>+     * Set the seed from current time. Since we have a RNG per context and we often bring
>+     * up several contexts at the same time, we xor in some additional values, namely
>+     * the context and its successor. We don't just use the context because it might be
>+     * possible to reverse engineer the context pointer if one guesses the time right.
>+     */
>+    random_setSeed(cx,
>+                   (PRMJ_Now() / 1000) ^
>+                   int64(cx) ^
>+                   int64(cx->link.next));
> }

For the first context, |cx == cx->link.next| as I understand it, so the seed is thus |timeSecs ^ (a ^ a)|, which collapses to |(timeSecs ^ 0) == timeSecs|.  The first context in the browser is apparently for the JS component loader, so it's going to be long-lived.  If we're really worried about pages snooping on each other in this way, we should be worried about pages snooping on components which might possibly take user-affected inputs (foxyproxy would be one such component) and which use Math.random() (fewer still, but not out of the question).  The first context even runs privileged code, making it more interesting to attack.  I suggest making the last piece of this int64(JS_LIKELY(cx->link.next != cx) ? cx->link.next : 0) as a precaution in this case so we'll actually do a small amount of fudging to an otherwise time-based seed in that case.

Comment 6

[Jeff Walden [:Waldo]]

Although, I suppose in that case the reverse-engineer-the-JSContext* attack is then feasible again; that probably doesn't matter that much, but if it does we could always use |cx->runtime| instead.

We really need to decide on a cryptographically-secure random number generation API, propose it for ES6, and implement it.  This half-concern, half-you're-doing-it-wrong attitude we have is just making more and more work for us.

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #5)
> (From update of attachment 400900 [details] [diff] [review])
> >+js_InitRandom(JSContext *cx)
> > {
> >-    /* Finally, set the seed from current time. */
> >-    random_setSeed(data, PRMJ_Now() / 1000);
> >+    /*
> >+     * Set the seed from current time. Since we have a RNG per context and we often bring
> >+     * up several contexts at the same time, we xor in some additional values, namely
> >+     * the context and its successor. We don't just use the context because it might be
> >+     * possible to reverse engineer the context pointer if one guesses the time right.
> >+     */
> >+    random_setSeed(cx,
> >+                   (PRMJ_Now() / 1000) ^
> >+                   int64(cx) ^
> >+                   int64(cx->link.next));
> > }
> 
> For the first context, |cx == cx->link.next| as I understand it,

No, there's a header, rt->contextList.

/be

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #6)
> We really need to decide on a cryptographically-secure random number generation
> API, propose it for ES6, and implement it.  This half-concern,
> half-you're-doing-it-wrong attitude we have is just making more and more work
> for us.

No one is going to use a new API (especially one that can't be emulated if not supplied by older browsers) for years.

Anyway bug 322529 proposes to strengthen Math.random() sufficiently. Then the only fear is stupid ND benchmarketing noticing the regression.

/be

Comment 9

[Justin Dolske [:Dolske]]

Comment on attachment 400900 [details] [diff] [review]
patch

>+    random_setSeed(cx,
>+                   (PRMJ_Now() / 1000) ^
>+                   int64(cx) ^
>+                   int64(cx->link.next));

The only nit I might add is that cx ^ cx->link.next won't give you many bits if they're close to each other.

Comment 11

[Andreas Gal :gal]

We should try to get this into the beta since this is observable. I will land the patch shortly.

Comment 12

[Johnathan Nightingale [:johnath]]

Hey - did this ever make it in? If not, can it?

Comment 13

[Andreas Gal :gal]

bug 511328 took over a large part of this (making rngSeed thread local). Will land the rest.

Comment 14

[Andreas Gal :gal]

I take that back. This patch wants the seed in the context. So its just a simple re-basing.

Comment 15

[Andreas Gal :gal]

Seriously lame that it took so long. Sorry. Jonathan, if you need this for any of the product branches, could you twiddle the flags and request approval accordingly? Otherwise this will flow into m-c with the next TM merge.

http://hg.mozilla.org/tracemonkey/rev/2851cb841760

Comment 16

[Johnathan Nightingale [:johnath]]

No sweat - let's see whether nightly testers uncover any excitement - feels pretty safe to take on branch, but I'm not clamoring for it yet, either. I'll tweak the flag anyhow, so that we get to it in the next triage pass.

Comment 17

[Johnathan Nightingale [:johnath]]

Comment on attachment 400900 [details] [diff] [review]
patch

Assuming this bakes on trunk without incident, it is a minor security fix and I think I understand Andreas to believe it's pretty safe as well.

Comment 18

[Daniel Veditz [:dveditz]]

has this landed on trunk yet?

Comment 19

[Andreas Gal :gal]

Not on trunk yet afaict. sayrer, can you lend a hand? Should be easy to cherry-pick without conflicts if needed.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

I'm fine with this landing on mozilla-central and mozilla-1.9.2 and mozilla-1.9.1 at the same time, as long as it does land at the same time :)

a=beltzner for 1.9.2.4 and 1.9.1.10

Comment 21

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/2851cb841760

Comment 22

[Reed Loden [:reed]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/23e155b902ac

Comment 23

[Reed Loden [:reed]]

Can't land this on 1.9.1 until bug 511328 lands there first...

Comment 24

[Reed Loden [:reed]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5c5efa71e43a

Comment 25

[Mike Hommey [:glandium]]

I don't know yet if third party software use these APIs, but this patch breaks ABI on 1.9.2. On 1.9.1, it also slightly unbreaks ABI from the changes from bug 511328.

Comment 26

[Mike Hommey [:glandium]]

Good news, none of the third party software using libmozjs in debian use the js_LeaveTrace and js_CanLeaveTrace functions which binary compatibility is broken by this change.

Comment 27

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

You should probably audit that 3rd-party software to make sure that none of it uses js_* (contra JS_*), because those aren't API.

Comment 28

[Mike Hommey [:glandium]]

Attachment: Backport for 1.9.0

Comment 29

[Mike Hommey [:glandium]]

Comment on attachment 453755 [details] [diff] [review]
Backport for 1.9.0

This is a backport of the Math.random() changes (both bug 511328 and this bug) from 1.9.1.10 on 1.9.0.x. We need that on Debian for our stable users, even though Mozilla dropped support for that branch. This backport seems to work properly, though I haven't tested if it does what it is supposed to (is there a known exploit or testcase, btw?)

Anyways, I'd appreciate some more eyes taking a look at this patch.

Comment 30

[Andreas Gal :gal]

Comment on attachment 453755 [details] [diff] [review]
Backport for 1.9.0

Looks good to me.

Comment 31

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 453755 [details] [diff] [review]
Backport for 1.9.0

Since Mike posted the backport here and got reviews, I'm going to go ahead and request approval1.9.0.next on his patch so that we can get it in CVS for future non-MoCo-driven 1.9.0-based security releases.

Ordinarily I'd volunteer to check it in, too, except that AFAIK /js is still a restricted partition in CVS, so if this gets approval, I'll need to ask someone with access to the /js partition and who hasn't forgotten how to use CVS to check the backport in there. :(

Comment 32

[Jesse Ruderman]

In addition to this bug and bug 511328, there's also bug 577512 and bug 464071 to consider in the backport.

Comment 33

[Daniel Veditz [:dveditz]]

Comment on attachment 453755 [details] [diff] [review]
Backport for 1.9.0

Approved for 1.9.0.20, a=dveditz

Comment 34

[Justin Dolske [:Dolske]]

Comment on attachment 453755 [details] [diff] [review]
Backport for 1.9.0

The r+ from waldo and gal were sufficient here.

Comment 35

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #33)
> Comment on attachment 453755 [details] [diff] [review]

Don't think there's much point in taking this on 1.9.0 without its followup, bug 577512.