Remove blink's use of RC4 for random value generation.

Remove blink's use of RC4 for random value generation.

This re-implements Blink's random number generator (wtf::cryptographicallyRandomValues) in terms of calling crypto::RandomBytes() directly, rather than using an ARC4 keystream that periodically stirs in system randomness.

Reason: RC4 (ARC4) has known weaknesses and has already been disabled as an accepted TLS cipher for Chrome M48. It should not be used internally for generating random numbers in Blink.

The way cryptographically random number generation worked in Blink prior to this patch is that wtf::cryptographicallyRandomValues() would generate random bytes using an ARC4 keystream. Every 1.6MB of generated data it would stir in randomness obtained via Platform::current()->cryptographicallyRandomValues().

The way it works after this patchset is that wtf::cryptographicallyRandomValues() directly calls Platform::current()->cryptographicallyRandomValues(), without layering on its own PRNG. The concrete implementation of Platform::cryptographicallyRandomValues() now calls crypto::RandBytes() [1], which provides good cryptographically secure random numbers by reading from hardware/system randomness sources.

The consequences of this change are:

 * The fixed sequence of random numbers seen by certain tests will have changed. I haven't observed this to be a problem with any of the tests though.

 * The performance characteristics of cryptographicallyRandomValues() have changed, for the worse. Measured using a microbenchmark, window.crypto.getRandomValues() is almost 5x slower now. This is not all that surprising since RC4 was pretty fast, and was only mixing in system randomness every 1.6MB (my test generated 256MB).

[1] Technically it is calling base::RandBytes(), but under the hood that calls crypto::RandBytes(). Will fix that dependency separately.

