From abdf67ee9fa976d8248aa8f74e6a2aec8b54ee92 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 26 Oct 2018 13:28:32 +0100 Subject: [PATCH 1/2] Cast number of operations to `uint` in MBEDTLS_ECP_BUDGET Context: The macro `MBEDTLS_ECP_BUDGET()` is called before performing a number of potentially time-consuming ECC operations. If restartable ECC is enabled, it wraps a call to `mbedtls_ecp_check_budget()` which in turn checks if the requested number of operations can be performed without exceeding the maximum number of consecutive ECC operations. Issue: The function `mbedtls_ecp_check_budget()` expects a the number of requested operations to be given as a value of type `unsigned`, while some calls of the wrapper macro `MBEDTLS_ECP_BUDGET()` use expressions of type `size_t`. This rightfully leads to warnings about implicit truncation from `size_t` to `unsigned` on some compilers. Fix: This commit makes the truncation explicit by adding an explicit cast to `unsigned` in the expansion of the `MBEDTLS_ECP_BUDGET()` macro. Justification: Functionally, the new version is equivalent to the previous code. The warning about truncation can be discarded because, as can be inferred from `ecp.h`, the number of requested operations is never larger than 1000. --- include/mbedtls/ecp.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/ecp.h b/include/mbedtls/ecp.h index 448549cfc..5db87524e 100644 --- a/include/mbedtls/ecp.h +++ b/include/mbedtls/ecp.h @@ -233,7 +233,9 @@ int mbedtls_ecp_check_budget( const mbedtls_ecp_group *grp, unsigned ops ); /* Utility macro for checking and updating ops budget */ -#define MBEDTLS_ECP_BUDGET( ops ) MBEDTLS_MPI_CHK( mbedtls_ecp_check_budget( grp, rs_ctx, ops ) ); +#define MBEDTLS_ECP_BUDGET( ops ) \ + MBEDTLS_MPI_CHK( mbedtls_ecp_check_budget( grp, rs_ctx, \ + (unsigned) (ops) ) ); #else /* MBEDTLS_ECP_RESTARTABLE */ From b10c66073f3f83359d8333768f9fed2733c11c87 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 26 Oct 2018 13:50:13 +0100 Subject: [PATCH 2/2] Detect unsigned integer overflow in mbedtls_ecp_check_budget() This commit modifies a bounds check in `mbedtls_ecp_check_budget()` to be correct even if the requested number of ECC operations would overflow the operation counter. --- library/ecp.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index b193ad4f8..de5725c70 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -248,9 +248,16 @@ int mbedtls_ecp_check_budget( const mbedtls_ecp_group *grp, else if( grp->pbits >= 384 ) ops *= 2; - /* avoid infinite loops: always allow first step */ - if( rs_ctx->ops_done != 0 && rs_ctx->ops_done + ops > ecp_max_ops ) + /* Avoid infinite loops: always allow first step. + * Because of that, however, it's not generally true + * that ops_done <= ecp_max_ops, so the check + * ops_done > ecp_max_ops below is mandatory. */ + if( ( rs_ctx->ops_done != 0 ) && + ( rs_ctx->ops_done > ecp_max_ops || + ops > ecp_max_ops - rs_ctx->ops_done ) ) + { return( MBEDTLS_ERR_ECP_IN_PROGRESS ); + } /* update running count */ rs_ctx->ops_done += ops;