Merge pull request #6579 from gilles-peskine-arm/negative-zero-from-add-2.28

yanesca · web-flow · commit e530b5b4c47e · 2022-11-16T14:06:04.000Z
Backport 2.28: Fix negative zero from bignum add/subtract
diff --git a/ChangeLog.d/negative-zero-from-add.txt b/ChangeLog.d/negative-zero-from-add.txt
@@ -0,0 +1,6 @@
+Bugfix
+   * In the bignum module, operations of the form (-A) - (+A) or (-A) - (-A)
+     with A > 0 created an unintended representation of the value 0 which was
+     not processed correctly by some bignum operations. Fix this. This had no
+     consequence on cryptography code, but might affect applications that call
+     bignum directly and use negative numbers.
diff --git a/include/mbedtls/bignum.h b/include/mbedtls/bignum.h
@@ -191,9 +191,27 @@ extern "C" {
  */
 typedef struct mbedtls_mpi
 {
-    int s;              /*!<  Sign: -1 if the mpi is negative, 1 otherwise */
-    size_t n;           /*!<  total # of limbs  */
-    mbedtls_mpi_uint *p;          /*!<  pointer to limbs  */
+    /** Sign: -1 if the mpi is negative, 1 otherwise.
+     *
+     * The number 0 must be represented with `s = +1`. Although many library
+     * functions treat all-limbs-zero as equivalent to a valid representation
+     * of 0 regardless of the sign bit, there are exceptions, so bignum
+     * functions and external callers must always set \c s to +1 for the
+     * number zero.
+     *
+     * Note that this implies that calloc() or `... = {0}` does not create
+     * a valid MPI representation. You must call mbedtls_mpi_init().
+     */
+    int s;
+
+    /** Total number of limbs in \c p.  */
+    size_t n;
+
+    /** Pointer to limbs.
+     *
+     * This may be \c NULL if \c n is 0.
+     */
+    mbedtls_mpi_uint *p;
 }
 mbedtls_mpi;
 
diff --git a/library/bignum.c b/library/bignum.c
@@ -1249,27 +1249,34 @@ int mbedtls_mpi_sub_abs( mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi
     return( ret );
 }
 
-/*
- * Signed addition: X = A + B
+/* Common function for signed addition and subtraction.
+ * Calculate A + B * flip_B where flip_B is 1 or -1.
  */
-int mbedtls_mpi_add_mpi( mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi *B )
+static int add_sub_mpi( mbedtls_mpi *X,
+                        const mbedtls_mpi *A, const mbedtls_mpi *B,
+                        int flip_B )
 {
     int ret, s;
     MPI_VALIDATE_RET( X != NULL );
     MPI_VALIDATE_RET( A != NULL );
     MPI_VALIDATE_RET( B != NULL );
 
     s = A->s;
-    if( A->s * B->s < 0 )
+    if( A->s * B->s * flip_B < 0 )
     {
-        if( mbedtls_mpi_cmp_abs( A, B ) >= 0 )
+        int cmp = mbedtls_mpi_cmp_abs( A, B );
+        if( cmp >= 0 )
         {
             MBEDTLS_MPI_CHK( mbedtls_mpi_sub_abs( X, A, B ) );
-            X->s =  s;
+            /* If |A| = |B|, the result is 0 and we must set the sign bit
+             * to +1 regardless of which of A or B was negative. Otherwise,
+             * since |A| > |B|, the sign is the sign of A. */
+            X->s = cmp == 0 ? 1 : s;
         }
         else
         {
             MBEDTLS_MPI_CHK( mbedtls_mpi_sub_abs( X, B, A ) );
+            /* Since |A| < |B|, the sign is the opposite of A. */
             X->s = -s;
         }
     }
@@ -1284,39 +1291,20 @@ int mbedtls_mpi_add_mpi( mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi
     return( ret );
 }
 
+/*
+ * Signed addition: X = A + B
+ */
+int mbedtls_mpi_add_mpi( mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi *B )
+{
+    return( add_sub_mpi( X, A, B, 1 ) );
+}
+
 /*
  * Signed subtraction: X = A - B
  */
 int mbedtls_mpi_sub_mpi( mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi *B )
 {
-    int ret, s;
-    MPI_VALIDATE_RET( X != NULL );
-    MPI_VALIDATE_RET( A != NULL );
-    MPI_VALIDATE_RET( B != NULL );
-
-    s = A->s;
-    if( A->s * B->s > 0 )
-    {
-        if( mbedtls_mpi_cmp_abs( A, B ) >= 0 )
-        {
-            MBEDTLS_MPI_CHK( mbedtls_mpi_sub_abs( X, A, B ) );
-            X->s =  s;
-        }
-        else
-        {
-            MBEDTLS_MPI_CHK( mbedtls_mpi_sub_abs( X, B, A ) );
-            X->s = -s;
-        }
-    }
-    else
-    {
-        MBEDTLS_MPI_CHK( mbedtls_mpi_add_abs( X, A, B ) );
-        X->s = s;
-    }
-
-cleanup:
-
-    return( ret );
+    return( add_sub_mpi( X, A, B, -1 ) );
 }
 
 /*
diff --git a/tests/include/test/helpers.h b/tests/include/test/helpers.h
@@ -360,20 +360,34 @@ void mbedtls_test_err_add_check( int high, int low,
 #if defined(MBEDTLS_BIGNUM_C)
 /** Read an MPI from a hexadecimal string.
  *
- * Like mbedtls_mpi_read_string(), but size the resulting bignum based
- * on the number of digits in the string. In particular, construct a
- * bignum with 0 limbs for an empty string, and a bignum with leading 0
- * limbs if the string has sufficiently many leading 0 digits.
- *
- * This is important so that the "0 (null)" and "0 (1 limb)" and
- * "leading zeros" test cases do what they claim.
+ * Like mbedtls_mpi_read_string(), but with tighter guarantees around
+ * edge cases.
+ *
+ * - This function guarantees that if \p s begins with '-' then the sign
+ *   bit of the result will be negative, even if the value is 0.
+ *   When this function encounters such a "negative 0", it
+ *   increments #mbedtls_test_case_uses_negative_0.
+ * - The size of the result is exactly the minimum number of limbs needed
+ *   to fit the digits in the input. In particular, this function constructs
+ *   a bignum with 0 limbs for an empty string, and a bignum with leading 0
+ *   limbs if the string has sufficiently many leading 0 digits.
+ *   This is important so that the "0 (null)" and "0 (1 limb)" and
+ *   "leading zeros" test cases do what they claim.
  *
  * \param[out] X        The MPI object to populate. It must be initialized.
  * \param[in] s         The null-terminated hexadecimal string to read from.
  *
  * \return \c 0 on success, an \c MBEDTLS_ERR_MPI_xxx error code otherwise.
  */
 int mbedtls_test_read_mpi( mbedtls_mpi *X, const char *s );
+
+/** Nonzero if the current test case had an input parsed with
+ * mbedtls_test_read_mpi() that is a negative 0 (`"-"`, `"-0"`, `"-00"`, etc.,
+ * constructing a result with the sign bit set to -1 and the value being
+ * all-limbs-0, which is not a valid representation in #mbedtls_mpi but is
+ * tested for robustness).
+ */
+extern unsigned mbedtls_test_case_uses_negative_0;
 #endif /* MBEDTLS_BIGNUM_C */
 
 #endif /* TEST_HELPERS_H */
diff --git a/tests/scripts/generate_bignum_tests.py b/tests/scripts/generate_bignum_tests.py
@@ -54,9 +54,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import itertools
 import sys
-import typing
 
 from abc import ABCMeta, abstractmethod
 from typing import Iterator, List, Tuple, TypeVar
@@ -68,20 +66,20 @@
 T = TypeVar('T') #pylint: disable=invalid-name
 
 def hex_to_int(val: str) -> int:
-    return int(val, 16) if val else 0
+    """Implement the syntax accepted by mbedtls_test_read_mpi().
+
+    This is a superset of what is accepted by mbedtls_test_read_mpi_core().
+    """
+    if val in ['', '-']:
+        return 0
+    return int(val, 16)
 
 def quote_str(val) -> str:
     return "\"{}\"".format(val)
 
 def combination_pairs(values: List[T]) -> List[Tuple[T, T]]:
     """Return all pair combinations from input values."""
-    # The return value is cast, as older versions of mypy are unable to derive
-    # the specific type returned by itertools.combinations_with_replacement.
-    return typing.cast(
-        List[Tuple[T, T]],
-        list(itertools.combinations_with_replacement(values, 2))
-    )
-
+    return [(x, y) for x in values for y in values]
 
 class BignumTarget(test_data_generation.BaseTarget, metaclass=ABCMeta):
     #pylint: disable=abstract-method
@@ -105,7 +103,8 @@ class BignumOperation(BignumTarget, metaclass=ABCMeta):
     """
     symbol = ""
     input_values = [
-        "", "0", "7b", "-7b",
+        "", "0", "-", "-0",
+        "7b", "-7b",
         "0000000000000000123", "-0000000000000000123",
         "1230000000000000000", "-1230000000000000000"
     ] # type: List[str]
@@ -120,6 +119,11 @@ def __init__(self, val_a: str, val_b: str) -> None:
     def arguments(self) -> List[str]:
         return [quote_str(self.arg_a), quote_str(self.arg_b), self.result()]
 
+    def description_suffix(self) -> str:
+        #pylint: disable=no-self-use # derived classes need self
+        """Text to add at the end of the test case description."""
+        return ""
+
     def description(self) -> str:
         """Generate a description for the test case.
 
@@ -133,6 +137,9 @@ def description(self) -> str:
                 self.symbol,
                 self.value_description(self.arg_b)
             )
+            description_suffix = self.description_suffix()
+            if description_suffix:
+                self.case_description += " " + description_suffix
         return super().description()
 
     @abstractmethod
@@ -153,6 +160,8 @@ def value_description(val) -> str:
         """
         if val == "":
             return "0 (null)"
+        if val == "-":
+            return "negative 0 (null)"
         if val == "0":
             return "0 (1 limb)"
 
@@ -228,8 +237,21 @@ class BignumAdd(BignumOperation):
         ]
     )
 
+    def __init__(self, val_a: str, val_b: str) -> None:
+        super().__init__(val_a, val_b)
+        self._result = self.int_a + self.int_b
+
+    def description_suffix(self) -> str:
+        if (self.int_a >= 0 and self.int_b >= 0):
+            return "" # obviously positive result or 0
+        if (self.int_a <= 0 and self.int_b <= 0):
+            return "" # obviously negative result or 0
+        # The sign of the result is not obvious, so indicate it
+        return ", result{}0".format('>' if self._result > 0 else
+                                    '<' if self._result < 0 else '=')
+
     def result(self) -> str:
-        return quote_str("{:x}".format(self.int_a + self.int_b))
+        return quote_str("{:x}".format(self._result))
 
 if __name__ == '__main__':
     # Use the section of the docstring relevant to the CLI as description
diff --git a/tests/src/helpers.c b/tests/src/helpers.c
@@ -107,6 +107,10 @@ void mbedtls_test_set_step( unsigned long step )
     mbedtls_test_info.step = step;
 }
 
+#if defined(MBEDTLS_BIGNUM_C)
+unsigned mbedtls_test_case_uses_negative_0 = 0;
+#endif
+
 void mbedtls_test_info_reset( void )
 {
     mbedtls_test_info.result = MBEDTLS_TEST_RESULT_SUCCESS;
@@ -116,6 +120,9 @@ void mbedtls_test_info_reset( void )
     mbedtls_test_info.filename = 0;
     memset( mbedtls_test_info.line1, 0, sizeof( mbedtls_test_info.line1 ) );
     memset( mbedtls_test_info.line2, 0, sizeof( mbedtls_test_info.line2 ) );
+#if defined(MBEDTLS_BIGNUM_C)
+    mbedtls_test_case_uses_negative_0 = 0;
+#endif
 }
 
 int mbedtls_test_equal( const char *test, int line_no, const char* filename,
@@ -426,14 +433,31 @@ void mbedtls_test_err_add_check( int high, int low,
 #if defined(MBEDTLS_BIGNUM_C)
 int mbedtls_test_read_mpi( mbedtls_mpi *X, const char *s )
 {
+    int negative = 0;
+    /* Always set the sign bit to -1 if the input has a minus sign, even for 0.
+     * This creates an invalid representation, which mbedtls_mpi_read_string()
+     * avoids but we want to be able to create that in test data. */
+    if( s[0] == '-' )
+    {
+        ++s;
+        negative = 1;
+    }
     /* mbedtls_mpi_read_string() currently retains leading zeros.
      * It always allocates at least one limb for the value 0. */
     if( s[0] == 0 )
     {
         mbedtls_mpi_free( X );
         return( 0 );
     }
-    else
-        return( mbedtls_mpi_read_string( X, 16, s ) );
+    int ret = mbedtls_mpi_read_string( X, 16, s );
+    if( ret != 0 )
+        return( ret );
+    if( negative )
+    {
+        if( mbedtls_mpi_cmp_int( X, 0 ) == 0 )
+            ++mbedtls_test_case_uses_negative_0;
+        X->s = -1;
+    }
+    return( 0 );
 }
 #endif
diff --git a/tests/suites/test_suite_bignum.function b/tests/suites/test_suite_bignum.function
@@ -11,10 +11,21 @@
  * constructing the value. */
 static int sign_is_valid( const mbedtls_mpi *X )
 {
+    /* Only +1 and -1 are valid sign bits, not e.g. 0 */
     if( X->s != 1 && X->s != -1 )
-        return( 0 ); // invalid sign bit, e.g. 0
-    if( mbedtls_mpi_bitlen( X ) == 0 && X->s != 1 )
-        return( 0 ); // negative zero
+        return( 0 );
+
+    /* The value 0 must be represented with the sign +1. A "negative zero"
+     * with s=-1 is an invalid representation. Forbid that. As an exception,
+     * we sometimes test the robustness of library functions when given
+     * a negative zero input. If a test case has a negative zero as input,
+     * we don't mind if the function has a negative zero output. */
+    if( ! mbedtls_test_case_uses_negative_0 &&
+        mbedtls_mpi_bitlen( X ) == 0 && X->s != 1 )
+    {
+        return( 0 );
+    }
+
     return( 1 );
 }
 
diff --git a/tests/suites/test_suite_bignum.generated.data b/tests/suites/test_suite_bignum.generated.data
diff --git a/tests/suites/test_suite_bignum.misc.data b/tests/suites/test_suite_bignum.misc.data
