Move crypto_helpers from sync to base

Move crypto_helpers from sync to crypto

crypto_helpers only depends on resources in base and is used by sync and
password_manager.

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=84223

[qsr (NOT THE RIGHT qsr), 2011-04-21 15:25:29]

Dependency cleanup. This patch remove a dependency from password_manager to sync
by putting crypto_utils in base.

[qsr (NOT THE RIGHT qsr), 2011-04-22 06:39:56]

Change this to move to crypto/ instead of base/

Adam, this is the patch I talked you about yesterday.

[agl, 2011-04-26 13:59:37]

Sorry for the lateness of my reply. If I haven't replied to a review in one
business day then I've likely missed the email and you should ping me about it.

http://codereview.chromium.org/6873156/diff/2001/chrome/browser/sync/util/use...
File chrome/browser/sync/util/user_settings.cc (right):

http://codereview.chromium.org/6873156/diff/2001/chrome/browser/sync/util/use...
chrome/browser/sync/util/user_settings.cc:274: int32 GetHashFromDigest(const
vector<uint8>& digest) {
Probably too late to change this now, but this function ends up putting a hole
in the bits returned.

http://codereview.chromium.org/6873156/diff/2001/crypto/md5_calculator.h
File crypto/md5_calculator.h (right):

http://codereview.chromium.org/6873156/diff/2001/crypto/md5_calculator.h#newc...
crypto/md5_calculator.h:19: class MD5Calculator {
I'm afraid that this wrapper object is too thin to be worthwhile. The (single?)
user should simply be updated to use the underlying base/md5.h because this adds
almost nothing.

http://codereview.chromium.org/6873156/diff/2001/crypto/random.cc
File crypto/random.cc (right):

http://codereview.chromium.org/6873156/diff/2001/crypto/random.cc#newcode17
crypto/random.cc:17: random_int = base::RandUint64();
This is not cryptographically strong on Windows.

http://codereview.chromium.org/6873156/diff/2001/crypto/random.cc#newcode25
crypto/random.cc:25: std::string random_bytes(kNumberBytes, ' ');
no need for a std::string here, a uint8[] will do.

http://codereview.chromium.org/6873156/diff/2001/crypto/random.cc#newcode28
crypto/random.cc:28: base::Base64Encode(random_bytes, &base64_encoded_bytes);
The function name suggests that the data should be hex encoded, not base64.

http://codereview.chromium.org/6873156/diff/2001/crypto/random.h
File crypto/random.h (right):

http://codereview.chromium.org/6873156/diff/2001/crypto/random.h#newcode11
crypto/random.h:11: void GetRandomBytes(char* output, int output_length);
Needs a comment. The first argument should probably be a void* and the second a
size_t.

// GetRandomBytes fills |output_length| bytes of |output| with cryptographically
strong random data.

http://codereview.chromium.org/6873156/diff/2001/crypto/random.h#newcode12
crypto/random.h:12: std::string Generate128BitRandomHexString();
// Generate128BitRandomHexString returns a string of length 32 containing
lower-case, hex encoded, cryptographically strong, random data.

[qsr (NOT THE RIGHT qsr), 2011-04-26 15:24:18]

http://codereview.chromium.org/6873156/diff/2001/crypto/random.cc
File crypto/random.cc (right):

http://codereview.chromium.org/6873156/diff/2001/crypto/random.cc#newcode25
crypto/random.cc:25: std::string random_bytes(kNumberBytes, ' ');
Except that the base64_encode function expect strings. If I want to use it, I'll
have to create a std::string after.

http://codereview.chromium.org/6873156/diff/2001/crypto/random.cc#newcode28
crypto/random.cc:28: base::Base64Encode(random_bytes, &base64_encoded_bytes);
As this is a refactoring, I will modify the name of the function, but not what
it does. I will rename the function Generate128BitRandomBase64String

[Ryan Sleevi, 2011-04-27 02:32:24]

Drive by review, as I saw came across this when looking at some open issues.

http://codereview.chromium.org/6873156/diff/7003/crypto/random.cc
File crypto/random.cc (right):

http://codereview.chromium.org/6873156/diff/7003/crypto/random.cc#newcode16
crypto/random.cc:16: void GetRandomBytes(void* output, size_t output_length) {
nit: Should the return be a bool instead?

Both CryptAcquireContext and CryptGenRandom can fail - the former especially if
AES is not available (< XP SP2). If CryptGenRandom fails, you'd be in a bad
state of thinking cryptographically strong random was acquired, when none
actually was.

http://codereview.chromium.org/6873156/diff/7003/crypto/random.cc#newcode23
crypto/random.cc:23: PROV_RSA_AES, CRYPT_VERIFYCONTEXT);
drive by: PROV_RSA_AES is only valid for XP SP2+. Since you're just wanting
cryptographically strong random, PROV_RSA_FULL should suffice. Though there is
one place where PROV_RSA_AES is used (incidentally, in symmetic_key_win.cc to
generating an AES key, which only Nigori/Sync calls), all the other places (SHA,
HMAC, RSA) use PROV_RSA_FULL.

Another thing to be concerned about is the overhead of acquiring a verify
context in every call, if this is frequently called code. When
http://crrev.com/50648 landed, it had to be reverted in http://crrev.com/50900
due to perf regressions. wtc can speak more, but my suspicion is that the
regressions may have been related to the overhead of needing to acquire the
context every time (which can end up going through a series of registry lookups
and module loads every call).

I'm not aware of any thread-safety issues in caching the handle returned,
perhaps as a base::LazyInstance, if perf turns out to be an issue. You may just
want to keep a eye for any perf impact. I suspect that it'd only be an issue if
this got put in a tight loop of 4-8 bytes a pass (like how lines 32-36 are), and
even then it may not be one.

http://codereview.chromium.org/6873156/diff/7003/crypto/random.cc#newcode27
crypto/random.cc:27: CryptGenRandom(safe_provider, output_length, output);
nit: I know the style guide recommends size_t for dealing with ranges of bytes,
as was agl's suggestion, but there is a subtle truncation issue here on 64-bit
(size_t vs DWORD) which, at least in the extremely rare/impossible circumstance
you're requesting >4GB of random, would leave the input buffer with
substantially less entropy.

In practice, this should never happen, but it's cheap to be defensive here. This
may just be a personal nit though, because I'd say it's likely a bug if you're
requesting that much cryptographically strong random.

http://codereview.chromium.org/6873156/diff/7003/crypto/random.h
File crypto/random.h (right):

http://codereview.chromium.org/6873156/diff/7003/crypto/random.h#newcode13
crypto/random.h:13: void GetRandomBytes(void* output, size_t output_length);
This makes the third implementation to get random bytes. Perhaps these should be
consolidated.

1: this
2: src/ppapi/shared_impl/crypto_impl.cc (brettw@)
3: src/webkit/glue/webkitclient_impl.cc (abarth@)

Prior to agl@'s comments, all three implementations were exactly the same -
iterate through a series of uint64s, copying into the target buffer - with #2
and #3 being straight up line for line the same.

All three make the claim of "cryptographically strong", although as agl@ pointed
out, it's not the case for RandUint64 on Windows.

[Benjamin.Lerman_google.com, 2011-04-27 08:05:59]

 Thanks for your input.

 Right now, I'm not very comfortable continuing on a change that important
on a platform I don't have access yet (I don't have a windows box to test
all those changes).

 I can either wait to get a windows box and continue on this but this make
take a week or so, or if someone with one is ready to help me land this, we
can do this quicker.

 Now, on the review itself:

- No problem for going from PROV_RSA_AES to PROV_RSA_FULL.
- For the case of failing, will it be acceptable to go back to the
old behavior if acquiring either the context or generating the bytes fail?
Because I don't see any way to fail gently if I cannot get any randomness.
- I can put the context in a lazy instance.
- I can refactor the 2 other places to depends on crypto part and remove the
duplication.

 Benjamin

[Ryan Sleevi, 2011-04-27 13:43:08]

> Now, on the review itself:
> - No problem for going from PROV_RSA_AES to PROV_RSA_FULL.
> - For the case of failing, will it be acceptable to go back to the
> old behavior if acquiring either the context or generating the bytes fail?
> Because I don't see any way to fail gently if I cannot get any randomness.

I'm not familiar enough with the code that (would) depend on this to know if
that would present a security concern to have the fallback path. If you don't
really need cryptographically strong random, and "enough" random will suffice
without any security impact, then you can avoid the whole issue of
CryptAcquireContext, potential failure, etc by simply always using RandUint64.
It would also seem to suggest you may want base/, instead of crypto/.

It's just an issue when "cryptographically strong" is added to the API contract
- since such implementations may fail/block (particularly if FIPS enters the
equation at all).

Alternatively, there is RtlGenRandom, which is what the Microsoft Base CSP calls
into to actually get random. You can find more at
http://blogs.msdn.com/b/michael_howard/archive/2005/01/14/353379.aspx . It looks
like both libjingle and NSS are using this for random, and rand_s in the CRT on
VS 2005+ are supposed to be using it.

If only on XP+, and only compiling with 2005+, then rand_s "should" be
cryptographically strong, at least according to the above. Perhaps the code was
correct all along, since RandUint64 is implemented in terms of rand_s.

> - I can put the context in a lazy instance.
> - I can refactor the 2 other places to depends on crypto part and remove the
> duplication.
>
> Benjamin

http://codereview.chromium.org/6873156/diff/7003/chrome/browser/sync/util/use...
File chrome/browser/sync/util/user_settings.cc (right):

http://codereview.chromium.org/6873156/diff/7003/chrome/browser/sync/util/use...
chrome/browser/sync/util/user_settings.cc:274: int32
GetHashFromDigest(MD5Digest& digest) {
nit: const MD5Digest&

[wtc, 2011-04-28 19:42:22]

agl: base::RandUint64() is cryptographically strong on Windows.
It is implemented using rand_s(), which calls the
RtlGenRandom function in advapi32.dll.

qsr: I suggest different homes for the two random functions below.

http://codereview.chromium.org/6873156/diff/7003/crypto/random.cc
File crypto/random.cc (right):

http://codereview.chromium.org/6873156/diff/7003/crypto/random.cc#newcode11
crypto/random.cc:11: #endif  // defined(OS_WIN)
Nit: this block should be moved after line 14.  See
Chromium Coding Style guide (the last example):
http://dev.chromium.org/developers/coding-style#TOC-Platform-specific-code

http://codereview.chromium.org/6873156/diff/7003/crypto/random.cc#newcode17
crypto/random.cc:17: #if defined(OS_WIN)
Optional: I would just eliminate the OS_WIN code.  There is
a faster way to get random bytes on Windows (calling the
undocumented but supported function RtlGenRandom in
advapi32.dll), and base::RandUint64() uses that method under
the hood.

http://codereview.chromium.org/6873156/diff/7003/crypto/random.h
File crypto/random.h (right):

http://codereview.chromium.org/6873156/diff/7003/crypto/random.h#newcode13
crypto/random.h:13: void GetRandomBytes(void* output, size_t output_length);
GetRandomBytes should be moved to base/rand_util.h.
It is closely related to the functions in base/rand_util.h.

http://codereview.chromium.org/6873156/diff/7003/crypto/random.h#newcode17
crypto/random.h:17: std::string Generate128BitRandomBase64String();
API design issue: this function is not generally useful.
It should ideally be moved back to where its callers are
(chrome/browser/sync/util).

http://codereview.chromium.org/6873156/diff/7003/crypto/random_unittest.cc
File crypto/random_unittest.cc (right):

http://codereview.chromium.org/6873156/diff/7003/crypto/random_unittest.cc#ne...
crypto/random_unittest.cc:10: std::string random_bytes(i+1, ' ');
Nit: Google Style Guide recommends adding spaces around '+'.

[Ryan Sleevi, 2011-04-29 06:52:10]

Just a note: abarth has pushed out a very similar CL to wtc's optional
suggestion in http://codereview.chromium.org/6904118/ , which updates
base/rand_util.h/.cc. The only difference is that it treats the result as a
string, rather than as an opaque set of bytes.

[abarth-chromium, 2011-04-29 07:47:11]

http://codereview.chromium.org/6873156/diff/7003/crypto/random.h
File crypto/random.h (right):

http://codereview.chromium.org/6873156/diff/7003/crypto/random.h#newcode17
crypto/random.h:17: std::string Generate128BitRandomBase64String();
http://codereview.chromium.org/6904118/ contains a function that generates
a random string of arbitrary length, which is generally useful.  :)

[wtc, 2011-04-29 18:07:08]

http://codereview.chromium.org/6873156/diff/7003/crypto/random.h
File crypto/random.h (right):

http://codereview.chromium.org/6873156/diff/7003/crypto/random.h#newcode17
crypto/random.h:17: std::string Generate128BitRandomBase64String();
On 2011/04/29 07:47:11, abarth wrote:
> http://codereview.chromium.org/6904118/ contains a function that generates
> a random string of arbitrary length, which is generally useful.  :)

