From 80aa6949756d327476750f9ea2c9700aa2a027c5 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 11 Oct 2018 12:40:59 -0500 Subject: [PATCH] Always push errors on BIO_read_asn1 failure. This is consistent with the old behavior of d2i_*_fp and avoids tripping Conscrypt's unnecessarily fragile error-handling (see https://github.com/google/conscrypt/pull/552). Additionally, by source inspection, CPython expects ASN1_R_HEADER_TOO_LONG on EOF, analogously to PEM_R_NO_START_LINE. Fix that. The other errors are a bit haphazard in the old implementation (that code is really hard to follow), so I didn't match it too carefully. In particular, OpenSSL would report ASN1_R_HEADER_TOO_LONG on some generic tag parsing, but that is inconsistent with ASN1_R_HEADER_TOO_LONG being an EOF signal. Update-Note: https://boringssl-review.googlesource.com/32106 may have caused some compatibility issues. This should fix it. Change-Id: Idfe2746ffd7733de4338e14c58a40753e98a791e Reviewed-on: https://boringssl-review.googlesource.com/c/32444 Reviewed-by: Steven Valdez Commit-Queue: David Benjamin CQ-Verified: CQ bot account: commit-bot@chromium.org --- crypto/bio/bio.c | 39 ++++++++++++++++++++++++++++++++++----- crypto/x509/x509_test.cc | 13 +++++++++++++ 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/crypto/bio/bio.c b/crypto/bio/bio.c index 881c14e1..fe40578b 100644 --- a/crypto/bio/bio.c +++ b/crypto/bio/bio.c @@ -61,6 +61,7 @@ #include #include +#include #include #include #include @@ -481,11 +482,28 @@ static int bio_read_all(BIO *bio, uint8_t **out, size_t *out_len, } } +// For compatibility with existing |d2i_*_bio| callers, |BIO_read_asn1| uses +// |ERR_LIB_ASN1| errors. +OPENSSL_DECLARE_ERROR_REASON(ASN1, ASN1_R_DECODE_ERROR) +OPENSSL_DECLARE_ERROR_REASON(ASN1, ASN1_R_HEADER_TOO_LONG) +OPENSSL_DECLARE_ERROR_REASON(ASN1, ASN1_R_NOT_ENOUGH_DATA) +OPENSSL_DECLARE_ERROR_REASON(ASN1, ASN1_R_TOO_LONG) + int BIO_read_asn1(BIO *bio, uint8_t **out, size_t *out_len, size_t max_len) { uint8_t header[6]; static const size_t kInitialHeaderLen = 2; - if (BIO_read(bio, header, kInitialHeaderLen) != (int) kInitialHeaderLen) { + int ret = BIO_read(bio, header, kInitialHeaderLen); + if (ret == 0) { + // Historically, OpenSSL returned |ASN1_R_HEADER_TOO_LONG| when |d2i_*_bio| + // could not read anything. CPython conditions on this to determine if |bio| + // was empty. + OPENSSL_PUT_ERROR(ASN1, ASN1_R_HEADER_TOO_LONG); + return 0; + } + + if (ret != (int) kInitialHeaderLen) { + OPENSSL_PUT_ERROR(ASN1, ASN1_R_NOT_ENOUGH_DATA); return 0; } @@ -494,6 +512,7 @@ int BIO_read_asn1(BIO *bio, uint8_t **out, size_t *out_len, size_t max_len) { if ((tag & 0x1f) == 0x1f) { // Long form tags are not supported. + OPENSSL_PUT_ERROR(ASN1, ASN1_R_DECODE_ERROR); return 0; } @@ -507,34 +526,41 @@ int BIO_read_asn1(BIO *bio, uint8_t **out, size_t *out_len, size_t max_len) { if ((tag & 0x20 /* constructed */) != 0 && num_bytes == 0) { // indefinite length. - return bio_read_all(bio, out, out_len, header, kInitialHeaderLen, - max_len); + if (!bio_read_all(bio, out, out_len, header, kInitialHeaderLen, + max_len)) { + OPENSSL_PUT_ERROR(ASN1, ASN1_R_NOT_ENOUGH_DATA); + return 0; + } + return 1; } if (num_bytes == 0 || num_bytes > 4) { + OPENSSL_PUT_ERROR(ASN1, ASN1_R_DECODE_ERROR); return 0; } if (BIO_read(bio, header + kInitialHeaderLen, num_bytes) != (int)num_bytes) { + OPENSSL_PUT_ERROR(ASN1, ASN1_R_NOT_ENOUGH_DATA); return 0; } header_len = kInitialHeaderLen + num_bytes; uint32_t len32 = 0; - unsigned i; - for (i = 0; i < num_bytes; i++) { + for (unsigned i = 0; i < num_bytes; i++) { len32 <<= 8; len32 |= header[kInitialHeaderLen + i]; } if (len32 < 128) { // Length should have used short-form encoding. + OPENSSL_PUT_ERROR(ASN1, ASN1_R_DECODE_ERROR); return 0; } if ((len32 >> ((num_bytes-1)*8)) == 0) { // Length should have been at least one byte shorter. + OPENSSL_PUT_ERROR(ASN1, ASN1_R_DECODE_ERROR); return 0; } @@ -544,6 +570,7 @@ int BIO_read_asn1(BIO *bio, uint8_t **out, size_t *out_len, size_t max_len) { if (len + header_len < len || len + header_len > max_len || len > INT_MAX) { + OPENSSL_PUT_ERROR(ASN1, ASN1_R_TOO_LONG); return 0; } len += header_len; @@ -551,11 +578,13 @@ int BIO_read_asn1(BIO *bio, uint8_t **out, size_t *out_len, size_t max_len) { *out = OPENSSL_malloc(len); if (*out == NULL) { + OPENSSL_PUT_ERROR(ASN1, ERR_R_MALLOC_FAILURE); return 0; } OPENSSL_memcpy(*out, header, header_len); if (BIO_read(bio, (*out) + header_len, len - header_len) != (int) (len - header_len)) { + OPENSSL_PUT_ERROR(ASN1, ASN1_R_NOT_ENOUGH_DATA); OPENSSL_free(*out); return 0; } diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index bf0b29a2..c42a7c82 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -1671,3 +1671,16 @@ TEST(X509Test, PEMX509Info) { PEM_X509_INFO_read_bio(bio.get(), infos.get(), nullptr, nullptr)); EXPECT_EQ(2 * OPENSSL_ARRAY_SIZE(kExpected), sk_X509_INFO_num(infos.get())); } + +TEST(X509Test, ReadBIOEmpty) { + bssl::UniquePtr bio(BIO_new_mem_buf(nullptr, 0)); + ASSERT_TRUE(bio); + + // CPython expects |ASN1_R_HEADER_TOO_LONG| on EOF, to terminate a series of + // certificates. + bssl::UniquePtr x509(d2i_X509_bio(bio.get(), nullptr)); + EXPECT_FALSE(x509); + uint32_t err = ERR_get_error(); + EXPECT_EQ(ERR_LIB_ASN1, ERR_GET_LIB(err)); + EXPECT_EQ(ASN1_R_HEADER_TOO_LONG, ERR_GET_REASON(err)); +}