Merge pull request #4297 from gilles-peskine-arm/ecp-add-fix-202104

Fix ECP arithmetic bug and read of zero-padded negative number

library/bignum.c

@@ -470,6 +470,7 @@ int mbedtls_mpi_read_string( mbedtls_mpi *X, int radix, const char *s )
 {
     int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
     size_t i, j, slen, n;
+    int sign = 1;
     mbedtls_mpi_uint d;
     mbedtls_mpi T;
     MPI_VALIDATE_RET( X != NULL );
@@ -480,6 +481,12 @@ int mbedtls_mpi_read_string( mbedtls_mpi *X, int radix, const char *s )
 
     mbedtls_mpi_init( &T );
 
+    if( s[0] == '-' )
+    {
+        ++s;
+        sign = -1;
+    }
+
     slen = strlen( s );
 
     if( radix == 16 )
@@ -494,12 +501,6 @@ int mbedtls_mpi_read_string( mbedtls_mpi *X, int radix, const char *s )
 
         for( i = slen, j = 0; i > 0; i--, j++ )
         {
-            if( i == 1 && s[i - 1] == '-' )
-            {
-                X->s = -1;
-                break;
-            }
-
             MBEDTLS_MPI_CHK( mpi_get_digit( &d, radix, s[i - 1] ) );
             X->p[j / ( 2 * ciL )] |= d << ( ( j % ( 2 * ciL ) ) << 2 );
         }
@@ -510,26 +511,15 @@ int mbedtls_mpi_read_string( mbedtls_mpi *X, int radix, const char *s )
 
         for( i = 0; i < slen; i++ )
         {
-            if( i == 0 && s[i] == '-' )
-            {
-                X->s = -1;
-                continue;
-            }
-
             MBEDTLS_MPI_CHK( mpi_get_digit( &d, radix, s[i] ) );
             MBEDTLS_MPI_CHK( mbedtls_mpi_mul_int( &T, X, radix ) );
-
-            if( X->s == 1 )
-            {
-                MBEDTLS_MPI_CHK( mbedtls_mpi_add_int( X, &T, d ) );
-            }
-            else
-            {
-                MBEDTLS_MPI_CHK( mbedtls_mpi_sub_int( X, &T, d ) );
-            }
+            MBEDTLS_MPI_CHK( mbedtls_mpi_add_int( X, &T, d ) );
         }
     }
 
+    if( sign < 0 && mbedtls_mpi_bitlen( X ) != 0 )
+        X->s = -1;
+
 cleanup:
 
     mbedtls_mpi_free( &T );

library/ecp.c

@@ -77,6 +77,8 @@
 #include "mbedtls/platform_util.h"
 #include "mbedtls/error.h"
 
+#include "ecp_invasive.h"
+
 #include <string.h>
 
 #if !defined(MBEDTLS_ECP_ALT)

library/ecp_curves.c

@@ -25,6 +25,8 @@
 #include "mbedtls/platform_util.h"
 #include "mbedtls/error.h"
 
+#include "ecp_invasive.h"
+
 #include <string.h>
 
 #if !defined(MBEDTLS_ECP_ALT)
@@ -1028,26 +1030,35 @@ static inline void sub32( uint32_t *dst, uint32_t src, signed char *carry )
     STORE32; i++;                               \
     cur = c > 0 ? c : 0; STORE32;               \
     cur = 0; while( ++i < MAX32 ) { STORE32; }  \
-    if( c < 0 ) fix_negative( N, c, bits );
+    if( c < 0 ) mbedtls_ecp_fix_negative( N, c, bits );
 
 /*
  * If the result is negative, we get it in the form
- * c * 2^(bits + 32) + N, with c negative and N positive shorter than 'bits'
+ * c * 2^bits + N, with c negative and N positive shorter than 'bits'
  */
-static inline void fix_negative( mbedtls_mpi *N, signed char c, size_t bits )
+MBEDTLS_STATIC_TESTABLE
+void mbedtls_ecp_fix_negative( mbedtls_mpi *N, signed char c, size_t bits )
 {
     size_t i;
 
-    /* Set N := N - 2^bits */
-    --N->p[0];
+    /* Set N := 2^bits - 1 - N. We know that 0 <= N < 2^bits, so
+     * set the absolute value to 0xfff...fff - N. There is no carry
+     * since we're subtracting from all-bits-one.  */
     for( i = 0; i <= bits / 8 / sizeof( mbedtls_mpi_uint ); i++ )
     {
         N->p[i] = ~(mbedtls_mpi_uint)0 - N->p[i];
     }
+    /* Add 1, taking care of the carry. */
+    i = 0;
+    do
+        ++N->p[i];
+    while( N->p[i++] == 0 && i <= bits / 8 / sizeof( mbedtls_mpi_uint ) );
+    /* Invert the sign.
+     * Now N = N0 - 2^bits where N0 is the initial value of N. */
     N->s = -1;
 
-    /* Add |c| * 2^(bits + 32) to the absolute value. Since c and N are
-    * negative, this adds c * 2^(bits + 32). */
+    /* Add |c| * 2^bits to the absolute value. Since c and N are
+    * negative, this adds c * 2^bits. */
     mbedtls_mpi_uint msw = (mbedtls_mpi_uint) -c;
 #if defined(MBEDTLS_HAVE_INT64)
     if( bits == 224 )

library/ecp_invasive.h

@@ -0,0 +1,51 @@
+/**
+ * \file ecp_invasive.h
+ *
+ * \brief ECP module: interfaces for invasive testing only.
+ *
+ * The interfaces in this file are intended for testing purposes only.
+ * They SHOULD NOT be made available in library integrations except when
+ * building the library for testing.
+ */
+/*
+ *  Copyright The Mbed TLS Contributors
+ *  SPDX-License-Identifier: Apache-2.0
+ *
+ *  Licensed under the Apache License, Version 2.0 (the "License"); you may
+ *  not use this file except in compliance with the License.
+ *  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ *  WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ */
+#ifndef MBEDTLS_ECP_INVASIVE_H
+#define MBEDTLS_ECP_INVASIVE_H
+
+#include "common.h"
+#include "mbedtls/ecp.h"
+
+#if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_ECP_C)
+
+#if defined(MBEDTLS_ECP_DP_SECP224R1_ENABLED) ||   \
+    defined(MBEDTLS_ECP_DP_SECP256R1_ENABLED) ||   \
+    defined(MBEDTLS_ECP_DP_SECP384R1_ENABLED)
+/* Preconditions:
+ *   - bits is a multiple of 64 or is 224
+ *   - c is -1 or -2
+ *   - 0 <= N < 2^bits
+ *   - N has room for bits plus one limb
+ *
+ * Behavior:
+ * Set N to c * 2^bits + old_value_of_N.
+ */
+void mbedtls_ecp_fix_negative( mbedtls_mpi *N, signed char c, size_t bits );
+#endif
+
+#endif /* MBEDTLS_TEST_HOOKS && MBEDTLS_ECP_C */
+
+#endif /* MBEDTLS_ECP_INVASIVE_H */