From 083f297a48e8c1dd5e02a5fa7be00586f8cb7dff Mon Sep 17 00:00:00 2001 From: Nicola Tuveri Date: Fri, 2 Aug 2019 01:33:05 +0300 Subject: [PATCH] Fix a SCA leak using BN_bn2bin() BN_bn2bin() is not constant-time and leaks the number of bits in the processed BIGNUM. The specialized methods in ecp_nistp224.c, ecp_nistp256.c and ecp_nistp521.c internally used BN_bn2bin() to convert scalars into the internal fixed length representation. This can leak during ECDSA/ECDH key generation or handling the nonce while generating an ECDSA signature, when using these implementations. The amount and risk of leaked information useful for a SCA attack varies for each of the three curves, as it depends mainly on the ratio between the bitlength of the curve subgroup order (governing the size of the secret nonce/key) and the limb size for the internal BIGNUM representation (which depends on the compilation target architecture). To fix this, we replace BN_bn2bin() with BN_bn2binpad(), bounding the output length to the width of the internal representation buffer: this length is public. Internally the final implementation of both BN_bn2binpad() and BN_bn2bin() already has masking in place to avoid leaking bn->top through memory access patterns. Memory access pattern still leaks bn->dmax, the size of the lazily allocated buffer for representing the BIGNUM, which is inevitable with the current BIGNUM architecture: reading past bn->dmax would be an out-of-bound read. As such, it's the caller responsibility to ensure that bn->dmax does not leak secret information, by explicitly expanding the internal BIGNUM buffer to a public value sufficient to avoid any lazy reallocation while manipulating it: this is already done at the top level alongside setting the BN_FLG_CONSTTIME. Finally, the internal implementation of BN_bn2binpad() indirectly calls BN_num_bits() via BN_num_bytes(): the current implementation of BN_num_bits() can leak information to a SCA attacker, and is addressed in the next commit. Thanks to David Schrammel and Samuel Weiser for reporting this issue through responsible disclosure. Reviewed-by: Matt Caswell Reviewed-by: Bernd Edlinger (Merged from https://github.com/openssl/openssl/pull/9511) (cherry picked from commit 805315d3a20f7274195eed75b06c391dacf3b197) --- crypto/ec/ecp_nistp224.c | 12 +++++------- crypto/ec/ecp_nistp256.c | 12 +++++------- crypto/ec/ecp_nistp521.c | 12 +++++------- 3 files changed, 15 insertions(+), 21 deletions(-) diff --git a/crypto/ec/ecp_nistp224.c b/crypto/ec/ecp_nistp224.c index 025273a144..a3ac109a6a 100644 --- a/crypto/ec/ecp_nistp224.c +++ b/crypto/ec/ecp_nistp224.c @@ -339,8 +339,6 @@ static int BN_to_felem(felem out, const BIGNUM *bn) felem_bytearray b_out; unsigned num_bytes; - /* BN_bn2bin eats leading zeroes */ - memset(b_out, 0, sizeof(b_out)); num_bytes = BN_num_bytes(bn); if (num_bytes > sizeof(b_out)) { ECerr(EC_F_BN_TO_FELEM, EC_R_BIGNUM_OUT_OF_RANGE); @@ -350,7 +348,7 @@ static int BN_to_felem(felem out, const BIGNUM *bn) ECerr(EC_F_BN_TO_FELEM, EC_R_BIGNUM_OUT_OF_RANGE); return 0; } - num_bytes = BN_bn2bin(bn, b_in); + num_bytes = BN_bn2binpad(bn, b_in, sizeof(b_in)); flip_endian(b_out, b_in, num_bytes); bin28_to_felem(out, b_out); return 1; @@ -1501,9 +1499,9 @@ int ec_GFp_nistp224_points_mul(const EC_GROUP *group, EC_POINT *r, ECerr(EC_F_EC_GFP_NISTP224_POINTS_MUL, ERR_R_BN_LIB); goto err; } - num_bytes = BN_bn2bin(tmp_scalar, tmp); + num_bytes = BN_bn2binpad(tmp_scalar, tmp, sizeof(tmp)); } else - num_bytes = BN_bn2bin(p_scalar, tmp); + num_bytes = BN_bn2binpad(p_scalar, tmp, sizeof(tmp)); flip_endian(secrets[i], tmp, num_bytes); /* precompute multiples */ if ((!BN_to_felem(x_out, p->X)) || @@ -1547,9 +1545,9 @@ int ec_GFp_nistp224_points_mul(const EC_GROUP *group, EC_POINT *r, ECerr(EC_F_EC_GFP_NISTP224_POINTS_MUL, ERR_R_BN_LIB); goto err; } - num_bytes = BN_bn2bin(tmp_scalar, tmp); + num_bytes = BN_bn2binpad(tmp_scalar, tmp, sizeof(tmp)); } else - num_bytes = BN_bn2bin(scalar, tmp); + num_bytes = BN_bn2binpad(scalar, tmp, sizeof(tmp)); flip_endian(g_secret, tmp, num_bytes); /* do the multiplication with generator precomputation */ batch_mul(x_out, y_out, z_out, diff --git a/crypto/ec/ecp_nistp256.c b/crypto/ec/ecp_nistp256.c index a21e5f78fc..64d5ef4d2c 100644 --- a/crypto/ec/ecp_nistp256.c +++ b/crypto/ec/ecp_nistp256.c @@ -161,8 +161,6 @@ static int BN_to_felem(felem out, const BIGNUM *bn) felem_bytearray b_out; unsigned num_bytes; - /* BN_bn2bin eats leading zeroes */ - memset(b_out, 0, sizeof(b_out)); num_bytes = BN_num_bytes(bn); if (num_bytes > sizeof(b_out)) { ECerr(EC_F_BN_TO_FELEM, EC_R_BIGNUM_OUT_OF_RANGE); @@ -172,7 +170,7 @@ static int BN_to_felem(felem out, const BIGNUM *bn) ECerr(EC_F_BN_TO_FELEM, EC_R_BIGNUM_OUT_OF_RANGE); return 0; } - num_bytes = BN_bn2bin(bn, b_in); + num_bytes = BN_bn2binpad(bn, b_in, sizeof(b_in)); flip_endian(b_out, b_in, num_bytes); bin32_to_felem(out, b_out); return 1; @@ -2128,9 +2126,9 @@ int ec_GFp_nistp256_points_mul(const EC_GROUP *group, EC_POINT *r, ECerr(EC_F_EC_GFP_NISTP256_POINTS_MUL, ERR_R_BN_LIB); goto err; } - num_bytes = BN_bn2bin(tmp_scalar, tmp); + num_bytes = BN_bn2binpad(tmp_scalar, tmp, sizeof(tmp)); } else - num_bytes = BN_bn2bin(p_scalar, tmp); + num_bytes = BN_bn2binpad(p_scalar, tmp, sizeof(tmp)); flip_endian(secrets[i], tmp, num_bytes); /* precompute multiples */ if ((!BN_to_felem(x_out, p->X)) || @@ -2176,9 +2174,9 @@ int ec_GFp_nistp256_points_mul(const EC_GROUP *group, EC_POINT *r, ECerr(EC_F_EC_GFP_NISTP256_POINTS_MUL, ERR_R_BN_LIB); goto err; } - num_bytes = BN_bn2bin(tmp_scalar, tmp); + num_bytes = BN_bn2binpad(tmp_scalar, tmp, sizeof(tmp)); } else - num_bytes = BN_bn2bin(scalar, tmp); + num_bytes = BN_bn2binpad(scalar, tmp, sizeof(tmp)); flip_endian(g_secret, tmp, num_bytes); /* do the multiplication with generator precomputation */ batch_mul(x_out, y_out, z_out, diff --git a/crypto/ec/ecp_nistp521.c b/crypto/ec/ecp_nistp521.c index 1e45f1eec5..d2d73526f6 100644 --- a/crypto/ec/ecp_nistp521.c +++ b/crypto/ec/ecp_nistp521.c @@ -184,8 +184,6 @@ static int BN_to_felem(felem out, const BIGNUM *bn) felem_bytearray b_out; unsigned num_bytes; - /* BN_bn2bin eats leading zeroes */ - memset(b_out, 0, sizeof(b_out)); num_bytes = BN_num_bytes(bn); if (num_bytes > sizeof(b_out)) { ECerr(EC_F_BN_TO_FELEM, EC_R_BIGNUM_OUT_OF_RANGE); @@ -195,7 +193,7 @@ static int BN_to_felem(felem out, const BIGNUM *bn) ECerr(EC_F_BN_TO_FELEM, EC_R_BIGNUM_OUT_OF_RANGE); return 0; } - num_bytes = BN_bn2bin(bn, b_in); + num_bytes = BN_bn2binpad(bn, b_in, sizeof(b_in)); flip_endian(b_out, b_in, num_bytes); bin66_to_felem(out, b_out); return 1; @@ -1968,9 +1966,9 @@ int ec_GFp_nistp521_points_mul(const EC_GROUP *group, EC_POINT *r, ECerr(EC_F_EC_GFP_NISTP521_POINTS_MUL, ERR_R_BN_LIB); goto err; } - num_bytes = BN_bn2bin(tmp_scalar, tmp); + num_bytes = BN_bn2binpad(tmp_scalar, tmp, sizeof(tmp)); } else - num_bytes = BN_bn2bin(p_scalar, tmp); + num_bytes = BN_bn2binpad(p_scalar, tmp, sizeof(tmp)); flip_endian(secrets[i], tmp, num_bytes); /* precompute multiples */ if ((!BN_to_felem(x_out, p->X)) || @@ -2014,9 +2012,9 @@ int ec_GFp_nistp521_points_mul(const EC_GROUP *group, EC_POINT *r, ECerr(EC_F_EC_GFP_NISTP521_POINTS_MUL, ERR_R_BN_LIB); goto err; } - num_bytes = BN_bn2bin(tmp_scalar, tmp); + num_bytes = BN_bn2binpad(tmp_scalar, tmp, sizeof(tmp)); } else - num_bytes = BN_bn2bin(scalar, tmp); + num_bytes = BN_bn2binpad(scalar, tmp, sizeof(tmp)); flip_endian(g_secret, tmp, num_bytes); /* do the multiplication with generator precomputation */ batch_mul(x_out, y_out, z_out, -- 2.25.1