From e23bc7d53de9c0acdb442748fdb23b4979865a98 Mon Sep 17 00:00:00 2001 From: Nicolas Williams Date: Mon, 29 Mar 2021 20:47:03 -0500 Subject: [PATCH] hx509: Fix warnings and leaks --- lib/hx509/ca.c | 15 +++++++----- lib/hx509/crypto.c | 2 ++ lib/hx509/error.c | 6 +++-- lib/hx509/hxtool.c | 21 ++++++++++++---- lib/hx509/keyset.c | 5 +++- lib/hx509/ks_file.c | 29 +++++++++++----------- lib/hx509/name.c | 60 +++++++++++++++++++++++++-------------------- lib/hx509/print.c | 5 ++++ lib/hx509/req.c | 6 ++--- lib/hx509/softp11.c | 8 ++++-- 10 files changed, 96 insertions(+), 61 deletions(-) diff --git a/lib/hx509/ca.c b/lib/hx509/ca.c index 807621c21..ca1376ce5 100644 --- a/lib/hx509/ca.c +++ b/lib/hx509/ca.c @@ -2741,7 +2741,8 @@ set_tbs(hx509_context context, realm); /* Populate requested certificate extensions from CSR/CSRPlus if allowed */ - ret = hx509_ca_tbs_set_from_csr(context, tbs, req); + if (ret == 0) + ret = hx509_ca_tbs_set_from_csr(context, tbs, req); if (ret == 0) ret = set_template(context, logf, cf, tbs); @@ -2939,6 +2940,8 @@ _hx509_ca_issue_certificate(hx509_context context, hx509_request_authorize_ku(req, ku); ret = get_cf(context, cf, logf, req, cprinc, &cf); + if (ret) + return ret; if ((ca = heim_config_get_string(context->hcontext, cf, "ca", NULL)) == NULL) { @@ -3050,9 +3053,8 @@ _hx509_ca_issue_certificate(hx509_context context, hx509_env_free(&env); /* All done with the TBS, sign/issue the certificate */ - ret = hx509_ca_sign(context, tbs, signer, &cert); - if (ret) - goto out; + if (ret == 0) + ret = hx509_ca_sign(context, tbs, signer, &cert); /* * Gather the certificate and chain into a MEMORY store, being careful not @@ -3063,8 +3065,9 @@ _hx509_ca_issue_certificate(hx509_context context, * the full chain in the issuer credential store and copying only the certs * (but not the private keys) is safer and easier to configure. */ - ret = hx509_certs_init(context, "MEMORY:certs", - HX509_CERTS_NO_PRIVATE_KEYS, NULL, out); + if (ret == 0) + ret = hx509_certs_init(context, "MEMORY:certs", + HX509_CERTS_NO_PRIVATE_KEYS, NULL, out); if (ret == 0) ret = hx509_certs_add(context, *out, cert); if (ret == 0 && send_chain) { diff --git a/lib/hx509/crypto.c b/lib/hx509/crypto.c index 77e721064..c69eabfa9 100644 --- a/lib/hx509/crypto.c +++ b/lib/hx509/crypto.c @@ -1625,6 +1625,8 @@ _hx509_private_key_export(hx509_context context, hx509_key_format_t format, heim_octet_string *data) { + data->length = 0; + data->data = NULL; if (key->ops->export == NULL) { hx509_clear_error_string(context); return HX509_UNIMPLEMENTED_OPERATION; diff --git a/lib/hx509/error.c b/lib/hx509/error.c index d3ebd1bf6..1ca9682d0 100644 --- a/lib/hx509/error.c +++ b/lib/hx509/error.c @@ -218,9 +218,11 @@ hx509_free_error_string(char *str) * @ingroup hx509_error */ -HX509_LIB_FUNCTION void HX509_LIB_CALL +HX509_LIB_NORETURN_FUNCTION + __attribute__ ((__noreturn__, __format__ (__printf__, 4, 5))) +void HX509_LIB_CALL hx509_err(hx509_context context, int exit_code, - int error_code, const char *fmt, ...) + int error_code, const char *fmt, ...) { va_list ap; const char *msg; diff --git a/lib/hx509/hxtool.c b/lib/hx509/hxtool.c index 43c4713d1..03e6819bd 100644 --- a/lib/hx509/hxtool.c +++ b/lib/hx509/hxtool.c @@ -412,17 +412,19 @@ cms_create_sd(struct cms_create_sd_options *opt, int argc, char **argv) size_t sz; void *p; int ret, flags = 0; - char *infile, *outfile = NULL; + const char *outfile = NULL; + char *infile, *freeme = NULL; memset(&contentType, 0, sizeof(contentType)); infile = argv[0]; if (argc < 2) { - ret = asprintf(&outfile, "%s.%s", infile, + ret = asprintf(&freeme, "%s.%s", infile, opt->pem_flag ? "pem" : "cms-signeddata"); - if (ret == -1 || outfile == NULL) + if (ret == -1 || freeme == NULL) errx(1, "out of memory"); + outfile = freeme; } else outfile = argv[1]; @@ -549,6 +551,7 @@ cms_create_sd(struct cms_create_sd_options *opt, int argc, char **argv) hx509_certs_free(&signer); free(o.data); + free(freeme); return 0; } @@ -843,6 +846,7 @@ pcert_validate(struct validate_options *opt, int argc, char **argv) hx509_certs_iter_f(context, certs, validate_f, ctx); hx509_certs_free(&certs); argv++; + free(sn); } hx509_validate_ctx_free(ctx); @@ -1363,7 +1367,7 @@ get_key(const char *fn, const char *type, int optbits, int ret = 0; if (type) { - struct hx509_generate_private_context *gen_ctx; + struct hx509_generate_private_context *gen_ctx = NULL; if (strcasecmp(type, "rsa") != 0) errx(1, "can only handle rsa keys for now"); @@ -1375,6 +1379,7 @@ get_key(const char *fn, const char *type, int optbits, ret = _hx509_generate_private_key_bits(context, gen_ctx, optbits); if (ret == 0) ret = _hx509_generate_private_key(context, gen_ctx, signer); + _hx509_generate_private_key_free(&gen_ctx); if (ret) hx509_err(context, 1, ret, "failed to generate private key of type %s", type); @@ -1420,6 +1425,7 @@ generate_key(struct generate_key_options *opt, int argc, char **argv) const char *type = opt->type_string ? opt->type_string : "rsa"; int bits = opt->key_bits_integer ? opt->key_bits_integer : 2048; + memset(&signer, 0, sizeof(signer)); get_key(argv[0], type, bits, &signer); hx509_private_key_free(&signer); return 0; @@ -1436,6 +1442,7 @@ request_create(struct request_create_options *opt, int argc, char **argv) const char *outfile = argv[0]; memset(&key, 0, sizeof(key)); + memset(&signer, 0, sizeof(signer)); get_key(opt->key_string, opt->generate_key_string, @@ -2507,6 +2514,7 @@ crl_sign(struct crl_sign_options *opt, int argc, char **argv) ret = hx509_certs_append(context, revoked, lock, sn); if (ret) hx509_err(context, 1, ret, "hx509_certs_append: %s", sn); + free(sn); } hx509_crl_add_revoked_certs(context, crl, revoked); @@ -2983,7 +2991,7 @@ acert1(struct acert_options *opt, size_t cert_num, hx509_cert cert, int *matched ekus_wanted = opt->has_eku_strings.num_strings; kus_wanted = opt->has_ku_strings.num_strings; wanted = sans_wanted + ekus_wanted + kus_wanted; - found = sans_found = ekus_found = kus_found = 0; + sans_found = ekus_found = kus_found = 0; if (e == NULL) { if (wanted) @@ -3080,6 +3088,8 @@ acert(struct acert_options *opt, int argc, char **argv) ret = acert1(opt, n++, cert, &matched); if (matched) break; + hx509_cert_free(cert); + cert = NULL; } if (cursor) (void) hx509_certs_end_seq(context, certs, cursor); @@ -3093,6 +3103,7 @@ acert(struct acert_options *opt, int argc, char **argv) if (ret) hx509_err(context, 1, ret, "Matching certificate did not meet " "requirements"); + hx509_cert_free(cert); free(sn); return 0; } diff --git a/lib/hx509/keyset.c b/lib/hx509/keyset.c index ef3465050..f25cdf4e4 100644 --- a/lib/hx509/keyset.c +++ b/lib/hx509/keyset.c @@ -561,11 +561,14 @@ hx509_certs_find(hx509_context context, break; if (_hx509_query_match_cert(context, q, c)) { *r = c; + c = NULL; break; } hx509_cert_free(c); + c = NULL; } + hx509_cert_free(c); hx509_certs_end_seq(context, certs, cursor); if (ret) return ret; @@ -573,7 +576,7 @@ hx509_certs_find(hx509_context context, * Return HX509_CERT_NOT_FOUND if no certificate in certs matched * the query. */ - if (c == NULL) { + if (*r == NULL) { hx509_clear_error_string(context); return HX509_CERT_NOT_FOUND; } diff --git a/lib/hx509/ks_file.c b/lib/hx509/ks_file.c index b22093cd4..880668b45 100644 --- a/lib/hx509/ks_file.c +++ b/lib/hx509/ks_file.c @@ -548,7 +548,7 @@ store_func(hx509_context context, void *ctx, hx509_cert c) { struct store_ctx *sc = ctx; heim_octet_string data; - int ret; + int ret = 0; if (hx509_cert_have_private_key_only(c)) { data.length = 0; @@ -564,15 +564,17 @@ store_func(hx509_context context, void *ctx, hx509_cert c) /* Can't store both. Well, we could, but nothing will support it */ if (data.data) { fwrite(data.data, data.length, 1, sc->f); - free(data.data); } else if (_hx509_cert_private_key_exportable(c) && !(sc->store_flags & HX509_CERTS_STORE_NO_PRIVATE_KEYS)) { hx509_private_key key = _hx509_cert_private_key(c); + free(data.data); + data.length = 0; + data.data = NULL; ret = _hx509_private_key_export(context, key, HX509_KEY_FORMAT_DER, &data); - fwrite(data.data, data.length, 1, sc->f); - free(data.data); + if (ret == 0 && data.length) + fwrite(data.data, data.length, 1, sc->f); } break; case USE_PEM: @@ -583,23 +585,20 @@ store_func(hx509_context context, void *ctx, hx509_cert c) ret = _hx509_private_key_export(context, key, HX509_KEY_FORMAT_DER, &priv_key); - if (ret) { - free(data.data); - break; - } - hx509_pem_write(context, _hx509_private_pem_name(key), NULL, sc->f, - priv_key.data, priv_key.length); + if (ret == 0) + ret = hx509_pem_write(context, _hx509_private_pem_name(key), NULL, + sc->f, priv_key.data, priv_key.length); free(priv_key.data); } - if (data.data) { - hx509_pem_write(context, "CERTIFICATE", NULL, sc->f, - data.data, data.length); - free(data.data); + if (ret == 0 && data.data) { + ret = hx509_pem_write(context, "CERTIFICATE", NULL, sc->f, + data.data, data.length); } break; } - return 0; + free(data.data); + return ret; } static int diff --git a/lib/hx509/name.c b/lib/hx509/name.c index 9b6a156af..67a0141be 100644 --- a/lib/hx509/name.c +++ b/lib/hx509/name.c @@ -358,29 +358,29 @@ _hx509_Name_to_string(const Name *n, char **str) return 0; } -#define COPYCHARARRAY(_ds,_el,_l,_n) \ - (_l) = strlen(_ds->u._el); \ - (_n) = malloc((_l) * sizeof((_n)[0])); \ - if ((_n) == NULL) \ - return ENOMEM; \ - for (i = 0; i < (_l); i++) \ +#define COPYCHARARRAY(_ds,_el,_l,_n) \ + (_l) = strlen(_ds->u._el); \ + (_n) = malloc((_l + 1) * sizeof((_n)[0])); \ + if ((_n) == NULL) \ + return ENOMEM; \ + for (i = 0; i < (_l); i++) \ (_n)[i] = _ds->u._el[i] -#define COPYVALARRAY(_ds,_el,_l,_n) \ - (_l) = _ds->u._el.length; \ - (_n) = malloc((_l) * sizeof((_n)[0])); \ - if ((_n) == NULL) \ - return ENOMEM; \ - for (i = 0; i < (_l); i++) \ +#define COPYVALARRAY(_ds,_el,_l,_n) \ + (_l) = _ds->u._el.length; \ + (_n) = malloc((_l + 1) * sizeof((_n)[0])); \ + if ((_n) == NULL) \ + return ENOMEM; \ + for (i = 0; i < (_l); i++) \ (_n)[i] = _ds->u._el.data[i] -#define COPYVOIDARRAY(_ds,_el,_l,_n) \ - (_l) = _ds->u._el.length; \ - (_n) = malloc((_l) * sizeof((_n)[0])); \ - if ((_n) == NULL) \ - return ENOMEM; \ - for (i = 0; i < (_l); i++) \ +#define COPYVOIDARRAY(_ds,_el,_l,_n) \ + (_l) = _ds->u._el.length; \ + (_n) = malloc((_l + 1) * sizeof((_n)[0])); \ + if ((_n) == NULL) \ + return ENOMEM; \ + for (i = 0; i < (_l); i++) \ (_n)[i] = ((unsigned char *)_ds->u._el.data)[i] @@ -423,7 +423,7 @@ dsstringprep(const DirectoryString *ds, uint32_t **rname, size_t *rlen) ret = wind_utf8ucs4_length(ds->u.utf8String, &len); if (ret) return ret; - name = malloc(len * sizeof(name[0])); + name = malloc((len + 1) * sizeof(name[0])); if (name == NULL) return ENOMEM; ret = wind_utf8ucs4(ds->u.utf8String, name, &len); @@ -440,7 +440,7 @@ dsstringprep(const DirectoryString *ds, uint32_t **rname, size_t *rlen) /* try a couple of times to get the length right, XXX gross */ for (i = 0; i < 4; i++) { *rlen = *rlen * 2; - if ((*rname = malloc(*rlen * sizeof((*rname)[0]))) == NULL) { + if ((*rname = malloc((rlen[0] + 1) * sizeof((*rname)[0]))) == NULL) { ret = ENOMEM; break; } @@ -579,9 +579,9 @@ _hx509_name_modify(hx509_context context, { RelativeDistinguishedName rdn; size_t max_len = oidtomaxlen(oid); - int type_choice, ret; - const char *a = oidtostring(oid, &type_choice); char *s = NULL; + int type_choice = choice_DirectoryString_printableString; + int ret; /* * Check string length upper bounds. @@ -591,10 +591,13 @@ _hx509_name_modify(hx509_context context, * here. */ if (max_len && strlen(str) > max_len) { + char *a = oidtostring(oid, &type_choice); + ret = HX509_PARSING_NAME_FAILED; hx509_set_error_string(context, 0, ret, "RDN attribute %s value too " "long (max %llu): %s", a ? a : "", max_len, str); + free(a); return ret; } @@ -622,7 +625,7 @@ _hx509_name_modify(hx509_context context, */ rdn.val[0].value.element = type_choice; if ((s = strdup(str)) == NULL || - (ret = der_copy_oid(oid, &rdn.val[0].type))) { + der_copy_oid(oid, &rdn.val[0].type)) { free(rdn.val); free(s); return hx509_enomem(context); @@ -1392,7 +1395,9 @@ hx509_general_name_unparse(GeneralName *name, char **str) if ((ret = hx509_context_init(&context))) return ret; - return hx509_general_name_unparse2(context, name, str); + ret = hx509_general_name_unparse2(context, name, str); + hx509_context_free(&context); + return ret; } /** @@ -1511,8 +1516,9 @@ hx509_general_name_unparse2(hx509_context context, default: return EINVAL; } - if (strpool == NULL || - (*str = rk_strpoolcollect(strpool)) == NULL) + if (ret) + rk_strpoolfree(strpool); + else if (strpool == NULL || (*str = rk_strpoolcollect(strpool)) == NULL) return ENOMEM; - return 0; + return ret; } diff --git a/lib/hx509/print.c b/lib/hx509/print.c index 544001ebc..3309913f3 100644 --- a/lib/hx509/print.c +++ b/lib/hx509/print.c @@ -361,6 +361,7 @@ check_authorityKeyIdentifier(hx509_validate_ctx ctx, } } + free_AuthorityKeyIdentifier(&ai); return 0; } @@ -771,6 +772,7 @@ check_certificatePolicies(hx509_validate_ctx ctx, validate_print(ctx, HX509_VALIDATE_F_VERBOSE, " Unknown:%s", qoid); } + free_UserNotice(&un); } } else { validate_print(ctx, HX509_VALIDATE_F_VERBOSE, @@ -842,8 +844,11 @@ check_policyMappings(hx509_validate_ctx ctx, else validate_print(ctx, HX509_VALIDATE_F_VALIDATE, "ret=%d while decoding PolicyMappings\n", ret); + free(sdpoid); + free(idpoid); } + free_PolicyMappings(&pm); return 0; } diff --git a/lib/hx509/req.c b/lib/hx509/req.c index f0a7c2186..2c3fbc88d 100644 --- a/lib/hx509/req.c +++ b/lib/hx509/req.c @@ -518,14 +518,13 @@ get_exts(hx509_context context, const hx509_request req, Extensions *exts) { - uint64_t ku_num; size_t size; int ret = 0; exts->val = NULL; exts->len = 0; - if ((ku_num = KeyUsage2int(req->ku))) { + if (KeyUsage2int(req->ku)) { Extension e; memset(&e, 0, sizeof(e)); @@ -718,6 +717,7 @@ hx509_request_to_pkcs10(hx509_context context, abort(); free_CertificationRequest(&r); + free_Extensions(&exts); return ret; } @@ -899,9 +899,9 @@ hx509_request_parse_der(hx509_context context, out: free_CertificationRequest(&r); + free_Extensions(&exts); if (ret) hx509_request_free(req); - free_CertificationRequest(&r); return ret; } diff --git a/lib/hx509/softp11.c b/lib/hx509/softp11.c index 0a1445ba5..75f675579 100644 --- a/lib/hx509/softp11.c +++ b/lib/hx509/softp11.c @@ -311,7 +311,7 @@ add_st_object(void) return NULL; for (i = 0; i < soft_token.object.num_objs; i++) { - if (soft_token.object.objs == NULL) { + if (soft_token.object.objs[i] == NULL) { soft_token.object.objs[i] = o; break; } @@ -342,6 +342,9 @@ add_object_attribute(struct st_object *o, struct st_attr *a; int i; + if (pValue == NULL && ulValueLen) + return CKR_ARGUMENTS_BAD; + i = o->num_attributes; a = realloc(o->attrs, (i + 1) * sizeof(o->attrs[0])); if (a == NULL) @@ -352,7 +355,8 @@ add_object_attribute(struct st_object *o, o->attrs[i].attribute.pValue = malloc(ulValueLen); if (o->attrs[i].attribute.pValue == NULL && ulValueLen != 0) return CKR_DEVICE_MEMORY; - memcpy(o->attrs[i].attribute.pValue, pValue, ulValueLen); + if (ulValueLen) + memcpy(o->attrs[i].attribute.pValue, pValue, ulValueLen); o->attrs[i].attribute.ulValueLen = ulValueLen; o->num_attributes++;