Merge GH #1690 Fix bugs in CBC unpadding

5 files changed, 312 insertions, 59 deletions

diff --git a/src/fuzzer/mode_padding.cpp b/src/fuzzer/mode_padding.cpp
new file mode 100644
index 000000000..05cb1a8f6
--- /dev/null
+++ b/src/fuzzer/mode_padding.cpp
@@ -0,0 +1,150 @@
+/*
+* (C) 2018 Jack Lloyd
+*
+* Botan is released under the Simplified BSD License (see license.txt)
+*/
+
+#include "fuzzers.h"
+#include <botan/mode_pad.h>
+#include <botan/internal/tls_cbc.h>
+
+size_t ref_pkcs7_unpad(const uint8_t in[], size_t len)
+   {
+   if(len <= 2)
+      return len;
+
+   const size_t padding_length = in[len-1];
+
+   if(padding_length == 0 || padding_length > len)
+      return len;
+
+   const size_t padding_start = len - padding_length;
+
+   for(size_t i = padding_start; i != len; ++i)
+      {
+      if(in[i] != padding_length)
+         return len;
+      }
+
+   return len - padding_length;
+   }
+
+size_t ref_x923_unpad(const uint8_t in[], size_t len)
+   {
+   if(len <= 2)
+      return len;
+
+   const size_t padding_length = in[len-1];
+
+   if(padding_length == 0 || padding_length > len)
+      return len;
+   const size_t padding_start = len - padding_length;
+
+   for(size_t i = padding_start; i != len - 1; ++i)
+      {
+      if(in[i] != 0)
+         {
+         return len;
+         }
+      }
+
+   return len - padding_length;
+   }
+
+size_t ref_oneandzero_unpad(const uint8_t in[], size_t len)
+   {
+   if(len <= 2)
+      return len;
+
+   size_t idx = len - 1;
+
+   while(idx >= 0)
+      {
+      if(in[idx] == 0)
+         {
+         idx -= 1;
+         continue;
+         }
+      else if(in[idx] == 0x80)
+         {
+         return idx;
+         }
+      else
+         return len;
+      }
+
+   return len;
+   }
+
+size_t ref_esp_unpad(const uint8_t in[], size_t len)
+   {
+   if(len <= 2)
+      return len;
+
+   const size_t padding_bytes = in[len - 1];
+
+   if(padding_bytes == 0 || padding_bytes > len)
+      {
+      return len;
+      }
+
+   const size_t padding_start = len - padding_bytes;
+   for(size_t i = padding_start; i != len; ++i)
+      {
+      if(in[i] != (i - padding_start + 1))
+         {
+         return len;
+         }
+      }
+
+   return len - padding_bytes;
+   }
+
+uint16_t ref_tls_cbc_unpad(const uint8_t in[], size_t len)
+   {
+   if(len == 0)
+      return 0;
+
+   const size_t padding_length = in[(len-1)];
+
+   if(padding_length >= len)
+      return 0;
+
+   /*
+   * TLS v1.0 and up require all the padding bytes be the same value
+   * and allows up to 255 bytes.
+   */
+   for(size_t i = 0; i != 1 + padding_length; ++i)
+      {
+      if(in[(len-i-1)] != padding_length)
+         return 0;
+      }
+   return padding_length + 1;
+   }
+
+void fuzz(const uint8_t in[], size_t len)
+   {
+   Botan::PKCS7_Padding pkcs7;
+   const size_t ct_pkcs7 = pkcs7.unpad(in, len);
+   const size_t ref_pkcs7 = ref_pkcs7_unpad(in, len);
+   FUZZER_ASSERT_EQUAL(ct_pkcs7, ref_pkcs7);
+
+   Botan::ANSI_X923_Padding x923;
+   const size_t ct_x923 = x923.unpad(in, len);
+   const size_t ref_x923 = ref_x923_unpad(in, len);
+   FUZZER_ASSERT_EQUAL(ct_x923, ref_x923);
+
+   Botan::OneAndZeros_Padding oneandzero;
+   const size_t ct_oneandzero = oneandzero.unpad(in, len);
+   const size_t ref_oneandzero = ref_oneandzero_unpad(in, len);
+   FUZZER_ASSERT_EQUAL(ct_oneandzero, ref_oneandzero);
+
+   Botan::ESP_Padding esp;
+   const size_t ct_esp = esp.unpad(in, len);
+   const size_t ref_esp = ref_esp_unpad(in, len);
+   FUZZER_ASSERT_EQUAL(ct_esp, ref_esp);
+
+   const uint16_t ct_cbc = Botan::TLS::check_tls_cbc_padding(in, len);
+   const uint16_t ref_cbc = ref_tls_cbc_unpad(in, len);
+   FUZZER_ASSERT_EQUAL(ct_cbc, ref_cbc);
+   }
diff --git a/src/lib/modes/mode_pad/mode_pad.cpp b/src/lib/modes/mode_pad/mode_pad.cpp
index f93b2dccc..e65114c88 100644
--- a/src/lib/modes/mode_pad/mode_pad.cpp
+++ b/src/lib/modes/mode_pad/mode_pad.cpp
@@ -1,6 +1,6 @@
 /*
 * CBC Padding Methods
-* (C) 1999-2007,2013 Jack Lloyd
+* (C) 1999-2007,2013,2018 Jack Lloyd
 * (C) 2016 René Korthaus, Rohde & Schwarz Cybersecurity
 *
 * Botan is released under the Simplified BSD License (see license.txt)
@@ -51,26 +51,27 @@ void PKCS7_Padding::add_padding(secure_vector<uint8_t>& buffer,
 /*
 * Unpad with PKCS #7 Method
 */
