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>
diff --git a/crypto/bio/bio_test.cc b/crypto/bio/bio_test.cc
index 7115b98..65324d0 100644
--- a/crypto/bio/bio_test.cc
+++ b/crypto/bio/bio_test.cc
@@ -645,19 +645,25 @@
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 @@
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> {};
diff --git a/crypto/bio/file.c b/crypto/bio/file.c
index dd11e50..9b2a6ca 100644
--- a/crypto/bio/file.c
+++ b/crypto/bio/file.c
@@ -206,16 +206,16 @@
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;
diff --git a/crypto/x509/by_file.c b/crypto/x509/by_file.c
index 799e16d..989e179 100644
--- a/crypto/x509/by_file.c
+++ b/crypto/x509/by_file.c
@@ -228,7 +228,7 @@
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;
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 61bc54d..b25e8b3 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -7977,14 +7977,6 @@
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.
diff --git a/include/openssl/bio.h b/include/openssl/bio.h
index 33b020b..93f3c0c 100644
--- a/include/openssl/bio.h
+++ b/include/openssl/bio.h
@@ -492,22 +492,26 @@
// 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
diff --git a/ssl/ssl_file.cc b/ssl/ssl_file.cc
index 2b596b2..301f8d6 100644
--- a/ssl/ssl_file.cc
+++ b/ssl/ssl_file.cc
@@ -204,7 +204,7 @@
}
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 @@
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;
}
