Ensure BN_asc2bn, BN_dec2bn, and BN_hex2bn never give -0. See upstream's a0eed48d37a4b7beea0c966caf09ad46f4a92a44. Rather than import that, we should just ensure neg + zero isn't a possible state. Add some tests for asc2bn and dec2bn while we're here. Also fix a bug with dec2bn where it doesn't actually ignore trailing data as it's supposed to. Change-Id: I2385b67b740e57020c75a247bee254085ab7ce15 Reviewed-on: https://boringssl-review.googlesource.com/4484 Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/asn1/a_int.c b/crypto/asn1/a_int.c index eb0887a..2ecccc5 100644 --- a/crypto/asn1/a_int.c +++ b/crypto/asn1/a_int.c
@@ -416,7 +416,7 @@ OPENSSL_PUT_ERROR(ASN1, BN_to_ASN1_INTEGER, ASN1_R_NESTED_ASN1_ERROR); goto err; } - if (BN_is_negative(bn)) + if (BN_is_negative(bn) && !BN_is_zero(bn)) ret->type = V_ASN1_NEG_INTEGER; else ret->type=V_ASN1_INTEGER; j=BN_num_bits(bn);
diff --git a/crypto/bn/bn_test.cc b/crypto/bn/bn_test.cc index 20d15a1..43fd775 100644 --- a/crypto/bn/bn_test.cc +++ b/crypto/bn/bn_test.cc
@@ -107,6 +107,9 @@ static bool test_mod_exp_mont5(FILE *fp, BN_CTX *ctx); static bool test_sqrt(FILE *fp, BN_CTX *ctx); static bool test_bn2bin_padded(FILE *fp, BN_CTX *ctx); +static bool test_dec2bn(FILE *fp, BN_CTX *ctx); +static bool test_hex2bn(FILE *fp, BN_CTX *ctx); +static bool test_asc2bn(FILE *fp, BN_CTX *ctx); // g_results can be set to true to cause the result of each computation to be // printed. @@ -283,6 +286,24 @@ } fflush(stdout); + message(stdout, "BN_dec2bn"); + if (!test_dec2bn(stdout, ctx.get())) { + return 1; + } + fflush(stdout); + + message(stdout, "BN_hex2bn"); + if (!test_hex2bn(stdout, ctx.get())) { + return 1; + } + fflush(stdout); + + message(stdout, "BN_asc2bn"); + if (!test_asc2bn(stdout, ctx.get())) { + return 1; + } + fflush(stdout); + printf("PASS\n"); return 0; } @@ -1406,3 +1427,147 @@ return true; } + +static int DecimalToBIGNUM(ScopedBIGNUM *out, const char *in) { + BIGNUM *raw = NULL; + int ret = BN_dec2bn(&raw, in); + out->reset(raw); + return ret; +} + +static bool test_dec2bn(FILE *fp, BN_CTX *ctx) { + ScopedBIGNUM bn; + int ret = DecimalToBIGNUM(&bn, "0"); + if (ret != 1 || !BN_is_zero(bn.get()) || BN_is_negative(bn.get())) { + fprintf(stderr, "BN_dec2bn gave a bad result.\n"); + return false; + } + + ret = DecimalToBIGNUM(&bn, "256"); + if (ret != 3 || !BN_is_word(bn.get(), 256) || BN_is_negative(bn.get())) { + fprintf(stderr, "BN_dec2bn gave a bad result.\n"); + return false; + } + + ret = DecimalToBIGNUM(&bn, "-42"); + if (ret != 3 || !BN_abs_is_word(bn.get(), 42) || !BN_is_negative(bn.get())) { + fprintf(stderr, "BN_dec2bn gave a bad result.\n"); + return false; + } + + ret = DecimalToBIGNUM(&bn, "-0"); + if (ret != 2 || !BN_is_zero(bn.get()) || BN_is_negative(bn.get())) { + fprintf(stderr, "BN_dec2bn gave a bad result.\n"); + return false; + } + + ret = DecimalToBIGNUM(&bn, "42trailing garbage is ignored"); + if (ret != 2 || !BN_abs_is_word(bn.get(), 42) || BN_is_negative(bn.get())) { + fprintf(stderr, "BN_dec2bn gave a bad result.\n"); + return false; + } + + return true; +} + +static int HexToBIGNUM(ScopedBIGNUM *out, const char *in) { + BIGNUM *raw = NULL; + int ret = BN_hex2bn(&raw, in); + out->reset(raw); + return ret; +} + +static bool test_hex2bn(FILE *fp, BN_CTX *ctx) { + ScopedBIGNUM bn; + int ret = HexToBIGNUM(&bn, "0"); + if (ret != 1 || !BN_is_zero(bn.get()) || BN_is_negative(bn.get())) { + fprintf(stderr, "BN_hex2bn gave a bad result.\n"); + return false; + } + + ret = HexToBIGNUM(&bn, "256"); + if (ret != 3 || !BN_is_word(bn.get(), 0x256) || BN_is_negative(bn.get())) { + fprintf(stderr, "BN_hex2bn gave a bad result.\n"); + return false; + } + + ret = HexToBIGNUM(&bn, "-42"); + if (ret != 3 || !BN_abs_is_word(bn.get(), 0x42) || !BN_is_negative(bn.get())) { + fprintf(stderr, "BN_hex2bn gave a bad result.\n"); + return false; + } + + ret = HexToBIGNUM(&bn, "-0"); + if (ret != 2 || !BN_is_zero(bn.get()) || BN_is_negative(bn.get())) { + fprintf(stderr, "BN_hex2bn gave a bad result.\n"); + return false; + } + + ret = HexToBIGNUM(&bn, "abctrailing garbage is ignored"); + if (ret != 3 || !BN_is_word(bn.get(), 0xabc) || BN_is_negative(bn.get())) { + fprintf(stderr, "BN_hex2bn gave a bad result.\n"); + return false; + } + + return true; +} + +static ScopedBIGNUM ASCIIToBIGNUM(const char *in) { + BIGNUM *raw = NULL; + if (!BN_asc2bn(&raw, in)) { + return nullptr; + } + return ScopedBIGNUM(raw); +} + +static bool test_asc2bn(FILE *fp, BN_CTX *ctx) { + ScopedBIGNUM bn = ASCIIToBIGNUM("0"); + if (!bn || !BN_is_zero(bn.get()) || BN_is_negative(bn.get())) { + fprintf(stderr, "BN_asc2bn gave a bad result.\n"); + return false; + } + + bn = ASCIIToBIGNUM("256"); + if (!bn || !BN_is_word(bn.get(), 256) || BN_is_negative(bn.get())) { + fprintf(stderr, "BN_asc2bn gave a bad result.\n"); + return false; + } + + bn = ASCIIToBIGNUM("-42"); + if (!bn || !BN_abs_is_word(bn.get(), 42) || !BN_is_negative(bn.get())) { + fprintf(stderr, "BN_asc2bn gave a bad result.\n"); + return false; + } + + bn = ASCIIToBIGNUM("0x1234"); + if (!bn || !BN_is_word(bn.get(), 0x1234) || BN_is_negative(bn.get())) { + fprintf(stderr, "BN_asc2bn gave a bad result.\n"); + return false; + } + + bn = ASCIIToBIGNUM("0X1234"); + if (!bn || !BN_is_word(bn.get(), 0x1234) || BN_is_negative(bn.get())) { + fprintf(stderr, "BN_asc2bn gave a bad result.\n"); + return false; + } + + bn = ASCIIToBIGNUM("-0xabcd"); + if (!bn || !BN_abs_is_word(bn.get(), 0xabcd) || !BN_is_negative(bn.get())) { + fprintf(stderr, "BN_asc2bn gave a bad result.\n"); + return false; + } + + bn = ASCIIToBIGNUM("-0"); + if (!bn || !BN_is_zero(bn.get()) || BN_is_negative(bn.get())) { + fprintf(stderr, "BN_asc2bn gave a bad result.\n"); + return false; + } + + bn = ASCIIToBIGNUM("123trailing garbage is ignored"); + if (!bn || !BN_is_word(bn.get(), 123) || BN_is_negative(bn.get())) { + fprintf(stderr, "BN_asc2bn gave a bad result.\n"); + return false; + } + + return true; +}
diff --git a/crypto/bn/convert.c b/crypto/bn/convert.c index f764eed..9c7b9be 100644 --- a/crypto/bn/convert.c +++ b/crypto/bn/convert.c
@@ -263,20 +263,19 @@ bn->top = h; } -/* decode_dec decodes |i| bytes of decimal data from |in| and updates |bn|. */ -static void decode_dec(BIGNUM *bn, const char *in, int i) { - int j; +/* decode_dec decodes |in_len| bytes of decimal data from |in| and updates |bn|. */ +static void decode_dec(BIGNUM *bn, const char *in, int in_len) { + int i, j; BN_ULONG l = 0; - j = BN_DEC_NUM - (i % BN_DEC_NUM); + j = BN_DEC_NUM - (in_len % BN_DEC_NUM); if (j == BN_DEC_NUM) { j = 0; } l = 0; - while (*in) { + for (i = 0; i < in_len; i++) { l *= 10; - l += *in - '0'; - in++; + l += in[i] - '0'; if (++j == BN_DEC_NUM) { BN_mul_word(bn, BN_DEC_CONV); BN_add_word(bn, l); @@ -320,7 +319,6 @@ ret = *outp; BN_zero(ret); } - ret->neg = neg; /* i is the number of hex digests; */ if (bn_expand(ret, i * 4) == NULL) { @@ -330,6 +328,9 @@ decode(ret, in, i); bn_correct_top(ret); + if (!BN_is_zero(ret)) { + ret->neg = neg; + } *outp = ret; return num; @@ -440,7 +441,7 @@ } } - if (*orig_in == '-') { + if (*orig_in == '-' && !BN_is_zero(*outp)) { (*outp)->neg = 1; }