Activate signature verification.

Signature is now mandatory in storage, when encryption is enabled.

Bug: b:170054326
Change-Id: If0720332dfa84b296b49ee75dc95e14c557b0684
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2615099
Commit-Queue: Leonid Baraz <lbaraz@chromium.org>
Reviewed-by: Zach Trudo <zatrudo@google.com>
Cr-Commit-Position: refs/heads/master@{#841359}

chrome/browser/BUILD.gn

@@ -1228,6 +1228,7 @@ static_library("browser") {
     "policy/messaging_layer/storage/resources/resource_interface.h",
     "policy/messaging_layer/storage/storage.cc",
     "policy/messaging_layer/storage/storage.h",
+    "policy/messaging_layer/storage/storage_configuration.cc",
     "policy/messaging_layer/storage/storage_configuration.h",
     "policy/messaging_layer/storage/storage_module.cc",
     "policy/messaging_layer/storage/storage_module.h",

chrome/browser/policy/messaging_layer/storage/storage.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/policy/messaging_layer/storage/storage.h"
 
+#include <cstdint>
 #include <utility>
 #include <vector>
 
@@ -22,6 +23,7 @@
 #include "base/threading/thread.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/policy/messaging_layer/encryption/encryption_module.h"
+#include "chrome/browser/policy/messaging_layer/encryption/verification.h"
 #include "chrome/browser/policy/messaging_layer/storage/storage_configuration.h"
 #include "chrome/browser/policy/messaging_layer/storage/storage_queue.h"
 #include "chrome/browser/policy/messaging_layer/util/status.h"
@@ -29,6 +31,7 @@
 #include "chrome/browser/policy/messaging_layer/util/statusor.h"
 #include "chrome/browser/policy/messaging_layer/util/task_runner_context.h"
 #include "components/policy/proto/record.pb.h"
+#include "third_party/boringssl/src/include/openssl/curve25519.h"
 #include "third_party/protobuf/src/google/protobuf/io/zero_copy_stream_impl.h"
 
 namespace reporting {
@@ -157,8 +160,9 @@ class Storage::QueueUploaderInterface : public StorageQueue::UploaderInterface {
 
 class Storage::KeyInStorage {
  public:
-  explicit KeyInStorage(const base::FilePath& directory)
-      : directory_(directory) {}
+  explicit KeyInStorage(base::StringPiece signature_verification_public_key,
+                        const base::FilePath& directory)
+      : verifier_(signature_verification_public_key), directory_(directory) {}
   ~KeyInStorage() = default;
 
   // Uploads signed encryption key to a file with an |index| >=
@@ -219,6 +223,24 @@ class Storage::KeyInStorage {
         signed_encryption_key_result.value().second.public_key_id());
   }
 
+  Status VerifySignature(const SignedEncryptionInfo& signed_encryption_key) {
+    if (signed_encryption_key.public_asymmetric_key().size() !=
+        X25519_PUBLIC_VALUE_LEN) {
+      return Status{error::FAILED_PRECONDITION, "Key size mismatch"};
+    }
+    char value_to_verify[sizeof(Encryptor::PublicKeyId) +
+                         X25519_PUBLIC_VALUE_LEN];
+    const Encryptor::PublicKeyId public_key_id =
+        signed_encryption_key.public_key_id();
+    memcpy(value_to_verify, &public_key_id, sizeof(Encryptor::PublicKeyId));
+    memcpy(value_to_verify + sizeof(Encryptor::PublicKeyId),
+           signed_encryption_key.public_asymmetric_key().data(),
+           X25519_PUBLIC_VALUE_LEN);
+    return verifier_.Verify(
+        std::string(value_to_verify, sizeof(value_to_verify)),
+        signed_encryption_key.signature());
+  }
+
  private:
   // Writes key into file. Called during key upload.
   Status WriteKeyInfoFile(uint64_t new_file_index,
@@ -375,8 +397,16 @@ class Storage::KeyInStorage {
         }
       }
 
-      // Parsed successfully.
-      // TODO(b/170054326): Validate signature.
+      // Parsed successfully. Verify signature of the whole "id"+"key" string.
+      const auto signature_verification_status =
+          VerifySignature(signed_encryption_key);
+      if (!signature_verification_status.ok()) {
+        LOG(WARNING) << "Loaded key failed verification, status="
+                     << signature_verification_status << ", full_name='"
+                     << key_file_it->second.MaybeAsASCII() << "'";
+        continue;
+      }
+
       // Validated successfully. Return file name and signed key proto.
       return std::make_pair(key_file_it->second, signed_encryption_key);
     }
@@ -392,6 +422,8 @@ class Storage::KeyInStorage {
   // to successfully encrypt records and for the server to then decrypt them.
   std::atomic<uint64_t> next_key_file_index_{0};
 
+  SignatureVerifier verifier_;
+
   const base::FilePath directory_;
 };
 
@@ -537,7 +569,9 @@ Storage::Storage(const StorageOptions& options,
                  StartUploadCb start_upload_cb)
     : options_(options),
       encryption_module_(encryption_module),
-      key_in_storage_(std::make_unique<KeyInStorage>(options.directory())),
+      key_in_storage_(std::make_unique<KeyInStorage>(
+          options.signature_verification_public_key(),
+          options.directory())),
       start_upload_cb_(std::move(start_upload_cb)) {}
 
 Storage::~Storage() = default;
@@ -571,7 +605,14 @@ Status Storage::Flush(Priority priority) {
 }
 
 void Storage::UpdateEncryptionKey(SignedEncryptionInfo signed_encryption_key) {
-  // TODO(b/170054326): Verify received key signature. Bail out if failed.
+  // Verify received key signature. Bail out if failed.
+  const auto signature_verification_status =
+      key_in_storage_->VerifySignature(signed_encryption_key);
+  if (!signature_verification_status.ok()) {
+    LOG(WARNING) << "Key failed verification, status="
+                 << signature_verification_status;
+    return;
+  }
 
   // Assign the received key to encryption module.
   encryption_module_->UpdateAsymmetricKey(

chrome/browser/policy/messaging_layer/storage/storage_configuration.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/policy/messaging_layer/storage/storage_configuration.h"
+
+namespace reporting {
+
+StorageOptions::StorageOptions() = default;
+StorageOptions::StorageOptions(const StorageOptions& options) = default;
+StorageOptions& StorageOptions::operator=(const StorageOptions& options) =
+    default;
+StorageOptions::~StorageOptions() = default;
+
+}  // namespace reporting

chrome/browser/policy/messaging_layer/storage/storage_configuration.h

@@ -5,6 +5,12 @@
 #ifndef CHROME_BROWSER_POLICY_MESSAGING_LAYER_STORAGE_STORAGE_CONFIGURATION_H_
 #define CHROME_BROWSER_POLICY_MESSAGING_LAYER_STORAGE_STORAGE_CONFIGURATION_H_
 
+#include <string>
+
+#include "base/files/file_path.h"
+#include "base/strings/string_piece.h"
+#include "base/time/time.h"
+
 namespace reporting {
 
 // Storage options class allowing to set parameters individually, e.g.:
@@ -16,13 +22,20 @@ namespace reporting {
 //                 callback);
 class StorageOptions {
  public:
-  StorageOptions() = default;
-  StorageOptions(const StorageOptions& options) = default;
-  StorageOptions& operator=(const StorageOptions& options) = default;
+  StorageOptions();
+  StorageOptions(const StorageOptions& options);
+  StorageOptions& operator=(const StorageOptions& options);
+  ~StorageOptions();
   StorageOptions& set_directory(const base::FilePath& directory) {
     directory_ = directory;
     return *this;
   }
+  StorageOptions& set_signature_verification_public_key(
+      base::StringPiece signature_verification_public_key) {
+    signature_verification_public_key_ =
+        std::string(signature_verification_public_key);
+    return *this;
+  }
   StorageOptions& set_max_record_size(size_t max_record_size) {
     max_record_size_ = max_record_size;
     return *this;
@@ -40,6 +53,9 @@ class StorageOptions {
     return *this;
   }
   const base::FilePath& directory() const { return directory_; }
+  base::StringPiece signature_verification_public_key() const {
+    return signature_verification_public_key_;
+  }
   size_t max_record_size() const { return max_record_size_; }
   uint64_t max_total_files_size() const { return max_total_files_size_; }
   uint64_t max_total_memory_size() const { return max_total_memory_size_; }
@@ -49,6 +65,10 @@ class StorageOptions {
   // Subdirectory of the location assigned for this Storage.
   base::FilePath directory_;
 
+  // Public key for signature verification when encryption key
+  // is delivered to Storage.
+  std::string signature_verification_public_key_;
+
   // Maximum record size.
   size_t max_record_size_ = 1 * 1024LL * 1024LL;  // 1 MiB
 

chrome/browser/policy/messaging_layer/storage/storage_unittest.cc

@@ -438,6 +438,8 @@ class StorageTest
       // Enable encryption.
       scoped_feature_list_.InitFromCommandLine(
           {EncryptionModule::kEncryptedReporting}, {});
+      // Generate signing key pair.
+      ED25519_keypair(signature_verification_public_key_, signing_private_key_);
       // Create decryption module.
       auto decryptor_result = Decryptor::Create();
       ASSERT_OK(decryptor_result.status()) << decryptor_result.status();
@@ -495,9 +497,16 @@ class StorageTest
   }
 
   StorageOptions BuildTestStorageOptions() const {
-    return StorageOptions()
-        .set_directory(base::FilePath(location_.GetPath()))
-        .set_single_file_size(::testing::get<1>(GetParam()));
+    auto options = StorageOptions()
+                       .set_directory(base::FilePath(location_.GetPath()))
+                       .set_single_file_size(::testing::get<1>(GetParam()));
+    if (::testing::get<0>(GetParam())) {
+      // Encryption enabled.
+      options.set_signature_verification_public_key(std::string(
+          reinterpret_cast<const char*>(signature_verification_public_key_),
+          ED25519_PUBLIC_KEY_LEN));
+    }
+    return options;
   }
 
   StatusOr<std::unique_ptr<Storage::UploaderInterface>> BuildMockUploader(
@@ -534,8 +543,9 @@ class StorageTest
   void GenerateAndDeliverKey(Storage* storage) {
     ASSERT_TRUE(decryptor_) << "Decryptor not created";
     // Generate new pair of private key and public value.
-    uint8_t public_value[X25519_PUBLIC_VALUE_LEN];
     uint8_t private_key[X25519_PRIVATE_KEY_LEN];
+    Encryptor::PublicKeyId public_key_id;
+    uint8_t public_value[X25519_PUBLIC_VALUE_LEN];
     X25519_keypair(public_value, private_key);
     TestEvent<StatusOr<Encryptor::PublicKeyId>> prepare_key_pair;
     decryptor_->RecordKeyPair(
@@ -546,13 +556,28 @@ class StorageTest
         prepare_key_pair.cb());
     auto prepare_key_result = prepare_key_pair.result();
     ASSERT_OK(prepare_key_result.status());
-    Encryptor::PublicKeyId new_public_key_id = prepare_key_result.ValueOrDie();
+    public_key_id = prepare_key_result.ValueOrDie();
     // Deliver public key to storage.
     SignedEncryptionInfo signed_encryption_key;
     signed_encryption_key.set_public_asymmetric_key(std::string(
         reinterpret_cast<const char*>(public_value), X25519_PUBLIC_VALUE_LEN));
-    signed_encryption_key.set_public_key_id(new_public_key_id);
-    // TODO(b/170054326): Add key signature.
+    signed_encryption_key.set_public_key_id(public_key_id);
+    // Sign public key.
+    uint8_t
+        value_to_sign[sizeof(Encryptor::PublicKeyId) + X25519_PUBLIC_VALUE_LEN];
+    memcpy(value_to_sign, &public_key_id, sizeof(Encryptor::PublicKeyId));
+    memcpy(value_to_sign + sizeof(Encryptor::PublicKeyId), public_value,
+           X25519_PUBLIC_VALUE_LEN);
+    uint8_t signature[ED25519_SIGNATURE_LEN];
+    ASSERT_THAT(ED25519_sign(signature, value_to_sign, sizeof(value_to_sign),
+                             signing_private_key_),
+                Eq(1));
+    signed_encryption_key.set_signature(std::string(
+        reinterpret_cast<const char*>(signature), ED25519_SIGNATURE_LEN));
+    // Double check signature.
+    ASSERT_THAT(ED25519_verify(value_to_sign, sizeof(value_to_sign), signature,
+                               signature_verification_public_key_),
+                Eq(1));
     storage->UpdateEncryptionKey(signed_encryption_key);
   }
 
@@ -561,6 +586,9 @@ class StorageTest
 
   base::test::ScopedFeatureList scoped_feature_list_;
 
+  uint8_t signature_verification_public_key_[ED25519_PUBLIC_KEY_LEN];
+  uint8_t signing_private_key_[ED25519_PRIVATE_KEY_LEN];
+
   base::ScopedTempDir location_;
   scoped_refptr<Decryptor> decryptor_;
   scoped_refptr<Storage> storage_;