Make CBC padding constant time

Maximilian Blochberger points out that while unpadding was constant
time, padding operation leaked the length of the plaintext. This is
probably not too serious in most circumstances but is not desirable
behavior.

2 files changed, 49 insertions, 30 deletions

diff --git a/src/lib/tls/tls_cbc/tls_cbc.cpp b/src/lib/tls/tls_cbc/tls_cbc.cpp
index a23fa8595..426336ca1 100644
--- a/src/lib/tls/tls_cbc/tls_cbc.cpp
+++ b/src/lib/tls/tls_cbc/tls_cbc.cpp
@@ -1,6 +1,6 @@
 /*
 * TLS CBC Record Handling
-* (C) 2012,2013,2014,2015,2016 Jack Lloyd
+* (C) 2012,2013,2014,2015,2016,2020 Jack Lloyd
 * (C) 2016 Juraj Somorovsky
 * (C) 2016 Matthias Gierlings
 * (C) 2016 Daniel Neus, Rohde & Schwarz Cybersecurity
@@ -143,6 +143,7 @@ void TLS_CBC_HMAC_AEAD_Encryption::set_associated_data(const uint8_t ad[], size_
    if(use_encrypt_then_mac())
       {
       // AAD hack for EtM
+      // EtM uses ciphertext size instead of plaintext size for AEAD input
       const uint16_t pt_size = make_uint16(assoc_data()[11], assoc_data()[12]);
       const uint16_t enc_size = static_cast<uint16_t>(round_up(iv_size() + pt_size + 1, block_size()));
       assoc_data()[11] = get_byte<uint16_t>(0, enc_size);
@@ -150,12 +151,36 @@ void TLS_CBC_HMAC_AEAD_Encryption::set_associated_data(const uint8_t ad[], size_
       }
    }
 
-void TLS_CBC_HMAC_AEAD_Encryption::cbc_encrypt_record(uint8_t buf[], size_t buf_size)
+void TLS_CBC_HMAC_AEAD_Encryption::cbc_encrypt_record(
+   secure_vector<uint8_t>& buffer, size_t offset, size_t padding_length)
    {
+   // We always do short padding:
+   BOTAN_ASSERT_NOMSG(padding_length <= 16);
+
+   buffer.resize(buffer.size() + padding_length);
+
+   const uint8_t padding_val = static_cast<uint8_t>(padding_length - 1);
+
+   CT::poison(&padding_val, 1);
+   CT::poison(&padding_length, 1);
+   CT::poison(buffer.data(), buffer.size());
+
+   const size_t last_block_starts = buffer.size() - block_size();
+   const size_t padding_starts = buffer.size() - padding_length;
+   for(size_t i = last_block_starts; i != buffer.size(); ++i)
+      {
+      auto add_padding = CT::Mask<uint8_t>(CT::Mask<size_t>::is_gte(i, padding_starts));
+      buffer[i] = add_padding.select(padding_val, buffer[i]);
+      }
+
+   CT::unpoison(padding_val);
+   CT::unpoison(padding_length);
+   CT::unpoison(buffer.data(), buffer.size());
+
    cbc().start(cbc_state());
-   cbc().process(buf, buf_size);
+   cbc().process(&buffer[offset], buffer.size() - offset);
 
-   cbc_state().assign(buf + buf_size - block_size(), buf + buf_size);
+   cbc_state().assign(&buffer[buffer.size() - block_size()], &buffer[buffer.size()]);
    }
 
 size_t TLS_CBC_HMAC_AEAD_Encryption::output_length(size_t input_length) const
@@ -167,18 +192,19 @@ size_t TLS_CBC_HMAC_AEAD_Encryption::output_length(size_t input_length) const
 void TLS_CBC_HMAC_AEAD_Encryption::finish(secure_vector<uint8_t>& buffer, size_t offset)
    {
    update(buffer, offset);
-   buffer.resize(offset); // truncate, leaving just header
-   const size_t header_size = offset;
 
-   buffer.insert(buffer.end(), msg().begin(), msg().end());
+   const size_t msg_size = msg().size();
 
-   const size_t input_size = msg().size() + 1 + (use_encrypt_then_mac() ? 0 : tag_size());
+   const size_t input_size = msg_size + 1 + (use_encrypt_then_mac() ? 0 : tag_size());
    const size_t enc_size = round_up(input_size, block_size());
-   const size_t pad_val = enc_size - input_size;
-   const size_t buf_size = enc_size + (use_encrypt_then_mac() ? tag_size() : 0);
+   BOTAN_DEBUG_ASSERT(enc_size % block_size() == 0);
+
+   const uint8_t padding_val = static_cast<uint8_t>(enc_size - input_size);
+   const size_t padding_length = static_cast<size_t>(padding_val) + 1;
 
-   BOTAN_ASSERT(enc_size % block_size() == 0,
-                "Buffer is an even multiple of block size");
+   buffer.reserve(offset + msg_size + padding_length + tag_size());
+   buffer.resize(offset + msg_size);
+   copy_mem(&buffer[offset], msg().data(), msg_size);
 
    mac().update(assoc_data());
 
@@ -189,25 +215,17 @@ void TLS_CBC_HMAC_AEAD_Encryption::finish(secure_vector<uint8_t>& buffer, size_t
          mac().update(cbc_state());
          }
 
-      for(size_t i = 0; i != pad_val + 1; ++i)
-         buffer.push_back(static_cast<uint8_t>(pad_val));
-      cbc_encrypt_record(&buffer[header_size], enc_size);
+      cbc_encrypt_record(buffer, offset, padding_length);
+      mac().update(&buffer[offset], enc_size);
+      buffer.resize(buffer.size() + tag_size());
+      mac().final(&buffer[buffer.size() - tag_size()]);
       }
-
-   // EtM also uses ciphertext size instead of plaintext size for AEAD input
-   const uint8_t* mac_input = (use_encrypt_then_mac() ? &buffer[header_size] : msg().data());
-   const size_t mac_input_len = (use_encrypt_then_mac() ? enc_size : msg().size());
-
-   mac().update(mac_input, mac_input_len);
-
-   buffer.resize(buffer.size() + tag_size());
-   mac().final(&buffer[buffer.size() - tag_size()]);
-
-   if(use_encrypt_then_mac() == false)
+   else
       {
-      for(size_t i = 0; i != pad_val + 1; ++i)
-         buffer.push_back(static_cast<uint8_t>(pad_val));
-      cbc_encrypt_record(&buffer[header_size], buf_size);
+      mac().update(&buffer[offset], msg_size);
+      buffer.resize(buffer.size() + tag_size());
+      mac().final(&buffer[buffer.size() - tag_size()]);
+      cbc_encrypt_record(buffer, offset, padding_length);
       }
    }
 
diff --git a/src/lib/tls/tls_cbc/tls_cbc.h b/src/lib/tls/tls_cbc/tls_cbc.h
index 284a51391..938714218 100644
--- a/src/lib/tls/tls_cbc/tls_cbc.h
+++ b/src/lib/tls/tls_cbc/tls_cbc.h
@@ -132,7 +132,8 @@ class BOTAN_TEST_API TLS_CBC_HMAC_AEAD_Encryption final : public TLS_CBC_HMAC_AE
 
       void finish(secure_vector<uint8_t>& final_block, size_t offset = 0) override;
    private:
-      void cbc_encrypt_record(uint8_t record_contents[], size_t record_len);
+      void cbc_encrypt_record(secure_vector<uint8_t>& buffer, size_t offset,
+                              size_t padding_length);
    };
 
 /**