Bug 475585 (CVE-2008-5913)

Re-seed Math.random() for each window/frame/context

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Core
JavaScript Engine
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: johnath, Assigned: gal)

Tracking

({privacy})

Trunk
mozilla1.9.3a5
privacy
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking1.9.2 needed, status1.9.2 .4-fixed, blocking1.9.1 needed, status1.9.1 .10-fixed)

Details

(Whiteboard: [sg:low], fixed-in-tracemonkey)

Attachments

(2 attachments)

(Reporter)

Description

9 years ago
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.

Updated

9 years ago
Blocks: 464071
Whiteboard: [sg:low]
(Assignee)

Comment 1

8 years ago
Created attachment 400900 [details] [diff] [review]
patch
Assignee: general → gal
(Assignee)

Comment 2

8 years ago
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.
(Assignee)

Updated

8 years ago
Attachment #400900 - Flags: review?(jwalden+bmo)
Attachment #400900 - Flags: review?(dolske)
(Assignee)

Comment 3

8 years ago
Fly-by reviews welcome too.
(Assignee)

Comment 4

8 years ago
dmandelin suggested seeding with the context pointer. dolske suggested making sure we don't just seed with one pointer.
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.
Attachment #400900 - Flags: review?(jwalden+bmo) → review+
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.
(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
(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
Attachment #400900 - Flags: review?(dolske) → review+
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.
(Assignee)

Updated

8 years ago
Duplicate of this bug: 464071
(Assignee)

Comment 11

8 years ago
We should try to get this into the beta since this is observable. I will land the patch shortly.
Flags: wanted1.9.2?
(Reporter)

Comment 12

8 years ago
Hey - did this ever make it in? If not, can it?
(Assignee)

Comment 13

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

Comment 14

8 years ago
I take that back. This patch wants the seed in the context. So its just a simple re-basing.
(Assignee)

Comment 15

8 years ago
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
Whiteboard: [sg:low] → [sg:low], fixed-in-tracemonkey
(Reporter)

Comment 16

8 years ago
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.
(Reporter)

Comment 17

8 years ago
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.
Attachment #400900 - Flags: approval1.9.2.3?
status1.9.1: --- → wanted
status1.9.2: --- → wanted
Flags: wanted1.9.2?
has this landed on trunk yet?
blocking1.9.1: --- → needed
blocking1.9.2: --- → needed
(Assignee)

Comment 19

7 years ago
Not on trunk yet afaict. sayrer, can you lend a hand? Should be easy to cherry-pick without conflicts if needed.
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
Attachment #400900 - Flags: approval1.9.2.4?
Attachment #400900 - Flags: approval1.9.2.4+
Attachment #400900 - Flags: approval1.9.1.10+

Comment 21

7 years ago
http://hg.mozilla.org/mozilla-central/rev/2851cb841760
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Alias: CVE-2008-5913
Keywords: privacy
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/23e155b902ac
status1.9.2: wanted → .4-fixed
Target Milestone: --- → mozilla1.9.3a5
Keywords: checkin-needed
Depends on: 511328
Can't land this on 1.9.1 until bug 511328 lands there first...
Keywords: checkin-needed
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5c5efa71e43a
status1.9.1: wanted → .10-fixed
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.
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.
You should probably audit that 3rd-party software to make sure that none of it uses js_* (contra JS_*), because those aren't API.
Created attachment 453755 [details] [diff] [review]
Backport for 1.9.0
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.
Attachment #453755 - Flags: review?(dolske)
Attachment #453755 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 30

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

Looks good to me.
Attachment #453755 - Flags: review+
Attachment #453755 - Flags: review?(jwalden+bmo) → review+
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. :(
Attachment #453755 - Flags: approval1.9.0.next?

Comment 32

7 years ago
In addition to this bug and bug 511328, there's also bug 577512 and bug 464071 to consider in the backport.
Comment on attachment 453755 [details] [diff] [review]
Backport for 1.9.0

Approved for 1.9.0.20, a=dveditz
Attachment #453755 - Flags: approval1.9.0.next? → approval1.9.0.next+
Comment on attachment 453755 [details] [diff] [review]
Backport for 1.9.0

The r+ from waldo and gal were sufficient here.
Attachment #453755 - Flags: review?(dolske)
(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.
You need to log in before you can comment on or make changes to this bug.