From eee9552212ecc9e19bc09ea8a1b8428dc7394f45 Mon Sep 17 00:00:00 2001 From: Pauli Date: Thu, 6 Jul 2017 10:37:10 +1000 Subject: [PATCH] Bounds check string functions in apps. This includes strcat, strcpy and sprintf. In the x509 app, the code has been cleaned up as well. Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/3868) --- apps/enc.c | 10 +++++----- apps/pkcs12.c | 8 +++++--- apps/s_time.c | 28 +++++++++++++++++----------- apps/x509.c | 33 +++++++++++++-------------------- 4 files changed, 40 insertions(+), 39 deletions(-) diff --git a/apps/enc.c b/apps/enc.c index 338307330a..cc6fa0a1c3 100644 --- a/apps/enc.c +++ b/apps/enc.c @@ -312,7 +312,7 @@ int enc_main(int argc, char **argv) for (;;) { char prompt[200]; - sprintf(prompt, "enter %s %s password:", + BIO_snprintf(prompt, sizeof(prompt), "enter %s %s password:", OBJ_nid2ln(EVP_CIPHER_nid(cipher)), (enc) ? "encryption" : "decryption"); strbuf[0] = '\0'; @@ -565,7 +565,7 @@ int enc_main(int argc, char **argv) #endif release_engine(e); OPENSSL_free(pass); - return (ret); + return ret; } static void show_ciphers(const OBJ_NAME *name, void *arg) @@ -599,7 +599,7 @@ static int set_hex(char *in, unsigned char *out, int size) n = strlen(in); if (n > (size * 2)) { BIO_printf(bio_err, "hex string is too long\n"); - return (0); + return 0; } memset(out, 0, size); for (i = 0; i < n; i++) { @@ -609,7 +609,7 @@ static int set_hex(char *in, unsigned char *out, int size) break; if (!isxdigit(j)) { BIO_printf(bio_err, "non-hex digit\n"); - return (0); + return 0; } j = (unsigned char)OPENSSL_hexchar2int(j); if (i & 1) @@ -617,5 +617,5 @@ static int set_hex(char *in, unsigned char *out, int size) else out[i / 2] = (j << 4); } - return (1); + return 1; } diff --git a/apps/pkcs12.c b/apps/pkcs12.c index 82d2bb972e..2ec8fdc856 100644 --- a/apps/pkcs12.c +++ b/apps/pkcs12.c @@ -27,6 +27,8 @@ NON_EMPTY_TRANSLATION_UNIT # define CLCERTS 0x8 # define CACERTS 0x10 +#define PASSWD_BUF_SIZE 2048 + static int get_cert_chain(X509 *cert, X509_STORE *store, STACK_OF(X509) **chain); int dump_certs_keys_p12(BIO *out, const PKCS12 *p12, @@ -119,7 +121,7 @@ int pkcs12_main(int argc, char **argv) { char *infile = NULL, *outfile = NULL, *keyname = NULL, *certfile = NULL; char *name = NULL, *csp_name = NULL; - char pass[2048] = "", macpass[2048] = ""; + char pass[PASSWD_BUF_SIZE] = "", macpass[PASSWD_BUF_SIZE] = ""; int export_cert = 0, options = 0, chain = 0, twopass = 0, keytype = 0; int iter = PKCS12_DEFAULT_ITER, maciter = PKCS12_DEFAULT_ITER; # ifndef OPENSSL_NO_RC2 @@ -455,7 +457,7 @@ int pkcs12_main(int argc, char **argv) } if (!twopass) - strcpy(macpass, pass); + OPENSSL_strlcpy(macpass, pass, sizeof(macpass)); p12 = PKCS12_create(cpass, name, key, ucert, certs, key_pbe, cert_pbe, iter, -1, keytype); @@ -583,7 +585,7 @@ int pkcs12_main(int argc, char **argv) OPENSSL_free(badpass); OPENSSL_free(passin); OPENSSL_free(passout); - return (ret); + return ret; } int dump_certs_keys_p12(BIO *out, const PKCS12 *p12, const char *pass, diff --git a/apps/s_time.c b/apps/s_time.c index c4f4037363..b10c7e1da7 100644 --- a/apps/s_time.c +++ b/apps/s_time.c @@ -49,7 +49,13 @@ static SSL *doConnection(SSL *scon, const char *host, SSL_CTX *ctx); +/* + * Define a HTTP get command globally. + * Also define the size of the command, this is two bytes less than + * the size of the string because the %s is replaced by the URL. + */ static const char fmt_http_get_cmd[] = "GET %s HTTP/1.0\r\n\r\n"; +static const size_t fmt_http_get_cmd_size = sizeof(fmt_http_get_cmd) - 2; typedef enum OPTION_choice { OPT_ERR = -1, OPT_EOF = 0, OPT_HELP, @@ -173,7 +179,7 @@ int s_time_main(int argc, char **argv) break; case OPT_WWW: www_path = opt_arg(); - buf_size = strlen(www_path) + sizeof(fmt_http_get_cmd); + buf_size = strlen(www_path) + fmt_http_get_cmd_size; if (buf_size > sizeof(buf)) { BIO_printf(bio_err, "%s: -www option is too long\n", prog); goto end; @@ -230,9 +236,9 @@ int s_time_main(int argc, char **argv) goto end; if (www_path != NULL) { - sprintf(buf, fmt_http_get_cmd, www_path); - buf_len = strlen(buf); - if (SSL_write(scon, buf, buf_len) <= 0) + buf_len = BIO_snprintf(buf, sizeof(buf), fmt_http_get_cmd, + www_path); + if (buf_len <= 0 || SSL_write(scon, buf, buf_len) <= 0) goto end; while ((i = SSL_read(scon, buf, sizeof(buf))) > 0) bytes_read += i; @@ -288,9 +294,8 @@ int s_time_main(int argc, char **argv) } if (www_path != NULL) { - sprintf(buf, fmt_http_get_cmd, www_path); - buf_len = strlen(buf); - if (SSL_write(scon, buf, buf_len) <= 0) + buf_len = BIO_snprintf(buf, sizeof(buf), fmt_http_get_cmd, www_path); + if (buf_len <= 0 || SSL_write(scon, buf, buf_len) <= 0) goto end; while (SSL_read(scon, buf, sizeof(buf)) > 0) continue; @@ -319,8 +324,9 @@ int s_time_main(int argc, char **argv) goto end; if (www_path != NULL) { - sprintf(buf, "GET %s HTTP/1.0\r\n\r\n", www_path); - if (SSL_write(scon, buf, strlen(buf)) <= 0) + buf_len = BIO_snprintf(buf, sizeof(buf), fmt_http_get_cmd, + www_path); + if (buf_len <= 0 || SSL_write(scon, buf, buf_len) <= 0) goto end; while ((i = SSL_read(scon, buf, sizeof(buf))) > 0) bytes_read += i; @@ -361,7 +367,7 @@ int s_time_main(int argc, char **argv) end: SSL_free(scon); SSL_CTX_free(ctx); - return (ret); + return ret; } /*- @@ -375,7 +381,7 @@ static SSL *doConnection(SSL *scon, const char *host, SSL_CTX *ctx) fd_set readfds; if ((conn = BIO_new(BIO_s_connect())) == NULL) - return (NULL); + return NULL; BIO_set_conn_hostname(conn, host); diff --git a/apps/x509.c b/apps/x509.c index 484192bbf1..840e12778b 100644 --- a/apps/x509.c +++ b/apps/x509.c @@ -890,34 +890,27 @@ int x509_main(int argc, char **argv) ASN1_OBJECT_free(objtmp); release_engine(e); OPENSSL_free(passin); - return (ret); + return ret; } -static ASN1_INTEGER *x509_load_serial(const char *CAfile, const char *serialfile, - int create) +static ASN1_INTEGER *x509_load_serial(const char *CAfile, + const char *serialfile, int create) { - char *buf = NULL, *p; + char *buf = NULL; ASN1_INTEGER *bs = NULL; BIGNUM *serial = NULL; - size_t len; - len = ((serialfile == NULL) - ? (strlen(CAfile) + strlen(POSTFIX) + 1) - : (strlen(serialfile))) + 1; - buf = app_malloc(len, "serial# buffer"); if (serialfile == NULL) { - strcpy(buf, CAfile); - for (p = buf; *p; p++) - if (*p == '.') { - *p = '\0'; - break; - } - strcat(buf, POSTFIX); - } else { - strcpy(buf, serialfile); + const char *p = strchr(CAfile, '.'); + size_t len = p != NULL ? (size_t)(p - CAfile) : strlen(CAfile); + + buf = app_malloc(len + sizeof(POSTFIX), "serial# buffer"); + memcpy(buf, CAfile, len); + memcpy(buf + len, POSTFIX, sizeof(POSTFIX)); + serialfile = buf; } - serial = load_serial(buf, create, NULL); + serial = load_serial(serialfile, create, NULL); if (serial == NULL) goto end; @@ -926,7 +919,7 @@ static ASN1_INTEGER *x509_load_serial(const char *CAfile, const char *serialfile goto end; } - if (!save_serial(buf, NULL, serial, &bs)) + if (!save_serial(serialfile, NULL, serial, &bs)) goto end; end: -- 2.25.1