Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Constant-time enhancements for bignum.c and ecdsa.c #3309

Closed
wants to merge 4 commits into from
Closed

fix: Constant-time enhancements for bignum.c and ecdsa.c #3309

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7ea6cb8
fix: Constant-time enhancements for bignum.c and ecdsa.c
scottarc May 6, 2020
c4dc4ee
chore: Fix whitespace, remove extraneous comment
scottarc May 7, 2020
506c8dc
Attempt #2 to fix Travis CI
scottarc May 7, 2020
f8a714a
Simplify constant declaration macros for 32/64-bit
scottarc May 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 106 additions & 1 deletion include/mbedtls/bignum.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,18 +124,42 @@
#endif /* !MBEDTLS_HAVE_INT64 */
typedef int64_t mbedtls_mpi_sint;
typedef uint64_t mbedtls_mpi_uint;
#if !defined(MBEDTLS_MPI_UINT_BITS)
#define MBEDTLS_MPI_UINT_BITS 64
#endif /* MBEDTLS_MPI_UINT_BITS */
#if !defined(MBEDTLS_MPI_HALF_MASK)
#define MBEDTLS_MPI_HALF_MASK 0xffffffff
#endif /* MBEDTLS_MPI_HALF_MASK */
#if !defined(MBEDTLS_MPI_HALF_BITS)
#define MBEDTLS_MPI_HALF_BITS 32
#endif /* MBEDTLS_MPI_HALF_BITS */
#if !defined(MBEDTLS_MPI_UINT_MASK)
#define MBEDTLS_MPI_UINT_MASK 0x7fffffffffffffffLL
#endif /* MBEDTLS_MPI_UINT_MASK */
#elif defined(__GNUC__) && ( \
defined(__amd64__) || defined(__x86_64__) || \
defined(__ppc64__) || defined(__powerpc64__) || \
defined(__ia64__) || defined(__alpha__) || \
( defined(__sparc__) && defined(__arch64__) ) || \
defined(__s390x__) || defined(__mips64) || \
defined(__aarch64__) )
#if !defined(MBEDTLS_HAVE_INT64)
#if !defined(MBEDTLS_HAVE_INT64)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is indentation change?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My IDE did that. I'll correct it on my next commit.

#define MBEDTLS_HAVE_INT64
#endif /* MBEDTLS_HAVE_INT64 */
typedef int64_t mbedtls_mpi_sint;
typedef uint64_t mbedtls_mpi_uint;
#if !defined(MBEDTLS_MPI_UINT_BITS)
#define MBEDTLS_MPI_UINT_BITS 64
#endif /* MBEDTLS_MPI_UINT_BITS */
#if !defined(MBEDTLS_MPI_UINT_MASK)
scottarc marked this conversation as resolved.
Show resolved Hide resolved
#define MBEDTLS_MPI_UINT_MASK 0x7fffffffffffffffLL
#endif /* MBEDTLS_MPI_UINT_MASK */
#if !defined(MBEDTLS_MPI_HALF_MASK)
#define MBEDTLS_MPI_HALF_MASK 0xffffffff
#endif /* MBEDTLS_MPI_HALF_MASK */
#if !defined(MBEDTLS_MPI_HALF_BITS)
#define MBEDTLS_MPI_HALF_BITS 32
#endif /* MBEDTLS_MPI_HALF_BITS */
#if !defined(MBEDTLS_NO_UDBL_DIVISION)
/* mbedtls_t_udbl defined as 128-bit unsigned int */
typedef unsigned int mbedtls_t_udbl __attribute__((mode(TI)));
Expand All @@ -151,6 +175,18 @@
#endif /* !MBEDTLS_HAVE_INT64 */
typedef int64_t mbedtls_mpi_sint;
typedef uint64_t mbedtls_mpi_uint;
#if !defined(MBEDTLS_MPI_UINT_BITS)
#define MBEDTLS_MPI_UINT_BITS 64
#endif /* MBEDTLS_MPI_UINT_BITS */
#if !defined(MBEDTLS_MPI_UINT_MASK)
#define MBEDTLS_MPI_UINT_MASK 0x7fffffffffffffffLL
#endif /* MBEDTLS_MPI_UINT_MASK */
#if !defined(MBEDTLS_MPI_HALF_MASK)
#define MBEDTLS_MPI_HALF_MASK 0xffffffff
#endif /* MBEDTLS_MPI_HALF_MASK */
#if !defined(MBEDTLS_MPI_HALF_BITS)
#define MBEDTLS_MPI_HALF_BITS 32
#endif /* MBEDTLS_MPI_HALF_BITS */
#if !defined(MBEDTLS_NO_UDBL_DIVISION)
/* mbedtls_t_udbl defined as 128-bit unsigned int */
typedef __uint128_t mbedtls_t_udbl;
Expand All @@ -160,6 +196,18 @@
/* Force 64-bit integers with unknown compiler */
typedef int64_t mbedtls_mpi_sint;
typedef uint64_t mbedtls_mpi_uint;
#if !defined(MBEDTLS_MPI_UINT_BITS)
#define MBEDTLS_MPI_UINT_BITS 64
#endif /* MBEDTLS_MPI_UINT_BITS */
#if !defined(MBEDTLS_MPI_UINT_MASK)
#define MBEDTLS_MPI_UINT_MASK 0x7fffffffffffffffLL
#endif /* MBEDTLS_MPI_UINT_MASK */
#if !defined(MBEDTLS_MPI_HALF_MASK)
#define MBEDTLS_MPI_HALF_MASK 0xffffffff
#endif /* MBEDTLS_MPI_HALF_MASK */
#if !defined(MBEDTLS_MPI_HALF_BITS)
#define MBEDTLS_MPI_HALF_BITS 32
#endif /* MBEDTLS_MPI_HALF_BITS */
#endif
#endif /* !MBEDTLS_HAVE_INT32 */