abarth: yes.  Your RandString function is equivalent to
the GetRandomBytes function above.  The Generate128BitRandomBase64String
function also encodes the output in base64 so that it is
truly a "text" string.

[qsr (NOT THE RIGHT qsr), 2011-05-02 15:16:30]

Ok, let's try this once more.

 I move GetRandomBytes to base/rand_util. I did not modify the implementation of
GetRandomBytesAsString, because otherwise I must instantiate an array to drop it
afterwards. The current implementation seems better.

 I also modified ppapi/shared_impl/crypto_impl.cc and
webkit/glue/webkitclient_impl.cc to use this method instead of reimplementing
it. I did not get rid of the method because one is part of an API, and the other
has not the same signature and is used as the value of a pointer to a function.

 I get rid of the md5 classes and instead use base/md5.h.

 I put the base64 encoded version in browser/common because it is used by sync
and password_manager.

[abarth-chromium, 2011-05-02 18:23:20]

http://codereview.chromium.org/6873156/diff/11004/base/rand_util.cc
File base/rand_util.cc (right):

http://codereview.chromium.org/6873156/diff/11004/base/rand_util.cc#newcode52
base/rand_util.cc:52: }
I'd recommend re-implementing RandBytesAsString in terms of this function.  You
can use the WriteInto function from string_util to make that a one-liner.

