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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

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.

Comment 1

[Jason Orendorff [:jorendorff]]

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

Comment 2

[Jason Orendorff [:jorendorff]]

Aside: The random number seed should be per-global (stored in JSCompartment) rather than per-context anyway.

Comment 3

[Jason Orendorff [:jorendorff]]

Talked it over with decoder and settled on the rt->nonce++ approach.

Patch posted in bug 820180.

Comment 4

[Jason Orendorff [:jorendorff]]

Fixed in bug 820180.