Fix d2i_*_bio on partial reads.

If BIO_read returns partial reads, d2i_*_bio currently fails. This is a
partial (hah) regression from 419144adce049b5341bd94d355c52d099eac56e3.
The old a_d2i_fp.c code did *not* tolerate partial reads in the ASN.1
header, but it *did* tolerate them in the ASN.1 body. Since partial
reads are more likely to land in the body than the header, I think we
can say d2i_*_bio was "supposed to" tolerate this but had a bug in the
first few bytes.

Fix it for both cases. Add a regression test for this and the partial
write case (which works fine).

See also https://github.com/google/conscrypt/pull/587.

Change-Id: I886f6388f0b80621960e196cf2a56f5c02a14a04
Reviewed-on: https://boringssl-review.googlesource.com/c/33484
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>

crypto/bio/bio.c

diff --git a/crypto/bio/bio.c b/crypto/bio/bio.c
index fe40578..7d97c3e 100644
--- a/crypto/bio/bio.c
+++ b/crypto/bio/bio.c
@@ -482,6 +482,31 @@
   }
 }
 
+// bio_read_full reads |len| bytes |bio| and writes them into |out|. It
+// tolerates partial reads from |bio| and returns one on success or zero if a
+// read fails before |len| bytes are read. On failure, it additionally sets
+// |*out_eof_on_first_read| to whether the error was due to |bio| returning zero
+// on the first read. |out_eof_on_first_read| may be NULL to discard the value.
+static int bio_read_full(BIO *bio, uint8_t *out, int *out_eof_on_first_read,
+                         size_t len) {
+  int first_read = 1;
+  while (len > 0) {
+    int todo = len <= INT_MAX ? (int)len : INT_MAX;
+    int ret = BIO_read(bio, out, todo);
+    if (ret <= 0) {
+      if (out_eof_on_first_read != NULL) {
+        *out_eof_on_first_read = first_read && ret == 0;
+      }
+      return 0;
+    }
+    out += ret;
+    len -= (size_t)ret;
+    first_read = 0;
+  }
+
+  return 1;
+}
+
 // For compatibility with existing |d2i_*_bio| callers, |BIO_read_asn1| uses
 // |ERR_LIB_ASN1| errors.
 OPENSSL_DECLARE_ERROR_REASON(ASN1, ASN1_R_DECODE_ERROR)
@@ -493,17 +518,16 @@
   uint8_t header[6];
 
   static const size_t kInitialHeaderLen = 2;
-  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);
+  int eof_on_first_read;
+  if (!bio_read_full(bio, header, &eof_on_first_read, kInitialHeaderLen)) {
+    if (eof_on_first_read) {
+      // 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);
+    } else {
+      OPENSSL_PUT_ERROR(ASN1, ASN1_R_NOT_ENOUGH_DATA);
+    }
     return 0;
   }
 
@@ -539,8 +563,7 @@
       return 0;
     }
 
-    if (BIO_read(bio, header + kInitialHeaderLen, num_bytes) !=
-        (int)num_bytes) {
+    if (!bio_read_full(bio, header + kInitialHeaderLen, NULL, num_bytes)) {
       OPENSSL_PUT_ERROR(ASN1, ASN1_R_NOT_ENOUGH_DATA);
       return 0;
     }
@@ -582,8 +605,7 @@
     return 0;
   }
   OPENSSL_memcpy(*out, header, header_len);
