From 2b314fa3a9b065fc5f0b2fe9567b009afed94d20 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 2 Aug 2016 12:49:38 -0400 Subject: [PATCH] Tolerate -0 better in BN_bn2{dec,hex} Negative zeros are nuts, but it will probably be a while before we've fixed everything that can create them. Fix both to consistently print '-0' rather than '0' so failures are easier to diagnose (BN_cmp believes the values are different.) Change-Id: Ic38d90601b43f66219d8f44ca085432106cf98e3 Reviewed-on: https://boringssl-review.googlesource.com/9073 Commit-Queue: David Benjamin Reviewed-by: Adam Langley Commit-Queue: Adam Langley CQ-Verified: CQ bot account: commit-bot@chromium.org --- crypto/bn/bn_test.cc | 52 ++++++++++++++++++++++++++++++++++---------- crypto/bn/convert.c | 25 ++++++++++----------- 2 files changed, 52 insertions(+), 25 deletions(-) diff --git a/crypto/bn/bn_test.cc b/crypto/bn/bn_test.cc index 14dcead1..554d0c66 100644 --- a/crypto/bn/bn_test.cc +++ b/crypto/bn/bn_test.cc @@ -1138,7 +1138,7 @@ static bool TestNegativeZero(BN_CTX *ctx) { return false; } if (!BN_is_zero(c.get()) || BN_is_negative(c.get())) { - fprintf(stderr, "Multiplication test failed!\n"); + fprintf(stderr, "Multiplication test failed.\n"); return false; } @@ -1152,7 +1152,7 @@ static bool TestNegativeZero(BN_CTX *ctx) { return false; } if (!BN_is_zero(d.get()) || BN_is_negative(d.get())) { - fprintf(stderr, "Division test failed!\n"); + fprintf(stderr, "Division test failed.\n"); return false; } @@ -1164,7 +1164,27 @@ static bool TestNegativeZero(BN_CTX *ctx) { return false; } if (!BN_is_zero(c.get()) || BN_is_negative(c.get())) { - fprintf(stderr, "Division test failed!\n"); + fprintf(stderr, "Division test failed.\n"); + return false; + } + + // Test that BN_set_negative will not produce a negative zero. + BN_zero(a.get()); + BN_set_negative(a.get(), 1); + if (BN_is_negative(a.get())) { + fprintf(stderr, "BN_set_negative produced a negative zero.\n"); + return false; + } + + // Test that forcibly creating a negative zero does not break |BN_bn2hex| or + // |BN_bn2dec|. + a->neg = 1; + ScopedOpenSSLString dec(BN_bn2dec(a.get())); + ScopedOpenSSLString hex(BN_bn2hex(a.get())); + if (!dec || !hex || + strcmp(dec.get(), "-0") != 0 || + strcmp(hex.get(), "-0") != 0) { + fprintf(stderr, "BN_bn2dec or BN_bn2hex failed with negative zero.\n"); return false; } @@ -1183,39 +1203,43 @@ static bool TestBadModulus(BN_CTX *ctx) { BN_zero(zero.get()); if (BN_div(a.get(), b.get(), BN_value_one(), zero.get(), ctx)) { - fprintf(stderr, "Division by zero succeeded!\n"); + fprintf(stderr, "Division by zero unexpectedly succeeded.\n"); return false; } ERR_clear_error(); if (BN_mod_mul(a.get(), BN_value_one(), BN_value_one(), zero.get(), ctx)) { - fprintf(stderr, "BN_mod_mul with zero modulus succeeded!\n"); + fprintf(stderr, "BN_mod_mul with zero modulus unexpectedly succeeded.\n"); return false; } ERR_clear_error(); if (BN_mod_exp(a.get(), BN_value_one(), BN_value_one(), zero.get(), ctx)) { - fprintf(stderr, "BN_mod_exp with zero modulus succeeded!\n"); + fprintf(stderr, "BN_mod_exp with zero modulus unexpectedly succeeded.\n"); return 0; } ERR_clear_error(); if (BN_mod_exp_mont(a.get(), BN_value_one(), BN_value_one(), zero.get(), ctx, NULL)) { - fprintf(stderr, "BN_mod_exp_mont with zero modulus succeeded!\n"); + fprintf(stderr, + "BN_mod_exp_mont with zero modulus unexpectedly succeeded.\n"); return 0; } ERR_clear_error(); if (BN_mod_exp_mont_consttime(a.get(), BN_value_one(), BN_value_one(), zero.get(), ctx, nullptr)) { - fprintf(stderr, "BN_mod_exp_mont_consttime with zero modulus succeeded!\n"); + fprintf(stderr, + "BN_mod_exp_mont_consttime with zero modulus unexpectedly " + "succeeded.\n"); return 0; } ERR_clear_error(); if (BN_MONT_CTX_set(mont.get(), zero.get(), ctx)) { - fprintf(stderr, "BN_MONT_CTX_set succeeded for zero modulus!\n"); + fprintf(stderr, + "BN_MONT_CTX_set unexpectedly succeeded for zero modulus.\n"); return false; } ERR_clear_error(); @@ -1227,21 +1251,25 @@ static bool TestBadModulus(BN_CTX *ctx) { } if (BN_MONT_CTX_set(mont.get(), b.get(), ctx)) { - fprintf(stderr, "BN_MONT_CTX_set succeeded for even modulus!\n"); + fprintf(stderr, + "BN_MONT_CTX_set unexpectedly succeeded for even modulus.\n"); return false; } ERR_clear_error(); if (BN_mod_exp_mont(a.get(), BN_value_one(), BN_value_one(), b.get(), ctx, NULL)) { - fprintf(stderr, "BN_mod_exp_mont with even modulus succeeded!\n"); + fprintf(stderr, + "BN_mod_exp_mont with even modulus unexpectedly succeeded.\n"); return 0; } ERR_clear_error(); if (BN_mod_exp_mont_consttime(a.get(), BN_value_one(), BN_value_one(), b.get(), ctx, nullptr)) { - fprintf(stderr, "BN_mod_exp_mont_consttime with even modulus succeeded!\n"); + fprintf(stderr, + "BN_mod_exp_mont_consttime with even modulus unexpectedly " + "succeeded.\n"); return 0; } ERR_clear_error(); diff --git a/crypto/bn/convert.c b/crypto/bn/convert.c index 9125bf84..1392a705 100644 --- a/crypto/bn/convert.c +++ b/crypto/bn/convert.c @@ -204,17 +204,14 @@ int BN_bn2cbb_padded(CBB *out, size_t len, const BIGNUM *in) { static const char hextable[] = "0123456789abcdef"; char *BN_bn2hex(const BIGNUM *bn) { - int i, j, v, z = 0; - char *buf; - char *p; - - buf = OPENSSL_malloc(bn->top * BN_BYTES * 2 + 2); + char *buf = OPENSSL_malloc(1 /* leading '-' */ + 1 /* zero is non-empty */ + + bn->top * BN_BYTES * 2 + 1 /* trailing NUL */); if (buf == NULL) { OPENSSL_PUT_ERROR(BN, ERR_R_MALLOC_FAILURE); return NULL; } - p = buf; + char *p = buf; if (bn->neg) { *(p++) = '-'; } @@ -223,10 +220,11 @@ char *BN_bn2hex(const BIGNUM *bn) { *(p++) = '0'; } - for (i = bn->top - 1; i >= 0; i--) { - for (j = BN_BITS2 - 8; j >= 0; j -= 8) { + int z = 0; + for (int i = bn->top - 1; i >= 0; i--) { + for (int j = BN_BITS2 - 8; j >= 0; j -= 8) { /* strip leading zeros */ - v = ((int)(bn->d[i] >> (long)j)) & 0xff; + int v = ((int)(bn->d[i] >> (long)j)) & 0xff; if (z || v != 0) { *(p++) = hextable[v >> 4]; *(p++) = hextable[v & 0x0f]; @@ -399,14 +397,15 @@ char *BN_bn2dec(const BIGNUM *a) { #define BUF_REMAIN (num + 3 - (size_t)(p - buf)) p = buf; lp = bn_data; + + if (BN_is_negative(t)) { + *p++ = '-'; + } + if (BN_is_zero(t)) { *(p++) = '0'; *(p++) = '\0'; } else { - if (BN_is_negative(t)) { - *p++ = '-'; - } - while (!BN_is_zero(t)) { *lp = BN_div_word(t, BN_DEC_CONV); lp++;