Expand All @@ -170,6 +218,18 @@
#endif /* !MBEDTLS_HAVE_INT32 */
typedef int32_t mbedtls_mpi_sint;
typedef uint32_t mbedtls_mpi_uint;
#if !defined(MBEDTLS_MPI_UINT_BITS)
#define MBEDTLS_MPI_UINT_BITS 32
#endif /* MBEDTLS_MPI_UINT_BITS */
#if !defined(MBEDTLS_MPI_UINT_MASK)
#define MBEDTLS_MPI_UINT_MASK 0x7fffffff
#endif /* MBEDTLS_MPI_UINT_MASK */
#if !defined(MBEDTLS_MPI_HALF_MASK)
#define MBEDTLS_MPI_HALF_MASK 0xffff
#endif /* MBEDTLS_MPI_HALF_MASK */
#if !defined(MBEDTLS_MPI_HALF_BITS)
#define MBEDTLS_MPI_HALF_BITS 16
#endif /* MBEDTLS_MPI_HALF_BITS */
#if !defined(MBEDTLS_NO_UDBL_DIVISION)
typedef uint64_t mbedtls_t_udbl;
#define MBEDTLS_HAVE_UDBL
Expand Down Expand Up @@ -546,6 +606,27 @@ int mbedtls_mpi_write_binary( const mbedtls_mpi *X, unsigned char *buf,
int mbedtls_mpi_write_binary_le( const mbedtls_mpi *X,
unsigned char *buf, size_t buflen );

/**
* \brief Return the index of the highest non-zero limb in X.
*
* \param out
* \param X The MPI to analyze.
* This must point to an initialized MPI.
*
* \return Index of the highest nonzero limb in X.
*/
int mbedtls_mpi_first_nonzero(size_t *out, const mbedtls_mpi *X);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably doesn't need to be a public function. Currently it doesn't seem to be used outside bignum.c, and the fact that its description involves limbs is an indication that it's rather concerned with internal implementation details.

(In the past we have often exposed too much in the public API of each module, and we're trying to get better at it.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On another note, the function description doesn't state if it's constant-time.


/**
* \brief Is this equal to zero? (Constant-time)
*
* \param X The MPI to analyze.
* This must point to an initialized MPI.
*
* \return 1 if all limbs in X == 0, 0 otherwise.
*/
int mbedtls_mpi_is_zero( mbedtls_mpi *X );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note on naming: it would probably be good to use the _ct suffix consistently for constant-time functions. That way we can get used to seeing it and notice when we're using a function that doesn't have it :)

(In an ideal world it should be the other way round: all functions would be constant-time except perhaps a few exceptions that would have a scary suffix such as _leaky. But we can't do that right now.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second note: is that not a bit redundant with mbedtls_mpi_cmp_int_ct()? (Or with mbedtls_mpi_cmp_int() if we decide to make it constant-time?)


/**
* \brief Perform a left-shift on an MPI: X <<= count
*
Expand Down Expand Up @@ -594,6 +675,30 @@ int mbedtls_mpi_cmp_abs( const mbedtls_mpi *X, const mbedtls_mpi *Y );
*/
int mbedtls_mpi_cmp_mpi( const mbedtls_mpi *X, const mbedtls_mpi *Y );

/**
* \brief Compare two MPIs. Constant-time.
*
* \param X The left-hand MPI. This must point to an initialized MPI.
* \param Y The right-hand MPI. This must point to an initialized MPI.
*
* \return \c 1 if \p X is greater than \p Y.
* \return \c -1 if \p X is lesser than \p Y.
* \return \c 0 if \p X is equal to \p Y.
*/
int mbedtls_mpi_cmp_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way you're adding this function is very reasonable and is what we usually do when we want to extend an existing function. However, in this particular instance I don't think being leaky was ever part of the contract of the original function, so I think we can consider just changing the original function to the new constant-time implementation.

The obvious concern is performance, but unless the difference is demonstrably large enough, security and code size usually trump performance. (Code size matters especially for LTS branches, where people want to be able to pick up updates and still fit in the flash of already deployed devices.)

Could you try replacing the implementation of the existing function with your new CT implementation, running programs/test/benchmark and checking if it makes a measurable difference in performance?


/**
* \brief Compare an MPI with an integer. Constant-time.
*
* \param X The left-hand MPI. This must point to an initialized MPI.
* \param z The integer value to compare \p X to.
*
* \return \c 1 if \p X is greater than \p z.
* \return \c -1 if \p X is lesser than \p z.
* \return \c 0 if \p X is equal to \p z.
*/
int mbedtls_mpi_cmp_int_ct( const mbedtls_mpi *X, mbedtls_mpi_sint z );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above.


/**
* \brief Check if an MPI is less than the other in constant time.
*
Expand Down
Loading