From 762f1ebe8d1b26e78cd4923f832a611c8a5f0a89 Mon Sep 17 00:00:00 2001 From: Masahiro Yamada Date: Mon, 6 Feb 2017 21:15:01 +0900 Subject: [PATCH 1/3] cert_create: fix memory leak bug caused by key container overwrite In the current code, both key_load() and key_create() call key_new() to allocate a key container (and they do not free it even if they fail). If a specific key is not given by the command option, key_load() fails, then key_create() is called. At this point, the key container that has been allocated in key_load() is still alive, and it is overwritten by a new key container created by key_create(). Move the key_new() call to the main() function to make sure it is called just once for each descriptor. While we are here, let's fix one more bug; the error handling code ERROR("Malloc error while loading '%s'\n", keys[i].fn); is wrong because keys[i].fn is NULL pointer unless a specific key is given by the command option. This code could be run in either case. Signed-off-by: Masahiro Yamada --- tools/cert_create/include/key.h | 1 + tools/cert_create/src/key.c | 13 +------------ tools/cert_create/src/main.c | 11 ++++++----- 3 files changed, 8 insertions(+), 17 deletions(-) diff --git a/tools/cert_create/include/key.h b/tools/cert_create/include/key.h index f60997f0fb..433f72cef1 100644 --- a/tools/cert_create/include/key.h +++ b/tools/cert_create/include/key.h @@ -73,6 +73,7 @@ typedef struct key_s { /* Exported API */ int key_init(void); key_t *key_get_by_opt(const char *opt); +int key_new(key_t *key); int key_create(key_t *key, int type); int key_load(key_t *key, unsigned int *err_code); int key_store(key_t *key); diff --git a/tools/cert_create/src/key.c b/tools/cert_create/src/key.c index a7ee75969c..47c152c72c 100644 --- a/tools/cert_create/src/key.c +++ b/tools/cert_create/src/key.c @@ -49,7 +49,7 @@ /* * Create a new key container */ -static int key_new(key_t *key) +int key_new(key_t *key) { /* Create key pair container */ key->key = EVP_PKEY_new(); @@ -123,11 +123,6 @@ int key_create(key_t *key, int type) return 0; } - /* Create OpenSSL key container */ - if (!key_new(key)) { - return 0; - } - if (key_create_fn[type]) { return key_create_fn[type](key); } @@ -140,12 +135,6 @@ int key_load(key_t *key, unsigned int *err_code) FILE *fp = NULL; EVP_PKEY *k = NULL; - /* Create OpenSSL key container */ - if (!key_new(key)) { - *err_code = KEY_ERR_MALLOC; - return 0; - } - if (key->fn) { /* Load key from file */ fp = fopen(key->fn, "r"); diff --git a/tools/cert_create/src/main.c b/tools/cert_create/src/main.c index c58f41deab..dac9e57c0d 100644 --- a/tools/cert_create/src/main.c +++ b/tools/cert_create/src/main.c @@ -367,6 +367,11 @@ int main(int argc, char *argv[]) /* Load private keys from files (or generate new ones) */ for (i = 0 ; i < num_keys ; i++) { + if (!key_new(&keys[i])) { + ERROR("Failed to allocate key container\n"); + exit(1); + } + /* First try to load the key from disk */ if (key_load(&keys[i], &err_code)) { /* Key loaded successfully */ @@ -374,11 +379,7 @@ int main(int argc, char *argv[]) } /* Key not loaded. Check the error code */ - if (err_code == KEY_ERR_MALLOC) { - /* Cannot allocate memory. Abort. */ - ERROR("Malloc error while loading '%s'\n", keys[i].fn); - exit(1); - } else if (err_code == KEY_ERR_LOAD) { + if (err_code == KEY_ERR_LOAD) { /* File exists, but it does not contain a valid private * key. Abort. */ ERROR("Error loading '%s'\n", keys[i].fn); From 559eb8b79afb03a3840b90f5070df692a693de05 Mon Sep 17 00:00:00 2001 From: Masahiro Yamada Date: Mon, 6 Feb 2017 19:00:11 +0900 Subject: [PATCH 2/3] cert_create: merge successive i2d_ASN1_INTEGER() calls The ext_new_nvcounter() function calls i2d_ASN1_INTEGER() twice; the first call to get the return value "sz", and the second one for writing data into the buffer. This is actually redundant. We can do both by one function call. Signed-off-by: Masahiro Yamada --- tools/cert_create/src/ext.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tools/cert_create/src/ext.c b/tools/cert_create/src/ext.c index 3f56edb756..b261951e2c 100644 --- a/tools/cert_create/src/ext.c +++ b/tools/cert_create/src/ext.c @@ -262,8 +262,7 @@ X509_EXTENSION *ext_new_nvcounter(int nid, int crit, int value) /* Encode counter */ counter = ASN1_INTEGER_new(); ASN1_INTEGER_set(counter, value); - sz = i2d_ASN1_INTEGER(counter, NULL); - i2d_ASN1_INTEGER(counter, &p); + sz = i2d_ASN1_INTEGER(counter, &p); /* Create the extension */ ex = ext_new(nid, crit, p, sz); From c893c73309aab3a9acfa0f0508a194a2078da556 Mon Sep 17 00:00:00 2001 From: Masahiro Yamada Date: Mon, 6 Feb 2017 19:47:44 +0900 Subject: [PATCH 3/3] cert_create: remove unneeded initializers These variables store return values of functions. Remove all of meaningless initializers. Signed-off-by: Masahiro Yamada --- tools/cert_create/src/cert.c | 10 +++++----- tools/cert_create/src/ext.c | 26 +++++++++++++------------- tools/cert_create/src/key.c | 15 +++++++-------- tools/cert_create/src/main.c | 13 ++++++------- 4 files changed, 31 insertions(+), 33 deletions(-) diff --git a/tools/cert_create/src/cert.c b/tools/cert_create/src/cert.c index a559832ea5..375c66bf11 100644 --- a/tools/cert_create/src/cert.c +++ b/tools/cert_create/src/cert.c @@ -103,10 +103,10 @@ int cert_new(cert_t *cert, int days, int ca, STACK_OF(X509_EXTENSION) * sk) cert_t *issuer_cert = &certs[cert->issuer]; EVP_PKEY *ikey = keys[issuer_cert->key].key; X509 *issuer = issuer_cert->x; - X509 *x = NULL; - X509_EXTENSION *ex = NULL; - X509_NAME *name = NULL; - ASN1_INTEGER *sno = NULL; + X509 *x; + X509_EXTENSION *ex; + X509_NAME *name; + ASN1_INTEGER *sno; int i, num; /* Create the certificate structure */ @@ -202,7 +202,7 @@ int cert_init(void) cert_t *cert_get_by_opt(const char *opt) { - cert_t *cert = NULL; + cert_t *cert; unsigned int i; for (i = 0; i < num_certs; i++) { diff --git a/tools/cert_create/src/ext.c b/tools/cert_create/src/ext.c index b261951e2c..a50919eee6 100644 --- a/tools/cert_create/src/ext.c +++ b/tools/cert_create/src/ext.c @@ -181,13 +181,13 @@ X509_EXTENSION *ext_new(int nid, int crit, unsigned char *data, int len) X509_EXTENSION *ext_new_hash(int nid, int crit, const EVP_MD *md, unsigned char *buf, size_t len) { - X509_EXTENSION *ex = NULL; - ASN1_OCTET_STRING *octet = NULL; - HASH *hash = NULL; - ASN1_OBJECT *algorithm = NULL; - X509_ALGOR *x509_algor = NULL; + X509_EXTENSION *ex; + ASN1_OCTET_STRING *octet; + HASH *hash; + ASN1_OBJECT *algorithm; + X509_ALGOR *x509_algor; unsigned char *p = NULL; - int sz = -1; + int sz; /* OBJECT_IDENTIFIER with hash algorithm */ algorithm = OBJ_nid2obj(md->type); @@ -254,10 +254,10 @@ X509_EXTENSION *ext_new_hash(int nid, int crit, const EVP_MD *md, */ X509_EXTENSION *ext_new_nvcounter(int nid, int crit, int value) { - X509_EXTENSION *ex = NULL; - ASN1_INTEGER *counter = NULL; + X509_EXTENSION *ex; + ASN1_INTEGER *counter; unsigned char *p = NULL; - int sz = -1; + int sz; /* Encode counter */ counter = ASN1_INTEGER_new(); @@ -291,9 +291,9 @@ X509_EXTENSION *ext_new_nvcounter(int nid, int crit, int value) */ X509_EXTENSION *ext_new_key(int nid, int crit, EVP_PKEY *k) { - X509_EXTENSION *ex = NULL; - unsigned char *p = NULL; - int sz = -1; + X509_EXTENSION *ex; + unsigned char *p; + int sz; /* Encode key */ BIO *mem = BIO_new(BIO_s_mem()); @@ -315,7 +315,7 @@ X509_EXTENSION *ext_new_key(int nid, int crit, EVP_PKEY *k) ext_t *ext_get_by_opt(const char *opt) { - ext_t *ext = NULL; + ext_t *ext; unsigned int i; /* Sequential search. This is not a performance concern since the number diff --git a/tools/cert_create/src/key.c b/tools/cert_create/src/key.c index 47c152c72c..ce0e4da6ee 100644 --- a/tools/cert_create/src/key.c +++ b/tools/cert_create/src/key.c @@ -62,7 +62,7 @@ int key_new(key_t *key) static int key_create_rsa(key_t *key) { - RSA *rsa = NULL; + RSA *rsa; rsa = RSA_generate_key(RSA_KEY_BITS, RSA_F4, NULL, NULL); if (rsa == NULL) { @@ -83,7 +83,7 @@ err: #ifndef OPENSSL_NO_EC static int key_create_ecdsa(key_t *key) { - EC_KEY *ec = NULL; + EC_KEY *ec; ec = EC_KEY_new_by_curve_name(NID_X9_62_prime256v1); if (ec == NULL) { @@ -132,8 +132,8 @@ int key_create(key_t *key, int type) int key_load(key_t *key, unsigned int *err_code) { - FILE *fp = NULL; - EVP_PKEY *k = NULL; + FILE *fp; + EVP_PKEY *k; if (key->fn) { /* Load key from file */ @@ -162,7 +162,7 @@ int key_load(key_t *key, unsigned int *err_code) int key_store(key_t *key) { - FILE *fp = NULL; + FILE *fp; if (key->fn) { fp = fopen(key->fn, "w"); @@ -185,7 +185,6 @@ int key_init(void) { cmd_opt_t cmd_opt; key_t *key; - int rc = 0; unsigned int i; for (i = 0; i < num_keys; i++) { @@ -200,12 +199,12 @@ int key_init(void) } } - return rc; + return 0; } key_t *key_get_by_opt(const char *opt) { - key_t *key = NULL; + key_t *key; unsigned int i; /* Sequential search. This is not a performance concern since the number diff --git a/tools/cert_create/src/main.c b/tools/cert_create/src/main.c index dac9e57c0d..c9c962223c 100644 --- a/tools/cert_create/src/main.c +++ b/tools/cert_create/src/main.c @@ -134,7 +134,6 @@ static void print_help(const char *cmd, const struct option *long_opt) printf("\t%s [OPTIONS]\n\n", cmd); printf("Available options:\n"); - i = 0; opt = long_opt; while (opt->name) { p = line; @@ -261,12 +260,12 @@ static const cmd_opt_t common_cmd_opt[] = { int main(int argc, char *argv[]) { - STACK_OF(X509_EXTENSION) * sk = NULL; - X509_EXTENSION *cert_ext = NULL; - ext_t *ext = NULL; - key_t *key = NULL; - cert_t *cert = NULL; - FILE *file = NULL; + STACK_OF(X509_EXTENSION) * sk; + X509_EXTENSION *cert_ext; + ext_t *ext; + key_t *key; + cert_t *cert; + FILE *file; int i, j, ext_nid, nvctr; int c, opt_idx = 0; const struct option *cmd_opt;