Roll CL forward 2363868 with fixes after revert by 2371704.

It appears that crypto::EnsureOpenSSLInit needs to be called before
any other OpenSSL function is called. This function is thread-safe,
and OpenSSL will only ever be initialized once. Multiple sequential
calls to EnsureOpenSSLInit are ignored by OpenSSL itself.

Also, HKDF expansion added to produce a uniformly random symmetric key
instead of using a shared secret directly.

Bug: 1121075
Change-Id: I9b9be85f7082b0fc047da901ae54856d25b0bbf1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2372898Reviewed-by: Adam Langley <agl@chromium.org>
Reviewed-by: Zach Trudo <zatrudo@google.com>
Commit-Queue: Leonid Baraz <lbaraz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#801181}

chrome/browser/BUILD.gn

@@ -1167,8 +1167,6 @@ static_library("browser") {
     "policy/homepage_location_policy_handler.h",
     "policy/javascript_policy_handler.cc",
     "policy/javascript_policy_handler.h",
-    "policy/messaging_layer/encryption/decryption.cc",
-    "policy/messaging_layer/encryption/decryption.h",
     "policy/messaging_layer/encryption/encryption.cc",
     "policy/messaging_layer/encryption/encryption.h",
     "policy/messaging_layer/encryption/encryption_module.cc",

chrome/browser/policy/messaging_layer/encryption/decryption.cc

@@ -4,44 +4,152 @@
 
 #include <string>
 
+#include "base/containers/span.h"
 #include "base/hash/hash.h"
+#include "base/memory/ptr_util.h"
 #include "base/strings/strcat.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_piece.h"
 #include "base/task/post_task.h"
 #include "base/task_runner.h"
 #include "chrome/browser/policy/messaging_layer/encryption/decryption.h"
+#include "chrome/browser/policy/messaging_layer/util/status.h"
+#include "chrome/browser/policy/messaging_layer/util/statusor.h"
+#include "crypto/aead.h"
+#include "crypto/openssl_util.h"
+#include "third_party/boringssl/src/include/openssl/curve25519.h"
+#include "third_party/boringssl/src/include/openssl/digest.h"
+#include "third_party/boringssl/src/include/openssl/hkdf.h"
 
 namespace reporting {
 
-DecryptorBase::Handle::Handle(scoped_refptr<DecryptorBase> decryptor)
-    : decryptor_(decryptor) {}
+Decryptor::Handle::Handle(base::StringPiece shared_secret,
+                          scoped_refptr<Decryptor> decryptor)
+    : shared_secret_(shared_secret), decryptor_(decryptor) {}
 
-DecryptorBase::Handle::~Handle() = default;
+Decryptor::Handle::~Handle() = default;
 
-DecryptorBase::DecryptorBase()
+void Decryptor::Handle::AddToRecord(base::StringPiece data,
+                                    base::OnceCallback<void(Status)> cb) {
+  // Add piece of data to the record.
+  record_.append(data.data(), data.size());
+  std::move(cb).Run(Status::StatusOK());
+}
+
+void Decryptor::Handle::CloseRecord(
+    base::OnceCallback<void(StatusOr<base::StringPiece>)> cb) {
+  // Make sure the record self-destructs when returning from this method.
+  const auto self_destruct = base::WrapUnique(this);
+
+  // Decrypt the data with symmetric key using AEAD interface.
+  crypto::Aead aead(crypto::Aead::CHACHA20_POLY1305);
+
+  // Produce symmetric key from shared secret using HKDF.
+  // Since the original keys were only used once, no salt and context is needed.
+  const auto out_symmetric_key = std::make_unique<uint8_t[]>(aead.KeyLength());
+  if (!HKDF(out_symmetric_key.get(), aead.KeyLength(), /*digest=*/EVP_sha256(),
+            reinterpret_cast<const uint8_t*>(shared_secret_.data()),
+            shared_secret_.size(),
+            /*salt=*/nullptr, /*salt_len=*/0,
+            /*info=*/nullptr, /*info_len=*/0)) {
+    std::move(cb).Run(
+        Status(error::INTERNAL, "Symmetric key extraction failed"));
+    return;
+  }
+
+  // Use the symmetric key for data decryption.
+  aead.Init(base::make_span(out_symmetric_key.get(), aead.KeyLength()));
+
+  // Set nonce to 0s, since a symmetric key is only used once.
+  // Note: if we ever start reusing the same symmetric key, we will need
+  // to generate new nonce for every record and transfer it to the peer.
+  std::string nonce(aead.NonceLength(), 0);
+
+  // Decrypt collected record.
+  std::string decrypted;
+  if (!aead.Open(record_, nonce, std::string(), &decrypted)) {
+    std::move(cb).Run(Status(error::INTERNAL, "Failed to decrypt"));
+    return;
+  }
+  record_.clear();  // Free unused memory.
+
+  // Return decrypted record.
+  std::move(cb).Run(decrypted);
+}
+
+void Decryptor::OpenRecord(base::StringPiece shared_secret,
+                           base::OnceCallback<void(StatusOr<Handle*>)> cb) {
+  std::move(cb).Run(new Handle(shared_secret, this));
+}
+
+StatusOr<std::string> Decryptor::DecryptSecret(
+    base::StringPiece private_key,
+    base::StringPiece peer_public_value) {
+  // Verify the keys.
+  if (private_key.size() != X25519_PRIVATE_KEY_LEN) {
+    return Status(
+        error::FAILED_PRECONDITION,
+        base::StrCat({"Private key size mismatch, expected=",
+                      base::NumberToString(X25519_PRIVATE_KEY_LEN),
+                      " actual=", base::NumberToString(private_key.size())}));
+  }
+  if (peer_public_value.size() != X25519_PUBLIC_VALUE_LEN) {
+    return Status(
+        error::FAILED_PRECONDITION,
+        base::StrCat({"Public key size mismatch, expected=",
+                      base::NumberToString(X25519_PUBLIC_VALUE_LEN), " actual=",
+                      base::NumberToString(peer_public_value.size())}));
+  }
+
+  // Compute shared secret.
+  uint8_t out_shared_value[X25519_SHARED_KEY_LEN];
+  if (!X25519(out_shared_value,
+              reinterpret_cast<const uint8_t*>(private_key.data()),
+              reinterpret_cast<const uint8_t*>(peer_public_value.data()))) {
+    return Status(error::DATA_LOSS, "Curve25519 decryption failed");
+  }
+
+  return std::string(reinterpret_cast<const char*>(out_shared_value),
+                     X25519_SHARED_KEY_LEN);
+}
+
+Decryptor::Decryptor()
     : keys_sequenced_task_runner_(base::ThreadPool::CreateSequencedTaskRunner(
           {base::TaskPriority::BEST_EFFORT, base::MayBlock()})) {
   DETACH_FROM_SEQUENCE(keys_sequence_checker_);
 }
 
-DecryptorBase::~DecryptorBase() = default;
+Decryptor::~Decryptor() = default;
 
-void DecryptorBase::RecordKeyPair(base::StringPiece private_key,
-                                  base::StringPiece public_key,
-                                  base::OnceCallback<void(Status)> cb) {
+void Decryptor::RecordKeyPair(base::StringPiece private_key,
+                              base::StringPiece public_key,
+                              base::OnceCallback<void(Status)> cb) {
   // Schedule key recording on the sequenced task runner.
   keys_sequenced_task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(
           [](std::string public_key, KeyInfo key_info,
              base::OnceCallback<void(Status)> cb,
-             scoped_refptr<DecryptorBase> decryptor) {
+             scoped_refptr<Decryptor> decryptor) {
             DCHECK_CALLED_ON_VALID_SEQUENCE(decryptor->keys_sequence_checker_);
             Status result;
-            if (!decryptor->keys_
-                     .emplace(base::PersistentHash(public_key.data(),
-                                                   public_key.size()),
-                              key_info)
-                     .second) {
+            if (key_info.private_key.size() != X25519_PRIVATE_KEY_LEN) {
+              result = Status(
+                  error::FAILED_PRECONDITION,
+                  base::StrCat(
+                      {"Private key size mismatch, expected=",
+                       base::NumberToString(X25519_PRIVATE_KEY_LEN), " actual=",
+                       base::NumberToString(key_info.private_key.size())}));
+            } else if (public_key.size() != X25519_PUBLIC_VALUE_LEN) {
+              result = Status(
+                  error::FAILED_PRECONDITION,
+                  base::StrCat(
+                      {"Public key size mismatch, expected=",
+                       base::NumberToString(X25519_PUBLIC_VALUE_LEN),
+                       " actual=", base::NumberToString(public_key.size())}));
+            } else if (!decryptor->keys_
+                            .emplace(base::PersistentHash(public_key), key_info)
+                            .second) {
               result = Status(error::ALREADY_EXISTS,
                               base::StrCat({"Public key='", public_key,
                                             "' already recorded"}));
@@ -59,7 +167,7 @@ void DecryptorBase::RecordKeyPair(base::StringPiece private_key,
           std::move(cb), base::WrapRefCounted(this)));
 }
 
-void DecryptorBase::RetrieveMatchingPrivateKey(
+void Decryptor::RetrieveMatchingPrivateKey(
     uint32_t public_key_id,
     base::OnceCallback<void(StatusOr<std::string>)> cb) {
   // Schedule key retrieval on the sequenced task runner.
@@ -68,9 +176,13 @@ void DecryptorBase::RetrieveMatchingPrivateKey(
       base::BindOnce(
           [](uint32_t public_key_id,
              base::OnceCallback<void(StatusOr<std::string>)> cb,
-             scoped_refptr<DecryptorBase> decryptor) {
+             scoped_refptr<Decryptor> decryptor) {
             DCHECK_CALLED_ON_VALID_SEQUENCE(decryptor->keys_sequence_checker_);
             auto key_info_it = decryptor->keys_.find(public_key_id);
+            if (key_info_it != decryptor->keys_.end()) {
+              DCHECK_EQ(key_info_it->second.private_key.size(),
+                        static_cast<size_t>(X25519_PRIVATE_KEY_LEN));
+            }
             // Schedule response on a generic thread pool.
             base::ThreadPool::PostTask(
                 FROM_HERE,
@@ -88,4 +200,10 @@ void DecryptorBase::RetrieveMatchingPrivateKey(
           public_key_id, std::move(cb), base::WrapRefCounted(this)));
 }
 
+StatusOr<scoped_refptr<Decryptor>> Decryptor::Create() {
+  // Make sure OpenSSL is initialized, in order to avoid data races later.
+  crypto::EnsureOpenSSLInit();
+  return base::WrapRefCounted(new Decryptor());
+}
+
 }  // namespace reporting

chrome/browser/policy/messaging_layer/encryption/decryption.h

@@ -11,6 +11,7 @@
 #include "base/containers/flat_map.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_refptr.h"
+#include "base/optional.h"
 #include "base/strings/string_piece.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -19,14 +20,16 @@
 
 namespace reporting {
 
-// Interface to the encryption.
+// Full implementation of Decryptor, intended for use in tests and potentially
+// in reporting server (wrapped in a Java class).
+//
+// Curve25519 decryption of the symmetric key with asymmetric private key.
+// ChaCha20_Poly1305 decryption and verification of a record in place with
+// symmetric key.
+//
 // Instantiated by an implementation-specific factory:
-//   StatusOr<scoped_refptr<DecryptorBase>> Create(
-//       implementation-specific parameters);
-// The implementation class should never be used directly by the server code.
-// Note: Production implementation should be written or enclosed in Java code
-// for the server to use.
-class DecryptorBase : public base::RefCountedThreadSafe<DecryptorBase> {
+//   StatusOr<scoped_refptr<Decryptor>> Create();
+class Decryptor : public base::RefCountedThreadSafe<Decryptor> {
  public:
   // Decryption record handle, which is created by |OpenRecord| and can accept
   // pieces of data to be decrypted as one record by calling |AddToRecord|
@@ -34,43 +37,47 @@ class DecryptorBase : public base::RefCountedThreadSafe<DecryptorBase> {
   // is called.
   class Handle {
    public:
-    // Adds piece of data to the record.
-    virtual void AddToRecord(base::StringPiece data,
-                             base::OnceCallback<void(Status)> cb) = 0;
+    Handle(base::StringPiece shared_secret, scoped_refptr<Decryptor> decryptor);
+    Handle(const Handle& other) = delete;
+    Handle& operator=(const Handle& other) = delete;
+    ~Handle();
+
+    // Adds piece of encrypted data to the record.
+    void AddToRecord(base::StringPiece data,
+                     base::OnceCallback<void(Status)> cb);
 
     // Closes and attempts to decrypt the record. Hands over the decrypted data
     // to be processed by the server (or Status if unsuccessful). Accesses key
     // store to attempt all private keys that are considered to be valid,
     // starting with the one that matches the hash. Self-destructs after the
     // callback.
-    virtual void CloseRecord(
-        base::OnceCallback<void(StatusOr<base::StringPiece>)> cb) = 0;
+    void CloseRecord(base::OnceCallback<void(StatusOr<base::StringPiece>)> cb);
 
-   protected:
-    explicit Handle(scoped_refptr<DecryptorBase> decryptor);
-
-    // Destructor is non-public, because the object can only self-destruct by
-    // |CloseRecord|.
-    virtual ~Handle();
+   private:
+    // Shared secret based on which symmetric key is produced.
+    const std::string shared_secret_;
 
-    DecryptorBase* decryptor() const { return decryptor_.get(); }
+    // Accumulated data to decrypt.
+    std::string record_;
 
-   private:
-    scoped_refptr<DecryptorBase> decryptor_;
+    scoped_refptr<Decryptor> decryptor_;
   };
 
-  // Factory method creates new record to collect data and decrypt them with the
-  // given encrypted key. Hands the handle raw pointer over to the callback, or
-  // error status (e.g., “decryption is not enabled yet”)
-  virtual void OpenRecord(base::StringPiece encrypted_key,
-                          base::OnceCallback<void(StatusOr<Handle*>)> cb) = 0;
+  // Factory method to instantiate the Decryptor.
+  static StatusOr<scoped_refptr<Decryptor>> Create();
+
+  // Factory method creates a new record to collect data and decrypt them with
+  // the given encrypted key. Hands the handle raw pointer over to the callback,
+  // or error status.
+  void OpenRecord(base::StringPiece encrypted_key,
+                  base::OnceCallback<void(StatusOr<Handle*>)> cb);
 
-  // Decrypts symmetric key with asymmetric private key and returns unencrypted
-  // key or error status (e.g., “decryption is not enabled yet”)
-  virtual StatusOr<std::string> DecryptKey(base::StringPiece public_key,
-                                           base::StringPiece encrypted_key) = 0;
+  // Recreates shared secret from local private key and peer public value and
+  // returns it or error status.
+  StatusOr<std::string> DecryptSecret(base::StringPiece public_key,
+                                      base::StringPiece peer_public_value);
 
-  // Records a key pair (store only private key).
+  // Records a key pair (stores only private key).
   // Executes on a sequenced thread, returns with callback.
   void RecordKeyPair(base::StringPiece private_key,
                      base::StringPiece public_key,
@@ -82,23 +89,23 @@ class DecryptorBase : public base::RefCountedThreadSafe<DecryptorBase> {
       uint32_t public_key_id,
       base::OnceCallback<void(StatusOr<std::string>)> cb);
 
- protected:
-  DecryptorBase();
-  virtual ~DecryptorBase();
-
  private:
-  friend base::RefCountedThreadSafe<DecryptorBase>;
+  friend base::RefCountedThreadSafe<Decryptor>;
+  Decryptor();
+  ~Decryptor();
 
-  // Map of hash(public_key)->{public key, private key, time stamp}
+  // Map of hash(public_key)->{private key, time stamp}
   // Private key is located by the hash of a public key, sent together with the
   // encrypted record. Keys older than pre-defined threshold are discarded.
+  // Time stamp allows to drop outdated keys (not implemented yet).
   struct KeyInfo {
     std::string private_key;
     base::Time time_stamp;
   };
   base::flat_map<uint32_t, KeyInfo> keys_;
 
-  // Sequential task runner for all keys_ activities: recording, lookup, purge.
+  // Sequential task runner for all keys_ activities:
+  // recording, lookup, purge.
   scoped_refptr<base::SequencedTaskRunner> keys_sequenced_task_runner_;
 
   SEQUENCE_CHECKER(keys_sequence_checker_);

chrome/browser/policy/messaging_layer/encryption/encryption.cc

@@ -2,31 +2,136 @@
 // 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/encryption/encryption.h"
+
 #include <string>
 #include <utility>
 
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/containers/span.h"
 #include "base/hash/hash.h"
+#include "base/memory/ptr_util.h"
+#include "base/strings/strcat.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/task/post_task.h"
 #include "base/task_runner.h"
-#include "chrome/browser/policy/messaging_layer/encryption/encryption.h"
+#include "chrome/browser/policy/messaging_layer/util/status.h"
+#include "chrome/browser/policy/messaging_layer/util/statusor.h"
+#include "crypto/aead.h"
+#include "crypto/openssl_util.h"
+#include "third_party/boringssl/src/include/openssl/curve25519.h"
+#include "third_party/boringssl/src/include/openssl/digest.h"
+#include "third_party/boringssl/src/include/openssl/hkdf.h"
 
 namespace reporting {
 
-EncryptorBase::Handle::Handle(scoped_refptr<EncryptorBase> encryptor)
+Encryptor::Handle::Handle(scoped_refptr<Encryptor> encryptor)
     : encryptor_(encryptor) {}
 
-EncryptorBase::Handle::~Handle() = default;
+Encryptor::Handle::~Handle() = default;
+
+void Encryptor::Handle::AddToRecord(base::StringPiece data,
+                                    base::OnceCallback<void(Status)> cb) {
+  // Append new data to the record.
+  record_.append(data.data(), data.size());
+  std::move(cb).Run(Status::StatusOK());
+}
+
+void Encryptor::Handle::CloseRecord(
+    base::OnceCallback<void(StatusOr<EncryptedRecord>)> cb) {
+  // Retrieves asymmetric public key to use.
+  encryptor_->RetrieveAsymmetricKey(base::BindOnce(
+      &Handle::ProduceEncryptedRecord, base::Unretained(this), std::move(cb)));
+}
+
+void Encryptor::Handle::ProduceEncryptedRecord(
+    base::OnceCallback<void(StatusOr<EncryptedRecord>)> cb,
+    StatusOr<std::string> asymmetric_key_result) {
+  // Make sure the record self-destructs when returning from this method.
+  const auto self_destruct = base::WrapUnique(this);
+
+  // Validate keys.
+  if (!asymmetric_key_result.ok()) {
+    std::move(cb).Run(asymmetric_key_result.status());
+    return;
+  }
+  const auto& asymmetric_key = asymmetric_key_result.ValueOrDie();
+  if (asymmetric_key.size() != X25519_PUBLIC_VALUE_LEN) {
+    std::move(cb).Run(Status(
+        error::INTERNAL,
+        base::StrCat({"Asymmetric key size mismatch, expected=",
+                      base::NumberToString(X25519_PUBLIC_VALUE_LEN), " actual=",
+                      base::NumberToString(asymmetric_key.size())})));
+    return;
+  }
+
+  // Generate new pair of private key and public value.
+  uint8_t out_public_value[X25519_PUBLIC_VALUE_LEN];
+  uint8_t out_private_key[X25519_PRIVATE_KEY_LEN];
+  X25519_keypair(out_public_value, out_private_key);
+
+  // Compute shared secret.
+  uint8_t out_shared_secret[X25519_SHARED_KEY_LEN];
+  if (!X25519(out_shared_secret, out_private_key,
+              reinterpret_cast<const uint8_t*>(asymmetric_key.data()))) {
+    std::move(cb).Run(Status(error::DATA_LOSS, "Curve25519 encryption failed"));
+    return;
+  }
+
+  // Encrypt the data with symmetric key using AEAD interface.
+  crypto::Aead aead(crypto::Aead::CHACHA20_POLY1305);
+
+  // Produce symmetric key from shared secret using HKDF.
+  // Since the keys above are only used once, no salt and context is provided.
+  const auto out_symmetric_key = std::make_unique<uint8_t[]>(aead.KeyLength());
+  if (!HKDF(out_symmetric_key.get(), aead.KeyLength(), /*digest=*/EVP_sha256(),
+            out_shared_secret, X25519_SHARED_KEY_LEN,
+            /*salt=*/nullptr, /*salt_len=*/0,
+            /*info=*/nullptr, /*info_len=*/0)) {
+    std::move(cb).Run(
+        Status(error::INTERNAL, "Symmetric key extraction failed"));
+    return;
+  }
+
+  // Use the symmetric key for data encryption.
+  aead.Init(base::make_span(out_symmetric_key.get(), aead.KeyLength()));
 
-EncryptorBase::EncryptorBase()
+  // Set nonce to 0s, since a symmetric key is only used once.
+  // Note: if we ever start reusing the same symmetric key, we will need
+  // to generate new nonce for every record and transfer it to the peer.
+  std::string nonce(aead.NonceLength(), 0);
+
+  // Prepare encrypted record.
+  EncryptedRecord encrypted_record;
+  encrypted_record.mutable_encryption_info()->set_public_key_id(
+      base::PersistentHash(asymmetric_key));
+  encrypted_record.mutable_encryption_info()->set_encryption_key(
+      reinterpret_cast<const char*>(out_public_value), X25519_PUBLIC_VALUE_LEN);
+
+  // Encrypt the whole record.
+  if (!aead.Seal(record_, nonce, std::string(),
+                 encrypted_record.mutable_encrypted_wrapped_record()) ||
+      encrypted_record.encrypted_wrapped_record().empty()) {
+    std::move(cb).Run(Status(error::INTERNAL, "Failed to encrypt the record"));
+    return;
+  }
+  record_.clear();  // Free unused memory.
+
+  // Return EncryptedRecord.
+  std::move(cb).Run(encrypted_record);
+}
+
+Encryptor::Encryptor()
     : asymmetric_key_sequenced_task_runner_(
           base::ThreadPool::CreateSequencedTaskRunner(
               {base::TaskPriority::BEST_EFFORT, base::MayBlock()})) {
   DETACH_FROM_SEQUENCE(asymmetric_key_sequence_checker_);
 }
 
-EncryptorBase::~EncryptorBase() = default;
+Encryptor::~Encryptor() = default;
 
-void EncryptorBase::UpdateAsymmetricKey(
+void Encryptor::UpdateAsymmetricKey(
     base::StringPiece new_key,
     base::OnceCallback<void(Status)> response_cb) {
   if (new_key.empty()) {
@@ -37,25 +142,29 @@ void EncryptorBase::UpdateAsymmetricKey(
 
   // Schedule key update on the sequenced task runner.
   asymmetric_key_sequenced_task_runner_->PostTask(
-      FROM_HERE, base::BindOnce(
-                     [](base::StringPiece new_key,
-                        scoped_refptr<EncryptorBase> encryptor) {
-                       encryptor->asymmetric_key_ = std::string(new_key);
-                     },
-                     new_key, base::WrapRefCounted(this)));
+      FROM_HERE,
+      base::BindOnce(
+          [](base::StringPiece new_key, scoped_refptr<Encryptor> encryptor) {
+            encryptor->asymmetric_key_ = std::string(new_key);
+          },
+          std::string(new_key), base::WrapRefCounted(this)));
 
   // Response OK not waiting for the update.
   std::move(response_cb).Run(Status::StatusOK());
 }
 
-void EncryptorBase::RetrieveAsymmetricKey(
+void Encryptor::OpenRecord(base::OnceCallback<void(StatusOr<Handle*>)> cb) {
+  std::move(cb).Run(new Handle(this));
+}
+
+void Encryptor::RetrieveAsymmetricKey(
     base::OnceCallback<void(StatusOr<std::string>)> cb) {
-  // Schedule key retrueval on the sequenced task runner.
+  // Schedule key retrieval on the sequenced task runner.
   asymmetric_key_sequenced_task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(
           [](base::OnceCallback<void(StatusOr<std::string>)> cb,
-             scoped_refptr<EncryptorBase> encryptor) {
+             scoped_refptr<Encryptor> encryptor) {
             DCHECK_CALLED_ON_VALID_SEQUENCE(
                 encryptor->asymmetric_key_sequence_checker_);
             StatusOr<std::string> response;
@@ -76,25 +185,10 @@ void EncryptorBase::RetrieveAsymmetricKey(
           std::move(cb), base::WrapRefCounted(this)));
 }
 
-void EncryptorBase::EncryptKey(
-    base::StringPiece symmetric_key,
-    base::OnceCallback<void(StatusOr<std::pair<uint32_t, std::string>>)> cb) {
-  RetrieveAsymmetricKey(base::BindOnce(
-      [](base::StringPiece symmetric_key,
-         scoped_refptr<EncryptorBase> encryptor,
-         base::OnceCallback<void(StatusOr<std::pair<uint32_t, std::string>>)>
-             cb,
-         StatusOr<std::string> asymmetric_key_result) {
-        if (!asymmetric_key_result.ok()) {
-          std::move(cb).Run(asymmetric_key_result.status());
-          return;
-        }
-        const auto& asymmetric_key = asymmetric_key_result.ValueOrDie();
-        std::move(cb).Run(std::make_pair(
-            base::PersistentHash(asymmetric_key),
-            encryptor->EncryptSymmetricKey(symmetric_key, asymmetric_key)));
-      },
-      std::string(symmetric_key), base::WrapRefCounted(this), std::move(cb)));
+StatusOr<scoped_refptr<Encryptor>> Encryptor::Create() {
+  // Make sure OpenSSL is initialized, in order to avoid data races later.
+  crypto::EnsureOpenSSLInit();
+  return base::WrapRefCounted(new Encryptor());
 }
 
 }  // namespace reporting

chrome/browser/policy/messaging_layer/encryption/encryption.h

@@ -19,12 +19,25 @@
 
 namespace reporting {
 
-// Interface to the encryption.
-// Instantiated by an implementation-specific factory:
-//   StatusOr<scoped_refptr<EncryptorBase>> Create(
-//       implementation-specific parameters);
+// Full implementation of Encryptor, intended for use in reporting client.
+// ChaCha20_Poly1305 AEAD encryption of a record in place with symmetric key.
+// Curve25519 encryption of the symmetric key with asymmetric public key.
+//
+// We generate new Curve25519 public/private keys pair for each record.
+// Then we produce Curve25519 shared secret from our private key and peer's
+// public key, and use it for ChaCha20_Poly1305 AEAD encryption of the record.
+// We send out our public value (calling it encrypted symmetric key) together
+// with encrypted record.
+//
+// Upon receiving the encrypted message the peer will produce the same shared
+// secret by combining their private key and our public key, and use it as
+// a symmetric key for ChaCha20_Poly1305 decryption and validation of the
+// record.
+//
+// Instantiated by a factory:
+//   StatusOr<scoped_refptr<Encryptor>> Create();
 // The implementation class should never be used directly by the client code.
-class EncryptorBase : public base::RefCountedThreadSafe<EncryptorBase> {
+class Encryptor : public base::RefCountedThreadSafe<Encryptor> {
  public:
   // Encryption record handle, which is created by |OpenRecord| and can accept
   // pieces of data to be encrypted as one record by calling |AddToRecord|
@@ -32,30 +45,41 @@ class EncryptorBase : public base::RefCountedThreadSafe<EncryptorBase> {
   // is called.
   class Handle {
    public:
+    explicit Handle(scoped_refptr<Encryptor> encryptor);
+    Handle(const Handle& other) = delete;
+    Handle& operator=(const Handle& other) = delete;
+    ~Handle();
+
     // Adds piece of data to the record.
-    virtual void AddToRecord(base::StringPiece data,
-                             base::OnceCallback<void(Status)> cb) = 0;
+    void AddToRecord(base::StringPiece data,
+                     base::OnceCallback<void(Status)> cb);
 
     // Closes and encrypts the record, hands over the data (encrypted with
     // symmetric key) and the key (encrypted with asymmetric key) to be recorded
     // by the client (or Status if unsuccessful). Self-destructs after the
     // callback.
-    virtual void CloseRecord(
-        base::OnceCallback<void(StatusOr<EncryptedRecord>)> cb) = 0;
-
-   protected:
-    explicit Handle(scoped_refptr<EncryptorBase> encryptor);
+    void CloseRecord(base::OnceCallback<void(StatusOr<EncryptedRecord>)> cb);
 
-    // Destructor is non-public, because the object can only self-destruct by
-    // |CloseRecord|.
-    virtual ~Handle();
+   private:
+    // Helper method to compose EncryptedRecord. Called by |CloseRecord|
+    // as a callback after asynchronous retrieval of the asymmetric key.
+    void ProduceEncryptedRecord(
+        base::OnceCallback<void(StatusOr<EncryptedRecord>)> cb,
+        StatusOr<std::string> asymmetric_key_result);
 
-    EncryptorBase* encryptor() const { return encryptor_.get(); }
+    // Accumulated data to encrypt.
+    std::string record_;
 
-   private:
-    scoped_refptr<EncryptorBase> encryptor_;
+    scoped_refptr<Encryptor> encryptor_;
   };
 
+  // Factory method to instantiate the Encryptor.
+  static StatusOr<scoped_refptr<Encryptor>> Create();
+
+  // Factory method creates new record to collect data and encrypt them.
+  // Hands the Handle raw pointer over to the callback, or error status).
+  void OpenRecord(base::OnceCallback<void(StatusOr<Handle*>)> cb);
+
   // Delivers public asymmetric key to the implementation.
   // To affect specific record, must happen before Handle::CloseRecord
   // (it is OK to do it after OpenRecord and Handle::AddToRecord).
@@ -63,35 +87,16 @@ class EncryptorBase : public base::RefCountedThreadSafe<EncryptorBase> {
   void UpdateAsymmetricKey(base::StringPiece new_key,
                            base::OnceCallback<void(Status)> response_cb);
 
-  // Factory method creates new record to collect data and encrypt them.
-  // Hands the Handle raw pointer over to the callback, or error status
-  // (e.g., “encryption is not enabled yet”).
-  virtual void OpenRecord(base::OnceCallback<void(StatusOr<Handle*>)> cb) = 0;
-
-  // Encrypts symmetric key with asymmetric public key, returns encrypted key
-  // and the hash of the public key used or error status (e.g., “decryption is
-  // not enabled yet”)
-  void EncryptKey(
-      base::StringPiece key,
-      base::OnceCallback<void(StatusOr<std::pair<uint32_t, std::string>>)> cb);
-
- protected:
-  EncryptorBase();
-  virtual ~EncryptorBase();
-
- private:
-  friend class base::RefCountedThreadSafe<EncryptorBase>;
-
-  // Synchronously encrypts symmetric key with asymmetric.
-  // Called by |EncryptKey|.
-  virtual std::string EncryptSymmetricKey(base::StringPiece symmetric_key,
-                                          base::StringPiece asymmetric_key) = 0;
-
   // Retrieves the current public key.
   // Executes on a sequenced thread, returns with callback.
   void RetrieveAsymmetricKey(
       base::OnceCallback<void(StatusOr<std::string>)> cb);
 
+ private:
+  friend class base::RefCountedThreadSafe<Encryptor>;
+  Encryptor();
+  ~Encryptor();
+
   // Public key used for asymmetric encryption of symmetric key.
   base::Optional<std::string> asymmetric_key_;
 

chrome/browser/policy/messaging_layer/encryption/fake_decryption.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/encryption/fake_decryption.h"
-
-#include "base/memory/ptr_util.h"
-#include "chrome/browser/policy/messaging_layer/util/status.h"
-#include "chrome/browser/policy/messaging_layer/util/statusor.h"
-
-namespace reporting {
-namespace test {
-
-namespace {
-// Decryption record handle for FakeDecryptor.
-class MockRecordHandle : public DecryptorBase::Handle {
- public:
-  explicit MockRecordHandle(base::StringPiece symmetric_key,
-                            scoped_refptr<DecryptorBase> decryptor)
-      : Handle(decryptor), symmetric_key_(symmetric_key) {}
-
-  MockRecordHandle(const MockRecordHandle& other) = delete;
-  MockRecordHandle& operator=(const MockRecordHandle& other) = delete;
-
-  void AddToRecord(base::StringPiece data,
-                   base::OnceCallback<void(Status)> cb) override {
-    // Add piece of data to the record.
-    record_.append(data.data(), data.size());
-    std::move(cb).Run(Status::StatusOK());
-  }
-
-  void CloseRecord(
-      base::OnceCallback<void(StatusOr<base::StringPiece>)> cb) override {
-    // Decrypt data in place by XORing every byte with the bytes of symmetric
-    // key.
-    size_t key_i = 0;
-    for (auto& record_byte : record_) {
-      record_byte ^= symmetric_key_[key_i++];
-      if (key_i >= symmetric_key_.size()) {
-        key_i = 0;
-      }
-    }
-    std::move(cb).Run(record_);
-    delete this;
-  }
-
- private:
-  // Symmetric key.
-  const std::string symmetric_key_;
-
-  // Accumulated decrypted data.
-  std::string record_;
-};
-
-}  // namespace
-
-StatusOr<scoped_refptr<DecryptorBase>> FakeDecryptor::Create() {
-  return base::WrapRefCounted(new FakeDecryptor());
-}
-
-FakeDecryptor::FakeDecryptor() = default;
-FakeDecryptor::~FakeDecryptor() = default;
-
-void FakeDecryptor::OpenRecord(base::StringPiece encrypted_key,
-                               base::OnceCallback<void(StatusOr<Handle*>)> cb) {
-  std::move(cb).Run(new MockRecordHandle(encrypted_key, this));
-}
-
-StatusOr<std::string> FakeDecryptor::DecryptKey(
-    base::StringPiece private_key,
-    base::StringPiece encrypted_key) {
-  if (private_key.empty()) {
-    return Status{error::FAILED_PRECONDITION, "Private key not provided"};
-  }
-  // Decrypt symmetric key.
-  // Private key is assumed to be a reverse string to the public key.
-  // If symmetric key was encrypted XORing bytes with a public key "012",
-  // decryption will use private key "210" and XOR will be from the last to the
-  // first bytes.
-  std::string unencrypted_key;
-  unencrypted_key.reserve(encrypted_key.size());
-  size_t key_i = 0;
-  for (const auto& key_byte : encrypted_key) {
-    unencrypted_key.push_back(key_byte ^
-                              private_key[private_key.size() - ++key_i]);
-    if (key_i >= private_key.size()) {
-      key_i = 0;
-    }
-  }
-  return unencrypted_key;
-}
-
-}  // namespace test
-}  // namespace reporting

chrome/browser/policy/messaging_layer/encryption/fake_decryption.h

-// 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.
-
-#ifndef CHROME_BROWSER_POLICY_MESSAGING_LAYER_ENCRYPTION_FAKE_DECRYPTION_H_
-#define CHROME_BROWSER_POLICY_MESSAGING_LAYER_ENCRYPTION_FAKE_DECRYPTION_H_
-
-#include <string>
-
-#include "base/callback.h"
-#include "base/memory/scoped_refptr.h"
-#include "base/optional.h"
-#include "base/strings/string_piece.h"
-#include "chrome/browser/policy/messaging_layer/encryption/decryption.h"
-#include "chrome/browser/policy/messaging_layer/util/status.h"
-#include "chrome/browser/policy/messaging_layer/util/statusor.h"
-
-namespace reporting {
-namespace test {
-
-// Fake implementation of DecryptorBase, intended for use in tests of
-// reporting client.
-// Key decryption with asymmetric private key is done by per-byte XOR in reverse
-// order: public and private key are reverse, so if the encryption used XOR with
-// public key "012", decryption will use private key "210" and XOR will be from
-// the last to the first bytes.
-// Record decryption with symmetric key is done by per-byte XOR.
-class FakeDecryptor : public DecryptorBase {
- public:
-  // Factory method
-  static StatusOr<scoped_refptr<DecryptorBase>> Create();
-
-  void OpenRecord(base::StringPiece encrypted_key,
-                  base::OnceCallback<void(StatusOr<Handle*>)> cb) override;
-
-  StatusOr<std::string> DecryptKey(base::StringPiece private_key,
-                                   base::StringPiece encrypted_key) override;
-
- private:
-  FakeDecryptor();
-  ~FakeDecryptor() override;
-};
-
-}  // namespace test
-}  // namespace reporting
-
-#endif  // CHROME_BROWSER_POLICY_MESSAGING_LAYER_ENCRYPTION_FAKE_DECRYPTION_H_

chrome/browser/policy/messaging_layer/encryption/fake_encryption.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/encryption/fake_encryption.h"
-
-#include "base/bind.h"
-#include "base/callback.h"
-#include "base/hash/hash.h"
-#include "base/memory/ptr_util.h"
-#include "chrome/browser/policy/messaging_layer/util/status.h"
-#include "chrome/browser/policy/messaging_layer/util/statusor.h"
-#include "crypto/random.h"
-
-namespace reporting {
-namespace test {
-
-namespace {
-// Encryption record handle for FakeEncryptor.
-class MockRecordHandle : public EncryptorBase::Handle {
- public:
-  explicit MockRecordHandle(base::StringPiece symmetric_key,
-                            scoped_refptr<EncryptorBase> encryptor)
-      : Handle(encryptor), symmetric_key_(symmetric_key) {}
-
-  MockRecordHandle(const MockRecordHandle& other) = delete;
-  MockRecordHandle& operator=(const MockRecordHandle& other) = delete;
-
-  void AddToRecord(base::StringPiece data,
-                   base::OnceCallback<void(Status)> cb) override {
-    // Append new data to the record.
-    record_.append(data.data(), data.size());
-    std::move(cb).Run(Status::StatusOK());
-  }
-
-  void CloseRecord(
-      base::OnceCallback<void(StatusOr<EncryptedRecord>)> cb) override {
-    // Encrypt all collected data in place by XORing every byte with the
-    // symmetric key.
-    size_t key_i = 0;
-    for (auto& record_byte : record_) {
-      record_byte ^= symmetric_key_[key_i++];
-      if (key_i >= symmetric_key_.size()) {
-        key_i = 0;
-      }
-    }
-    // Encrypt the symmetric key.
-    encryptor()->EncryptKey(
-        symmetric_key_,
-        base::BindOnce(
-            [](MockRecordHandle* handle,
-               base::OnceCallback<void(StatusOr<EncryptedRecord>)> cb,
-               StatusOr<std::pair<uint32_t, std::string>>
-                   encrypted_key_result) {
-              if (!encrypted_key_result.ok()) {
-                std::move(cb).Run(encrypted_key_result.status());
-              } else {
-                EncryptedRecord encrypted_record;
-                encrypted_record.mutable_encryption_info()->set_public_key_id(
-                    encrypted_key_result.ValueOrDie().first);
-                encrypted_record.mutable_encryption_info()->set_encryption_key(
-                    encrypted_key_result.ValueOrDie().second);
-                encrypted_record.set_encrypted_wrapped_record(handle->record_);
-                std::move(cb).Run(encrypted_record);
-              }
-              delete handle;
-            },
-            base::Unretained(this),  // will self-destruct.
-            std::move(cb)));
-  }
-
- private:
-  // Symmetric key.
-  const std::string symmetric_key_;
-
-  // Accumulated encrypted data.
-  std::string record_;
-};
-
-}  // namespace
-
-StatusOr<scoped_refptr<EncryptorBase>> FakeEncryptor::Create() {
-  return base::WrapRefCounted(new FakeEncryptor());
-}
-
-FakeEncryptor::FakeEncryptor() = default;
-FakeEncryptor::~FakeEncryptor() = default;
-
-void FakeEncryptor::OpenRecord(base::OnceCallback<void(StatusOr<Handle*>)> cb) {
-  // For fake implementation just generate random byte string.
-  constexpr size_t symmetric_key_size = 8;
-  char symmetric_key[8];
-  crypto::RandBytes(symmetric_key, symmetric_key_size);
-  std::move(cb).Run(new MockRecordHandle(
-      std::string(symmetric_key, symmetric_key_size), this));
-}
-
-std::string FakeEncryptor::EncryptSymmetricKey(
-    base::StringPiece symmetric_key,
-    base::StringPiece asymmetric_key) {
-  // Encrypt symmetric key with public asymmetric one: XOR byte by byte.
-  std::string encrypted_key;
-  encrypted_key.reserve(symmetric_key.size());
-  size_t asymmetric_i = 0;
-  for (const auto& key_byte : symmetric_key) {
-    encrypted_key.push_back(key_byte ^ asymmetric_key[asymmetric_i++]);
-    if (asymmetric_i >= asymmetric_key.size()) {
-      asymmetric_i = 0;
-    }
-  }
-  return encrypted_key;
-}
-
-}  // namespace test
-}  // namespace reporting

chrome/browser/policy/messaging_layer/encryption/fake_encryption.h

-// 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.
-
-#ifndef CHROME_BROWSER_POLICY_MESSAGING_LAYER_ENCRYPTION_FAKE_ENCRYPTION_H_
-#define CHROME_BROWSER_POLICY_MESSAGING_LAYER_ENCRYPTION_FAKE_ENCRYPTION_H_
-
-#include <string>
-
-#include "base/callback.h"
-#include "base/hash/hash.h"
-#include "base/memory/ref_counted.h"
-#include "base/strings/string_piece.h"
-#include "chrome/browser/policy/messaging_layer/encryption/encryption.h"
-#include "chrome/browser/policy/messaging_layer/util/status.h"
-#include "chrome/browser/policy/messaging_layer/util/statusor.h"
-
-namespace reporting {
-namespace test {
-
-// Fake implementation of EncryptorBase, intended for use in tests of
-// reporting client.
-// Record encryption with symmetric key is done by per-byte XOR.
-// Key encryption with asymmetric public key is also done by per-byte XOR.
-class FakeEncryptor : public EncryptorBase {
- public:
-  // Factory method
-  static StatusOr<scoped_refptr<EncryptorBase>> Create();
-
-  void OpenRecord(base::OnceCallback<void(StatusOr<Handle*>)> cb) override;
-
- private:
-  FakeEncryptor();
-  ~FakeEncryptor() override;
-
-  std::string EncryptSymmetricKey(base::StringPiece symmetric_key,
-                                  base::StringPiece asymmetric_key) override;
-};
-
-}  // namespace test
-}  // namespace reporting
-
-#endif  // CHROME_BROWSER_POLICY_MESSAGING_LAYER_ENCRYPTION_FAKE_ENCRYPTION_H_

chrome/test/BUILD.gn

@@ -3451,11 +3451,9 @@ test("unit_tests") {
     "../browser/policy/file_selection_dialogs_policy_handler_unittest.cc",
     "../browser/policy/homepage_location_policy_handler_unittest.cc",
     "../browser/policy/javascript_policy_handler_unittest.cc",
-    "../browser/policy/messaging_layer/encryption/fake_decryption.cc",
-    "../browser/policy/messaging_layer/encryption/fake_decryption.h",
-    "../browser/policy/messaging_layer/encryption/fake_encryption.cc",
-    "../browser/policy/messaging_layer/encryption/fake_encryption.h",
-    "../browser/policy/messaging_layer/encryption/fake_encryption_unittest.cc",
+    "../browser/policy/messaging_layer/encryption/decryption.cc",
+    "../browser/policy/messaging_layer/encryption/decryption.h",
+    "../browser/policy/messaging_layer/encryption/encryption_unittest.cc",
     "../browser/policy/messaging_layer/encryption/test_encryption_module.cc",
     "../browser/policy/messaging_layer/encryption/test_encryption_module.h",
     "../browser/policy/messaging_layer/public/report_client_unittest.cc",