-size_t PKCS7_Padding::unpad(const uint8_t block[], size_t size) const
+size_t PKCS7_Padding::unpad(const uint8_t input[], size_t input_length) const
    {
-   CT::poison(block,size);
+   if(input_length <= 2)
+      return input_length;
+
+   CT::poison(input, input_length);
    size_t bad_input = 0;
-   const uint8_t last_byte = block[size-1];
+   const uint8_t last_byte = input[input_length-1];
 
-   bad_input |= CT::expand_mask<size_t>(last_byte > size);
+   bad_input |= CT::expand_mask<size_t>(last_byte > input_length);
 
-   size_t pad_pos = size - last_byte;
-   size_t i = size - 2;
-   while(i)
+   const size_t pad_pos = input_length - last_byte;
+
+   for(size_t i = 0; i != input_length - 1; ++i)
       {
-      bad_input |= (~CT::is_equal(block[i],last_byte)) & CT::expand_mask<uint8_t>(i >= pad_pos);
-      --i;
+      const uint8_t in_range = CT::expand_mask<uint8_t>(i >= pad_pos);
+      bad_input |= in_range & (~CT::is_equal(input[i], last_byte));
       }
 
-   CT::conditional_copy_mem(bad_input,&pad_pos,&size,&pad_pos,1);
-   CT::unpoison(block,size);
-   CT::unpoison(pad_pos);
-   return pad_pos;
+   CT::unpoison(input, input_length);
+   return CT::conditional_return(bad_input, input_length, pad_pos);
    }
 
 /*
@@ -92,25 +93,27 @@ void ANSI_X923_Padding::add_padding(secure_vector<uint8_t>& buffer,
 /*
 * Unpad with ANSI X9.23 Method
 */
