Added crypto random-number generator

base/rand_util.h

Index: base/rand_util.h
diff --git a/base/rand_util.h b/base/rand_util.h
index c5c4ef88114427a8ddf8ddebdd9499422e2f14fe..9c1f2060b873ebf93ee9a500d1366fa2c13ef1cc 100644
--- a/base/rand_util.h
+++ b/base/rand_util.h
@@ -32,12 +32,16 @@ BASE_EXPORT double RandDouble();
 // the range [0, 1). Thread-safe.
 BASE_EXPORT double BitsToOpenEndedUnitInterval(uint64 bits);
 
-// Fills |output_length| bytes of |output| with cryptographically strong random
-// data.
+// Fills |output_length| bytes of |output| with random data.
+// Note: The random data is cryptographically secure, but use the RNG in crypto/
+// for security-sensitive purposes instead of this function.

[Ryan Sleevi, 2012/07/31 23:03:48]

nit: I'm mixed on specifically referencing crypto/ from base/, since generally
we treat comments like code (in that it's a layering violation). However, saying
"don't use this" without saying "What to use instead" equally is undesirable.

It also feels conflicting because it specifically says it's cryptographically
secure, but then puts someone elsewhere, without context of why (eg: to make
sure we clearly know where code /is/ depending on cryptographically secure rng)

Perhaps simply say:
// For cryptographically strong random, use crypto::RandBytes()

and the same below. I'd rather go for the layering violation than leave people
clueless as to the 'right' way.

[Mehrdad, 2012/07/31 23:36:33]

I feel that not saying this function is cryptographically secure would be very
dangerous.

If we don't document the fact that it's secure, then the implementation could
change later (e.g. someone might notice a performance boost with some other
library), suddenly jeopardizing essentially all of Chrome's security.


Is it really a good idea to remove that part of the comment?

I can try rewording it some other way if this way isn't good enough, but I'm not
sure if it's a good idea not to mention the security of this function at all.

[Ryan Sleevi, 2012/08/01 17:08:55]

My view of the motivation of crypto::RandBytes() is to specifically move away
from using base::RandBytes as a source of cryptographically strong entropy. I
feel that leaving the comment in (in particular, the header) does not encourage
nor reflect this view.

I think if any comments about cryptographic security should be made, then it
should be made in base/rand_util.cc, so that if anyone is changing the
implementation (as opposed to depending on its semantics) are informed of this.

That said, my understanding of the motivation of this change was specifically to
allow base::RandBytes to be changed (by changing all the crypto-RNG dependents
to depend on crypto::RandBytes). If that's not part of this change set, or if it
simply emerged because it wasn't obvious that base::RandBytes was
cryptographically strong, then perhaps this change should be abandoned?

[Mehrdad, 2012/08/01 17:19:03]

No, I agree, RandBytes should definitely be in crypto... that was the entire
reason I made this change.

I'll add a comment to rand_util.cc instead, that sounds like a good idea.

 BASE_EXPORT void RandBytes(void* output, size_t output_length);
 
-// Fills a string of length |length| with with cryptographically strong random
-// data and returns it.  |length| should be nonzero.
+// Fills a string of length |length| with with random data and returns it.
+// |length| should be nonzero.
+//
+// Note: The random data is cryptographically secure, but use the RNG in crypto/
+// for security-sensitive purposes instead of this function.
 //
 // Note that this is a variation of |RandBytes| with a different return type.
 BASE_EXPORT std::string RandBytesAsString(size_t length);