Bug 54083 – Add WTF::cryptographicallyRandomNumber

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Add WTF::cryptographicallyRandomNumber

[Adam Barth]

Created attachment 81775 [details]
Patch

[WebKit Review Bot]

Attachment 81775 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7847345

[Adam Barth]

Created attachment 81780 [details]
Patch

[Build Bot]

Attachment 81775 [details] did not build on win:
Build output: http://queues.webkit.org/results/7819395

[Adam Barth]

Created attachment 81781 [details]
Patch

[Early Warning System Bot]

Attachment 81775 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7724346

[Eric Seidel (no email)]

Comment on attachment 81775 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81775&action=review

Seems OK, but I would give it one more round of bathing.

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:51
> +static int randomStreamInitialized;
> +static struct ARC4Stream randomStream;
> +static int streamCount;

Really this should just all be a class and have one shared instance of said class.  A bunch of globals and inline global functions isn't very webkitty.

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:56
> +inline void initMutexIfNeeded()

WebKit would use a method with a static variable in it, I think.  No?  sharedMutex(), etc.

> Source/JavaScriptCore/wtf/OSRandomSource.cpp:59
> +    // FIXME: What do we do if we can't read /dev/urandom?

CRASH() maybe?

> Source/JavaScriptCore/wtf/OSRandomSource.cpp:63
> +    // WARNING: When adding new sources of OS randomness, the randomness must
> +    //          be of cryptographic quality!

Do you want to add a line like "if you ahve any doubt, please CC abarth on the bug?" :)

> Source/JavaScriptCore/wtf/OSRandomSource.h:34
> +// partially filled.  Rather than calling this function directly, consider

Seems odd that there is no way to return to the caller that it was a partial fill.

[Adam Barth]

Created attachment 81784 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 81784 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81784&action=review

Wow, looks so much better!

Of course now that you've cleaned it up more, I understand it more, and thus have more questions about the rest of it.  I'd like to see one more round if you have the stamina.

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:44
> +struct ARC4Stream {

This could be internal to the class, but it's OK as is.

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:78
> +    for (int n = 0; n < 256; n++)
> +        m_stream.s[n] = n;
> +    m_stream.i = 0;
> +    m_stream.j = 0;

This feels like the constructor for the stream class.

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:81
> +void ARC4RandomNumberGenerator::addRandomData(unsigned char *data, int length)

* goes to the left.  Should this take a Vector/CString type as input?

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:85
> +        m_stream.i = (m_stream.i + 1);

This is just ++.

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:87
> +        m_stream.j = (m_stream.j + si + data[n % length]);

This is just +=.

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:89
> +        m_stream.s[m_stream.i] = m_stream.s[m_stream.j];
> +        m_stream.s[m_stream.j] = si;

I don't really understand what this is doing, but that's probably OK. :)

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:105
> +    m_count = 1600000;

What's this magical value do?  It looks like it makes us stir every 40,000 numbers.  Why?  Seems we should add that as a comment?

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:110
> +    if (m_count <= 0 || !m_initialized)

Does count ever wrap?  Or is this just a double-check of m_initialized? (since they're both set to 0 in teh constructor).

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:116
> +    m_stream.i = (m_stream.i + 1);

++.  This feels familiar.

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:118
> +    m_stream.j = (m_stream.j + si);

Again +=.  Difficult to tell what this is doing.  Should this be sharing code with addRandomData?

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:132
> +    val = getByte() << 24;
> +    val |= getByte() << 16;
> +    val |= getByte() << 8;
> +    val |= getByte();
> +    return val;

I would probably have just written this as one line, but maybe that's lazy of me.

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:143
> +    m_count -= 4;

Why is this decrement not in getWord() itself?

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:160
> +        if (--m_count <= 0)
> +            stir();

Isn't this just stirIfNeeded()?

[Early Warning System Bot]

Attachment 81784 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7819417

[Adam Barth]

I'm slightly hesitant to change the core algorithm for fear of screwing it up, but I'll try to be careful.

[Adam Barth]

Comment on attachment 81784 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81784&action=review

>> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:44

> 
> This could be internal to the class, but it's OK as is.

It's already in an anonymous namespace!  How much more hidden do you want.  :)

>> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:78
>> +    for (int n = 0; n < 256; n++)
>> +        m_stream.s[n] = n;
>> +    m_stream.i = 0;
>> +    m_stream.j = 0;
> 
> This feels like the constructor for the stream class.

Done.

>> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:81
>> +void ARC4RandomNumberGenerator::addRandomData(unsigned char *data, int length)
> 
> * goes to the left.  Should this take a Vector/CString type as input?

I've moved the * but left it non-vectorized.

>> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:85
>> +        m_stream.i = (m_stream.i + 1);
> 
> This is just ++.

k

>> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:87
>> +        m_stream.j = (m_stream.j + si + data[n % length]);
> 
> This is just +=.

k

>> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:89
>> +        m_stream.s[m_stream.j] = si;
> 
> I don't really understand what this is doing, but that's probably OK. :)

It's just RC4.

>> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:105
>> +    m_count = 1600000;
> 
> What's this magical value do?  It looks like it makes us stir every 40,000 numbers.  Why?  Seems we should add that as a comment?

It's just some state of the RC4 algorithm.  I think the value is pretty arbitrary with a large margin of safety.

>> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:110
>> +    if (m_count <= 0 || !m_initialized)
> 
> Does count ever wrap?  Or is this just a double-check of m_initialized? (since they're both set to 0 in teh constructor).

m_count goes down every time you pull out randomness.  I've removed m_initialized.

>> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:116
>> +    m_stream.i = (m_stream.i + 1);
> 
> ++.  This feels familiar.

done.

>> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:118
>> +    m_stream.j = (m_stream.j + si);
> 
> Again +=.  Difficult to tell what this is doing.  Should this be sharing code with addRandomData?

Hum..  That seems like too risky a change.  I've changed this to +=, but I haven't changed this to share code.

>> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:143
>> +    m_count -= 4;
> 
> Why is this decrement not in getWord() itself?

Maybe so stirIfNeeded sees the lower count?  Dunno.

>> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:160
>> +        if (--m_count <= 0)
>> +            stir();
> 
> Isn't this just stirIfNeeded()?

Done.

[Adam Barth]

Created attachment 81845 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 81845 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81845&action=review

Looks good.  Thanks for all the cleanup!

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:164
> +        stirIfNeeded();

I would have just put all the stirIfNeeded and count stuff in getByte and bumped the initial m_count up by how much one is supposed to disgard at the beginning (or just reset the m_count after the initial discard).  But this is OK.

[Adam Barth]

Comment on attachment 81845 [details]
Patch

Clearing flags on attachment: 81845

Committed r78149: <http://trac.webkit.org/changeset/78149>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/78149 might have broken Qt Windows 32-bit Release

[Patrick R. Gansterer]

Can we merge OSRandomSource.h with RandomNumber.h? They are doing very similar things and both contain only one function.

[Adam Barth]

(In reply to comment #18)
> Can we merge OSRandomSource.h with RandomNumber.h? They are doing very similar things and both contain only one function.

Conceptually there are very different different.  OSRandomSource is an abstraction for OS-specific hardware randomness used internally by WTF.  RandomNumber is a weak PRNG for use by clients of WTF.  It would make more sense to have RandomNumberSeed use OSRandomSource on platforms where it's available.