Last Comment Bug 475585 - (CVE-2008-5913) Re-seed Math.random() for each window/frame/context
(CVE-2008-5913)
: Re-seed Math.random() for each window/frame/context
Status: RESOLVED FIXED
[sg:low], fixed-in-tracemonkey
: privacy
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla1.9.3a5
Assigned To: Andreas Gal :gal
:
:
Mentors:
Depends on: 511328
Blocks: 464071
  Show dependency treegraph
 
Reported: 2009-01-27 12:18 PST by Johnathan Nightingale [:johnath]
Modified: 2010-08-17 15:07 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
needed
.4-fixed
needed
.10-fixed


Attachments
patch (5.93 KB, patch)
2009-09-15 16:31 PDT, Andreas Gal :gal
jwalden+bmo: review+
dolske: review+
mbeltzner: approval1.9.2.4+
mbeltzner: approval1.9.1.10+
Details | Diff | Splinter Review
Backport for 1.9.0 (5.64 KB, patch)
2010-06-24 09:36 PDT, Mike Hommey [:glandium]
jwalden+bmo: review+
gal: review+
dveditz: approval1.9.0.next+
Details | Diff | Splinter Review

Description Johnathan Nightingale [:johnath] 2009-01-27 12:18:00 PST
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 2009-09-15 16:31:09 PDT
Created attachment 400900 [details] [diff] [review]
patch
Comment 2 Andreas Gal :gal 2009-09-15 16:33:26 PDT
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 2009-09-15 16:35:08 PDT
Fly-by reviews welcome too.
Comment 4 Andreas Gal :gal 2009-09-15 16:35:58 PDT
dmandelin suggested seeding with the context pointer. dolske suggested making sure we don't just seed with one pointer.
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2009-09-15 17:19:00 PDT
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] (remove +bmo to email) 2009-09-15 17:26:57 PDT
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] 2009-09-15 19:29:30 PDT
(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] 2009-09-15 19:32:10 PDT
(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] 2009-09-16 15:10:32 PDT
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 10 Andreas Gal :gal 2009-09-17 11:27:35 PDT
*** Bug 464071 has been marked as a duplicate of this bug. ***
Comment 11 Andreas Gal :gal 2009-09-17 11:30:20 PDT
We should try to get this into the beta since this is observable. I will land the patch shortly.
Comment 12 Johnathan Nightingale [:johnath] 2010-03-18 07:55:45 PDT
Hey - did this ever make it in? If not, can it?
Comment 13 Andreas Gal :gal 2010-03-18 08:17:18 PDT
bug 511328 took over a large part of this (making rngSeed thread local). Will land the rest.
Comment 14 Andreas Gal :gal 2010-03-18 08:19:19 PDT
I take that back. This patch wants the seed in the context. So its just a simple re-basing.
Comment 15 Andreas Gal :gal 2010-03-18 08:30:11 PDT
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] 2010-03-18 09:02:00 PDT
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] 2010-03-18 09:11:18 PDT
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] 2010-03-31 13:18:44 PDT
has this landed on trunk yet?
Comment 19 Andreas Gal :gal 2010-03-31 13:36:24 PDT
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] 2010-04-05 10:18:09 PDT
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 22 Reed Loden [:reed] (use needinfo?) 2010-04-12 21:38:33 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/23e155b902ac
Comment 23 Reed Loden [:reed] (use needinfo?) 2010-04-12 22:35:36 PDT
Can't land this on 1.9.1 until bug 511328 lands there first...
Comment 24 Reed Loden [:reed] (use needinfo?) 2010-04-13 14:30:43 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5c5efa71e43a
Comment 25 Mike Hommey [:glandium] 2010-05-03 23:26:28 PDT
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] 2010-05-04 00:43:55 PDT
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) 2010-05-04 06:21:10 PDT
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] 2010-06-24 09:36:03 PDT
Created attachment 453755 [details] [diff] [review]
Backport for 1.9.0
Comment 29 Mike Hommey [:glandium] 2010-06-24 09:39:53 PDT
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 2010-06-24 09:42:40 PDT
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) 2010-07-18 22:08:07 PDT
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 2010-07-19 11:15:26 PDT
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] 2010-07-22 19:22:44 PDT
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] 2010-08-14 22:20:44 PDT
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: gavin@gavinsharp.com] 2010-08-17 15:07:11 PDT
(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.

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