Issue metadata
Sign in to add a comment
|
BoringSSL: off by one error in BN_bn2dec |
||||||||||||||||||||
Issue descriptionIn my code, X509_print started to fail after the following change: https://boringssl.googlesource.com/boringssl/+/958aaf1ea1b481e8ef32970d5b0add80504be4b2 I think there seems to be off by one error in the code. Although the code allocate bn_data_num * sizeof(BN_ULONG) size of memory, it cannot use the last one BN_ULONG size data. As a result, the function seems to return NULL in my case. diff --git a/crypto/bn/convert.c b/crypto/bn/convert.c index 38648c6..ed2e3b2 100644 --- a/crypto/bn/convert.c +++ b/crypto/bn/convert.c @@ -414,7 +414,7 @@ char *BN_bn2dec(const BIGNUM *a) { goto err; } lp++; - if (lp - bn_data >= bn_data_num) { + if (lp - bn_data > bn_data_num) { goto err; } } As far as I have investigated, crypto/x509v3/v3_cpols.c might not check data returned by i2s_ASN1_INTEGER, and that seems to call BIO_put with null data. I suppose following hack might also needed. diff --git a/crypto/x509v3/v3_cpols.c b/crypto/x509v3/v3_cpols.c index d67dcb0..9d9b556 100644 --- a/crypto/x509v3/v3_cpols.c +++ b/crypto/x509v3/v3_cpols.c @@ -469,6 +469,8 @@ static void print_notice(BIO *out, USERNOTICE *notice, int indent) if (i) BIO_puts(out, ", "); tmp = i2s_ASN1_INTEGER(NULL, num); + if (tmp == NULL) + continue BIO_puts(out, tmp); OPENSSL_free(tmp); } Note that the patches are for commit 958aaf1ea1b481e8ef32970d5b0add80504be4b2.
,
Aug 22 2016
Yeah, looks like the same issue came up upstream. Will import. https://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=099e2968ed3c7d256cda048995626664082b1b30
,
Aug 23 2016
,
Aug 23 2016
The following revision refers to this bug: https://boringssl.googlesource.com/boringssl.git/+/7c040756178e14a4d181b6d93abb3827c93189c4 commit 7c040756178e14a4d181b6d93abb3827c93189c4 Author: David Benjamin <davidben@google.com> Date: Tue Aug 23 05:19:01 2016 Rewrite BN_bn2dec. 958aaf1ea1b481e8ef32970d5b0add80504be4b2, imported from upstream, had an off-by-one error. Reproducing the failure is fairly easy as it can't even serialize 1. See also upstream's 099e2968ed3c7d256cda048995626664082b1b30. Rewrite the function completely with CBB and add a basic test. BUG= chromium:639740 Change-Id: I41a91514c4bb9e83854824ed5258ffe4e49d9491 Reviewed-on: https://boringssl-review.googlesource.com/10540 Commit-Queue: David Benjamin <davidben@google.com> Commit-Queue: Adam Langley <agl@google.com> Reviewed-by: Adam Langley <agl@google.com> CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> [modify] https://crrev.com/7c040756178e14a4d181b6d93abb3827c93189c4/crypto/bn/bn_test.cc [modify] https://crrev.com/7c040756178e14a4d181b6d93abb3827c93189c4/crypto/bn/convert.c
,
Aug 23 2016
The following revision refers to this bug: https://boringssl.googlesource.com/boringssl.git/+/7c040756178e14a4d181b6d93abb3827c93189c4 commit 7c040756178e14a4d181b6d93abb3827c93189c4 Author: David Benjamin <davidben@google.com> Date: Tue Aug 23 05:19:01 2016 Rewrite BN_bn2dec. 958aaf1ea1b481e8ef32970d5b0add80504be4b2, imported from upstream, had an off-by-one error. Reproducing the failure is fairly easy as it can't even serialize 1. See also upstream's 099e2968ed3c7d256cda048995626664082b1b30. Rewrite the function completely with CBB and add a basic test. BUG= chromium:639740 Change-Id: I41a91514c4bb9e83854824ed5258ffe4e49d9491 Reviewed-on: https://boringssl-review.googlesource.com/10540 Commit-Queue: David Benjamin <davidben@google.com> Commit-Queue: Adam Langley <agl@google.com> Reviewed-by: Adam Langley <agl@google.com> CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> [modify] https://crrev.com/7c040756178e14a4d181b6d93abb3827c93189c4/crypto/bn/bn_test.cc [modify] https://crrev.com/7c040756178e14a4d181b6d93abb3827c93189c4/crypto/bn/convert.c
,
Aug 25 2016
|
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by yyanagisawa@chromium.org
, Aug 22 2016Oops, my patch in #0 causes off by one. We must always check lp position before assignment. diff --git a/crypto/bn/convert.c b/crypto/bn/convert.c index 38648c6..0db604a 100644 --- a/crypto/bn/convert.c +++ b/crypto/bn/convert.c @@ -409,14 +409,14 @@ char *BN_bn2dec(const BIGNUM *a) { *(p++) = '\0'; } else { while (!BN_is_zero(t)) { + if (lp - bn_data >= bn_data_num) { + goto err; + } *lp = BN_div_word(t, BN_DEC_CONV); if (*lp == (BN_ULONG)-1) { goto err; } lp++; - if (lp - bn_data >= bn_data_num) { - goto err; - } } lp--; /* We now have a series of blocks, BN_DEC_NUM chars