Fix a SCA leak using BN_bn2bin()
authorNicola Tuveri <nic.tuv@gmail.com>
Thu, 1 Aug 2019 22:33:05 +0000 (01:33 +0300)
committerNicola Tuveri <nic.tuv@gmail.com>
Fri, 6 Sep 2019 12:56:45 +0000 (15:56 +0300)
commit2e9d293447b95c2a69eb5ff07fe974361d779444
tree55985607bb655c1bea4112f3128c8e9e272275b6
parent55611d549bcf65e0de04938adbf403ccf02f241b
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 <matt@openssl.org>
Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
(Merged from https://github.com/openssl/openssl/pull/9793)
crypto/ec/ecp_nistp224.c
crypto/ec/ecp_nistp256.c
crypto/ec/ecp_nistp521.c