From 04384929a6405dded8f85597002a0235feac9e34 Mon Sep 17 00:00:00 2001 From: Anne Redulla Date: Wed, 12 Jul 2023 13:53:30 +1000 Subject: [PATCH 01/11] [ssci] Added Shipped field to READMEs This CL adds the Shipped field (and may update the License File field) in Chromium READMEs. Changes were automatically created, so if you disagree with any of them (e.g. a package is used only for testing purposes and is not shipped), comment the suggested change and why. See the LSC doc at go/lsc-chrome-metadata. Bug: b:285450740 Change-Id: I63755d8a42ea69ff6d3a4e0c21ddacd2b7ce9053 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61545 Auto-Submit: Anne Redulla Reviewed-by: David Benjamin Commit-Queue: David Benjamin (cherry picked from commit 1f786137e4045b88d26035306ef826ff4a30befd) --- third_party/fiat/README.chromium | 1 + 1 file changed, 1 insertion(+) diff --git a/third_party/fiat/README.chromium b/third_party/fiat/README.chromium index 73c5ba20b5..fe48544eb4 100644 --- a/third_party/fiat/README.chromium +++ b/third_party/fiat/README.chromium @@ -5,6 +5,7 @@ Version: git (see METADATA) License: MIT License File: LICENSE Security Critical: yes +Shipped: yes Description: See README.md and METADATA. From 380c70789ddbb5ec30118a8d4775cc0884c16df3 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 14 Jul 2023 11:53:05 -0400 Subject: [PATCH 02/11] Define _DEFAULT_SOURCE for getentropy in musl glibc and musl do not make getentropy available from unless _BSD_SOURCE (or _GNU_SOURCE) is defined. _BSD_SOURCE, in glibc, triggers a deprecation warning to use _DEFAULT_SOURCE instead. It seems _DEFAULT_SOURCE might be fairly broadly defined, but some Emscripten-based toolchain (which uses musl) didn't end up defining it for some reason. Just do it explicitly in the source file so it always works. Change-Id: I4532d4adb9f8ed55c43763ca2dd426b5fa1b4f5c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61625 Reviewed-by: Bob Beck Commit-Queue: David Benjamin Auto-Submit: David Benjamin (cherry picked from commit 2a900c16a4110e9304e4a2face2d287ac6c7975a) --- crypto/rand_extra/fuchsia.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crypto/rand_extra/fuchsia.c b/crypto/rand_extra/fuchsia.c index d4fb9797ad..99c8500563 100644 --- a/crypto/rand_extra/fuchsia.c +++ b/crypto/rand_extra/fuchsia.c @@ -12,6 +12,10 @@ * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ +#if !defined(_DEFAULT_SOURCE) +#define _DEFAULT_SOURCE // Needed for getentropy on musl and glibc +#endif + #include #include "../fipsmodule/rand/internal.h" From 790cec16cd617d5bd4b87c63ef883caa43267d3e Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 14 Jul 2023 17:27:07 -0400 Subject: [PATCH 03/11] Fix error handling in bssl_shim socket object We'd leak the socket if Connect failed in the middle. (This doesn't especially matter. The test process would just exit anyway.) Change-Id: I8e1f252781810b1d8ef3c41bd707dfebb0371e60 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61665 Reviewed-by: Bob Beck Auto-Submit: David Benjamin Commit-Queue: Bob Beck (cherry picked from commit a4f8755f8e66b77ca2230f376bc5d5d54b28544e) --- ssl/test/bssl_shim.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index 74bd50c617..c9002c95dd 100644 --- a/ssl/test/bssl_shim.cc +++ b/ssl/test/bssl_shim.cc @@ -129,8 +129,8 @@ class OwnedSocket { break; } } - closesocket(sock_); } + closesocket(sock_); } drain_on_close_ = false; From c9050cad26b5c3c0eb6047f051f38037c7d6e785 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 15 Jul 2023 02:44:54 -0400 Subject: [PATCH 04/11] Replace BIO_snprintf with snprintf within the library Our BIO_snprintf is just a thin wrapper over the libc one, and we already call it directly in other places. Just call the libc one consistently. Change-Id: Ia7daf26b9789ddcecab67118c4ec4a077aad5a22 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61685 Reviewed-by: Bob Beck Commit-Queue: Bob Beck Auto-Submit: David Benjamin (cherry picked from commit 23d6e4cce97a9b66a53fb4286341fd02d2b99e40) --- crypto/asn1/a_gentm.c | 6 +++--- crypto/asn1/a_strex.c | 14 +++++++------- crypto/asn1/a_utctm.c | 6 +++--- crypto/bio/bio_test.cc | 8 +++----- crypto/bio/connect.c | 2 +- crypto/bytestring/cbs.c | 2 +- crypto/conf/conf.c | 2 +- crypto/err/err.c | 16 ++++++++-------- crypto/fipsmodule/ec/p256-nistz_test.cc | 2 +- crypto/x509/by_dir.c | 3 +-- crypto/x509v3/v3_alt.c | 5 ++--- ssl/ssl_cipher.cc | 4 ++-- ssl/ssl_decrepit.c | 2 +- 13 files changed, 34 insertions(+), 38 deletions(-) diff --git a/crypto/asn1/a_gentm.c b/crypto/asn1/a_gentm.c index c505b6ba90..7849e3c11c 100644 --- a/crypto/asn1/a_gentm.c +++ b/crypto/asn1/a_gentm.c @@ -122,9 +122,9 @@ ASN1_GENERALIZEDTIME *ASN1_GENERALIZEDTIME_adj(ASN1_GENERALIZEDTIME *s, } char buf[16]; - BIO_snprintf(buf, sizeof(buf), "%04d%02d%02d%02d%02d%02dZ", - data.tm_year + 1900, data.tm_mon + 1, data.tm_mday, data.tm_hour, - data.tm_min, data.tm_sec); + snprintf(buf, sizeof(buf), "%04d%02d%02d%02d%02d%02dZ", data.tm_year + 1900, + data.tm_mon + 1, data.tm_mday, data.tm_hour, data.tm_min, + data.tm_sec); int free_s = 0; if (s == NULL) { diff --git a/crypto/asn1/a_strex.c b/crypto/asn1/a_strex.c index b1fd47c59c..7bcbdfb29d 100644 --- a/crypto/asn1/a_strex.c +++ b/crypto/asn1/a_strex.c @@ -88,18 +88,18 @@ static int do_esc_char(uint32_t c, unsigned long flags, char *do_quotes, char buf[16]; // Large enough for "\\W01234567". unsigned char u8 = (unsigned char)c; if (c > 0xffff) { - BIO_snprintf(buf, sizeof(buf), "\\W%08" PRIX32, c); + snprintf(buf, sizeof(buf), "\\W%08" PRIX32, c); } else if (c > 0xff) { - BIO_snprintf(buf, sizeof(buf), "\\U%04" PRIX32, c); + snprintf(buf, sizeof(buf), "\\U%04" PRIX32, c); } else if ((flags & ASN1_STRFLGS_ESC_MSB) && c > 0x7f) { - BIO_snprintf(buf, sizeof(buf), "\\%02X", c); + snprintf(buf, sizeof(buf), "\\%02X", c); } else if ((flags & ASN1_STRFLGS_ESC_CTRL) && is_control_character(c)) { - BIO_snprintf(buf, sizeof(buf), "\\%02X", c); + snprintf(buf, sizeof(buf), "\\%02X", c); } else if (flags & ASN1_STRFLGS_ESC_2253) { // See RFC 2253, sections 2.4 and 4. if (c == '\\' || c == '"') { // Quotes and backslashes are always escaped, quoted or not. - BIO_snprintf(buf, sizeof(buf), "\\%c", (int)c); + snprintf(buf, sizeof(buf), "\\%c", (int)c); } else if (c == ',' || c == '+' || c == '<' || c == '>' || c == ';' || (is_first && (c == ' ' || c == '#')) || (is_last && (c == ' '))) { @@ -110,13 +110,13 @@ static int do_esc_char(uint32_t c, unsigned long flags, char *do_quotes, } return maybe_write(out, &u8, 1) ? 1 : -1; } - BIO_snprintf(buf, sizeof(buf), "\\%c", (int)c); + snprintf(buf, sizeof(buf), "\\%c", (int)c); } else { return maybe_write(out, &u8, 1) ? 1 : -1; } } else if ((flags & ESC_FLAGS) && c == '\\') { // If any escape flags are set, also escape backslashes. - BIO_snprintf(buf, sizeof(buf), "\\%c", (int)c); + snprintf(buf, sizeof(buf), "\\%c", (int)c); } else { return maybe_write(out, &u8, 1) ? 1 : -1; } diff --git a/crypto/asn1/a_utctm.c b/crypto/asn1/a_utctm.c index 53f563a208..da1d2bab83 100644 --- a/crypto/asn1/a_utctm.c +++ b/crypto/asn1/a_utctm.c @@ -123,9 +123,9 @@ ASN1_UTCTIME *ASN1_UTCTIME_adj(ASN1_UTCTIME *s, int64_t posix_time, int offset_d } char buf[14]; - BIO_snprintf(buf, sizeof(buf), "%02d%02d%02d%02d%02d%02dZ", - data.tm_year % 100, data.tm_mon + 1, data.tm_mday, data.tm_hour, - data.tm_min, data.tm_sec); + snprintf(buf, sizeof(buf), "%02d%02d%02d%02d%02d%02dZ", data.tm_year % 100, + data.tm_mon + 1, data.tm_mday, data.tm_hour, data.tm_min, + data.tm_sec); int free_s = 0; if (s == NULL) { diff --git a/crypto/bio/bio_test.cc b/crypto/bio/bio_test.cc index 1211dc19a2..e80570d0d3 100644 --- a/crypto/bio/bio_test.cc +++ b/crypto/bio/bio_test.cc @@ -48,7 +48,7 @@ static std::string LastSocketError() { return strerror(errno); } #else static std::string LastSocketError() { char buf[DECIMAL_SIZE(int) + 1]; - BIO_snprintf(buf, sizeof(buf), "%d", WSAGetLastError()); + snprintf(buf, sizeof(buf), "%d", WSAGetLastError()); return buf; } #endif @@ -97,11 +97,9 @@ TEST(BIOTest, SocketConnect) { char hostname[80]; if (ss.ss_family == AF_INET6) { - BIO_snprintf(hostname, sizeof(hostname), "[::1]:%d", - ntohs(sin6->sin6_port)); + snprintf(hostname, sizeof(hostname), "[::1]:%d", ntohs(sin6->sin6_port)); } else if (ss.ss_family == AF_INET) { - BIO_snprintf(hostname, sizeof(hostname), "127.0.0.1:%d", - ntohs(sin->sin_port)); + snprintf(hostname, sizeof(hostname), "127.0.0.1:%d", ntohs(sin->sin_port)); } // Connect to it with a connect BIO. diff --git a/crypto/bio/connect.c b/crypto/bio/connect.c index 5b65c6f712..15626cd551 100644 --- a/crypto/bio/connect.c +++ b/crypto/bio/connect.c @@ -526,7 +526,7 @@ int BIO_set_conn_port(BIO *bio, const char *port_str) { int BIO_set_conn_int_port(BIO *bio, const int *port) { char buf[DECIMAL_SIZE(int) + 1]; - BIO_snprintf(buf, sizeof(buf), "%d", *port); + snprintf(buf, sizeof(buf), "%d", *port); return BIO_set_conn_port(bio, buf); } diff --git a/crypto/bytestring/cbs.c b/crypto/bytestring/cbs.c index 77f8922fae..fb247fd7a7 100644 --- a/crypto/bytestring/cbs.c +++ b/crypto/bytestring/cbs.c @@ -699,7 +699,7 @@ int CBS_is_unsigned_asn1_integer(const CBS *cbs) { static int add_decimal(CBB *out, uint64_t v) { char buf[DECIMAL_SIZE(uint64_t) + 1]; - BIO_snprintf(buf, sizeof(buf), "%" PRIu64, v); + snprintf(buf, sizeof(buf), "%" PRIu64, v); return CBB_add_bytes(out, (const uint8_t *)buf, strlen(buf)); } diff --git a/crypto/conf/conf.c b/crypto/conf/conf.c index 978f034d92..df38e75f34 100644 --- a/crypto/conf/conf.c +++ b/crypto/conf/conf.c @@ -574,7 +574,7 @@ static int def_load_bio(CONF *conf, BIO *in, long *out_error_line) { if (out_error_line != NULL) { *out_error_line = eline; } - BIO_snprintf(btmp, sizeof btmp, "%ld", eline); + snprintf(btmp, sizeof btmp, "%ld", eline); ERR_add_error_data(2, "line ", btmp); if (v != NULL) { diff --git a/crypto/err/err.c b/crypto/err/err.c index 547d77ca38..e6fafe7fdd 100644 --- a/crypto/err/err.c +++ b/crypto/err/err.c @@ -559,17 +559,17 @@ char *ERR_error_string_n(uint32_t packed_error, char *buf, size_t len) { char lib_buf[64], reason_buf[64]; if (lib_str == NULL) { - BIO_snprintf(lib_buf, sizeof(lib_buf), "lib(%u)", lib); + snprintf(lib_buf, sizeof(lib_buf), "lib(%u)", lib); lib_str = lib_buf; } if (reason_str == NULL) { - BIO_snprintf(reason_buf, sizeof(reason_buf), "reason(%u)", reason); - reason_str = reason_buf; - } + snprintf(reason_buf, sizeof(reason_buf), "reason(%u)", reason); + reason_str = reason_buf; + } - BIO_snprintf(buf, len, "error:%08" PRIx32 ":%s:OPENSSL_internal:%s", - packed_error, lib_str, reason_str); + snprintf(buf, len, "error:%08" PRIx32 ":%s:OPENSSL_internal:%s", packed_error, + lib_str, reason_str); if (strlen(buf) == len - 1) { // output may be truncated; make sure we always have 5 colon-separated @@ -622,8 +622,8 @@ void ERR_print_errors_cb(ERR_print_errors_callback_t callback, void *ctx) { } ERR_error_string_n(packed_error, buf, sizeof(buf)); - BIO_snprintf(buf2, sizeof(buf2), "%lu:%s:%s:%d:%s\n", thread_hash, buf, - file, line, (flags & ERR_FLAG_STRING) ? data : ""); + snprintf(buf2, sizeof(buf2), "%lu:%s:%s:%d:%s\n", thread_hash, buf, file, + line, (flags & ERR_FLAG_STRING) ? data : ""); if (callback(buf2, strlen(buf2), ctx) <= 0) { break; } diff --git a/crypto/fipsmodule/ec/p256-nistz_test.cc b/crypto/fipsmodule/ec/p256-nistz_test.cc index 6d86b05bc6..81cb923bb4 100644 --- a/crypto/fipsmodule/ec/p256-nistz_test.cc +++ b/crypto/fipsmodule/ec/p256-nistz_test.cc @@ -198,7 +198,7 @@ static std::string FieldElementToString(const BN_ULONG a[P256_LIMBS]) { std::string ret; for (size_t i = P256_LIMBS-1; i < P256_LIMBS; i--) { char buf[2 * BN_BYTES + 1]; - BIO_snprintf(buf, sizeof(buf), BN_HEX_FMT2, a[i]); + snprintf(buf, sizeof(buf), BN_HEX_FMT2, a[i]); ret += buf; } return ret; diff --git a/crypto/x509/by_dir.c b/crypto/x509/by_dir.c index 8e55edff47..0603600d38 100644 --- a/crypto/x509/by_dir.c +++ b/crypto/x509/by_dir.c @@ -312,8 +312,7 @@ static int get_cert_by_subject(X509_LOOKUP *xl, int type, X509_NAME *name, hent = NULL; } for (;;) { - BIO_snprintf(b->data, b->max, "%s/%08lx.%s%d", ent->dir, h, postfix, - k); + snprintf(b->data, b->max, "%s/%08lx.%s%d", ent->dir, h, postfix, k); #ifndef OPENSSL_NO_POSIX_IO #if defined(_WIN32) && !defined(stat) #define stat _stat diff --git a/crypto/x509v3/v3_alt.c b/crypto/x509v3/v3_alt.c index ddd112a2eb..e3c15f5b99 100644 --- a/crypto/x509v3/v3_alt.c +++ b/crypto/x509v3/v3_alt.c @@ -173,13 +173,12 @@ STACK_OF(CONF_VALUE) *i2v_GENERAL_NAME(const X509V3_EXT_METHOD *method, case GEN_IPADD: p = gen->d.ip->data; if (gen->d.ip->length == 4) { - BIO_snprintf(oline, sizeof(oline), "%d.%d.%d.%d", p[0], p[1], p[2], - p[3]); + snprintf(oline, sizeof(oline), "%d.%d.%d.%d", p[0], p[1], p[2], p[3]); } else if (gen->d.ip->length == 16) { oline[0] = 0; for (i = 0; i < 8; i++) { uint16_t v = ((uint16_t)p[0] << 8) | p[1]; - BIO_snprintf(htmp, sizeof(htmp), "%X", v); + snprintf(htmp, sizeof(htmp), "%X", v); p += 2; OPENSSL_strlcat(oline, htmp, sizeof(oline)); if (i != 7) { diff --git a/ssl/ssl_cipher.cc b/ssl/ssl_cipher.cc index 917fb12ba6..d5a2708c7e 100644 --- a/ssl/ssl_cipher.cc +++ b/ssl/ssl_cipher.cc @@ -1779,8 +1779,8 @@ const char *SSL_CIPHER_description(const SSL_CIPHER *cipher, char *buf, return "Buffer too small"; } - BIO_snprintf(buf, len, "%-23s Kx=%-8s Au=%-4s Enc=%-9s Mac=%-4s\n", - cipher->name, kx, au, enc, mac); + snprintf(buf, len, "%-23s Kx=%-8s Au=%-4s Enc=%-9s Mac=%-4s\n", cipher->name, + kx, au, enc, mac); return buf; } diff --git a/ssl/ssl_decrepit.c b/ssl/ssl_decrepit.c index 32f703e1f9..a155c0f46a 100644 --- a/ssl/ssl_decrepit.c +++ b/ssl/ssl_decrepit.c @@ -150,7 +150,7 @@ int SSL_add_dir_cert_subjects_to_stack(STACK_OF(X509_NAME) *stack, break; } - int r = BIO_snprintf(buf, sizeof(buf), "%s/%s", path, dirent->d_name); + int r = snprintf(buf, sizeof(buf), "%s/%s", path, dirent->d_name); if (r <= 0 || r >= (int)sizeof(buf) || !SSL_add_file_cert_subjects_to_stack(stack, buf)) { From 327a1ce4ff87e52ec8ddc24d13f4dc7374630c42 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 14 Jul 2023 12:45:20 -0400 Subject: [PATCH 05/11] Update RSATest.BadKey to use a plausible key size One less thing to update when we next bump the minimum. Bug: 607 Change-Id: I50bd01b14499a7ed7904a5e111c3e1df57eaa144 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61645 Reviewed-by: Bob Beck Auto-Submit: David Benjamin Commit-Queue: David Benjamin (cherry picked from commit bc80d929f87dc03bf4106f466b7d88b45d52e02f) --- crypto/rsa_extra/rsa_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crypto/rsa_extra/rsa_test.cc b/crypto/rsa_extra/rsa_test.cc index cca203093c..128a4bd072 100644 --- a/crypto/rsa_extra/rsa_test.cc +++ b/crypto/rsa_extra/rsa_test.cc @@ -528,7 +528,7 @@ TEST(RSATest, BadKey) { ASSERT_TRUE(BN_set_word(e.get(), RSA_F4)); // Generate a bad key. - ASSERT_TRUE(RSA_generate_key_ex(key.get(), 512, e.get(), nullptr)); + ASSERT_TRUE(RSA_generate_key_ex(key.get(), 2048, e.get(), nullptr)); ASSERT_TRUE(BN_add(key->p, key->p, BN_value_one())); // Bad keys are detected. From 8251160b2364bc368f1b1258838de93ce61ee430 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 17 Jul 2023 15:13:26 -0400 Subject: [PATCH 06/11] Silence -Wformat-truncation in newer GCCs https://boringssl-review.googlesource.com/c/boringssl/+/61685 had the side effect of unobscuring some snprintf calls to GCC. There are a couple instances that cannot truncate, but GCC doesn't know this because it doesn't know the bounds on struct tm. Fortunately, -Wformat-truncation, at level 1, is satisfied by checking the return value, so do that. Change-Id: Iad3ae0d51a951c10f1b706be7f6e127f0b9e6dee Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61705 Auto-Submit: David Benjamin Commit-Queue: Bob Beck Reviewed-by: Bob Beck Commit-Queue: David Benjamin (cherry picked from commit 39eee12d0da9e5e1bff77fd82309f8e7d670d9a6) --- crypto/asn1/a_gentm.c | 10 +++++++--- crypto/asn1/a_utctm.c | 10 +++++++--- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/crypto/asn1/a_gentm.c b/crypto/asn1/a_gentm.c index 7849e3c11c..c0c730d469 100644 --- a/crypto/asn1/a_gentm.c +++ b/crypto/asn1/a_gentm.c @@ -59,6 +59,7 @@ #include #include +#include #include #include @@ -122,9 +123,12 @@ ASN1_GENERALIZEDTIME *ASN1_GENERALIZEDTIME_adj(ASN1_GENERALIZEDTIME *s, } char buf[16]; - snprintf(buf, sizeof(buf), "%04d%02d%02d%02d%02d%02dZ", data.tm_year + 1900, - data.tm_mon + 1, data.tm_mday, data.tm_hour, data.tm_min, - data.tm_sec); + int ret = snprintf(buf, sizeof(buf), "%04d%02d%02d%02d%02d%02dZ", + data.tm_year + 1900, data.tm_mon + 1, data.tm_mday, + data.tm_hour, data.tm_min, data.tm_sec); + if (ret != (int)(sizeof(buf) - 1)) { + abort(); // |snprintf| should neither truncate nor write fewer bytes. + } int free_s = 0; if (s == NULL) { diff --git a/crypto/asn1/a_utctm.c b/crypto/asn1/a_utctm.c index da1d2bab83..f79114d891 100644 --- a/crypto/asn1/a_utctm.c +++ b/crypto/asn1/a_utctm.c @@ -59,6 +59,7 @@ #include #include +#include #include #include @@ -123,9 +124,12 @@ ASN1_UTCTIME *ASN1_UTCTIME_adj(ASN1_UTCTIME *s, int64_t posix_time, int offset_d } char buf[14]; - snprintf(buf, sizeof(buf), "%02d%02d%02d%02d%02d%02dZ", data.tm_year % 100, - data.tm_mon + 1, data.tm_mday, data.tm_hour, data.tm_min, - data.tm_sec); + int ret = snprintf(buf, sizeof(buf), "%02d%02d%02d%02d%02d%02dZ", + data.tm_year % 100, data.tm_mon + 1, data.tm_mday, + data.tm_hour, data.tm_min, data.tm_sec); + if (ret != (int)(sizeof(buf) - 1)) { + abort(); // |snprintf| should neither truncate nor write fewer bytes. + } int free_s = 0; if (s == NULL) { From 0c4dfde9db7c6e5767aa42cf91d979446cf273ec Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 14 Jul 2023 16:30:43 -0400 Subject: [PATCH 07/11] Support Android's "baremetal" target This corresponds to the libcrypto_baremetal build target in Android, which is an embedded-style platform that uses a subset of the bionic libc. It will also, eventually, use getentropy for its PRNG. As part of this, generalize the OPENSSL_TRUSTY exclusion for file BIOs to OPENSSL_NO_FILESYSTEM. Upstream OpenSSL uses OPENSSL_NO_STDIO, but that excludes all of FILE entirely. We already require FILE in quite a few places (urandom.c, self_test.c) for writing to stderr, and FILE is part of C standard library. So, let's tentatively say that we require you have FILE and stderr. Instead, OPENSSL_NO_FILESYSTEM is saying you don't have fopen. You're still required to have the three std{in,out,err} FILEs, and given a FILE, you need to allow the standard operations on it. (Possibly in forms that always fail.) To keep us honest, whenever a function is excluded, I've dropped it from the header too, and followed callers up the chain. I have not attempted to make the tests work when these are excluded. Later CLs in this series will do the same for NO_SOCK and NO_POSIX_IO. This was a little tedious, but not too bad. (I assume we'll end up changing our minds on this a lot. For now, let's try this.) I haven't yet restored OPENSSL_RAND_TRUSTY or removed the OPENSSL_TRUSTY ifdef on file.c. Having a separate CL makes it a bit easier to revert if something goes wrong. This depends on https://android-review.googlesource.com/c/platform/bionic/+/2659335, which fixes the header bionic uses for getentropy. Bug: 629, b:291102972 Change-Id: Idd839cd3fa4253128de54bd1be7da261dbcdeb7c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61726 Auto-Submit: David Benjamin Reviewed-by: Bob Beck Commit-Queue: Bob Beck (cherry picked from commit 5ba5db1a29ef54dc3ee2efbc5bdb3d95b77fc928) --- crypto/bio/file.c | 8 ++++++++ crypto/conf/conf.c | 10 ++++------ crypto/x509/by_dir.c | 4 ++-- crypto/x509/by_file.c | 4 ++-- crypto/x509/x509_d2.c | 5 +++-- include/openssl/bio.h | 4 ++++ include/openssl/conf.h | 2 ++ include/openssl/ssl.h | 10 ++++++++++ include/openssl/target.h | 21 ++++++++++++++++++--- include/openssl/x509.h | 6 ++++-- ssl/ssl_decrepit.c | 5 +++-- ssl/ssl_file.cc | 10 ++++++---- ssl/ssl_x509.cc | 2 ++ 13 files changed, 68 insertions(+), 23 deletions(-) diff --git a/crypto/bio/file.c b/crypto/bio/file.c index b6d4c1a1d8..d33346df93 100644 --- a/crypto/bio/file.c +++ b/crypto/bio/file.c @@ -73,6 +73,8 @@ #include +// TODO(crbug.com/boringssl/629): Remove this in favor of the more fine-grained +// OPENSSL_NO_FILESYSTEM ifdef. #if !defined(OPENSSL_TRUSTY) #include @@ -94,6 +96,7 @@ #define BIO_FP_WRITE 0x04 #define BIO_FP_APPEND 0x08 +#if !defined(OPENSSL_NO_FILESYSTEM) BIO *BIO_new_file(const char *filename, const char *mode) { BIO *ret; FILE *file; @@ -119,6 +122,7 @@ BIO *BIO_new_file(const char *filename, const char *mode) { return ret; } +#endif // !OPENSSL_NO_FILESYSTEM BIO *BIO_new_fp(FILE *stream, int close_flag) { BIO *ret = BIO_new(BIO_s_file()); @@ -205,6 +209,7 @@ static long file_ctrl(BIO *b, int cmd, long num, void *ptr) { _setmode(_fileno(b->ptr), num & BIO_FP_TEXT ? _O_TEXT : _O_BINARY); #endif break; +#if !defined(OPENSSL_NO_FILESYSTEM) case BIO_C_SET_FILENAME: file_free(b); b->shutdown = (int)num & BIO_CLOSE; @@ -237,6 +242,7 @@ static long file_ctrl(BIO *b, int cmd, long num, void *ptr) { b->ptr = fp; b->init = 1; break; +#endif // !OPENSSL_NO_FILESYSTEM case BIO_C_GET_FILE_PTR: // the ptr parameter is actually a FILE ** in this case. if (ptr != NULL) { @@ -296,6 +302,7 @@ int BIO_set_fp(BIO *bio, FILE *file, int close_flag) { return (int)BIO_ctrl(bio, BIO_C_SET_FILE_PTR, close_flag, (char *)file); } +#if !defined(OPENSSL_NO_FILESYSTEM) int BIO_read_filename(BIO *bio, const char *filename) { return (int)BIO_ctrl(bio, BIO_C_SET_FILENAME, BIO_CLOSE | BIO_FP_READ, (char *)filename); @@ -316,6 +323,7 @@ int BIO_rw_filename(BIO *bio, const char *filename) { BIO_CLOSE | BIO_FP_READ | BIO_FP_WRITE, (char *)filename); } +#endif // !OPENSSL_NO_FILESYSTEM long BIO_tell(BIO *bio) { return BIO_ctrl(bio, BIO_C_FILE_TELL, 0, NULL); } diff --git a/crypto/conf/conf.c b/crypto/conf/conf.c index df38e75f34..5d24eb5a68 100644 --- a/crypto/conf/conf.c +++ b/crypto/conf/conf.c @@ -387,7 +387,7 @@ static void clear_comments(CONF *conf, char *p) { } } -static int def_load_bio(CONF *conf, BIO *in, long *out_error_line) { +int NCONF_load_bio(CONF *conf, BIO *in, long *out_error_line) { static const size_t CONFBUFSIZE = 512; int bufnum = 0, i, ii; BUF_MEM *buff = NULL; @@ -585,6 +585,7 @@ static int def_load_bio(CONF *conf, BIO *in, long *out_error_line) { return 0; } +#if !defined(OPENSSL_NO_FILESYSTEM) int NCONF_load(CONF *conf, const char *filename, long *out_error_line) { BIO *in = BIO_new_file(filename, "rb"); int ret; @@ -594,15 +595,12 @@ int NCONF_load(CONF *conf, const char *filename, long *out_error_line) { return 0; } - ret = def_load_bio(conf, in, out_error_line); + ret = NCONF_load_bio(conf, in, out_error_line); BIO_free(in); return ret; } - -int NCONF_load_bio(CONF *conf, BIO *bio, long *out_error_line) { - return def_load_bio(conf, bio, out_error_line); -} +#endif // !OPENSSL_NO_FILESYSTEM int CONF_parse_list(const char *list, char sep, int remove_whitespace, int (*list_cb)(const char *elem, size_t len, void *usr), diff --git a/crypto/x509/by_dir.c b/crypto/x509/by_dir.c index 0603600d38..c96ddacda0 100644 --- a/crypto/x509/by_dir.c +++ b/crypto/x509/by_dir.c @@ -64,7 +64,7 @@ #include #include -#if !defined(OPENSSL_TRUSTY) +#if !defined(OPENSSL_NO_FILESYSTEM) #include "../internal.h" #include "internal.h" @@ -403,4 +403,4 @@ static int get_cert_by_subject(X509_LOOKUP *xl, int type, X509_NAME *name, return ok; } -#endif // OPENSSL_TRUSTY +#endif // OPENSSL_NO_FILESYSTEM diff --git a/crypto/x509/by_file.c b/crypto/x509/by_file.c index 7c056f0815..33bd978192 100644 --- a/crypto/x509/by_file.c +++ b/crypto/x509/by_file.c @@ -62,7 +62,7 @@ #include "internal.h" -#ifndef OPENSSL_NO_STDIO +#if !defined(OPENSSL_NO_FILESYSTEM) static int by_file_ctrl(X509_LOOKUP *ctx, int cmd, const char *argc, long argl, char **ret); @@ -279,4 +279,4 @@ int X509_load_cert_crl_file(X509_LOOKUP *ctx, const char *file, int type) { return count; } -#endif // OPENSSL_NO_STDIO +#endif // !OPENSSL_NO_FILESYSTEM diff --git a/crypto/x509/x509_d2.c b/crypto/x509/x509_d2.c index 748bd88acd..7db1d2f340 100644 --- a/crypto/x509/x509_d2.c +++ b/crypto/x509/x509_d2.c @@ -57,7 +57,8 @@ #include #include -#ifndef OPENSSL_NO_STDIO +#if !defined(OPENSSL_NO_FILESYSTEM) + int X509_STORE_set_default_paths(X509_STORE *ctx) { X509_LOOKUP *lookup; @@ -107,4 +108,4 @@ int X509_STORE_load_locations(X509_STORE *ctx, const char *file, return 1; } -#endif +#endif // !OPENSSL_NO_FILESYSTEM diff --git a/include/openssl/bio.h b/include/openssl/bio.h index 625cf03f37..385e3fcb58 100644 --- a/include/openssl/bio.h +++ b/include/openssl/bio.h @@ -513,9 +513,11 @@ OPENSSL_EXPORT int BIO_get_fd(BIO *bio, int *out_fd); // BIO_s_file returns a BIO_METHOD that wraps a |FILE|. OPENSSL_EXPORT const BIO_METHOD *BIO_s_file(void); +#if !defined(OPENSSL_NO_FILESYSTEM) // BIO_new_file creates a file BIO by opening |filename| with the given mode. // See the |fopen| manual page for details of the mode argument. OPENSSL_EXPORT BIO *BIO_new_file(const char *filename, const char *mode); +#endif // BIO_new_fp creates a new file BIO that wraps the given |FILE|. If // |close_flag| is |BIO_CLOSE|, then |fclose| will be called on |stream| when @@ -533,6 +535,7 @@ OPENSSL_EXPORT int BIO_get_fp(BIO *bio, FILE **out_file); // returns one on success and zero otherwise. OPENSSL_EXPORT int BIO_set_fp(BIO *bio, FILE *file, int close_flag); +#if !defined(OPENSSL_NO_FILESYSTEM) // 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. @@ -552,6 +555,7 @@ OPENSSL_EXPORT int BIO_append_filename(BIO *bio, const char *filename); // as the |FILE| for |bio|. It returns one on success and zero otherwise. The // |FILE| will be closed when |bio| is freed. OPENSSL_EXPORT int BIO_rw_filename(BIO *bio, const char *filename); +#endif // OPENSSL_NO_FILESYSTEM // BIO_tell returns the file offset of |bio|, or a negative number on error or // if |bio| does not support the operation. diff --git a/include/openssl/conf.h b/include/openssl/conf.h index 04b7af34f9..586b9e0fa9 100644 --- a/include/openssl/conf.h +++ b/include/openssl/conf.h @@ -99,12 +99,14 @@ OPENSSL_EXPORT CONF *NCONF_new(void *method); // NCONF_free frees all the data owned by |conf| and then |conf| itself. OPENSSL_EXPORT void NCONF_free(CONF *conf); +#if !defined(OPENSSL_NO_FILESYSTEM) // NCONF_load parses the file named |filename| and adds the values found to // |conf|. It returns one on success and zero on error. In the event of an // error, if |out_error_line| is not NULL, |*out_error_line| is set to the // number of the line that contained the error. OPENSSL_EXPORT int NCONF_load(CONF *conf, const char *filename, long *out_error_line); +#endif // NCONF_load_bio acts like |NCONF_load| but reads from |bio| rather than from // a named file. diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index ed586777aa..4bdd0df256 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -1313,6 +1313,7 @@ OPENSSL_EXPORT int SSL_use_RSAPrivateKey_ASN1(SSL *ssl, const uint8_t *der, #define SSL_FILETYPE_PEM 1 #define SSL_FILETYPE_ASN1 2 +#if !defined(OPENSSL_NO_FILESYSTEM) OPENSSL_EXPORT int SSL_CTX_use_RSAPrivateKey_file(SSL_CTX *ctx, const char *file, int type); OPENSSL_EXPORT int SSL_use_RSAPrivateKey_file(SSL *ssl, const char *file, @@ -1334,6 +1335,7 @@ OPENSSL_EXPORT int SSL_use_PrivateKey_file(SSL *ssl, const char *file, // success and zero on failure. OPENSSL_EXPORT int SSL_CTX_use_certificate_chain_file(SSL_CTX *ctx, const char *file); +#endif // !OPENSSL_NO_FILESYSTEM // SSL_CTX_use_certificate_chain_file configures certificates for |ssl|. It // reads the contents of |file| as a PEM-encoded leaf certificate followed @@ -2948,6 +2950,7 @@ OPENSSL_EXPORT void SSL_CTX_set_cert_store(SSL_CTX *ctx, X509_STORE *store); // SSL_CTX_get_cert_store returns |ctx|'s certificate store. OPENSSL_EXPORT X509_STORE *SSL_CTX_get_cert_store(const SSL_CTX *ctx); +#if !defined(OPENSSL_NO_FILESYSTEM) // SSL_CTX_set_default_verify_paths loads the OpenSSL system-default trust // anchors into |ctx|'s store. It returns one on success and zero on failure. OPENSSL_EXPORT int SSL_CTX_set_default_verify_paths(SSL_CTX *ctx); @@ -2964,6 +2967,7 @@ OPENSSL_EXPORT int SSL_CTX_set_default_verify_paths(SSL_CTX *ctx); OPENSSL_EXPORT int SSL_CTX_load_verify_locations(SSL_CTX *ctx, const char *ca_file, const char *ca_dir); +#endif // !OPENSSL_NO_FILESYSTEM // SSL_get_verify_result returns the result of certificate verification. It is // either |X509_V_OK| or a |X509_V_ERR_*| value. @@ -3125,20 +3129,24 @@ OPENSSL_EXPORT int SSL_add_client_CA(SSL *ssl, X509 *x509); // ownership of |x509|. OPENSSL_EXPORT int SSL_CTX_add_client_CA(SSL_CTX *ctx, X509 *x509); +#if !defined(OPENSSL_NO_FILESYSTEM) // SSL_load_client_CA_file opens |file| and reads PEM-encoded certificates from // it. It returns a newly-allocated stack of the certificate subjects or NULL // on error. Duplicates in |file| are ignored. OPENSSL_EXPORT STACK_OF(X509_NAME) *SSL_load_client_CA_file(const char *file); +#endif // SSL_dup_CA_list makes a deep copy of |list|. It returns the new list on // success or NULL on allocation error. OPENSSL_EXPORT STACK_OF(X509_NAME) *SSL_dup_CA_list(STACK_OF(X509_NAME) *list); +#if !defined(OPENSSL_NO_FILESYSTEM) // SSL_add_file_cert_subjects_to_stack behaves like |SSL_load_client_CA_file| // but appends the result to |out|. It returns one on success or zero on // error. OPENSSL_EXPORT int SSL_add_file_cert_subjects_to_stack(STACK_OF(X509_NAME) *out, const char *file); +#endif // SSL_add_bio_cert_subjects_to_stack behaves like // |SSL_add_file_cert_subjects_to_stack| but reads from |bio|. @@ -5464,11 +5472,13 @@ OPENSSL_EXPORT int SSL_CTX_set_tmp_ecdh(SSL_CTX *ctx, const EC_KEY *ec_key); // |ec_key|'s curve. The remainder of |ec_key| is ignored. OPENSSL_EXPORT int SSL_set_tmp_ecdh(SSL *ssl, const EC_KEY *ec_key); +#if !defined(OPENSSL_NO_FILESYSTEM) // SSL_add_dir_cert_subjects_to_stack lists files in directory |dir|. It calls // |SSL_add_file_cert_subjects_to_stack| on each file and returns one on success // or zero on error. This function is deprecated. OPENSSL_EXPORT int SSL_add_dir_cert_subjects_to_stack(STACK_OF(X509_NAME) *out, const char *dir); +#endif // SSL_CTX_enable_tls_channel_id calls |SSL_CTX_set_tls_channel_id_enabled|. OPENSSL_EXPORT int SSL_CTX_enable_tls_channel_id(SSL_CTX *ctx); diff --git a/include/openssl/target.h b/include/openssl/target.h index 4a6f40bab2..737aa4c9fe 100644 --- a/include/openssl/target.h +++ b/include/openssl/target.h @@ -82,10 +82,13 @@ #define OPENSSL_WINDOWS #endif -// Trusty isn't Linux but currently defines __linux__. As a workaround, we -// exclude it here. +// Trusty and Android baremetal aren't't Linux but currently define __linux__. +// As a workaround, we exclude them here. +// // TODO(b/169780122): Remove this workaround once Trusty no longer defines it. -#if defined(__linux__) && !defined(__TRUSTY__) +// TODO(b/291101350): Remove this workaround once Android baremetal no longer +// defines it. +#if defined(__linux__) && !defined(__TRUSTY__) && !defined(ANDROID_BAREMETAL) #define OPENSSL_LINUX #endif @@ -100,6 +103,7 @@ // platforms must introduce their own defines. #if defined(__TRUSTY__) #define OPENSSL_TRUSTY +#define OPENSSL_NO_FILESYSTEM #define OPENSSL_NO_POSIX_IO #define OPENSSL_NO_SOCK #define OPENSSL_NO_THREADS_CORRUPT_MEMORY_AND_LEAK_SECRETS_IF_THREADED @@ -109,6 +113,17 @@ // other platform is not supported. Other embedded platforms must introduce // their own defines. #if defined(OPENSSL_NANOLIBC) +#define OPENSSL_NO_FILESYSTEM +#define OPENSSL_NO_POSIX_IO +#define OPENSSL_NO_SOCK +#define OPENSSL_NO_THREADS_CORRUPT_MEMORY_AND_LEAK_SECRETS_IF_THREADED +#endif + +// Android baremetal is an embedded target that uses a subset of bionic. +// Defining this on any other platform is not supported. Other embedded +// platforms must introduce their own defines. +#if defined(ANDROID_BAREMETAL) +#define OPENSSL_NO_FILESYSTEM #define OPENSSL_NO_POSIX_IO #define OPENSSL_NO_SOCK #define OPENSSL_NO_THREADS_CORRUPT_MEMORY_AND_LEAK_SECRETS_IF_THREADED diff --git a/include/openssl/x509.h b/include/openssl/x509.h index 0db1cea1aa..ce4fc78b2d 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -2905,8 +2905,10 @@ OPENSSL_EXPORT X509 *X509_STORE_CTX_get0_cert(X509_STORE_CTX *ctx); OPENSSL_EXPORT X509_LOOKUP *X509_STORE_add_lookup(X509_STORE *v, X509_LOOKUP_METHOD *m); +#if !defined(OPENSSL_NO_FILESYSTEM) OPENSSL_EXPORT X509_LOOKUP_METHOD *X509_LOOKUP_hash_dir(void); OPENSSL_EXPORT X509_LOOKUP_METHOD *X509_LOOKUP_file(void); +#endif OPENSSL_EXPORT int X509_STORE_add_cert(X509_STORE *ctx, X509 *x); OPENSSL_EXPORT int X509_STORE_add_crl(X509_STORE *ctx, X509_CRL *x); @@ -2914,7 +2916,7 @@ OPENSSL_EXPORT int X509_STORE_add_crl(X509_STORE *ctx, X509_CRL *x); OPENSSL_EXPORT int X509_LOOKUP_ctrl(X509_LOOKUP *ctx, int cmd, const char *argc, long argl, char **ret); -#ifndef OPENSSL_NO_STDIO +#if !defined(OPENSSL_NO_FILESYSTEM) OPENSSL_EXPORT int X509_load_cert_file(X509_LOOKUP *ctx, const char *file, int type); OPENSSL_EXPORT int X509_load_crl_file(X509_LOOKUP *ctx, const char *file, @@ -2930,7 +2932,7 @@ OPENSSL_EXPORT int X509_LOOKUP_by_subject(X509_LOOKUP *ctx, int type, X509_NAME *name, X509_OBJECT *ret); OPENSSL_EXPORT int X509_LOOKUP_shutdown(X509_LOOKUP *ctx); -#ifndef OPENSSL_NO_STDIO +#if !defined(OPENSSL_NO_FILESYSTEM) OPENSSL_EXPORT int X509_STORE_load_locations(X509_STORE *ctx, const char *file, const char *dir); OPENSSL_EXPORT int X509_STORE_set_default_paths(X509_STORE *ctx); diff --git a/ssl/ssl_decrepit.c b/ssl/ssl_decrepit.c index a155c0f46a..c6df9a11ed 100644 --- a/ssl/ssl_decrepit.c +++ b/ssl/ssl_decrepit.c @@ -110,7 +110,8 @@ #include -#if !defined(OPENSSL_WINDOWS) && !defined(OPENSSL_PNACL) +#if !defined(OPENSSL_WINDOWS) && !defined(OPENSSL_PNACL) && \ + !defined(OPENSSL_NO_FILESYSTEM) #include #include @@ -162,4 +163,4 @@ int SSL_add_dir_cert_subjects_to_stack(STACK_OF(X509_NAME) *stack, return ret; } -#endif // !WINDOWS && !PNACL +#endif // !WINDOWS && !PNACL && !OPENSSL_NO_FILESYSTEM diff --git a/ssl/ssl_file.cc b/ssl/ssl_file.cc index b0ef284abd..229dff7b56 100644 --- a/ssl/ssl_file.cc +++ b/ssl/ssl_file.cc @@ -199,6 +199,11 @@ static int add_bio_cert_subjects_to_stack(STACK_OF(X509_NAME) *out, BIO *bio, return 1; } +int SSL_add_bio_cert_subjects_to_stack(STACK_OF(X509_NAME) *out, BIO *bio) { + return add_bio_cert_subjects_to_stack(out, bio, /*allow_empty=*/true); +} + +#if !defined(OPENSSL_NO_FILESYSTEM) STACK_OF(X509_NAME) *SSL_load_client_CA_file(const char *file) { bssl::UniquePtr in(BIO_new_file(file, "r")); if (in == nullptr) { @@ -222,10 +227,6 @@ int SSL_add_file_cert_subjects_to_stack(STACK_OF(X509_NAME) *out, return SSL_add_bio_cert_subjects_to_stack(out, in.get()); } -int SSL_add_bio_cert_subjects_to_stack(STACK_OF(X509_NAME) *out, BIO *bio) { - return add_bio_cert_subjects_to_stack(out, bio, /*allow_empty=*/true); -} - int SSL_use_certificate_file(SSL *ssl, const char *file, int type) { int reason_code; BIO *in; @@ -560,6 +561,7 @@ static int use_certificate_chain_file(SSL_CTX *ctx, SSL *ssl, const char *file) return ret; } +#endif // !OPENSSL_NO_FILESYSTEM int SSL_CTX_use_certificate_chain_file(SSL_CTX *ctx, const char *file) { diff --git a/ssl/ssl_x509.cc b/ssl/ssl_x509.cc index 88e3eabc78..ac9c1000f9 100644 --- a/ssl/ssl_x509.cc +++ b/ssl/ssl_x509.cc @@ -734,6 +734,7 @@ void SSL_CTX_set_verify_depth(SSL_CTX *ctx, int depth) { X509_VERIFY_PARAM_set_depth(ctx->param, depth); } +#if !defined(OPENSSL_NO_FILESYSTEM) int SSL_CTX_set_default_verify_paths(SSL_CTX *ctx) { check_ssl_ctx_x509_method(ctx); return X509_STORE_set_default_paths(ctx->cert_store); @@ -744,6 +745,7 @@ int SSL_CTX_load_verify_locations(SSL_CTX *ctx, const char *ca_file, check_ssl_ctx_x509_method(ctx); return X509_STORE_load_locations(ctx->cert_store, ca_file, ca_dir); } +#endif // !OPENSSL_NO_FILESYSTEM long SSL_get_verify_result(const SSL *ssl) { check_ssl_x509_method(ssl); From 2efb1a5ad0d056a36d63f752293d3c90b48234b4 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 14 Jul 2023 16:42:52 -0400 Subject: [PATCH 08/11] Unconditionally include BIO_set_fd and BIO_get_fd These functions are just wrappers over BIO_ctrls, shared between the fd and socket BIOs. Though we don't currently support one, it is conceivable that there would be a platform with socket BIOs but not fd BIOs. In that case, the BIO_get_fd function would still be useful to implement SSL_get_rfd. (And someone could conceivably implement it in another BIO.) Bug: 629 Change-Id: I7ac3561f76af86af32d70b1c6265c4caeaecb129 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61727 Commit-Queue: David Benjamin Reviewed-by: Bob Beck (cherry picked from commit 68beac6373aeee787e0919b240c1a8177554cac8) --- crypto/bio/fd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crypto/bio/fd.c b/crypto/bio/fd.c index aac8bcb3c0..c128e8181d 100644 --- a/crypto/bio/fd.c +++ b/crypto/bio/fd.c @@ -268,6 +268,8 @@ static const BIO_METHOD methods_fdp = { const BIO_METHOD *BIO_s_fd(void) { return &methods_fdp; } +#endif // OPENSSL_NO_POSIX_IO + int BIO_set_fd(BIO *bio, int fd, int close_flag) { return (int)BIO_int_ctrl(bio, BIO_C_SET_FD, close_flag, fd); } @@ -275,5 +277,3 @@ int BIO_set_fd(BIO *bio, int fd, int close_flag) { int BIO_get_fd(BIO *bio, int *out_fd) { return (int)BIO_ctrl(bio, BIO_C_GET_FD, 0, (char *) out_fd); } - -#endif // OPENSSL_NO_POSIX_IO From 515ec081f395cd372d2faec6c4cb17af13daa8cd Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 14 Jul 2023 17:19:21 -0400 Subject: [PATCH 09/11] Test non-blocking socket BIOs As part of this, factor out some of the socket bits. I tried to write the sockaddr mess in a way that's strict-aliasing-clean, at least as far as code we own goes. But the API is really not designed for it, and who knows what effective type the underlying libc functions expect. (Fortunately it's mostly syscalls, which definitely escape the abstract machine.) Change-Id: I12621f6c40f074ff7423dd46ddceca120ba63db9 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61728 Reviewed-by: Bob Beck Commit-Queue: David Benjamin (cherry picked from commit 2b5d6ba0341588e4bbd53d6055118e5472657b49) AWS-LC: Changed OwnedSocket::release to be compatible with C++11. --- crypto/bio/bio_test.cc | 297 +++++++++++++++++++++++++++++++++++------ 1 file changed, 254 insertions(+), 43 deletions(-) diff --git a/crypto/bio/bio_test.cc b/crypto/bio/bio_test.cc index e80570d0d3..ceffab28ec 100644 --- a/crypto/bio/bio_test.cc +++ b/crypto/bio/bio_test.cc @@ -14,6 +14,7 @@ #include #include +#include #include @@ -30,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -41,11 +43,13 @@ OPENSSL_MSVC_PRAGMA(warning(push, 3)) OPENSSL_MSVC_PRAGMA(warning(pop)) #endif - #if !defined(OPENSSL_WINDOWS) +using Socket = int; +#define INVALID_SOCKET (-1) static int closesocket(int sock) { return close(sock); } static std::string LastSocketError() { return strerror(errno); } #else +using Socket = SOCKET; static std::string LastSocketError() { char buf[DECIMAL_SIZE(int) + 1]; snprintf(buf, sizeof(buf), "%d", WSAGetLastError()); @@ -53,76 +57,283 @@ static std::string LastSocketError() { } #endif -class ScopedSocket { +class OwnedSocket { public: - explicit ScopedSocket(int sock) : sock_(sock) {} - ~ScopedSocket() { closesocket(sock_); } + OwnedSocket() = default; + explicit OwnedSocket(Socket sock) : sock_(sock) {} + OwnedSocket(OwnedSocket &&other) { *this = std::move(other); } + ~OwnedSocket() { reset(); } + OwnedSocket &operator=(OwnedSocket &&other) { + reset(other.release()); + return *this; + } + + bool is_valid() const { return sock_ != INVALID_SOCKET; } + Socket get() const { return sock_; } + Socket release() { + Socket temp = std::move(sock_); + sock_ = INVALID_SOCKET; + return temp; + } + + void reset(Socket sock = INVALID_SOCKET) { + if (is_valid()) { + closesocket(sock_); + } + + sock_ = sock; + } private: - const int sock_; + Socket sock_ = INVALID_SOCKET; +}; + +struct SockaddrStorage { + SockaddrStorage() : storage() , len(sizeof(storage)) {} + + int family() const { return storage.ss_family; } + + sockaddr *addr_mut() { return reinterpret_cast(&storage); } + const sockaddr *addr() const { + return reinterpret_cast(&storage); + } + + sockaddr_in ToIPv4() const { + if (family() != AF_INET || len != sizeof(sockaddr_in)) { + abort(); + } + // These APIs were seemingly designed before C's strict aliasing rule, and + // C++'s strict union handling. Make a copy so the compiler does not read + // this as an aliasing violation. + sockaddr_in ret; + OPENSSL_memcpy(&ret, &storage, sizeof(ret)); + return ret; + } + + sockaddr_in6 ToIPv6() const { + if (family() != AF_INET6 || len != sizeof(sockaddr_in6)) { + abort(); + } + // These APIs were seemingly designed before C's strict aliasing rule, and + // C++'s strict union handling. Make a copy so the compiler does not read + // this as an aliasing violation. + sockaddr_in6 ret; + OPENSSL_memcpy(&ret, &storage, sizeof(ret)); + return ret; + } + + sockaddr_storage storage; + socklen_t len; }; +static OwnedSocket Bind(int family, const sockaddr *addr, socklen_t addr_len) { + OwnedSocket sock(socket(family, SOCK_STREAM, 0)); + if (!sock.is_valid()) { + return OwnedSocket(); + } + + if (bind(sock.get(), addr, addr_len) != 0) { + return OwnedSocket(); + } + + return sock; +} + +static OwnedSocket ListenLoopback(int backlog) { + // Try binding to IPv6. + sockaddr_in6 sin6; + OPENSSL_memset(&sin6, 0, sizeof(sin6)); + sin6.sin6_family = AF_INET6; + if (inet_pton(AF_INET6, "::1", &sin6.sin6_addr) != 1) { + return OwnedSocket(); + } + OwnedSocket sock = + Bind(AF_INET6, reinterpret_cast(&sin6), sizeof(sin6)); + if (!sock.is_valid()) { + // Try binding to IPv4. + sockaddr_in sin; + OPENSSL_memset(&sin, 0, sizeof(sin)); + sin.sin_family = AF_INET; + if (inet_pton(AF_INET, "127.0.0.1", &sin.sin_addr) != 1) { + return OwnedSocket(); + } + sock = Bind(AF_INET, reinterpret_cast(&sin), sizeof(sin)); + } + if (!sock.is_valid()) { + return OwnedSocket(); + } + + if (listen(sock.get(), backlog) != 0) { + return OwnedSocket(); + } + + return sock; +} + +static bool SocketSetNonBlocking(Socket sock) { +#if defined(OPENSSL_WINDOWS) + u_long arg = 1; + return ioctlsocket(sock, FIONBIO, &arg) == 0; +#else + int flags = fcntl(sock, F_GETFL, 0); + if (flags < 0) { + return false; + } + flags |= O_NONBLOCK; + return fcntl(sock, F_SETFL, flags) == 0; +#endif +} + +enum class WaitType { kRead, kWrite }; + +static bool WaitForSocket(Socket sock, WaitType wait_type) { + // Use an arbitrary 5 second timeout, so the test doesn't hang indefinitely if + // there's an issue. + static const int kTimeoutSeconds = 5; +#if defined(OPENSSL_WINDOWS) + fd_set read_set, write_set; + FD_ZERO(&read_set); + FD_ZERO(&write_set); + fd_set *wait_set = wait_type == WaitType::kRead ? &read_set : &write_set; + FD_SET(sock, wait_set); + timeval timeout; + timeout.tv_sec = kTimeoutSeconds; + timeout.tv_usec = 0; + if (select(0 /* unused on Windows */, &read_set, &write_set, nullptr, + &timeout) <= 0) { + return false; + } + return FD_ISSET(sock, wait_set); +#else + short events = wait_type == WaitType::kRead ? POLLIN : POLLOUT; + pollfd fd = {/*fd=*/sock, events, /*revents=*/0}; + return poll(&fd, 1, kTimeoutSeconds * 1000) == 1 && (fd.revents & events); +#endif +} + TEST(BIOTest, SocketConnect) { static const char kTestMessage[] = "test"; - int listening_sock = -1; - socklen_t len = 0; - sockaddr_storage ss; - struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)&ss; - struct sockaddr_in *sin = (struct sockaddr_in *)&ss; - OPENSSL_memset(&ss, 0, sizeof(ss)); - - ss.ss_family = AF_INET6; - listening_sock = socket(AF_INET6, SOCK_STREAM, 0); - ASSERT_NE(-1, listening_sock) << LastSocketError(); - len = sizeof(*sin6); - ASSERT_EQ(1, inet_pton(AF_INET6, "::1", &sin6->sin6_addr)) - << LastSocketError(); - if (bind(listening_sock, (struct sockaddr *)sin6, sizeof(*sin6)) == -1) { - closesocket(listening_sock); - - ss.ss_family = AF_INET; - listening_sock = socket(AF_INET, SOCK_STREAM, 0); - ASSERT_NE(-1, listening_sock) << LastSocketError(); - len = sizeof(*sin); - ASSERT_EQ(1, inet_pton(AF_INET, "127.0.0.1", &sin->sin_addr)) - << LastSocketError(); - ASSERT_EQ(0, bind(listening_sock, (struct sockaddr *)sin, sizeof(*sin))) - << LastSocketError(); - } + OwnedSocket listening_sock = ListenLoopback(/*backlog=*/1); + ASSERT_TRUE(listening_sock.is_valid()) << LastSocketError(); - ScopedSocket listening_sock_closer(listening_sock); - ASSERT_EQ(0, listen(listening_sock, 1)) << LastSocketError(); - ASSERT_EQ(0, getsockname(listening_sock, (struct sockaddr *)&ss, &len)) + SockaddrStorage addr; + ASSERT_EQ(getsockname(listening_sock.get(), addr.addr_mut(), &addr.len), 0) << LastSocketError(); char hostname[80]; - if (ss.ss_family == AF_INET6) { - snprintf(hostname, sizeof(hostname), "[::1]:%d", ntohs(sin6->sin6_port)); - } else if (ss.ss_family == AF_INET) { - snprintf(hostname, sizeof(hostname), "127.0.0.1:%d", ntohs(sin->sin_port)); + if (addr.family() == AF_INET6) { + snprintf(hostname, sizeof(hostname), "[::1]:%d", + ntohs(addr.ToIPv6().sin6_port)); + } else { + snprintf(hostname, sizeof(hostname), "127.0.0.1:%d", + ntohs(addr.ToIPv4().sin_port)); } // Connect to it with a connect BIO. bssl::UniquePtr bio(BIO_new_connect(hostname)); ASSERT_TRUE(bio); - // Write a test message to the BIO. + // Write a test message to the BIO. This is assumed to be smaller than the + // transport buffer. ASSERT_EQ(static_cast(sizeof(kTestMessage)), - BIO_write(bio.get(), kTestMessage, sizeof(kTestMessage))); + BIO_write(bio.get(), kTestMessage, sizeof(kTestMessage))) + << LastSocketError(); // Accept the socket. - int sock = accept(listening_sock, (struct sockaddr *)&ss, &len); - ASSERT_NE(-1, sock) << LastSocketError(); - ScopedSocket sock_closer(sock); + OwnedSocket sock(accept(listening_sock.get(), addr.addr_mut(), &addr.len)); + ASSERT_TRUE(sock.is_valid()) << LastSocketError(); // Check the same message is read back out. char buf[sizeof(kTestMessage)]; ASSERT_EQ(static_cast(sizeof(kTestMessage)), - recv(sock, buf, sizeof(buf), 0)) + recv(sock.get(), buf, sizeof(buf), 0)) << LastSocketError(); EXPECT_EQ(Bytes(kTestMessage, sizeof(kTestMessage)), Bytes(buf, sizeof(buf))); } +TEST(BIOTest, SocketNonBlocking) { + OwnedSocket listening_sock = ListenLoopback(/*backlog=*/1); + ASSERT_TRUE(listening_sock.is_valid()) << LastSocketError(); + + // Connect to |listening_sock|. + SockaddrStorage addr; + ASSERT_EQ(getsockname(listening_sock.get(), addr.addr_mut(), &addr.len), 0) + << LastSocketError(); + OwnedSocket connect_sock(socket(addr.family(), SOCK_STREAM, 0)); + ASSERT_TRUE(connect_sock.is_valid()) << LastSocketError(); + ASSERT_EQ(connect(connect_sock.get(), addr.addr(), addr.len), 0) + << LastSocketError(); + ASSERT_TRUE(SocketSetNonBlocking(connect_sock.get())) << LastSocketError(); + bssl::UniquePtr connect_bio( + BIO_new_socket(connect_sock.get(), BIO_NOCLOSE)); + ASSERT_TRUE(connect_bio); + + // Make a corresponding accepting socket. + OwnedSocket accept_sock( + accept(listening_sock.get(), addr.addr_mut(), &addr.len)); + ASSERT_TRUE(accept_sock.is_valid()) << LastSocketError(); + ASSERT_TRUE(SocketSetNonBlocking(accept_sock.get())) << LastSocketError(); + bssl::UniquePtr accept_bio( + BIO_new_socket(accept_sock.get(), BIO_NOCLOSE)); + ASSERT_TRUE(accept_bio); + + // Exchange data through the socket. + static const char kTestMessage[] = "hello, world"; + + // Reading from |accept_bio| should not block. + char buf[sizeof(kTestMessage)]; + int ret = BIO_read(accept_bio.get(), buf, sizeof(buf)); + EXPECT_EQ(ret, -1); + EXPECT_TRUE(BIO_should_read(accept_bio.get())) << LastSocketError(); + + // Writing to |connect_bio| should eventually overflow the transport buffers + // and also give a retryable error. + int bytes_written = 0; + for (;;) { + ret = BIO_write(connect_bio.get(), kTestMessage, sizeof(kTestMessage)); + if (ret <= 0) { + EXPECT_EQ(ret, -1); + EXPECT_TRUE(BIO_should_write(connect_bio.get())) << LastSocketError(); + break; + } + bytes_written += ret; + } + EXPECT_GT(bytes_written, 0); + + // |accept_bio| should readable. Drain it. Note data is not always available + // from loopback immediately, notably on macOS, so wait for the socket first. + int bytes_read = 0; + while (bytes_read < bytes_written) { + ASSERT_TRUE(WaitForSocket(accept_sock.get(), WaitType::kRead)) + << LastSocketError(); + ret = BIO_read(accept_bio.get(), buf, sizeof(buf)); + ASSERT_GT(ret, 0); + bytes_read += ret; + } + + // |connect_bio| should become writeable again. + ASSERT_TRUE(WaitForSocket(accept_sock.get(), WaitType::kWrite)) + << LastSocketError(); + ret = BIO_write(connect_bio.get(), kTestMessage, sizeof(kTestMessage)); + EXPECT_EQ(static_cast(sizeof(kTestMessage)), ret); + + ASSERT_TRUE(WaitForSocket(accept_sock.get(), WaitType::kRead)) + << LastSocketError(); + ret = BIO_read(accept_bio.get(), buf, sizeof(buf)); + EXPECT_EQ(static_cast(sizeof(kTestMessage)), ret); + EXPECT_EQ(Bytes(buf), Bytes(kTestMessage)); + + // Close one socket. We should get an EOF out the other. + connect_bio.reset(); + connect_sock.reset(); + + ASSERT_TRUE(WaitForSocket(accept_sock.get(), WaitType::kRead)) + << LastSocketError(); + ret = BIO_read(accept_bio.get(), buf, sizeof(buf)); + EXPECT_EQ(ret, 0) << LastSocketError(); + EXPECT_FALSE(BIO_should_read(accept_bio.get())); +} + TEST(BIOTest, Printf) { // Test a short output, a very long one, and various sizes around // 256 (the size of the buffer) to ensure edge cases are correct. From dda9ad6e920168dbb8d20716fa7876154bb77212 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 14 Jul 2023 17:04:13 -0400 Subject: [PATCH 10/11] Tidy up error handling for sockets vs fds On Windows, sockets and fds are different, so we need to be a little carefully. The fd functions (which are really a userspace construct inside the libc) report errors by writing to errno: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/read?view=msvc-170 While the socket functions (which are really thin wrappers over Windows HANDLEs) use WSAGetLastError: https://learn.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-recv https://learn.microsoft.com/en-us/windows/win32/winsock/error-codes-errno-h-errno-and-wsagetlasterror-2 Moreover, the error values are different, so we shouldn't mix them together: https://learn.microsoft.com/en-us/windows/win32/winsock/windows-sockets-error-codes-2 https://learn.microsoft.com/en-us/cpp/c-runtime-library/errno-constants?view=msvc-170 Finally, by borrowing OpenSSL's distinct OPENSSL_NO_SOCK and OPENSSL_NO_POSIX_IO options, we arguably should account for all combinations of one or the other being missing. (Ugh.) To account for that, I've moved bio_fd_should_retry into its own file that isn't conditioned on anything. It only depends on , which is part of the C standard library, and used elsewhere already. Change-Id: I0519d7d68c32062e1220ffca0ab57a9cac9f7e5f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61729 Commit-Queue: David Benjamin Reviewed-by: Bob Beck (cherry picked from commit df13691019be62da728e0c63463fe51bb5d7c29d) --- crypto/CMakeLists.txt | 1 + crypto/bio/connect.c | 8 ++-- crypto/bio/errno.c | 92 ++++++++++++++++++++++++++++++++++++++ crypto/bio/fd.c | 48 +------------------- crypto/bio/internal.h | 20 ++++++--- crypto/bio/socket.c | 4 +- crypto/bio/socket_helper.c | 9 ++++ 7 files changed, 124 insertions(+), 58 deletions(-) create mode 100644 crypto/bio/errno.c diff --git a/crypto/CMakeLists.txt b/crypto/CMakeLists.txt index 19309121df..c391f36302 100644 --- a/crypto/CMakeLists.txt +++ b/crypto/CMakeLists.txt @@ -313,6 +313,7 @@ add_library( bio/bio.c bio/bio_mem.c bio/connect.c + bio/errno.c bio/fd.c bio/file.c bio/hexdump.c diff --git a/crypto/bio/connect.c b/crypto/bio/connect.c index 15626cd551..49f182f268 100644 --- a/crypto/bio/connect.c +++ b/crypto/bio/connect.c @@ -233,7 +233,7 @@ static int conn_state(BIO *bio, BIO_CONNECT *c) { BIO_clear_retry_flags(bio); ret = connect(bio->num, (struct sockaddr*) &c->them, c->them_length); if (ret < 0) { - if (bio_fd_should_retry(ret)) { + if (bio_socket_should_retry(ret)) { BIO_set_flags(bio, (BIO_FLAGS_IO_SPECIAL | BIO_FLAGS_SHOULD_RETRY)); c->state = BIO_CONN_S_BLOCKED_CONNECT; bio->retry_reason = BIO_RR_CONNECT; @@ -252,7 +252,7 @@ static int conn_state(BIO *bio, BIO_CONNECT *c) { case BIO_CONN_S_BLOCKED_CONNECT: i = bio_sock_error(bio->num); if (i) { - if (bio_fd_should_retry(ret)) { + if (bio_socket_should_retry(ret)) { BIO_set_flags(bio, (BIO_FLAGS_IO_SPECIAL | BIO_FLAGS_SHOULD_RETRY)); c->state = BIO_CONN_S_BLOCKED_CONNECT; bio->retry_reason = BIO_RR_CONNECT; @@ -366,7 +366,7 @@ static int conn_read(BIO *bio, char *out, int out_len) { ret = (int)recv(bio->num, out, out_len, 0); BIO_clear_retry_flags(bio); if (ret <= 0) { - if (bio_fd_should_retry(ret)) { + if (bio_socket_should_retry(ret)) { BIO_set_retry_read(bio); } } @@ -390,7 +390,7 @@ static int conn_write(BIO *bio, const char *in, int in_len) { ret = (int)send(bio->num, in, in_len, 0); BIO_clear_retry_flags(bio); if (ret <= 0) { - if (bio_fd_should_retry(ret)) { + if (bio_socket_should_retry(ret)) { BIO_set_retry_write(bio); } } diff --git a/crypto/bio/errno.c b/crypto/bio/errno.c new file mode 100644 index 0000000000..901ea0c4c1 --- /dev/null +++ b/crypto/bio/errno.c @@ -0,0 +1,92 @@ +/* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) + * All rights reserved. + * + * This package is an SSL implementation written + * by Eric Young (eay@cryptsoft.com). + * The implementation was written so as to conform with Netscapes SSL. + * + * This library is free for commercial and non-commercial use as long as + * the following conditions are aheared to. The following conditions + * apply to all code found in this distribution, be it the RC4, RSA, + * lhash, DES, etc., code; not just the SSL code. The SSL documentation + * included with this distribution is covered by the same copyright terms + * except that the holder is Tim Hudson (tjh@cryptsoft.com). + * + * Copyright remains Eric Young's, and as such any Copyright notices in + * the code are not to be removed. + * If this package is used in a product, Eric Young should be given attribution + * as the author of the parts of the library used. + * This can be in the form of a textual message at program startup or + * in documentation (online or textual) provided with the package. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * 1. Redistributions of source code must retain the copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * 3. All advertising materials mentioning features or use of this software + * must display the following acknowledgement: + * "This product includes cryptographic software written by + * Eric Young (eay@cryptsoft.com)" + * The word 'cryptographic' can be left out if the rouines from the library + * being used are not cryptographic related :-). + * 4. If you include any Windows specific code (or a derivative thereof) from + * the apps directory (application code) you must include an acknowledgement: + * "This product includes software written by Tim Hudson (tjh@cryptsoft.com)" + * + * THIS SOFTWARE IS PROVIDED BY ERIC YOUNG ``AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + * + * The licence and distribution terms for any publically available version or + * derivative of this code cannot be changed. i.e. this code cannot simply be + * copied and put under another distribution licence + * [including the GNU Public Licence.] */ + +#include + +#include + +#include "internal.h" + + +int bio_errno_should_retry(int return_value) { + if (return_value != -1) { + return 0; + } + + return +#ifdef EWOULDBLOCK + errno == EWOULDBLOCK || +#endif +#ifdef ENOTCONN + errno == ENOTCONN || +#endif +#ifdef EINTR + errno == EINTR || +#endif +#ifdef EAGAIN + errno == EAGAIN || +#endif +#ifdef EPROTO + errno == EPROTO || +#endif +#ifdef EINPROGRESS + errno == EINPROGRESS || +#endif +#ifdef EALREADY + errno == EALREADY || +#endif + 0; +} diff --git a/crypto/bio/fd.c b/crypto/bio/fd.c index c128e8181d..3b38db8028 100644 --- a/crypto/bio/fd.c +++ b/crypto/bio/fd.c @@ -65,9 +65,6 @@ #include #else #include -OPENSSL_MSVC_PRAGMA(warning(push, 3)) -#include -OPENSSL_MSVC_PRAGMA(warning(pop)) #endif #include @@ -77,59 +74,18 @@ OPENSSL_MSVC_PRAGMA(warning(pop)) #include "../internal.h" -static int bio_fd_non_fatal_error(int err) { - if ( -#ifdef EWOULDBLOCK - err == EWOULDBLOCK || -#endif -#ifdef WSAEWOULDBLOCK - err == WSAEWOULDBLOCK || -#endif -#ifdef ENOTCONN - err == ENOTCONN || -#endif -#ifdef EINTR - err == EINTR || -#endif -#ifdef EAGAIN - err == EAGAIN || -#endif -#ifdef EPROTO - err == EPROTO || -#endif -#ifdef EINPROGRESS - err == EINPROGRESS || -#endif -#ifdef EALREADY - err == EALREADY || -#endif - 0) { - return 1; - } - return 0; -} - #if defined(OPENSSL_WINDOWS) - #define BORINGSSL_ERRNO (int)GetLastError() #define BORINGSSL_CLOSE _close #define BORINGSSL_LSEEK _lseek #define BORINGSSL_READ _read #define BORINGSSL_WRITE _write #else - #define BORINGSSL_ERRNO errno #define BORINGSSL_CLOSE close #define BORINGSSL_LSEEK lseek #define BORINGSSL_READ read #define BORINGSSL_WRITE write #endif -int bio_fd_should_retry(int i) { - if (i == -1) { - return bio_fd_non_fatal_error(BORINGSSL_ERRNO); - } - return 0; -} - BIO *BIO_new_fd(int fd, int close_flag) { BIO *ret = BIO_new(BIO_s_fd()); if (ret == NULL) { @@ -161,7 +117,7 @@ static int fd_read(BIO *b, char *out, int outl) { ret = (int)BORINGSSL_READ(b->num, out, outl); BIO_clear_retry_flags(b); if (ret <= 0) { - if (bio_fd_should_retry(ret)) { + if (bio_errno_should_retry(ret)) { BIO_set_retry_read(b); } } @@ -173,7 +129,7 @@ static int fd_write(BIO *b, const char *in, int inl) { int ret = (int)BORINGSSL_WRITE(b->num, in, inl); BIO_clear_retry_flags(b); if (ret <= 0) { - if (bio_fd_should_retry(ret)) { + if (bio_errno_should_retry(ret)) { BIO_set_retry_write(b); } } diff --git a/crypto/bio/internal.h b/crypto/bio/internal.h index 8ed27dae5f..ea2aa6a993 100644 --- a/crypto/bio/internal.h +++ b/crypto/bio/internal.h @@ -78,7 +78,9 @@ extern "C" { #endif -// BIO_ip_and_port_to_socket_and_addr creates a socket and fills in |*out_addr| +#if !defined(OPENSSL_NO_SOCK) + +// bio_ip_and_port_to_socket_and_addr creates a socket and fills in |*out_addr| // and |*out_addr_length| with the correct values for connecting to |hostname| // on |port_str|. It returns one on success or zero on error. int bio_ip_and_port_to_socket_and_addr(int *out_sock, @@ -87,21 +89,27 @@ int bio_ip_and_port_to_socket_and_addr(int *out_sock, const char *hostname, const char *port_str); -// BIO_socket_nbio sets whether |sock| is non-blocking. It returns one on +// bio_socket_nbio sets whether |sock| is non-blocking. It returns one on // success and zero otherwise. int bio_socket_nbio(int sock, int on); -// BIO_clear_socket_error clears the last system socket error. +// bio_clear_socket_error clears the last system socket error. // // TODO(fork): remove all callers of this. void bio_clear_socket_error(void); -// BIO_sock_error returns the last socket error on |sock|. +// bio_sock_error returns the last socket error on |sock|. int bio_sock_error(int sock); -// BIO_fd_should_retry returns non-zero if |return_value| indicates an error +// bio_socket_should_retry returns non-zero if |return_value| indicates an error +// and the last socket error indicates that it's non-fatal. +int bio_socket_should_retry(int return_value); + +#endif // !OPENSSL_NO_SOCK + +// bio_errno_should_retry returns non-zero if |return_value| indicates an error // and |errno| indicates that it's non-fatal. -int bio_fd_should_retry(int return_value); +int bio_errno_should_retry(int return_value); #if defined(__cplusplus) diff --git a/crypto/bio/socket.c b/crypto/bio/socket.c index 492e841ee5..c86b618b20 100644 --- a/crypto/bio/socket.c +++ b/crypto/bio/socket.c @@ -102,7 +102,7 @@ static int sock_read(BIO *b, char *out, int outl) { #endif BIO_clear_retry_flags(b); if (ret <= 0) { - if (bio_fd_should_retry(ret)) { + if (bio_socket_should_retry(ret)) { BIO_set_retry_read(b); } } @@ -118,7 +118,7 @@ static int sock_write(BIO *b, const char *in, int inl) { #endif BIO_clear_retry_flags(b); if (ret <= 0) { - if (bio_fd_should_retry(ret)) { + if (bio_socket_should_retry(ret)) { BIO_set_retry_write(b); } } diff --git a/crypto/bio/socket_helper.c b/crypto/bio/socket_helper.c index 6ad8a773fa..0b62974b92 100644 --- a/crypto/bio/socket_helper.c +++ b/crypto/bio/socket_helper.c @@ -122,4 +122,13 @@ int bio_sock_error(int sock) { return error; } +int bio_socket_should_retry(int return_value) { +#if defined(OPENSSL_WINDOWS) + return return_value == -1 && WSAGetLastError() == WSAEWOULDBLOCK; +#else + // On POSIX platforms, sockets and fds are the same. + return bio_errno_should_retry(return_value); +#endif +} + #endif // OPENSSL_NO_SOCK From 974956ea55e6f5a35c1bc11208bf206936c8f1f3 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 19 Jul 2023 13:56:50 -0400 Subject: [PATCH 11/11] Silence -Wformat-truncation warning in crypto/err/err.c This warning was being tripped because lib_buf and reason_buf made GCC, incorrectly, believe that the strings could get that long, and then attempted to sum up the snprintf to 120, obtained by inlining some things. Those buffers were larger than they needed to be, so bringing it down is sufficient to silence things. That said, the buffer bounds are supplied by the caller and it is expected that truncation can occur, so the warning is just incorrect. The warning can also be silenced by checking the snprintf return value. As we're already trying to detect truncation, we may as well do it with the return value and skip the extra strlen call. Either of the two changes is sufficient to suppress the warning, but both seem worthwhile, so I've done them both. Change-Id: Ia1b1de67bba55da6f0d07e3682165a1820ce2c9e Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61805 Auto-Submit: David Benjamin Reviewed-by: Bob Beck Commit-Queue: Bob Beck (cherry picked from commit 6bd1e1504670dc96a76eb9858da4117bba586a41) --- crypto/err/err.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/crypto/err/err.c b/crypto/err/err.c index e6fafe7fdd..02cadffbd3 100644 --- a/crypto/err/err.c +++ b/crypto/err/err.c @@ -557,22 +557,21 @@ char *ERR_error_string_n(uint32_t packed_error, char *buf, size_t len) { const char *lib_str = err_lib_error_string(packed_error); const char *reason_str = err_reason_error_string(packed_error); - char lib_buf[64], reason_buf[64]; + char lib_buf[32], reason_buf[32]; if (lib_str == NULL) { snprintf(lib_buf, sizeof(lib_buf), "lib(%u)", lib); lib_str = lib_buf; } - if (reason_str == NULL) { - snprintf(reason_buf, sizeof(reason_buf), "reason(%u)", reason); - reason_str = reason_buf; - } - - snprintf(buf, len, "error:%08" PRIx32 ":%s:OPENSSL_internal:%s", packed_error, - lib_str, reason_str); + if (reason_str == NULL) { + snprintf(reason_buf, sizeof(reason_buf), "reason(%u)", reason); + reason_str = reason_buf; + } - if (strlen(buf) == len - 1) { - // output may be truncated; make sure we always have 5 colon-separated + int ret = snprintf(buf, len, "error:%08" PRIx32 ":%s:OPENSSL_internal:%s", + packed_error, lib_str, reason_str); + if (ret >= 0 && (size_t)ret >= len) { + // The output was truncated; make sure we always have 5 colon-separated // fields, i.e. 4 colons. static const unsigned num_colons = 4; unsigned i;