Merge GH #765 Rewrite CBC unpadding operations as const time

3 files changed, 74 insertions, 47 deletions

diff --git a/src/lib/modes/cbc/cbc.cpp b/src/lib/modes/cbc/cbc.cpp
index 592ff95e9..9fbb7023f 100644
--- a/src/lib/modes/cbc/cbc.cpp
+++ b/src/lib/modes/cbc/cbc.cpp
@@ -19,8 +19,8 @@ CBC_Mode::CBC_Mode(BlockCipher* cipher, BlockCipherModePaddingMethod* padding) :
    {
    if(m_padding && !m_padding->valid_blocksize(cipher->block_size()))
       throw Invalid_Argument("Padding " + m_padding->name() +
-                                  " cannot be used with " +
-                                  cipher->name() + "/CBC");
+                             " cannot be used with " +
+                             cipher->name() + "/CBC");
    }
 
 void CBC_Mode::clear()
@@ -243,6 +243,10 @@ void CBC_Decryption::finish(secure_vector<byte>& buffer, size_t offset)
 
    const size_t pad_bytes = BS - padding().unpad(&buffer[buffer.size()-BS], BS);
    buffer.resize(buffer.size() - pad_bytes); // remove padding
+   if(pad_bytes == 0 && padding().name() != "NoPadding")
+      {
+      throw Decoding_Error(name());
+      }
    }
 
 void CBC_Decryption::reset()
diff --git a/src/lib/modes/mode_pad/mode_pad.cpp b/src/lib/modes/mode_pad/mode_pad.cpp
index eb4ae42be..c84c2030e 100644
--- a/src/lib/modes/mode_pad/mode_pad.cpp
+++ b/src/lib/modes/mode_pad/mode_pad.cpp
@@ -8,6 +8,7 @@
 
 #include <botan/mode_pad.h>
 #include <botan/exceptn.h>
