Bug 54083 - Add WTF::cryptographicallyRandomNumber
Summary: Add WTF::cryptographicallyRandomNumber
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 22049 54155 54156
  Show dependency treegraph
 
Reported: 2011-02-09 02:10 PST by Adam Barth
Modified: 2011-02-09 17:46 PST (History)
9 users (show)

See Also:


Attachments
Patch (25.74 KB, patch)
2011-02-09 02:18 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (25.77 KB, patch)
2011-02-09 02:36 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (25.79 KB, patch)
2011-02-09 02:40 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (26.18 KB, patch)
2011-02-09 03:12 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (26.08 KB, patch)
2011-02-09 11:55 PST, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-02-09 02:10:10 PST
Add WTF::cryptographicallyRandomNumber
Comment 1 Adam Barth 2011-02-09 02:18:46 PST
Created attachment 81775 [details]
Patch
Comment 2 WebKit Review Bot 2011-02-09 02:29:01 PST
Attachment 81775 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7847345
Comment 3 Adam Barth 2011-02-09 02:36:54 PST
Created attachment 81780 [details]
Patch
Comment 4 Build Bot 2011-02-09 02:36:57 PST
Attachment 81775 [details] did not build on win:
Build output: http://queues.webkit.org/results/7819395
Comment 5 Adam Barth 2011-02-09 02:40:06 PST
Created attachment 81781 [details]
Patch
Comment 6 Early Warning System Bot 2011-02-09 02:43:09 PST
Attachment 81775 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7724346
Comment 7 Eric Seidel (no email) 2011-02-09 02:48:44 PST
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.
Comment 8 Adam Barth 2011-02-09 03:12:05 PST
Created attachment 81784 [details]
Patch
Comment 9 Eric Seidel (no email) 2011-02-09 03:21:12 PST
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()?
Comment 10 Early Warning System Bot 2011-02-09 04:24:02 PST
Attachment 81784 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7819417
Comment 11 Adam Barth 2011-02-09 11:07:58 PST
I'm slightly hesitant to change the core algorithm for fear of screwing it up, but I'll try to be careful.
Comment 12 Adam Barth 2011-02-09 11:55:00 PST
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.
Comment 13 Adam Barth 2011-02-09 11:55:59 PST
Created attachment 81845 [details]
Patch
Comment 14 Eric Seidel (no email) 2011-02-09 14:56:55 PST
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.
Comment 15 Adam Barth 2011-02-09 15:12:10 PST
Comment on attachment 81845 [details]
Patch

Clearing flags on attachment: 81845

Committed r78149: <http://trac.webkit.org/changeset/78149>
Comment 16 Adam Barth 2011-02-09 15:12:15 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 WebKit Review Bot 2011-02-09 15:27:58 PST
http://trac.webkit.org/changeset/78149 might have broken Qt Windows 32-bit Release
Comment 18 Patrick R. Gansterer 2011-02-09 16:20:35 PST
Can we merge OSRandomSource.h with RandomNumber.h? They are doing very similar things and both contain only one function.
Comment 19 Adam Barth 2011-02-09 16:32:11 PST
(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.