Switch VM permission service to use UnguessableToken

Even though GenerateGUID() is supposed to return cryptographically
strong data, it is better to use a dedicated implementation for the
token that is not supposed to be guessed by a 3rd party.

Note that we still use strings to exchange tokens, as we are using DBus
and not mojo (which would ensure type safety and would not expose
implementation details such as the fact that token is 2 64-bit numbers).

Bug: 1071872
Change-Id: I3014864201ee0af5e1d402dcf7ecb6c2e360f513
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2289213Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Daniel Ng <danielng@google.com>
Commit-Queue: Dmitry Torokhov <dtor@chromium.org>
Cr-Commit-Position: refs/heads/master@{#787450}

chrome/browser/chromeos/dbus/vm/vm_permission_service_provider.cc

@@ -6,10 +6,11 @@
 
 #include <memory>
 #include <utility>
+#include <vector>
 
 #include "base/feature_list.h"
-#include "base/guid.h"
 #include "base/logging.h"
+#include "base/strings/string_number_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/plugin_vm/plugin_vm_manager.h"
 #include "chrome/browser/chromeos/plugin_vm/plugin_vm_manager_factory.h"
@@ -23,6 +24,32 @@
 #include "dbus/message.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
+namespace {
+
+base::UnguessableToken TokenFromString(const std::string& str) {
+  static constexpr int kBytesPerUint64 = sizeof(uint64_t) / sizeof(uint8_t);
+  static constexpr int kMaxBytesPerToken = 2 * kBytesPerUint64;
+
+  std::vector<uint8_t> bytes;
+  if (!base::HexStringToBytes(str, &bytes) || bytes.size() == 0 ||
+      bytes.size() > kMaxBytesPerToken) {
+    return base::UnguessableToken();
+  }
+
+  uint64_t high = 0, low = 0;
+  int count = 0;
+  std::for_each(std::rbegin(bytes), std::rend(bytes), [&](auto byte) {
+    auto* p = count < kBytesPerUint64 ? &low : &high;
+    int pos = count < kBytesPerUint64 ? count : count - kBytesPerUint64;
+    *p += static_cast<uint64_t>(byte) << (pos * 8);
+    count++;
+  });
+
+  return base::UnguessableToken::Deserialize(high, low);
+}
+
+}  // namespace
+
 namespace chromeos {
 
 VmPermissionServiceProvider::VmInfo::VmInfo(std::string owner_id,
@@ -126,11 +153,11 @@ void VmPermissionServiceProvider::RegisterVm(
   // to re-launch the VM, we do not need to update them after this.
   UpdateVmPermissions(vm.get());
 
-  const std::string token(base::GenerateGUID());
+  const base::UnguessableToken token(base::UnguessableToken::Create());
   vms_[token] = std::move(vm);
 
   vm_permission_service::RegisterVmResponse payload;
-  payload.set_token(token);
+  payload.set_token(token.ToString());
 
   dbus::MessageWriter writer(response.get());
   writer.AppendProtoAsArrayOfBytes(payload);
@@ -245,9 +272,18 @@ void VmPermissionServiceProvider::GetPermissions(
     return;
   }
 
-  auto iter = vms_.find(request.token());
+  auto token = TokenFromString(request.token());
+  if (!token) {
+    LOG(ERROR) << "Malformed token '" << request.token() << "'";
+    std::move(response_sender)
+        .Run(dbus::ErrorResponse::FromMethodCall(
+            method_call, DBUS_ERROR_INVALID_ARGS, "Malformed token"));
+    return;
+  }
+
+  auto iter = vms_.find(token);
   if (iter == vms_.end()) {
-    LOG(ERROR) << "Invalid token " << request.token();
+    LOG(ERROR) << "Invalid token " << token;
     std::move(response_sender)
         .Run(dbus::ErrorResponse::FromMethodCall(
             method_call, DBUS_ERROR_INVALID_ARGS, "Invalid token"));

chrome/browser/chromeos/dbus/vm/vm_permission_service_provider.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_CHROMEOS_DBUS_VM_VM_PERMISSION_SERVICE_PROVIDER_H_
 #define CHROME_BROWSER_CHROMEOS_DBUS_VM_VM_PERMISSION_SERVICE_PROVIDER_H_
 
+#include <memory>
 #include <set>
 #include <string>
 #include <unordered_map>
@@ -12,6 +13,7 @@
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/unguessable_token.h"
 #include "chromeos/dbus/services/cros_dbus_service.h"
 #include "dbus/exported_object.h"
 
@@ -115,7 +117,9 @@ class VmPermissionServiceProvider
     ~VmInfo();
   };
 
-  using VmMap = std::unordered_map<std::string, std::unique_ptr<VmInfo>>;
+  using VmMap = std::unordered_map<base::UnguessableToken,
+                                   std::unique_ptr<VmInfo>,
+                                   base::UnguessableTokenHash>;
 
   // Called from ExportedObject when GetLicenseDataResponse() is exported as a
   // D-Bus method or failed to be exported.