Mark base::GenerateGUID as secure random

It's commonly assumed that GUIDs generated by base::GenerateGUID are
unguessable and will not collide, and this is in practice true since
it's backed by base::RandBytes.

The header for base::RandBytes did not guarantee being secure, but the
implementations were all required to be cryptographically strong random
number generators since crypto::RandBytes and base::UnguessableToken
already both depend on base::RandBytes (see https://crbug.com/140076).

This patch:

- Marks base::GenerateGUID as secure in the code comments.

- Migrates base::GenerateGUID from base::RandUint64 which is not
  guaranteed to be secure to base::RandBytes which is - see above.
  (It's not possible to migrate to crypto::RandBytes since that would
  introduce a circular dependency between base and crypto - see
  UnguessableToken::Create for a similar case).

- Marks base::RandBytes as secure random in its header, but clarifying
  that code outside base/ that depends on it being secure should
  continue to use the crypto/ wrapper.

- Cleans up some duplicated code in the implementations of
  base/rand_util_*.cc

Bug: none
Change-Id: I282bbd7d1883ba120c01280b941b9d7ecbef404c
Reviewed-on: https://chromium-review.googlesource.com/678731Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: John Mellor <johnme@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504389}

base/guid.cc

@@ -41,7 +41,10 @@ bool IsValidGUIDInternal(const base::StringPiece& guid, bool strict) {
 }  // namespace
 
 std::string GenerateGUID() {
-  uint64_t sixteen_bytes[2] = {base::RandUint64(), base::RandUint64()};
+  uint64_t sixteen_bytes[2];
+  // Use base::RandBytes instead of crypto::RandBytes, because crypto calls the
+  // base version directly, and to prevent the dependency from base/ to crypto/.
+  base::RandBytes(&sixteen_bytes, sizeof(sixteen_bytes));
 
   // Set the GUID to version 4 as described in RFC 4122, section 4.4.
   // The format of GUID version 4 must be xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx,

base/guid.h

@@ -15,12 +15,14 @@
 
 namespace base {
 
-// Generate a 128-bit (pseudo) random GUID in the form of version 4 as described
-// in RFC 4122, section 4.4.
+// Generate a 128-bit random GUID in the form of version 4 as described in
+// RFC 4122, section 4.4.
 // The format of GUID version 4 must be xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx,
 // where y is one of [8, 9, A, B].
 // The hexadecimal values "a" through "f" are output as lower case characters.
-// If GUID generation fails an empty string is returned.
+//
+// A cryptographically secure random source will be used, but consider using
+// UnguessableToken for greater type-safety if GUID format is unnecessary.
 BASE_EXPORT std::string GenerateGUID();
 
 // Returns true if the input string conforms to the version 4 GUID format.

base/rand_util.cc

@@ -16,6 +16,12 @@
 
 namespace base {
 
+uint64_t RandUint64() {
+  uint64_t number;
+  RandBytes(&number, sizeof(number));
+  return number;
+}
+
 int RandInt(int min, int max) {
   DCHECK_LE(min, max);
 

base/rand_util.h

@@ -35,22 +35,22 @@ BASE_EXPORT double RandDouble();
 // the range [0, 1). Thread-safe.
 BASE_EXPORT double BitsToOpenEndedUnitInterval(uint64_t bits);
 
-// Fills |output_length| bytes of |output| with random data.
+// Fills |output_length| bytes of |output| with random data. Thread-safe.
 //
-// WARNING:
-// Do not use for security-sensitive purposes.
-// See crypto/ for cryptographically secure random number generation APIs.
+// Although implementations are required to use a cryptographically secure
+// random number source, code outside of base/ that relies on this should use
+// crypto::RandBytes instead to ensure the requirement is easily discoverable.
 BASE_EXPORT void RandBytes(void* output, size_t output_length);
 
 // Fills a string of length |length| with random data and returns it.
-// |length| should be nonzero.
+// |length| should be nonzero. Thread-safe.
 //
 // Note that this is a variation of |RandBytes| with a different return type.
 // The returned string is likely not ASCII/UTF-8. Use with care.
 //
-// WARNING:
-// Do not use for security-sensitive purposes.
-// See crypto/ for cryptographically secure random number generation APIs.
+// Although implementations are required to use a cryptographically secure
+// random number source, code outside of base/ that relies on this should use
+// crypto::RandBytes instead to ensure the requirement is easily discoverable.
 BASE_EXPORT std::string RandBytesAsString(size_t length);
 
 #if defined(OS_POSIX) && !defined(OS_FUCHSIA)

base/rand_util_fuchsia.cc

@@ -12,12 +12,6 @@
 
 namespace base {
 
-uint64_t RandUint64() {
-  uint64_t number;
-  RandBytes(&number, sizeof(number));
-  return number;
-}
-
 void RandBytes(void* output, size_t output_length) {
   size_t remaining = output_length;
   unsigned char* cur = reinterpret_cast<unsigned char*>(output);

base/rand_util_nacl.cc

@@ -10,33 +10,18 @@
 
 #include "base/logging.h"
 
-namespace {
+namespace base {
 
-void GetRandomBytes(void* output, size_t num_bytes) {
+void RandBytes(void* output, size_t output_length) {
   char* output_ptr = static_cast<char*>(output);
-  while (num_bytes > 0) {
+  while (output_length > 0) {
     size_t nread;
-    const int error = nacl_secure_random(output_ptr, num_bytes, &nread);
+    const int error = nacl_secure_random(output_ptr, output_length, &nread);
     CHECK_EQ(error, 0);
-    CHECK_LE(nread, num_bytes);
+    CHECK_LE(nread, output_length);
     output_ptr += nread;
-    num_bytes -= nread;
+    output_length -= nread;
   }
 }
 
-}  // namespace
-
-namespace base {
-
-// NOTE: This function must be cryptographically secure. http://crbug.com/140076
-uint64_t RandUint64() {
-  uint64_t result;
-  GetRandomBytes(&result, sizeof(result));
-  return result;
-}
-
-void RandBytes(void* output, size_t output_length) {
-  GetRandomBytes(output, output_length);
-}
-
 }  // namespace base

base/rand_util_posix.cc

@@ -49,13 +49,6 @@ base::LazyInstance<URandomFd>::Leaky g_urandom_fd = LAZY_INSTANCE_INITIALIZER;
 
 namespace base {
 
-// NOTE: This function must be cryptographically secure. http://crbug.com/140076
-uint64_t RandUint64() {
-  uint64_t number;
-  RandBytes(&number, sizeof(number));
-  return number;
-}
-
 void RandBytes(void* output, size_t output_length) {
   const int urandom_fd = g_urandom_fd.Pointer()->fd();
   const bool success =

base/rand_util_win.cc

@@ -22,13 +22,6 @@
 
 namespace base {
 
-// NOTE: This function must be cryptographically secure. http://crbug.com/140076
-uint64_t RandUint64() {
-  uint64_t number;
-  RandBytes(&number, sizeof(number));
-  return number;
-}
-
 void RandBytes(void* output, size_t output_length) {
   char* output_ptr = static_cast<char*>(output);
   while (output_length > 0) {