Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(24)

Issue 1431233002: Remove blink's use of RC4 for random value generation. (Closed)

Created:
4 years, 6 months ago by eroman
Modified:
4 years, 6 months ago
Reviewers:
Nico, davidben
CC:
chromium-reviews, droger+watchlist_chromium.org, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, jam, darin-cc_chromium.org, blink-reviews, blink-reviews-wtf_chromium.org, Mikhail, jww
Base URL:
https://chromium.googlesource.com/chromium/src.git@random_asserts
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Total comments: 4

Patch Set 2 : Address David's comments #

Patch Set 3 : Undo the base::RandBytes() --> crypto::RandBytes() change (moved to another CL) #

Total comments: 2

Patch Set 4 : Remove unused header #

Patch Set 5 : rebase and reparent branch to origin/master #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -147 lines) Patch
M third_party/WebKit/Source/wtf/CryptographicallyRandomNumber.cpp View 1 2 3 2 chunks +4 lines, -147 lines 0 comments Download

Messages

Total messages: 20 (9 generated)
eroman
@davidben: Please review the use of crypto::RandBytes(), and the performance conclusions of this change (5x ...
4 years, 6 months ago (2015-11-10 00:27:24 UTC) #2
davidben
lgtm. I don't think the perf hit is worth worrying about. If we do end ...
4 years, 6 months ago (2015-11-10 01:13:22 UTC) #3
eroman
Thanks! Note I have simplified this change so it is now just a single file ...
4 years, 6 months ago (2015-11-10 02:02:43 UTC) #5
davidben
lgtm https://codereview.chromium.org/1431233002/diff/40001/third_party/WebKit/Source/wtf/CryptographicallyRandomNumber.cpp File third_party/WebKit/Source/wtf/CryptographicallyRandomNumber.cpp (right): https://codereview.chromium.org/1431233002/diff/40001/third_party/WebKit/Source/wtf/CryptographicallyRandomNumber.cpp#newcode21 third_party/WebKit/Source/wtf/CryptographicallyRandomNumber.cpp:21: #include <string.h> No longer needed?
4 years, 6 months ago (2015-11-10 02:14:35 UTC) #6
eroman
https://codereview.chromium.org/1431233002/diff/40001/third_party/WebKit/Source/wtf/CryptographicallyRandomNumber.cpp File third_party/WebKit/Source/wtf/CryptographicallyRandomNumber.cpp (right): https://codereview.chromium.org/1431233002/diff/40001/third_party/WebKit/Source/wtf/CryptographicallyRandomNumber.cpp#newcode21 third_party/WebKit/Source/wtf/CryptographicallyRandomNumber.cpp:21: #include <string.h> On 2015/11/10 02:14:35, davidben wrote: > No ...
4 years, 6 months ago (2015-11-10 02:17:54 UTC) #7
eroman
+thakis for OWNERS approval
4 years, 6 months ago (2015-11-10 02:26:20 UTC) #10
Nico
lgtm Also, getting a single random number is now two indirect hops. But if that ...
4 years, 6 months ago (2015-11-10 05:11:11 UTC) #11
commit-bot: I haz the power
This CL has an open dependency (Issue 1419293005 Patch 1). Please resolve the dependency and ...
4 years, 6 months ago (2015-11-10 06:05:43 UTC) #15
commit-bot: I haz the power
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
4 years, 6 months ago (2015-11-10 06:10:46 UTC) #18
commit-bot: I haz the power
Committed patchset #5 (id:80001)
4 years, 6 months ago (2015-11-10 07:43:43 UTC) #19
commit-bot: I haz the power
4 years, 6 months ago (2015-11-10 07:44:39 UTC) #20
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/9224aa4826d29930a8194a58dfd7170411bfc672
Cr-Commit-Position: refs/heads/master@{#358803}

Powered by Google App Engine
This is Rietveld 408576698