-  if (BIO_read(bio, (*out) + header_len, len - header_len) !=
-      (int) (len - header_len)) {
+  if (!bio_read_full(bio, (*out) + header_len, NULL, len - header_len)) {
     OPENSSL_PUT_ERROR(ASN1, ASN1_R_NOT_ENOUGH_DATA);
     OPENSSL_free(*out);
     return 0;

crypto/x509/x509_test.cc

diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index c42a7c8..a53ed7a 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -12,6 +12,7 @@
  * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
  * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */
 
+#include <algorithm>
 #include <functional>
 #include <string>
 #include <vector>
@@ -1684,3 +1685,62 @@
   EXPECT_EQ(ERR_LIB_ASN1, ERR_GET_LIB(err));
   EXPECT_EQ(ASN1_R_HEADER_TOO_LONG, ERR_GET_REASON(err));
 }
+
+TEST(X509Test, ReadBIOOneByte) {
+  bssl::UniquePtr<BIO> bio(BIO_new_mem_buf("\x30", 1));
+  ASSERT_TRUE(bio);
+
+  // CPython expects |ASN1_R_HEADER_TOO_LONG| on EOF, to terminate a series of
+  // certificates. This EOF appeared after some data, however, so we do not wish
+  // to signal EOF.
+  bssl::UniquePtr<X509> 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_NOT_ENOUGH_DATA, ERR_GET_REASON(err));
+}
+
+TEST(X509Test, PartialBIOReturn) {
+  // Create a filter BIO that only reads and writes one byte at a time.
+  bssl::UniquePtr<BIO_METHOD> method(BIO_meth_new(0, nullptr));
+  ASSERT_TRUE(method);
+  ASSERT_TRUE(BIO_meth_set_create(method.get(), [](BIO *b) -> int {
+    BIO_set_init(b, 1);
+    return 1;
+  }));
+  ASSERT_TRUE(
+      BIO_meth_set_read(method.get(), [](BIO *b, char *out, int len) -> int {
+        return BIO_read(BIO_next(b), out, std::min(len, 1));
+      }));
+  ASSERT_TRUE(BIO_meth_set_write(
+      method.get(), [](BIO *b, const char *in, int len) -> int {
+        return BIO_write(BIO_next(b), in, std::min(len, 1));
+      }));
+
+  bssl::UniquePtr<BIO> bio(BIO_new(method.get()));
+  ASSERT_TRUE(bio);
+  BIO *mem_bio = BIO_new(BIO_s_mem());
+  ASSERT_TRUE(mem_bio);
+  BIO_push(bio.get(), mem_bio);  // BIO_push takes ownership.
+
+  bssl::UniquePtr<X509> cert(CertFromPEM(kLeafPEM));
+  ASSERT_TRUE(cert);
+  uint8_t *der = nullptr;
+  int der_len = i2d_X509(cert.get(), &der);
+  ASSERT_GT(der_len, 0);
+  bssl::UniquePtr<uint8_t> free_der(der);
+
+  // Write the certificate into the BIO. Though we only write one byte at a
+  // time, the write should succeed.
+  ASSERT_EQ(1, i2d_X509_bio(bio.get(), cert.get()));
+  const uint8_t *der2;
+  size_t der2_len;
+  ASSERT_TRUE(BIO_mem_contents(mem_bio, &der2, &der2_len));
+  EXPECT_EQ(Bytes(der, static_cast<size_t>(der_len)), Bytes(der2, der2_len));
+
+  // Read the certificate back out of the BIO. Though we only read one byte at a
+  // time, the read should succeed.
+  bssl::UniquePtr<X509> cert2(d2i_X509_bio(bio.get(), nullptr));
+  ASSERT_TRUE(cert2);
+  EXPECT_EQ(0, X509_cmp(cert.get(), cert2.get()));
+}

include/openssl/bio.h

diff --git a/include/openssl/bio.h b/include/openssl/bio.h
index 8e2db65..da0dcdf 100644
--- a/include/openssl/bio.h
+++ b/include/openssl/bio.h
@@ -904,6 +904,7 @@
 
 BORINGSSL_MAKE_DELETER(BIO, BIO_free)
 BORINGSSL_MAKE_UP_REF(BIO, BIO_up_ref)
+BORINGSSL_MAKE_DELETER(BIO_METHOD, BIO_meth_free)
 
 BSSL_NAMESPACE_END