IV re-use with snapshots (#1536)

src/crypto/symmetric_key.h

@@ -28,21 +28,35 @@ namespace crypto
     GcmHeader(const std::vector<uint8_t>& data)
     {
       if (data.size() != RAW_DATA_SIZE)
+      {
         throw std::logic_error("Incompatible IV size");
+      }
 
       memcpy(tag, data.data(), sizeof(tag));
       memcpy(iv, data.data() + sizeof(tag), sizeof(iv));
     }
 
+    void set_iv_seq(uint64_t seq)
+    {
+      *reinterpret_cast<uint64_t*>(iv) = seq;
+    }
+
     void set_iv_id(uint64_t id)
     {
+      if (id > 0x7FFFFFFF)
+      {
+        throw std::logic_error(
+          fmt::format("id should fit in 31 bits of IV. Value is: 0x{0:x}", id));
+      }
+
       *reinterpret_cast<uint32_t*>(iv + IV_DELIMITER) =
         static_cast<uint32_t>(id);
     }
 
-    void set_iv_seq(uint64_t seq)
+    void set_iv_snapshot(bool is_snapshot)
     {
-      *reinterpret_cast<uint64_t*>(iv) = seq;
+      // Set very last bit in IV
+      iv[SIZE_IV - 1] |= (is_snapshot << ((sizeof(uint8_t) * 8) - 1));
     }
 
     void set_iv(uint8_t* iv_, size_t size)

src/kv/encryptor.h

@@ -24,10 +24,14 @@ namespace kv
     SpinLock lock;
 
     virtual void set_iv(
-      crypto::GcmHeader<crypto::GCM_SIZE_IV>& gcm_hdr, kv::Version version)
+      crypto::GcmHeader<crypto::GCM_SIZE_IV>& gcm_hdr,
+      kv::Version version,
+      bool is_snapshot = false)
     {
-      gcm_hdr.set_iv_id(iv_id);
+      // Warning: The same IV will get re-used on rollback!
       gcm_hdr.set_iv_seq(version);
+      gcm_hdr.set_iv_id(iv_id);
+      gcm_hdr.set_iv_snapshot(is_snapshot);
     }
 
     const crypto::KeyAesGcm& get_encryption_key(kv::Version version)
@@ -88,19 +92,21 @@ namespace kv
      * @param[out]  cipher            Encrypted ciphertext
      * @param[in]   version           Version used to retrieve the corresponding
      * encryption key
+     * @param[in]   is_snapshot       Indicates that the entry is a snapshot (to
+     * avoid IV re-use)
      */
     void encrypt(
       const std::vector<uint8_t>& plain,
       const std::vector<uint8_t>& additional_data,
       std::vector<uint8_t>& serialised_header,
       std::vector<uint8_t>& cipher,
-      kv::Version version) override
+      kv::Version version,
+      bool is_snapshot = false) override
     {
       crypto::GcmHeader<crypto::GCM_SIZE_IV> gcm_hdr;
       cipher.resize(plain.size());
 
-      // Set IV
-      set_iv(gcm_hdr, version);
+      set_iv(gcm_hdr, version, is_snapshot);
 
       get_encryption_key(version).encrypt(
         gcm_hdr.get_iv(), plain, additional_data, cipher.data(), gcm_hdr.tag);

src/kv/generic_serialise_wrapper.h

@@ -50,6 +50,7 @@ namespace kv
     W private_writer;
     W* current_writer;
     Version version;
+    bool is_snapshot;
 
     std::shared_ptr<AbstractTxEncryptor> crypto_util;
 
@@ -90,12 +91,13 @@ namespace kv
       std::shared_ptr<AbstractTxEncryptor> e,
       const Version& version_,
       bool is_snapshot_ = false) :
+      version(version_),
+      is_snapshot(is_snapshot_),
       crypto_util(e)
     {
       set_current_domain(SecurityDomain::PUBLIC);
-      serialise_internal(is_snapshot_);
-      serialise_internal(version_);
-      version = version_;
+      serialise_internal(is_snapshot);
+      serialise_internal(version);
     }
 
     void start_map(const std::string& name, SecurityDomain domain)
@@ -198,7 +200,8 @@ namespace kv
         serialised_public_domain,
         serialised_hdr,
         encrypted_private_domain,
-        version);
+        version,
+        is_snapshot);
 
       // Serialise entire tx
       // Format: gcm hdr (iv + tag) + len of public domain + public domain +

src/kv/kv_types.h

@@ -356,7 +356,8 @@ namespace kv
       const std::vector<uint8_t>& additional_data,
       std::vector<uint8_t>& serialised_header,
       std::vector<uint8_t>& cipher,