BUG=552749
Committed: https://crrev.com/9224aa4826d29930a8194a58dfd7170411bfc672
Cr-Commit-Position: refs/heads/master@{#358803}

[eroman, 2015-11-10 00:27:24]

eroman@chromium.org changed reviewers:
+ davidben@chromium.org, jww@chromium.org

[eroman, 2015-11-10 00:27:24]

@davidben: Please review the use of crypto::RandBytes(), and the performance
conclusions of this change (5x slower to generate random bytes in
microbenchmarks).

@jww: Please review the whole CL, with an emphasis on the Blink side.

[davidben, 2015-11-10 01:13:22]

lgtm. I don't think the perf hit is worth worrying about.

If we do end up caring, we can sort out the dependencies and switch to BoringSSL
RAND_bytes which has an RDRAND codepath, and a (fork-unsafe!) buffer we can
enable. But I'm sure we won't need it. That stuff's for servers that have to
generate lots of CBC mode IVs.

https://codereview.chromium.org/1431233002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/CryptographicallyRandomNumber.cpp (right):

https://codereview.chromium.org/1431233002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/CryptographicallyRandomNumber.cpp:21: #include
<string.h> // For memcpy().
Nit: Unless this is the Blink style, I probably wouldn't bother with the
comment. We don't typically add random comments when you, e.g. include <utility>
for std::pair.

https://codereview.chromium.org/1431233002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/CryptographicallyRandomNumber.cpp:37:
memcpy(&result, bytes, sizeof(bytes));
I think this falls under the character type escape hatch of strict aliasing, so
you don't need the memcpy. But I'm no language-lawyer.

[eroman, 2015-11-10 02:00:34]

Description was changed from

==========
Remove blink's use of RC4 for random value generation.

This re-implements Blink's random number generator
(wtf::cryptographicallyRandomValues) in terms of calling crypto::RandomBytes()
directly, rather than using an ARC4 keystream that periodically stirs in system
randomness.

Reason: RC4 (ARC4) has known weaknesses and has already been disabled as an
accepted TLS cipher for Chrome M48. It should not be used internally for
generating random numbers in Blink.

The way cryptographically random number generation worked in Blink prior to this
patch is that wtf::cryptographicallyRandomValues() would generate random bytes
using an ARC4 keystream. Every 1.6MB of generated data it would stir in
randomness obtained via Platform::current()->cryptographicallyRandomValues().

The way it works after this patchset is that
wtf::cryptographicallyRandomValues() directly calls
Platform::current()->cryptographicallyRandomValues(), without layering on its
own PRNG. The concrete implementation of
Platform::cryptographicallyRandomValues() now calls crypto::RandBytes(), which
provides good cryptographically secure random numbers by reading from
hardware/system randomness sources.

The consequences of this change are:

 * The fixed sequence of random numbers seen by certain tests will have changed.
I haven't observed this to be a problem with any of the tests though.

 * html_viewer has an added dependency on //crypto

 * The performance characteristics of cryptographicallyRandomValues() have
changed, for the worse. Measured using a microbenchmark,
window.crypto.getRandomValues() is almost 5x slower now. This is not all that
surprising since RC4 was pretty fast, and was only mixing in system randomness
every 1.6MB (my test generated 256MB).

BUG=552749
==========

to

==========
Remove blink's use of RC4 for random value generation.

This re-implements Blink's random number generator
(wtf::cryptographicallyRandomValues) in terms of calling crypto::RandomBytes()
directly, rather than using an ARC4 keystream that periodically stirs in system
randomness.

Reason: RC4 (ARC4) has known weaknesses and has already been disabled as an
accepted TLS cipher for Chrome M48. It should not be used internally for
generating random numbers in Blink.

The way cryptographically random number generation worked in Blink prior to this
patch is that wtf::cryptographicallyRandomValues() would generate random bytes
using an ARC4 keystream. Every 1.6MB of generated data it would stir in
randomness obtained via Platform::current()->cryptographicallyRandomValues().

The way it works after this patchset is that
wtf::cryptographicallyRandomValues() directly calls
Platform::current()->cryptographicallyRandomValues(), without layering on its
own PRNG. The concrete implementation of
Platform::cryptographicallyRandomValues() now calls crypto::RandBytes() [1],
which provides good cryptographically secure random numbers by reading from
hardware/system randomness sources.

The consequences of this change are:

 * The fixed sequence of random numbers seen by certain tests will have changed.
I haven't observed this to be a problem with any of the tests though.

 * The performance characteristics of cryptographicallyRandomValues() have
changed, for the worse. Measured using a microbenchmark,
window.crypto.getRandomValues() is almost 5x slower now. This is not all that
surprising since RC4 was pretty fast, and was only mixing in system randomness
every 1.6MB (my test generated 256MB).

[1] Technically it is calling base::RandBytes(), but under the hood that calls
crypto::RandBytes(). Will fix that dependency separately.

BUG=552749
==========

[eroman, 2015-11-10 02:02:43]

Thanks!

Note I have simplified this change so it is now just a single file change to
CryptographicallyRandomNumber.cpp.

I pulled out the base::RandBytes() --> crypto::RandBytes() to another change,
since really that is a dependency thing that can be done separately, and doesn't
affect the security.

https://codereview.chromium.org/1431233002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/CryptographicallyRandomNumber.cpp (right):

https://codereview.chromium.org/1431233002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/CryptographicallyRandomNumber.cpp:21: #include
<string.h> // For memcpy().
On 2015/11/10 01:13:21, davidben wrote:
> Nit: Unless this is the Blink style, I probably wouldn't bother with the
> comment. We don't typically add random comments when you, e.g. include
<utility>
> for std::pair.

Done.

https://codereview.chromium.org/1431233002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/CryptographicallyRandomNumber.cpp:37:
memcpy(&result, bytes, sizeof(bytes));
On 2015/11/10 01:13:21, davidben wrote:
> I think this falls under the character type escape hatch of strict aliasing,
so
> you don't need the memcpy. But I'm no language-lawyer.

Done.

I don't fully understand the language either, but we certainly make that
assumption elsewhere [1]

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/rand_util_win...

[davidben, 2015-11-10 02:14:35]

lgtm

https://codereview.chromium.org/1431233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/CryptographicallyRandomNumber.cpp (right):

https://codereview.chromium.org/1431233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/CryptographicallyRandomNumber.cpp:21: #include
<string.h>
No longer needed?

[eroman, 2015-11-10 02:17:54]

https://codereview.chromium.org/1431233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/CryptographicallyRandomNumber.cpp (right):

https://codereview.chromium.org/1431233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/CryptographicallyRandomNumber.cpp:21: #include
<string.h>
On 2015/11/10 02:14:35, davidben wrote:
> No longer needed?

Oops yeah, will remove :) Thanks!

[eroman, 2015-11-10 02:25:44]

eroman@chromium.org changed reviewers:
- jww@chromium.org

[eroman, 2015-11-10 02:26:19]

eroman@chromium.org changed reviewers:
+ thakis@chromium.org

[eroman, 2015-11-10 02:26:20]

+thakis for OWNERS approval

[Nico, 2015-11-10 05:11:11]

lgtm

Also, getting a single random number is now two indirect hops. But if that
matters, some perftest will let us know.

[eroman, 2015-11-10 06:05:12]

The CQ bit was checked by eroman@chromium.org

[eroman, 2015-11-10 06:05:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1431233002/#ps60001
(title: "Remove unused header")

[commit-bot: I haz the power, 2015-11-10 06:05:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-10 06:05:43]

This CL has an open dependency (Issue 1419293005 Patch 1). Please resolve the
dependency and try again.

[eroman, 2015-11-10 06:10:11]

The CQ bit was checked by eroman@chromium.org

[eroman, 2015-11-10 06:10:12]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1431233002/#ps80001
(title: "rebase and reparent branch to origin/master")

[commit-bot: I haz the power, 2015-11-10 06:10:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1431233002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1431233002/80001

[commit-bot: I haz the power, 2015-11-10 07:43:43]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-10 07:44:39]

Patchset 5 (id:??) landed as
https://crrev.com/9224aa4826d29930a8194a58dfd7170411bfc672
Cr-Commit-Position: refs/heads/master@{#358803}