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();
}