+#include <botan/internal/ct_utils.h>
 
 namespace Botan {
 
@@ -52,16 +53,24 @@ void PKCS7_Padding::add_padding(secure_vector<byte>& buffer,
 */
 size_t PKCS7_Padding::unpad(const byte block[], size_t size) const
    {
-   size_t position = block[size-1];
+   CT::poison(block,size);
+   size_t bad_input = 0;
+   const byte last_byte = block[size-1];
 
-   if(position > size)
-      throw Decoding_Error("Bad padding in " + name());
+   bad_input |= CT::expand_mask(last_byte > size);
 
-   for(size_t j = size-position; j != size-1; ++j)
-      if(block[j] != position)
-         throw Decoding_Error("Bad padding in " + name());
+   size_t pad_pos = size - last_byte;
+   size_t i = size - 2;
+   while(i)
+      {
+      bad_input |= ~CT::is_equal(block[i],last_byte) & CT::expand_mask(i >= pad_pos);
+      --i;
+      }
 
-   return (size-position);
+   CT::conditional_copy_mem(bad_input,&pad_pos,&size,&pad_pos,1);
+   CT::unpoison(block,size);
+   CT::unpoison(pad_pos);
+   return pad_pos;
    }
 
 /*
@@ -85,13 +94,23 @@ void ANSI_X923_Padding::add_padding(secure_vector<byte>& buffer,
 */
 size_t ANSI_X923_Padding::unpad(const byte block[], size_t size) const
    {
-   size_t position = block[size-1];
-   if(position > size)
-      throw Decoding_Error(name());
-   for(size_t j = size-position; j != size-1; ++j)
-      if(block[j] != 0)
-         throw Decoding_Error(name());
-   return (size-position);
+   CT::poison(block,size);
+   size_t bad_input = 0;
+   const size_t last_byte = block[size-1];
+
+   bad_input |= CT::expand_mask(last_byte > size);
+
+   size_t pad_pos = size - last_byte;
+   size_t i = size - 2;
+   while(i)
+      {
+      bad_input |= ~CT::is_zero(block[i]) & CT::expand_mask(i >= pad_pos);
+      --i;
+      }
+   CT::conditional_copy_mem(bad_input,&pad_pos,&size,&pad_pos,1);
+   CT::unpoison(block,size);
+   CT::unpoison(pad_pos);
+   return pad_pos;
    }
 
 /*
@@ -112,25 +131,34 @@ void OneAndZeros_Padding::add_padding(secure_vector<byte>& buffer,
 */
 size_t OneAndZeros_Padding::unpad(const byte block[], size_t size) const
    {
-   while(size)
+   CT::poison(block, size);
+   byte bad_input = 0;
+   byte seen_one = 0;
+   size_t pad_pos = size - 1;
+   size_t i = size;
+
+   while(i)
       {
-      if(block[size-1] == 0x80)
-         break;
-      if(block[size-1] != 0x00)
-         throw Decoding_Error(name());
-      size--;
+      seen_one |= CT::is_equal<byte>(block[i-1],0x80);
+      pad_pos -= CT::select<byte>(~seen_one, 1, 0);
+      bad_input |= ~CT::is_zero<byte>(block[i-1]) & ~seen_one;
+      i--;
       }
-   if(!size)
-      throw Decoding_Error(name());
-   return (size-1);
+   bad_input |= ~seen_one;
+
+   CT::conditional_copy_mem(size_t(bad_input),&pad_pos,&size,&pad_pos,1);
+   CT::unpoison(block, size);
+   CT::unpoison(pad_pos);
+
+   return pad_pos;
    }
 
 /*
 * Pad with ESP Padding Method
 */
 void ESP_Padding::add_padding(secure_vector<byte>& buffer,
-                                      size_t last_byte_pos,
-                                      size_t block_size) const
+                              size_t last_byte_pos,
+                              size_t block_size) const
    {
    byte pad_value = 0x01;
 
@@ -145,26 +173,22 @@ void ESP_Padding::add_padding(secure_vector<byte>& buffer,
 */
 size_t ESP_Padding::unpad(const byte block[], size_t size) const
    {
-   const byte last_byte = block[size-1];
-   if(last_byte > size)
-      {
-      throw Decoding_Error(name());
-      }
+   CT::poison(block,size);
+
+   const size_t last_byte = block[size-1];
+   size_t bad_input = 0;
+   bad_input |= CT::expand_mask(last_byte > size);
 
-   // try to do this in const time by looping over the entire block
-   const size_t pad_pos = size - last_byte;
+   size_t pad_pos = size - last_byte;
    size_t i = size - 1;
    while(i)
       {
-      if(block[i-1] != block[i]-1)
-         {
-         if(i > pad_pos)
-            {
-            throw Decoding_Error(name());
-            }
-         }
+      bad_input |= ~CT::is_equal<size_t>(size_t(block[i-1]),size_t(block[i])-1) & CT::expand_mask(i > pad_pos);
       --i;
       }
+   CT::conditional_copy_mem(bad_input,&pad_pos,&size,&pad_pos,1);
+   CT::unpoison(block, size);
+   CT::unpoison(pad_pos);
    return pad_pos;
    }
 
diff --git a/src/tests/test_pad.cpp b/src/tests/test_pad.cpp
index bf9e64c0d..c27e2607f 100644
--- a/src/tests/test_pad.cpp
+++ b/src/tests/test_pad.cpp
@@ -7,7 +7,7 @@
 #include "tests.h"
 
 #if defined(BOTAN_HAS_CIPHER_MODE_PADDING)
-  #include <botan/mode_pad.h>
+   #include <botan/mode_pad.h>
 #endif
 
 namespace Botan_Tests {
@@ -18,7 +18,7 @@ class Cipher_Mode_Padding_Tests : public Text_Based_Test
    {
    public:
       Cipher_Mode_Padding_Tests() :
-         Text_Based_Test("", "pad.vec", {"In", "Blocksize"},{"Out"})
+         Text_Based_Test("", "pad.vec", {"In", "Blocksize"}, {"Out"})
          {}
 
       Test::Result run_one_test(const std::string& header, const VarMap& vars) override
@@ -50,12 +50,11 @@ class Cipher_Mode_Padding_Tests : public Text_Based_Test
          if(expected.empty())
             {
             // This is an unpad an invalid input and ensure we reject
-            try
+            if(pad->unpad(input.data(), block_size) != block_size)
                {
-               pad->unpad(input.data(), block_size);
                result.test_failure("Did not reject invalid padding", Botan::hex_encode(input));
                }
-            catch(Botan::Decoding_Error&)
+            else
                {
                result.test_success("Rejected invalid padding");
                }
@@ -69,7 +68,7 @@ class Cipher_Mode_Padding_Tests : public Text_Based_Test
 
             buf.assign(expected.begin(), expected.end());
 
-            const size_t last_block = ( buf.size() < block_size ) ? 0 : buf.size() - block_size;
+            const size_t last_block = (buf.size() < block_size) ? 0 : buf.size() - block_size;
             const size_t pad_bytes = block_size - pad->unpad(&buf[last_block], block_size);
             buf.resize(buf.size() - pad_bytes); // remove padding
             result.test_eq("unpad", buf, input);