-size_t ANSI_X923_Padding::unpad(const uint8_t block[], size_t size) const
+size_t ANSI_X923_Padding::unpad(const uint8_t input[], size_t input_length) const
    {
-   CT::poison(block,size);
-   size_t bad_input = 0;
-   const size_t last_byte = block[size-1];
+   if(input_length <= 2)
+      return input_length;
 
-   bad_input |= CT::expand_mask<size_t>(last_byte > size);
+   CT::poison(input, input_length);
+   const size_t last_byte = input[input_length-1];
 
-   size_t pad_pos = size - last_byte;
-   size_t i = size - 2;
-   while(i)
+   uint8_t bad_input = 0;
+   bad_input |= CT::expand_mask<uint8_t>(last_byte > input_length);
+
+   const size_t pad_pos = input_length - last_byte;
+
+   for(size_t i = 0; i != input_length - 1; ++i)
       {
-      bad_input |= (~CT::is_zero(block[i])) & CT::expand_mask<uint8_t>(i >= pad_pos);
-      --i;
+      const uint8_t in_range = CT::expand_mask<uint8_t>(i >= pad_pos);
+      bad_input |= CT::expand_mask(input[i]) & in_range;
       }
-   CT::conditional_copy_mem(bad_input,&pad_pos,&size,&pad_pos,1);
-   CT::unpoison(block,size);
-   CT::unpoison(pad_pos);
-   return pad_pos;
+
+   CT::unpoison(input, input_length);
+   return CT::conditional_return(bad_input, input_length, pad_pos);
    }
 
 /*
@@ -129,28 +132,29 @@ void OneAndZeros_Padding::add_padding(secure_vector<uint8_t>& buffer,
 /*
 * Unpad with One and Zeros Method
 */
-size_t OneAndZeros_Padding::unpad(const uint8_t block[], size_t size) const
+size_t OneAndZeros_Padding::unpad(const uint8_t input[], size_t input_length) const
    {
-   CT::poison(block, size);
+   if(input_length <= 2)
+      return input_length;
+
+   CT::poison(input, input_length);
+
    uint8_t bad_input = 0;
    uint8_t seen_one = 0;
-   size_t pad_pos = size - 1;
-   size_t i = size;
+   size_t pad_pos = input_length - 1;
+   size_t i = input_length;
 
    while(i)
       {
-      seen_one |= CT::is_equal<uint8_t>(block[i-1],0x80);
+      seen_one |= CT::is_equal<uint8_t>(input[i-1], 0x80);
       pad_pos -= CT::select<uint8_t>(~seen_one, 1, 0);
-      bad_input |= ~CT::is_zero<uint8_t>(block[i-1]) & ~seen_one;
+      bad_input |= ~CT::is_zero<uint8_t>(input[i-1]) & ~seen_one;
       i--;
       }
    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;
+   CT::unpoison(input, input_length);
+   return CT::conditional_return(bad_input, input_length, pad_pos);
    }
 
 /*
@@ -171,25 +175,28 @@ void ESP_Padding::add_padding(secure_vector<uint8_t>& buffer,
 /*
 * Unpad with ESP Padding Method
 */
-size_t ESP_Padding::unpad(const uint8_t block[], size_t size) const
+size_t ESP_Padding::unpad(const uint8_t input[], size_t input_length) const
    {
-   CT::poison(block,size);
+   if(input_length <= 2)
+      return input_length;
 
-   const size_t last_byte = block[size-1];
-   size_t bad_input = 0;
-   bad_input |= CT::expand_mask<size_t>(last_byte > size);
+   CT::poison(input, input_length);
+
+   const size_t last_byte = input[input_length-1];
+   uint8_t bad_input = 0;
+   bad_input |= CT::is_zero(last_byte) | CT::expand_mask<uint8_t>(last_byte > input_length);
 
-   size_t pad_pos = size - last_byte;
-   size_t i = size - 1;
+   const size_t pad_pos = input_length - last_byte;
+   size_t i = input_length - 1;
    while(i)
       {
-      bad_input |= ~CT::is_equal<uint8_t>(size_t(block[i-1]),size_t(block[i])-1) & CT::expand_mask<uint8_t>(i > pad_pos);
+      const uint8_t in_range = CT::expand_mask<uint8_t>(i > pad_pos);
+      bad_input |= (~CT::is_equal<uint8_t>(input[i-1], input[i]-1)) & in_range;
       --i;
       }
-   CT::conditional_copy_mem(bad_input,&pad_pos,&size,&pad_pos,1);
-   CT::unpoison(block, size);
-   CT::unpoison(pad_pos);
-   return pad_pos;
+
+   CT::unpoison(input, input_length);
+   return CT::conditional_return(bad_input, input_length, pad_pos);
    }
 
 
