Closed Bug 819775 Opened 7 years ago Closed 7 years ago

js_InitRandom: Don't use pointer values in seeding the random number generator

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox21 + fixed
firefox-esr10 --- wontfix
firefox-esr17 --- wontfix
b2g18 --- wontfix

People

(Reporter: jorendorff, Unassigned)

Details

(Keywords: sec-moderate, Whiteboard: [js:t][adv-main21+])

Follow-up to bug 811355.

We have this code (word-wrapped for bugzilla):
> void
> js_InitRandom(JSContext *cx)
> {
>     /*
>      * 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->rngSeed,
>         (PRMJ_Now() / 1000) ^ int64_t(cx) ^ int64_t(cx->getNext()));
> }

cx->getNext() is always null here.

This is not exploitable by itself, but I'd be shocked if an attacker couldn't recover cx. sec-moderate.
What to do about it? It depends on the desired behavior.

Maybe all we're trying to avoid is opening a JS demo in four windows and having the random number stream be the same in all four; that is, avoid having the non-randomness be embarrassingly obvious. If that's the case, we can use rt->nonce++ instead of int64_t(cx).

If there is any security requirement at all, the weakest one I can think of is:

    Scripts in one domain should not be able to guess the RNG seed
    of a new window in another domain.

That is actually a reasonable property to consider providing. And we can file a separate bug in the clear for that, and land it, and it'll look like a general tightening-up rather than a fix for an existing issue.

(I don't know how to do it short of going to the OS for secure-random bits, at least once per runtime and more likely once per context. But that's an acceptable cost, right?)
Aside: The random number seed should be per-global (stored in JSCompartment) rather than per-context anyway.
Talked it over with decoder and settled on the rt->nonce++ approach.

Patch posted in bug 820180.
Whiteboard: [js:t]
Fixed in bug 820180.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [js:t] → [js:t][adv-main20+]
Whiteboard: [js:t][adv-main20+] → [js:t][adv-main21+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.