-      kv::Version version) = 0;
+      kv::Version version,
+      bool is_snapshot = false) = 0;
     virtual bool decrypt(
       const std::vector<uint8_t>& cipher,
       const std::vector<uint8_t>& additional_data,

src/kv/test/null_encryptor.h

@@ -15,7 +15,8 @@ namespace kv
       const std::vector<uint8_t>& additional_data,
       std::vector<uint8_t>& serialised_header,
       std::vector<uint8_t>& cipher,
-      kv::Version version) override
+      kv::Version version,
+      bool is_snapshot = false) override
     {
       cipher = plain;
     }

src/node/encryptor.h

@@ -19,10 +19,12 @@ namespace ccf
     std::atomic<size_t> seq_no{0};
 
     void set_iv(
-      crypto::GcmHeader<crypto::GCM_SIZE_IV>& gcm_hdr, kv::Version) override
+      crypto::GcmHeader<crypto::GCM_SIZE_IV>& gcm_hdr,
+      kv::Version,
+      bool) override
     {
-      gcm_hdr.set_iv_id(BaseEncryptor::iv_id);
       gcm_hdr.set_iv_seq(seq_no.fetch_add(1));
+      gcm_hdr.set_iv_id(BaseEncryptor::iv_id);
     }
 
     using BaseEncryptor::BaseEncryptor;

src/node/node_state.h

@@ -617,7 +617,6 @@ namespace ccf
         last_recovered_commit_idx);
 
       network.ledger_secrets->init(last_recovered_commit_idx + 1);
-      setup_encryptor(network.consensus_type);
       // KV term must be set before the first Tx is committed
       LOG_INFO_FMT(
         "Setting term on public recovery KV to {}", term_history.size() + 2);
@@ -634,6 +633,8 @@ namespace ccf
                          node_encrypt_kp->public_key_pem().raw(),
                          NodeStatus::PENDING});
 
+      setup_encryptor(network.consensus_type);
+
       LOG_INFO_FMT("Deleted previous nodes and added self as {}", self);
 
       kv::Version index = 0;
@@ -833,7 +834,7 @@ namespace ccf
       {
         recovery_encryptor =
           std::make_shared<RaftTxEncryptor>(network.ledger_secrets, true);
-        recovery_encryptor->set_iv_id(self); // RaftEncryptor uses node ID as iv
+        recovery_encryptor->set_iv_id(self); // RaftEncryptor uses node ID in iv
       }
       else
       {
@@ -1633,7 +1634,7 @@ namespace ccf
       else if (network.consensus_type == ConsensusType::RAFT)
       {
         encryptor = std::make_shared<RaftTxEncryptor>(network.ledger_secrets);
-        encryptor->set_iv_id(self); // RaftEncryptor uses node ID as iv
+        encryptor->set_iv_id(self); // RaftEncryptor uses node ID in iv
       }
       else
       {

src/node/test/encryptor.cpp

@@ -62,7 +62,7 @@ TEST_CASE(
   encryptor->encrypt(
     plain, additional_data, serialised_header2, cipher2, version);
 
-  // Cipher are different because IV is different
+  // Ciphers are different because IV is different
   REQUIRE(cipher != cipher2);
   REQUIRE(serialised_header != serialised_header2);
 }
@@ -90,7 +90,7 @@ TEST_CASE(
   encryptor_1->encrypt(
     plain, additional_data, serialised_header2, cipher2, version);
 
-  // Cipher are different because IV is different
+  // Ciphers are different because IV is different
   REQUIRE(cipher != cipher2);
   REQUIRE(serialised_header != serialised_header2);
 }
@@ -115,7 +115,37 @@ TEST_CASE("Two ciphers from same plaintext are different - PbftTxEncryptor")
   encryptor->encrypt(
     plain, additional_data, serialised_header2, cipher2, version);
 
-  // Cipher are different because IV is different
+  // Ciphers are different because IV is different
+  REQUIRE(cipher != cipher2);
+  REQUIRE(serialised_header != serialised_header2);
+}
+
+TEST_CASE(
+  "Different node ciphers from same plaintext with and without snapshots - "
+  "PbftTxEncryptor")
+{
+  auto secrets = std::make_shared<ccf::LedgerSecrets>();
+  secrets->init();
+  auto encryptor = std::make_shared<ccf::PbftTxEncryptor>(secrets);
+  encryptor->set_iv_id(0x7FFFFFFF);
+
+  std::vector<uint8_t> plain(128, 0x42);
+  std::vector<uint8_t> cipher;
+  std::vector<uint8_t> cipher2;
+  std::vector<uint8_t> serialised_header;
+  std::vector<uint8_t> serialised_header2;
+  std::vector<uint8_t> additional_data; // No additional data
+  kv::Version version = 10;
+
+  bool is_snapshot = false;
+  encryptor->encrypt(
+    plain, additional_data, serialised_header, cipher, version, is_snapshot);
+
+  is_snapshot = true;
+  encryptor->encrypt(
+    plain, additional_data, serialised_header2, cipher2, version, is_snapshot);
+
+  // Ciphers are different because IV is different
   REQUIRE(cipher != cipher2);
   REQUIRE(serialised_header != serialised_header2);
 }