Consistently open files in binary mode on Windows

BIO_*_filename, in upstream OpenSSL, opens in binary mode on Windows,
not text mode. We seem to have lost those ifdefs in the fork. But since
C mandates the 'b' suffix (POSIX just ignores it), apply it consistently
to all OSes for simplicity.

This fixes X509_FILETYPE_ASN1 in X509_STORE's file-based machinery on
Windows.

Also fix the various BIO_new_file calls to all specify binary mode.
Looking through them, I don't think any of them care (they're all
parsing PEM), but let's just apply it across the board so we don't have
to think about this.

Update-Note: BIO_read_filename, etc., now open in binary mode on
Windows. This matches OpenSSL behavior.

Change-Id: I7e555085d5c66ad2f205b476d0317570075bbadb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66009
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>

crypto/bio/bio_test.cc

@@ -645,19 +645,25 @@ TEST(BIOTest, Gets) {
         SCOPED_TRACE("file");
 
         // Test |BIO_new_file|.
-        bssl::UniquePtr<BIO> bio(BIO_new_file(file.path().c_str(), "r"));
+        bssl::UniquePtr<BIO> bio(BIO_new_file(file.path().c_str(), "rb"));
         ASSERT_TRUE(bio);
         check_bio_gets(bio.get());
 
+        // Test |BIO_read_filename|.
+        bio.reset(BIO_new(BIO_s_file()));
+        ASSERT_TRUE(bio);
+        ASSERT_TRUE(BIO_read_filename(bio.get(), file.path().c_str()));
+        check_bio_gets(bio.get());
+
         // Test |BIO_NOCLOSE|.
-        ScopedFILE file_obj = file.Open("r");
+        ScopedFILE file_obj = file.Open("rb");
         ASSERT_TRUE(file_obj);
         bio.reset(BIO_new_fp(file_obj.get(), BIO_NOCLOSE));
         ASSERT_TRUE(bio);
         check_bio_gets(bio.get());
 
         // Test |BIO_CLOSE|.
-        file_obj = file.Open("r");
+        file_obj = file.Open("rb");
         ASSERT_TRUE(file_obj);
         bio.reset(BIO_new_fp(file_obj.get(), BIO_CLOSE));
         ASSERT_TRUE(bio);
@@ -700,6 +706,24 @@ TEST(BIOTest, Gets) {
   EXPECT_EQ(c, 'a');
 }
 
+// Test that, on Windows, |BIO_read_filename| opens files in binary mode.
+TEST(BIOTest, BinaryMode) {
+  if (SkipTempFileTests()) {
+    GTEST_SKIP();
+  }
+
+  TemporaryFile file;
+  ASSERT_TRUE(file.Init("\r\n"));
+
+  // Reading from the file should give back the exact bytes we put in.
+  bssl::UniquePtr<BIO> bio(BIO_new(BIO_s_file()));
+  ASSERT_TRUE(bio);
+  ASSERT_TRUE(BIO_read_filename(bio.get(), file.path().c_str()));
+  char buf[2];
+  ASSERT_EQ(2, BIO_read(bio.get(), buf, 2));
+  EXPECT_EQ(Bytes(buf, 2), Bytes("\r\n"));
+}
+
 // Run through the tests twice, swapping |bio1| and |bio2|, for symmetry.
 class BIOPairTest : public testing::TestWithParam<bool> {};
 

crypto/bio/file.c

@@ -206,16 +206,16 @@ static long file_ctrl(BIO *b, int cmd, long num, void *ptr) {
       const char *mode;
       if (num & BIO_FP_APPEND) {
         if (num & BIO_FP_READ) {
-          mode = "a+";
+          mode = "ab+";
         } else {
-          mode = "a";
+          mode = "ab";
         }
       } else if ((num & BIO_FP_READ) && (num & BIO_FP_WRITE)) {
-        mode = "r+";
+        mode = "rb+";
       } else if (num & BIO_FP_WRITE) {
-        mode = "w";
+        mode = "wb";
       } else if (num & BIO_FP_READ) {
-        mode = "r";
+        mode = "rb";
       } else {
         OPENSSL_PUT_ERROR(BIO, BIO_R_BAD_FOPEN_MODE);
         ret = 0;

crypto/x509/by_file.c

@@ -228,7 +228,7 @@ int X509_load_cert_crl_file(X509_LOOKUP *ctx, const char *file, int type) {
   if (type != X509_FILETYPE_PEM) {
     return X509_load_cert_file(ctx, file, type);
   }
-  in = BIO_new_file(file, "r");
+  in = BIO_new_file(file, "rb");
   if (!in) {
     OPENSSL_PUT_ERROR(X509, ERR_R_SYS_LIB);
     return 0;

crypto/x509/x509_test.cc

@@ -7977,14 +7977,6 @@ TEST(X509Test, DirHash) {
   for (int type : {X509_FILETYPE_PEM, X509_FILETYPE_ASN1}) {
     SCOPED_TRACE(type);
 
-#if defined(OPENSSL_WINDOWS)
-    // TODO(davidben): X509_FILETYPE_ASN1 is currently broken on Windows due to
-    // some text vs binary mixup.
-    if (type == X509_FILETYPE_ASN1) {
-      continue;
-    }
-#endif
-
     // Generate some roots and fill a directory with OpenSSL's directory hash
     // format. The hash depends only on the name, so we do not need to
     // pre-generate the certificates. Test both DER and PEM.

include/openssl/bio.h

@@ -492,22 +492,26 @@ OPENSSL_EXPORT int BIO_set_fp(BIO *bio, FILE *file, int close_flag);
 
 // BIO_read_filename opens |filename| for reading and sets the result as the
 // |FILE| for |bio|. It returns one on success and zero otherwise. The |FILE|
-// will be closed when |bio| is freed.
+// will be closed when |bio| is freed. On Windows, the file is opened in binary
+// mode.
 OPENSSL_EXPORT int BIO_read_filename(BIO *bio, const char *filename);
 
 // BIO_write_filename opens |filename| for writing and sets the result as the
 // |FILE| for |bio|. It returns one on success and zero otherwise. The |FILE|
-// will be closed when |bio| is freed.
+// will be closed when |bio| is freed. On Windows, the file is opened in binary
+// mode.
 OPENSSL_EXPORT int BIO_write_filename(BIO *bio, const char *filename);
 
 // BIO_append_filename opens |filename| for appending and sets the result as
 // the |FILE| for |bio|. It returns one on success and zero otherwise. The
-// |FILE| will be closed when |bio| is freed.
+// |FILE| will be closed when |bio| is freed. On Windows, the file is opened in
+// binary mode.
 OPENSSL_EXPORT int BIO_append_filename(BIO *bio, const char *filename);
 
 // BIO_rw_filename opens |filename| for reading and writing and sets the result
 // as the |FILE| for |bio|. It returns one on success and zero otherwise. The
-// |FILE| will be closed when |bio| is freed.
+// |FILE| will be closed when |bio| is freed. On Windows, the file is opened in
+// binary mode.
 OPENSSL_EXPORT int BIO_rw_filename(BIO *bio, const char *filename);
 
 // BIO_tell returns the file offset of |bio|, or a negative number on error or

ssl/ssl_file.cc

@@ -204,7 +204,7 @@ int SSL_add_bio_cert_subjects_to_stack(STACK_OF(X509_NAME) *out, BIO *bio) {
 }
 
 STACK_OF(X509_NAME) *SSL_load_client_CA_file(const char *file) {
-  bssl::UniquePtr<BIO> in(BIO_new_file(file, "r"));
+  bssl::UniquePtr<BIO> in(BIO_new_file(file, "rb"));
   if (in == nullptr) {
     return nullptr;
   }
@@ -219,7 +219,7 @@ STACK_OF(X509_NAME) *SSL_load_client_CA_file(const char *file) {
 
 int SSL_add_file_cert_subjects_to_stack(STACK_OF(X509_NAME) *out,
                                         const char *file) {
-  bssl::UniquePtr<BIO> in(BIO_new_file(file, "r"));
+  bssl::UniquePtr<BIO> in(BIO_new_file(file, "rb"));
   if (in == nullptr) {
     return 0;
   }