[qsr (NOT THE RIGHT qsr), 2011-05-03 07:02:19]

> I'd recommend re-implementing RandBytesAsString in terms of this function. 
You
> can use the WriteInto function from string_util to make that a one-liner.

 I didn't know this function. Done.

[agl, 2011-05-03 14:53:40]

LGTM

http://codereview.chromium.org/6873156/diff/16001/base/rand_util.cc
File base/rand_util.cc (right):

http://codereview.chromium.org/6873156/diff/16001/base/rand_util.cc#newcode47
base/rand_util.cc:47: const char* random_int_bytes = reinterpret_cast<const
char*>(&random_int);
nit: you don't need this. memcpy takes a const void* src so &random_int will do
there.

http://codereview.chromium.org/6873156/diff/16001/base/rand_util_unittest.cc
File base/rand_util_unittest.cc (right):

http://codereview.chromium.org/6873156/diff/16001/base/rand_util_unittest.cc#...
base/rand_util_unittest.cc:33: char buffer[buffer_size];
you probably want to memset(buffer, 0, buffer_size) first.

http://codereview.chromium.org/6873156/diff/16001/chrome/common/random.cc
File chrome/common/random.cc (right):

http://codereview.chromium.org/6873156/diff/16001/chrome/common/random.cc#new...
chrome/common/random.cc:14: std::string random_bytes(kNumberBytes, ' ');
You have RandBytesAsString now, did you not want to use it?

