Repository: randombit/botan
Pull request #2733 (2021-04-28 22:12:01)

Change CT::copy_output semantics
Now if the flag is bad we always return an empty vector

Add explicit tests for CT::copy_output


Patch:
diff --git a/news.rst b/news.rst
index 8f41a5691..35643e98d 100644
--- a/news.rst
+++ b/news.rst
@@ -4,8 +4,8 @@ Release Notes
 Version 3.0.0, Not Yet Released
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-* Switch the build to C++17 mode; require at least GCC 9, Clang 7 or MSVC 2019.
-  (GH #2455)
+* Botan is now a C++17 codebase; compiler requirements have been
+  increased to GCC 9, Clang 7, or MSVC 2019. (GH #2455)
 
 * Support for TLS 1.0, TLS 1.1, and DTLS 1.0 have been removed (GH #2631)
 
@@ -151,7 +151,7 @@ Version 2.17.3, 2020-12-21
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 * CVE-2021-24115 Change base64, base58, base32, and hex encoding and
-  decoding opearations to run in constant time (GH #2549)
+  decoding operations to run in constant time (GH #2549)
 
 * Fix a build problem on PPC64 building with Clang (GH #2547)
 
@@ -173,7 +173,7 @@ Version 2.17.2, 2020-11-13
 
 * Resolve an issue in the modular square root algorithm where a loop
   to find a quadratic non-residue could, for a carefully chosen
-  composite modulus, not terminte in a timely manner. (GH #2482 #2476)
+  composite modulus, not terminate in a timely manner. (GH #2482 #2476)
 
 * Fix a regression in MinGW builds introduced in 2.17.1
 
diff --git a/src/lib/utils/ct_utils.cpp b/src/lib/utils/ct_utils.cpp
index cbcecfc28..ac2f90492 100644
--- a/src/lib/utils/ct_utils.cpp
+++ b/src/lib/utils/ct_utils.cpp
@@ -1,5 +1,5 @@
 /*
-* (C) 2018 Jack Lloyd
+* (C) 2018,2021 Jack Lloyd
 *
 * Botan is released under the Simplified BSD License (see license.txt)
 */
@@ -10,33 +10,56 @@ namespace Botan {
 
 namespace CT {
 
-secure_vector<uint8_t> copy_output(CT::Mask<uint8_t> bad_input,
+secure_vector<uint8_t> copy_output(CT::Mask<uint8_t> bad_input_u8,
                                    const uint8_t input[],
                                    size_t input_length,
                                    size_t offset)
    {
-   if(input_length == 0)
-      return secure_vector<uint8_t>();
-
    /*
-   * Ensure at runtime that offset <= input_length. This is an invalid input,
-   * but we can't throw without using the poisoned value. Instead, if it happens,
-   * set offset to be equal to the input length (so output_bytes becomes 0 and
-   * the returned vector is empty)
+   * We do not poison the input here because if we did we would have
+   * to unpoison it at exit. We assume instead that callers have
+   * already poisoned the input and will unpoison it at their own
+   * time.
    */
-   const auto valid_offset = CT::Mask<size_t>::is_lte(offset, input_length);
-   offset = valid_offset.select(offset, input_length);
-
-   const size_t output_bytes = input_length - offset;
+   CT::poison(&offset, sizeof(size_t));
 
    secure_vector<uint8_t> output(input_length);
 
+   auto bad_input = CT::Mask<size_t>::expand(bad_input_u8);
+
+   /*
+   * If the offset is greater than input_length then the arguments are
+   * invalid. Ideally we would through an exception but that leaks
+   * information about the offset. Instead treat it as if the input
+   * was invalid.
+   */
+   bad_input |= CT::Mask<size_t>::is_gt(offset, input_length);
+
+   /*
+   * If the input is invalid, then set offset == input_length as a result
+   * at the end we will set output_bytes == 0 causing the final result to
+   * be an empty vector.
+   */
+   offset = bad_input.select(input_length, offset);
+
    /*
    Move the desired output bytes to the front using a slow (O^n)
    but constant time loop that does not leak the value of the offset
    */
    for(size_t i = 0; i != input_length; ++i)
       {
+      /*
+      * If bad_input was set then we modified offset to equal the input_length.
+      * In that case, this_loop will be greater than input_length, and so is_eq
+      * mask will always be false. As a result none of the input values will be
+      * written to output.
+      *
+      * This is ignoring the possibility of integer overflow of offset + i. But
+      * for this to happen the input would have to consume nearly the entire
+      * address space, and we just allocated an output buffer of equal size.
+      */
+      const size_t this_loop = offset + i;
+
       /*
       start index from i rather than 0 since we know j must be >= i + offset
       to have any effect, and starting from i does not reveal information
@@ -44,12 +67,12 @@ secure_vector<uint8_t> copy_output(CT::Mask<uint8_t> bad_input,
       for(size_t j = i; j != input_length; ++j)
          {
          const uint8_t b = input[j];
-         const auto is_eq = CT::Mask<size_t>::is_equal(j, offset + i);
+         const auto is_eq = CT::Mask<size_t>::is_equal(j, this_loop);
          output[i] |= is_eq.if_set_return(b);
          }
       }
 
-   bad_input.if_set_zero_out(output.data(), output.size());
+   const size_t output_bytes = input_length - offset;
 
    CT::unpoison(output.data(), output.size());
    CT::unpoison(output_bytes);
diff --git a/src/lib/utils/ct_utils.h b/src/lib/utils/ct_utils.h
index a23312fdb..7a2ee2615 100644
--- a/src/lib/utils/ct_utils.h
+++ b/src/lib/utils/ct_utils.h
@@ -50,8 +50,7 @@ inline void poison(const T* p, size_t n)
 #if defined(BOTAN_HAS_VALGRIND)
    VALGRIND_MAKE_MEM_UNDEFINED(p, n * sizeof(T));
 #else
-   BOTAN_UNUSED(p);
-   BOTAN_UNUSED(n);
+   BOTAN_UNUSED(p, n);
 #endif
    }
 
@@ -61,8 +60,7 @@ inline void unpoison(const T* p, size_t n)
 #if defined(BOTAN_HAS_VALGRIND)
    VALGRIND_MAKE_MEM_DEFINED(p, n * sizeof(T));
 #else
-   BOTAN_UNUSED(p);
-   BOTAN_UNUSED(n);
+   BOTAN_UNUSED(p, n);
 #endif
    }
 
@@ -394,14 +392,22 @@ inline void conditional_swap_ptr(bool cnd, T& x, T& y)
    }
 
 /**
-* If bad_mask is unset, return in[delim_idx:input_length] copied to
-* new buffer. If bad_mask is set, return an all zero vector of
-* unspecified length.
+* If bad_mask is unset, return input[offset:input_length] copied to new
+* buffer. If bad_mask is set, return an empty vector. In all cases, the capacity
+* of the vector is equal to input_length
+*
+* This function attempts to avoid leaking the following:
+*  - if bad_input was set or not
+*  - the value of offset
+*  - the values in input[]
+*
+* This function leaks the value of input_length
 */
+BOTAN_TEST_API
 secure_vector<uint8_t> copy_output(CT::Mask<uint8_t> bad_input,
                                    const uint8_t input[],
                                    size_t input_length,
-                                   size_t delim_idx);
+                                   size_t offset);
 
 secure_vector<uint8_t> strip_leading_zeros(const uint8_t in[], size_t length);
 


Test patch:
diff --git a/src/tests/test_utils.cpp b/src/tests/test_utils.cpp
index 470b5eb2f..e9eda1a28 100644
--- a/src/tests/test_utils.cpp
+++ b/src/tests/test_utils.cpp
@@ -230,6 +230,46 @@ class CT_Mask_Tests final : public Test
          result.test_eq_sz("CT::is_less32", Botan::CT::Mask<uint32_t>::is_lt(0xFFFFFFFF, 5).value(), 0x00000000);
          result.test_eq_sz("CT::is_less32", Botan::CT::Mask<uint32_t>::is_lt(5, 0xFFFFFFFF).value(), 0xFFFFFFFF);
 
+         for(auto bad_input : { 0, 1 })
+            {
+            for(size_t input_length : { 0, 1, 2, 32 })
+               {
+               for(size_t offset = 0; offset != input_length + 1; ++offset)
+                  {
+                  const auto mask = Botan::CT::Mask<uint8_t>::expand(static_cast<uint8_t>(bad_input));
+
+                  std::vector<uint8_t> input(input_length);
+                  rng().randomize(input.data(), input.size());
+
+                  auto output = Botan::CT::copy_output(mask,
+                                                       input.data(),
+                                                       input.size(),
+                                                       offset);
+
+                  result.test_eq_sz("CT::copy_output capacity", output.capacity(), input.size());
+
+                  if(bad_input)
+                     {
+                     result.confirm("If bad input, no output", output.empty());
+                     }
+                  else
+                     {
+                     if(offset >= input_length)
+                        {
+                        result.confirm("If offset is too large, output is empty", output.empty());
+                        }
+                     else
+                        {
+                        result.test_eq_sz("CT::copy_output length", output.size(), input.size() - offset);
+
+                        for(size_t i = 0; i != output.size(); ++i)
+                           result.test_eq_sz("CT::copy_output offset", output[i], input[i + offset]);
+                        }
+                     }
+                  }
+               }
+            }
+
          return {result};
          }
    };
diff --git a/src/tests/unit_x509.cpp b/src/tests/unit_x509.cpp
index 8703abb93..3c9c810d1 100644
--- a/src/tests/unit_x509.cpp
+++ b/src/tests/unit_x509.cpp
@@ -468,8 +468,10 @@ Test::Result test_rdn_multielement_set_name()
 
    Botan::X509_Certificate cert(Test::data_file("x509/misc/rdn_set.crt"));
 
-   result.confirm("contains expected name components",
+   result.confirm("issuer DN contains expected name components",
                   cert.issuer_dn().get_attributes().size() == 4);
+   result.confirm("subject DN contains expected name components",
+                  cert.subject_dn().get_attributes().size() == 4);
 
    return result;
    }