From 9e5b50b54d1032634979c224f2dd11c84f2900b7 Mon Sep 17 00:00:00 2001 From: Billy Brumley Date: Thu, 26 Apr 2018 18:08:36 +0300 Subject: [PATCH] fix: BN_swap mishandles flags Reviewed-by: Rich Salz Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/6099) --- crypto/bn/bn_lib.c | 11 ++++--- test/bntest.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 4 deletions(-) diff --git a/crypto/bn/bn_lib.c b/crypto/bn/bn_lib.c index 91553d4391..5bb996e5bc 100644 --- a/crypto/bn/bn_lib.c +++ b/crypto/bn/bn_lib.c @@ -300,6 +300,11 @@ BIGNUM *BN_copy(BIGNUM *a, const BIGNUM *b) return a; } +#define FLAGS_DATA(flags) ((flags) & (BN_FLG_STATIC_DATA \ + | BN_FLG_CONSTTIME \ + | BN_FLG_SECURE)) +#define FLAGS_STRUCT(flags) ((flags) & (BN_FLG_MALLOCED)) + void BN_swap(BIGNUM *a, BIGNUM *b) { int flags_old_a, flags_old_b; @@ -327,10 +332,8 @@ void BN_swap(BIGNUM *a, BIGNUM *b) b->dmax = tmp_dmax; b->neg = tmp_neg; - a->flags = - (flags_old_a & BN_FLG_MALLOCED) | (flags_old_b & BN_FLG_STATIC_DATA); - b->flags = - (flags_old_b & BN_FLG_MALLOCED) | (flags_old_a & BN_FLG_STATIC_DATA); + a->flags = FLAGS_STRUCT(flags_old_a) | FLAGS_DATA(flags_old_b); + b->flags = FLAGS_STRUCT(flags_old_b) | FLAGS_DATA(flags_old_a); bn_check_top(a); bn_check_top(b); } diff --git a/test/bntest.c b/test/bntest.c index d5b5e0494e..629707ad16 100644 --- a/test/bntest.c +++ b/test/bntest.c @@ -151,6 +151,78 @@ static int rand_neg(void) } +static int test_swap(void) +{ + BIGNUM *a = NULL, *b = NULL, *c = NULL, *d = NULL; + int top, cond, st = 0; + + if (!TEST_ptr(a = BN_new()) + || !TEST_ptr(b = BN_new()) + || !TEST_ptr(c = BN_new()) + || !TEST_ptr(d = BN_new())) + goto err; + + BN_bntest_rand(a, 1024, 1, 0); + BN_bntest_rand(b, 1024, 1, 0); + BN_copy(c, a); + BN_copy(d, b); + top = BN_num_bits(a)/BN_BITS2; + + /* regular swap */ + BN_swap(a, b); + if (!equalBN("swap", a, d) + || !equalBN("swap", b, c)) + goto err; + + /* conditional swap: true */ + cond = 1; + BN_consttime_swap(cond, a, b, top); + if (!equalBN("cswap true", a, c) + || !equalBN("cswap true", b, d)) + goto err; + + /* conditional swap: false */ + cond = 0; + BN_consttime_swap(cond, a, b, top); + if (!equalBN("cswap false", a, c) + || !equalBN("cswap false", b, d)) + goto err; + + /* same tests but checking flag swap */ + BN_set_flags(a, BN_FLG_CONSTTIME); + + BN_swap(a, b); + if (!equalBN("swap, flags", a, d) + || !equalBN("swap, flags", b, c) + || !TEST_true(BN_get_flags(b, BN_FLG_CONSTTIME)) + || !TEST_false(BN_get_flags(a, BN_FLG_CONSTTIME))) + goto err; + + cond = 1; + BN_consttime_swap(cond, a, b, top); + if (!equalBN("cswap true, flags", a, c) + || !equalBN("cswap true, flags", b, d) + || !TEST_true(BN_get_flags(a, BN_FLG_CONSTTIME)) + || !TEST_false(BN_get_flags(b, BN_FLG_CONSTTIME))) + goto err; + + cond = 0; + BN_consttime_swap(cond, a, b, top); + if (!equalBN("cswap false, flags", a, c) + || !equalBN("cswap false, flags", b, d) + || !TEST_true(BN_get_flags(a, BN_FLG_CONSTTIME)) + || !TEST_false(BN_get_flags(b, BN_FLG_CONSTTIME))) + goto err; + + st = 1; + err: + BN_free(a); + BN_free(b); + BN_free(c); + BN_free(d); + return st; +} + static int test_sub(void) { BIGNUM *a = NULL, *b = NULL, *c = NULL; @@ -2118,6 +2190,7 @@ int setup_tests(void) ADD_TEST(test_badmod); ADD_TEST(test_expmodzero); ADD_TEST(test_smallprime); + ADD_TEST(test_swap); #ifndef OPENSSL_NO_EC2M ADD_TEST(test_gf2m_add); ADD_TEST(test_gf2m_mod); -- 2.25.1