[commit-bot: I haz the power, 2011-05-03 16:40:12]

Presubmit check for 6873156-12002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
base/rand_util.cc,base/rand_util.h,base/rand_util_unittest.cc

[brettw, 2011-05-03 17:36:53]

LGTM, just some minor comments before checkin.

http://codereview.chromium.org/6873156/diff/12002/base/rand_util.h
File base/rand_util.h (right):

http://codereview.chromium.org/6873156/diff/12002/base/rand_util.h#newcode32
base/rand_util.h:32: // fills |output_length| bytes of |output| with
cryptographically strong random
Be sure all comments are complete sentences with capital letters.

http://codereview.chromium.org/6873156/diff/12002/base/rand_util.h#newcode37
base/rand_util.h:37: BASE_API std::string RandBytesAsString(size_t length);
Can you update the comment for this function so it's more clear that this is the
same as RandBytes, just with a different return type?

http://codereview.chromium.org/6873156/diff/12002/chrome/browser/sync/util/us...
File chrome/browser/sync/util/user_settings.cc (right):

http://codereview.chromium.org/6873156/diff/12002/chrome/browser/sync/util/us...
chrome/browser/sync/util/user_settings.cc:363: MD5Final(&md5_digest,
&md5_context);
I'd put a blank line after this to help separate it from the SQL code below.

Also, I'd probably move the MD5Digest declaration to right before you need it
(preferred by the style guide). Ditto below.

[commit-bot: I haz the power, 2011-05-04 09:36:38]

Try job failure for 6873156-20001 on mac:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2011-05-04 12:59:02]

Try job failure for 6873156-20001 on mac:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2011-05-04 15:46:52]

Try job failure for 6873156-20001 on mac:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2011-05-04 17:06:48]

Try job failure for 6873156-20001 on mac:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2011-05-05 08:46:13]

Change committed as 84223