Remove uses of `strcpy`, `strcat`, and `sprintf`, and handle NULL in some functions. This change reimplements some OpenSSL changes based only on the description of the work in base.h. Change-Id: I1a8b3d2774216c43ab446aa56b31cbb40d58b29d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74847 Reviewed-by: David Benjamin <davidben@google.com> Reviewed-by: Bob Beck <bbe@google.com>
diff --git a/crypto/asn1/a_gentm.cc b/crypto/asn1/a_gentm.cc index e7ec9f7..ae4086f 100644 --- a/crypto/asn1/a_gentm.cc +++ b/crypto/asn1/a_gentm.cc
@@ -58,7 +58,8 @@ } ASN1_GENERALIZEDTIME *ASN1_GENERALIZEDTIME_adj(ASN1_GENERALIZEDTIME *s, - int64_t posix_time, int offset_day, + int64_t posix_time, + int offset_day, long offset_sec) { struct tm data; if (!OPENSSL_posix_to_tm(posix_time, &data)) { @@ -77,12 +78,11 @@ } char buf[16]; - int ret = sprintf(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(); // |sprintf| should write exactly the expected number of bytes. - } + 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); + // |snprintf| must write exactly 15 bytes (plus the NUL) to the buffer. + BSSL_CHECK(ret == static_cast<int>(sizeof(buf) - 1)); int free_s = 0; if (s == NULL) {
diff --git a/crypto/asn1/a_time.cc b/crypto/asn1/a_time.cc index 6aa1de9..6ab5d93 100644 --- a/crypto/asn1/a_time.cc +++ b/crypto/asn1/a_time.cc
@@ -69,15 +69,13 @@ } // Convert an ASN1_TIME structure to GeneralizedTime -ASN1_GENERALIZEDTIME *ASN1_TIME_to_generalizedtime(const ASN1_TIME *t, +ASN1_GENERALIZEDTIME *ASN1_TIME_to_generalizedtime(const ASN1_TIME *in, ASN1_GENERALIZEDTIME **out) { - ASN1_GENERALIZEDTIME *ret = NULL; - char *str; - - if (!ASN1_TIME_check(t)) { + if (!ASN1_TIME_check(in)) { return NULL; } + ASN1_GENERALIZEDTIME *ret = NULL; if (!out || !*out) { if (!(ret = ASN1_GENERALIZEDTIME_new())) { goto err; @@ -87,26 +85,30 @@ } // If already GeneralizedTime just copy across - if (t->type == V_ASN1_GENERALIZEDTIME) { - if (!ASN1_STRING_set(ret, t->data, t->length)) { + if (in->type == V_ASN1_GENERALIZEDTIME) { + if (!ASN1_STRING_set(ret, in->data, in->length)) { goto err; } goto done; } - // grow the string - if (!ASN1_STRING_set(ret, NULL, t->length + 2)) { + // Grow the string to accomodate the two-digit century. + if (!ASN1_STRING_set(ret, NULL, in->length + 2)) { goto err; } - str = (char *)ret->data; - // Work out the century and prepend - if (t->data[0] >= '5') { - strcpy(str, "19"); - } else { - strcpy(str, "20"); - } - strcat(str, (const char *)t->data); + { + char *const out_str = (char *)ret->data; + // |ASN1_STRING_set| also allocates an additional byte for a trailing NUL. + const size_t out_str_capacity = in->length + 2 + 1; + // Work out the century and prepend + if (in->data[0] >= '5') { + OPENSSL_strlcpy(out_str, "19", out_str_capacity); + } else { + OPENSSL_strlcpy(out_str, "20", out_str_capacity); + } + OPENSSL_strlcat(out_str, (const char *)in->data, out_str_capacity); + } done: if (out != NULL && *out == NULL) { @@ -128,7 +130,7 @@ int ASN1_TIME_set_string_X509(ASN1_TIME *s, const char *str) { CBS cbs; - CBS_init(&cbs, (const uint8_t*)str, strlen(str)); + CBS_init(&cbs, (const uint8_t *)str, strlen(str)); int type; struct tm tm; if (CBS_parse_utc_time(&cbs, /*out_tm=*/NULL,
diff --git a/crypto/asn1/a_utctm.cc b/crypto/asn1/a_utctm.cc index c25c39f..993dd5d 100644 --- a/crypto/asn1/a_utctm.cc +++ b/crypto/asn1/a_utctm.cc
@@ -60,8 +60,8 @@ return ASN1_UTCTIME_adj(s, posix_time, 0, 0); } -ASN1_UTCTIME *ASN1_UTCTIME_adj(ASN1_UTCTIME *s, int64_t posix_time, int offset_day, - long offset_sec) { +ASN1_UTCTIME *ASN1_UTCTIME_adj(ASN1_UTCTIME *s, int64_t posix_time, + int offset_day, long offset_sec) { struct tm data; if (!OPENSSL_posix_to_tm(posix_time, &data)) { return NULL; @@ -78,12 +78,11 @@ } char buf[14]; - int ret = sprintf(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(); // |sprintf| should write exactly the expected number of bytes. - } + 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); + // |snprintf| must write exactly 15 bytes (plus the NUL) to the buffer. + BSSL_CHECK(ret == static_cast<int>(sizeof(buf) - 1)); int free_s = 0; if (s == NULL) {
diff --git a/crypto/bio/connect.cc b/crypto/bio/connect.cc index c97f320..9056fff 100644 --- a/crypto/bio/connect.cc +++ b/crypto/bio/connect.cc
@@ -260,6 +260,9 @@ } static void BIO_CONNECT_free(BIO_CONNECT *c) { + if (c == nullptr) { + return; + } OPENSSL_free(c->param_hostname); OPENSSL_free(c->param_port); OPENSSL_free(c);
diff --git a/crypto/buf/buf.cc b/crypto/buf/buf.cc index 0010260..8c00082 100644 --- a/crypto/buf/buf.cc +++ b/crypto/buf/buf.cc
@@ -22,6 +22,9 @@ } void BUF_MEM_free(BUF_MEM *buf) { + if (buf == nullptr) { + return; + } OPENSSL_free(buf->data); OPENSSL_free(buf); }
diff --git a/crypto/conf/conf.cc b/crypto/conf/conf.cc index dfb690a..9b28697 100644 --- a/crypto/conf/conf.cc +++ b/crypto/conf/conf.cc
@@ -370,7 +370,6 @@ char *s, *p, *end; int again; long eline = 0; - char btmp[DECIMAL_SIZE(eline) + 1]; CONF_VALUE *v = NULL; CONF_SECTION *sv = NULL; char *section = NULL, *buf; @@ -551,8 +550,7 @@ if (out_error_line != NULL) { *out_error_line = eline; } - sprintf(btmp, "%ld", eline); - ERR_add_error_data(2, "line ", btmp); + ERR_add_error_dataf("line %ld", eline); value_free(v); return 0; }
diff --git a/crypto/fipsmodule/bn/ctx.cc.inc b/crypto/fipsmodule/bn/ctx.cc.inc index 7d012bb..8ddcf10 100644 --- a/crypto/fipsmodule/bn/ctx.cc.inc +++ b/crypto/fipsmodule/bn/ctx.cc.inc
@@ -79,6 +79,9 @@ // All |BN_CTX_start| calls must be matched with |BN_CTX_end|, otherwise the // function may use more memory than expected, potentially without bound if // done in a loop. Assert that all |BIGNUM|s have been released. + if (ctx == nullptr) { + return; + } assert(ctx->used == 0 || ctx->error); sk_BIGNUM_pop_free(ctx->bignums, BN_free); BN_STACK_cleanup(&ctx->stack);
diff --git a/crypto/fipsmodule/bn/exponentiation.cc.inc b/crypto/fipsmodule/bn/exponentiation.cc.inc index df19716..f193e55 100644 --- a/crypto/fipsmodule/bn/exponentiation.cc.inc +++ b/crypto/fipsmodule/bn/exponentiation.cc.inc
@@ -136,6 +136,9 @@ } static void BN_RECP_CTX_free(BN_RECP_CTX *recp) { + if (recp == nullptr) { + return; + } BN_free(&recp->N); BN_free(&recp->Nr); }
diff --git a/crypto/fipsmodule/bn/montgomery.cc.inc b/crypto/fipsmodule/bn/montgomery.cc.inc index 22927ee..9cf42f1 100644 --- a/crypto/fipsmodule/bn/montgomery.cc.inc +++ b/crypto/fipsmodule/bn/montgomery.cc.inc
@@ -45,6 +45,9 @@ } void BN_MONT_CTX_free(BN_MONT_CTX *mont) { + if (mont == nullptr) { + return; + } bn_mont_ctx_cleanup(mont); OPENSSL_free(mont); }
diff --git a/crypto/fipsmodule/rsa/blinding.cc.inc b/crypto/fipsmodule/rsa/blinding.cc.inc index 628f1ce..12ae91e 100644 --- a/crypto/fipsmodule/rsa/blinding.cc.inc +++ b/crypto/fipsmodule/rsa/blinding.cc.inc
@@ -58,6 +58,9 @@ } void BN_BLINDING_free(BN_BLINDING *r) { + if (r == nullptr) { + return; + } BN_free(r->A); BN_free(r->Ai); OPENSSL_free(r);
diff --git a/crypto/pem/pem_lib.cc b/crypto/pem/pem_lib.cc index 1c3e1ef..61ab551 100644 --- a/crypto/pem/pem_lib.cc +++ b/crypto/pem/pem_lib.cc
@@ -48,9 +48,9 @@ str = "BAD-TYPE"; } - strcat(buf, "Proc-Type: 4,"); - strcat(buf, str); - strcat(buf, "\n"); + OPENSSL_strlcat(buf, "Proc-Type: 4,", PEM_BUFSIZE); + OPENSSL_strlcat(buf, str, PEM_BUFSIZE); + OPENSSL_strlcat(buf, "\n", PEM_BUFSIZE); } // PEM_dek_info appends a DEK-Info header to |buf|, with an algorithm of |type| @@ -59,16 +59,22 @@ char *str) { static const unsigned char map[17] = "0123456789ABCDEF"; - strcat(buf, "DEK-Info: "); - strcat(buf, type); - strcat(buf, ","); - size_t buf_len = strlen(buf); - for (size_t i = 0; i < len; i++) { - buf[buf_len + i * 2] = map[(str[i] >> 4) & 0x0f]; - buf[buf_len + i * 2 + 1] = map[(str[i]) & 0x0f]; + OPENSSL_strlcat(buf, "DEK-Info: ", PEM_BUFSIZE); + OPENSSL_strlcat(buf, type, PEM_BUFSIZE); + OPENSSL_strlcat(buf, ",", PEM_BUFSIZE); + + const size_t used = strlen(buf); + const size_t available = PEM_BUFSIZE - used; + if (len * 2 < len || len * 2 + 2 < len || available < len * 2 + 2) { + return; } - buf[buf_len + len * 2] = '\n'; - buf[buf_len + len * 2 + 1] = '\0'; + + for (size_t i = 0; i < len; i++) { + buf[used + i * 2] = map[(str[i] >> 4) & 0x0f]; + buf[used + i * 2 + 1] = map[(str[i]) & 0x0f]; + } + buf[used + len * 2] = '\n'; + buf[used + len * 2 + 1] = '\0'; } void *PEM_ASN1_read(d2i_of_void *d2i, const char *name, FILE *fp, void **x,
diff --git a/crypto/x509/by_dir.cc b/crypto/x509/by_dir.cc index f7abf21..7f87814 100644 --- a/crypto/x509/by_dir.cc +++ b/crypto/x509/by_dir.cc
@@ -199,7 +199,7 @@ } data; int ok = 0; size_t i; - int j, k; + int k; uint32_t h; uint32_t hash_array[2]; int hash_index; @@ -242,8 +242,10 @@ size_t idx; BY_DIR_HASH htmp, *hent; ent = sk_BY_DIR_ENTRY_value(ctx->dirs, i); - j = strlen(ent->dir) + 1 + 8 + 6 + 1 + 1; - if (!BUF_MEM_grow(b, j)) { + if (!BUF_MEM_grow(b, strlen(ent->dir) + /* foreslash */ 1 + + /* h, in hex */ 8 + /* period */ 1 + + /* optional `postfix` */ 1 + + /* k */ DECIMAL_SIZE(k) + /* NUL */ 1)) { goto finish; } if (type == X509_LU_CRL && ent->hashes) { @@ -262,7 +264,8 @@ hent = NULL; } for (;;) { - sprintf(b->data, "%s/%08" PRIx32 ".%s%d", ent->dir, h, postfix, k); + snprintf(b->data, b->max, "%s/%08" PRIx32 ".%s%d", ent->dir, h, postfix, + k); if (type == X509_LU_X509) { if ((X509_load_cert_file(xl, b->data, ent->dir_type)) == 0) { // Don't expose the lower level error, All of these boil
diff --git a/crypto/x509/v3_alt.cc b/crypto/x509/v3_alt.cc index adc8f6d..7b9fb0a 100644 --- a/crypto/x509/v3_alt.cc +++ b/crypto/x509/v3_alt.cc
@@ -126,12 +126,12 @@ case GEN_IPADD: p = gen->d.ip->data; if (gen->d.ip->length == 4) { - sprintf(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]; - sprintf(htmp, "%X", v); + snprintf(htmp, sizeof(htmp), "%X", v); p += 2; OPENSSL_strlcat(oline, htmp, sizeof(oline)); if (i != 7) {
diff --git a/crypto/x509/v3_info.cc b/crypto/x509/v3_info.cc index 59a854b..ca52d01 100644 --- a/crypto/x509/v3_info.cc +++ b/crypto/x509/v3_info.cc
@@ -80,8 +80,8 @@ const AUTHORITY_INFO_ACCESS *ainfo = reinterpret_cast<const AUTHORITY_INFO_ACCESS *>(ext); ACCESS_DESCRIPTION *desc; - int nlen; - char objtmp[80], *ntmp; + int name_len; + char objtmp[80], *name; CONF_VALUE *vtmp; STACK_OF(CONF_VALUE) *tret = ret; @@ -96,16 +96,17 @@ tret = tmp; vtmp = sk_CONF_VALUE_value(tret, i); i2t_ASN1_OBJECT(objtmp, sizeof objtmp, desc->method); - nlen = strlen(objtmp) + 3 + strlen(vtmp->name) + 1; - ntmp = reinterpret_cast<char *>(OPENSSL_malloc(nlen)); - if (ntmp == NULL) { + + name_len = strlen(objtmp) + 3 + strlen(vtmp->name) + 1; + name = reinterpret_cast<char *>(OPENSSL_malloc(name_len)); + if (name == NULL) { goto err; } - strcpy(ntmp, objtmp); - strcat(ntmp, " - "); - strcat(ntmp, vtmp->name); + OPENSSL_strlcpy(name, objtmp, name_len); + OPENSSL_strlcat(name, " - ", name_len); + OPENSSL_strlcat(name, vtmp->name, name_len); OPENSSL_free(vtmp->name); - vtmp->name = ntmp; + vtmp->name = name; } if (ret == NULL && tret == NULL) { return sk_CONF_VALUE_new_null();
diff --git a/crypto/x509/x509_lu.cc b/crypto/x509/x509_lu.cc index 373015b..4e2df3b 100644 --- a/crypto/x509/x509_lu.cc +++ b/crypto/x509/x509_lu.cc
@@ -130,7 +130,7 @@ } void X509_STORE_free(X509_STORE *vfy) { - if (!CRYPTO_refcount_dec_and_test_zero(&vfy->references)) { + if (vfy == nullptr || !CRYPTO_refcount_dec_and_test_zero(&vfy->references)) { return; }
diff --git a/include/openssl/base.h b/include/openssl/base.h index 21afad8..8f16fb7 100644 --- a/include/openssl/base.h +++ b/include/openssl/base.h
@@ -13,25 +13,6 @@ // This file should be the first included by all BoringSSL headers. -// Remove this after importing the following to remove all instances of -// |sprintf|, |strcat|, and |strcpy| from the codebase. -// * https://github.com/openssl/openssl/commit/86ba26c80a49aee3c588d286d91eb3843529f7e2 -// * https://github.com/openssl/openssl/commit/60eba30f60de55e3c782469fa555eede82606099 -// * https://github.com/openssl/openssl/commit/a2371fa93365cc0bc0e46b9d65f3a47a074b1c30 -// -// Also after importing -// https://github.com/openssl/openssl/commit/e6e9170d6e28038768895e1af18e3aad8093bf4b -// to make the following functions accept a NULL parameter: -// * BIO_CONNECT_free -// * BUF_MEM_free -// * BN_CTX_free -// * BN_RECP_CTX_free -// * BN_MONT_CTX_free -// * BN_BLINDING_free -// * X509_STORE_free -// * SSL_SESSION_free -#error "Do not build BoringSSL at this revision" - #include <stddef.h> #include <stdint.h> #include <stdlib.h> @@ -421,7 +402,7 @@ #define BORINGSSL_NO_CXX #endif -} // extern C++ +} // extern C++ #endif // !BORINGSSL_NO_CXX #if defined(BORINGSSL_NO_CXX)
diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc index cf57365..52ce2d1 100644 --- a/ssl/ssl_session.cc +++ b/ssl/ssl_session.cc
@@ -830,6 +830,9 @@ } void SSL_SESSION_free(SSL_SESSION *session) { + if (session == nullptr) { + return; + } session->DecRefInternal(); }