diff --git a/src/lib/modes/mode_pad/mode_pad.h b/src/lib/modes/mode_pad/mode_pad.h
index cc196d251..25e4221af 100644
--- a/src/lib/modes/mode_pad/mode_pad.h
+++ b/src/lib/modes/mode_pad/mode_pad.h
@@ -39,11 +39,10 @@ class BOTAN_PUBLIC_API(2,0) BlockCipherModePaddingMethod
       /**
       * Remove padding bytes from block
       * @param block the last block
-      * @param size the size of the block in bytes
-      * @return number of padding bytes
+      * @param len the size of the block in bytes
+      * @return number of data bytes, or if the padding is invalid returns len
       */
-      virtual size_t unpad(const uint8_t block[],
-                           size_t size) const = 0;
+      virtual size_t unpad(const uint8_t block[], size_t len) const = 0;
 
       /**
       * @param block_size of the cipher
@@ -74,7 +73,7 @@ class BOTAN_PUBLIC_API(2,0) PKCS7_Padding final : public BlockCipherModePaddingM
 
       size_t unpad(const uint8_t[], size_t) const override;
 
-      bool valid_blocksize(size_t bs) const override { return (bs > 0 && bs < 256); }
+      bool valid_blocksize(size_t bs) const override { return (bs > 2 && bs < 256); }
 
       std::string name() const override { return "PKCS7"; }
    };
@@ -91,7 +90,7 @@ class BOTAN_PUBLIC_API(2,0) ANSI_X923_Padding final : public BlockCipherModePadd
 
       size_t unpad(const uint8_t[], size_t) const override;
 
-      bool valid_blocksize(size_t bs) const override { return (bs > 0 && bs < 256); }
+      bool valid_blocksize(size_t bs) const override { return (bs > 2 && bs < 256); }
 
       std::string name() const override { return "X9.23"; }
    };
@@ -108,7 +107,7 @@ class BOTAN_PUBLIC_API(2,0) OneAndZeros_Padding final : public BlockCipherModePa
 
       size_t unpad(const uint8_t[], size_t) const override;
 
-      bool valid_blocksize(size_t bs) const override { return (bs > 0); }
+      bool valid_blocksize(size_t bs) const override { return (bs > 2); }
 
       std::string name() const override { return "OneAndZeros"; }
    };
@@ -125,7 +124,7 @@ class BOTAN_PUBLIC_API(2,0) ESP_Padding final : public BlockCipherModePaddingMet
 
       size_t unpad(const uint8_t[], size_t) const override;
 
-      bool valid_blocksize(size_t bs) const override { return (bs > 0); }
+      bool valid_blocksize(size_t bs) const override { return (bs > 2 && bs < 256); }
 
       std::string name() const override { return "ESP"; }
    };
diff --git a/src/lib/utils/ct_utils.h b/src/lib/utils/ct_utils.h
index 4fd06ec3d..f4f881871 100644
--- a/src/lib/utils/ct_utils.h
+++ b/src/lib/utils/ct_utils.h
@@ -149,6 +149,14 @@ inline T is_lte(T a, T b)
    return CT::is_less(a, b) | CT::is_equal(a, b);
    }
 
+template<typename C, typename T>
+inline T conditional_return(C condvar, T left, T right)
+   {
+   const T val = CT::select(CT::expand_mask<T>(condvar), left, right);
+   CT::unpoison(val);
+   return val;
+   }
+
 template<typename T>
 inline T conditional_copy_mem(T value,
                               T* to,
diff --git a/src/tests/data/pad.vec b/src/tests/data/pad.vec
index ee24d3497..54efa4f1d 100644
--- a/src/tests/data/pad.vec
+++ b/src/tests/data/pad.vec
@@ -40,6 +40,10 @@ In  = FFFFFFFFFFFFFFFFFF
 Out = FFFFFFFFFFFFFFFFFF07070707070707
 Blocksize = 8
 
+In = 82
+Out = 8207070707070707
+Blocksize = 8
+
 [PKCS7_Invalid]
 In = FFFFFFFFFFFFFFFFFF07070706070707
 Blocksize = 8
@@ -47,6 +51,18 @@ Blocksize = 8
 In = FFFFFFFFFFFFFFFFFFFF070707070707
 Blocksize = 8
 
+In = 82040404
+Blocksize = 4
+
+In = 82050505
+Blocksize = 4
+
+In = 820606060606
+Blocksize = 6
+
+In = 8208080808080808
+Blocksize = 8
+
 [OneAndZeros]
 In  = FFFFFF
 Out = FFFFFF80000000000000000000000000
@@ -68,6 +84,14 @@ In  = FFFFFFFFFFFFFFFFFF
 Out = FFFFFFFFFFFFFFFFFF80000000000000
 Blocksize = 8
 
+In = FF
+Out = FF800000
+Blocksize = 4
+
+In = 
+Out = 80000000
+Blocksize = 4
+
 [OneAndZeros_Invalid]
 In  = FF80000000000008
 Blocksize = 8
@@ -99,6 +123,18 @@ In  = FFFFFFFFFFFFFFFFFF
 Out = FFFFFFFFFFFFFFFFFF00000000000007
 Blocksize = 8
 
+In = 7e00
+Out = 7e000002
+Blocksize = 4
+
+In = 7e
+Out = 7e000003
+Blocksize = 4
+
+In = 7e
+Out = 7e00000000000007
+Blocksize = 8
+
 [X9.23_Invalid]
 In = FFFFFFFFFFFFFFFFFF000000FFFFF00007
 Blocksize = 8
@@ -109,6 +145,21 @@ Blocksize = 8
 In = FFFFFF8000000000000000000000000D
 Blocksize = 16
 
+In = FF000004
+Blocksize = 4
+
+In = FF000005
+Blocksize = 4
+
+In = FF000006
+Blocksize = 4
+
+In = FF00000000000008
+Blocksize = 8
+
+In = 8020000000000008
+Blocksize = 8
+
 [ESP]
 In  = FFFFFF
 Out = FFFFFF0102030405060708090A0B0C0D
@@ -130,6 +181,34 @@ In  = FFFFFFFFFFFFFFFFFF
 Out = FFFFFFFFFFFFFFFFFF01020304050607
 Blocksize = 8
 
+In  = FFFFFFFFFFFFFFFFFFFFFFFFFFFFFF
+Out = FFFFFFFFFFFFFFFFFFFFFFFFFFFFFF01
+Blocksize = 8
+
+In  = FFFFFFFFFFFFFFFFFFFFFFFFFFFF
+Out = FFFFFFFFFFFFFFFFFFFFFFFFFFFF0102
+Blocksize = 8
+
+In  = FFFFFFFFFFFFFFFFFFFFFFFFFF
+Out = FFFFFFFFFFFFFFFFFFFFFFFFFF010203
+Blocksize = 8
+
+In  = FFFFFFFFFFFFFFFFFFFFFFFF
+Out = FFFFFFFFFFFFFFFFFFFFFFFF01020304
+Blocksize = 8
+
+In  = FFFFFFFFFFFFFFFFFFFFFF
+Out = FFFFFFFFFFFFFFFFFFFFFF0102030405
+Blocksize = 8
+
+In  = FFFFFFFFFFFFFFFFFFFF
+Out = FFFFFFFFFFFFFFFFFFFF010203040506
+Blocksize = 8
+
+In  = FFFFFFFFFFFFFFFFFF
+Out = FFFFFFFFFFFFFFFFFF01020304050607
+Blocksize = 8
+
 [ESP_Invalid]
 In = FF010202
 Blocksize = 4
@@ -137,6 +216,15 @@ Blocksize = 4
 In = FF010204
 Blocksize = 4
 
+In = FF000000
+Blocksize = 4
+
+In = 00000000
+Blocksize = 4
+
+In = 500ac5c5c5c5c5c5
+Blocksize = 8
+
 In = FFFFFF0102030405060708090A0B0C0F
 Blocksize = 16
 
@@ -145,3 +233,4 @@ Blocksize = 16
 
 In = FFFFFFFF0002030405060708090A0B0C
 Blocksize = 16
+