From c69e64cb89de7795b44664e3ed72fc555010bb3b Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: May 14 2021 18:53:02 +0000 Subject: [PATCH 1/8] Close file in casave on NSS database login error Discovered by Coverity Signed-off-by: Rob Crittenden --- diff --git a/src/casave.c b/src/casave.c index f2a5bb7..26b67cb 100644 --- a/src/casave.c +++ b/src/casave.c @@ -165,6 +165,7 @@ cm_casave_main_n(int fd, struct cm_store_ca *ca, struct cm_store_entry *e, PK11_InitPin(slot, NULL, ""); } if (PK11_NeedLogin(slot)) { + fclose(fp); cm_log(0, "NSS database %s requires login\n", state->nssdb); return CM_CERTSAVE_STATUS_INTERNAL_ERROR; } From 45d946003a91c45570072b6d508167f2465d04b3 Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: May 14 2021 18:53:02 +0000 Subject: [PATCH 2/8] Remove remaining reference to token variable in certread-n I had switched to using PK11_GetTokenName(slot) except in one spot which could lead to use of an uninitialized pointer in an error message. Change this and drop the token variable. Discovered by Coverity Signed-off-by: Rob Crittenden --- diff --git a/src/certread-n.c b/src/certread-n.c index 8452006..502d685 100644 --- a/src/certread-n.c +++ b/src/certread-n.c @@ -63,7 +63,6 @@ cm_certread_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, void *userdata) { int status = CM_SUB_STATUS_INTERNAL_ERROR, readwrite, ec; - const char *token; char *pin; PLArenaPool *arena; SECStatus error; @@ -254,7 +253,7 @@ cm_certread_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, (cb_data.n_attempts == 0)) { cm_log(0, "PIN was not needed to auth to token " "%s, though one was provided. " - "Treating this as an error.\n", token); + "Treating this as an error.\n", PK11_GetTokenName(slot)); PK11_FreeSlot(slot); if (NSS_ShutdownContext(ctx) != SECSuccess) { cm_log(1, "Error shutting down NSS.\n"); From baa10a98b65d7077e16f8aa364c43c56e9dba628 Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: May 14 2021 18:53:02 +0000 Subject: [PATCH 3/8] Free the thumbprint variable before returning This is probably a false-positive because if we know that the length of t is 0 then there is nothing to free, but it doesn't hurt anything and quiets the static analyzers. Discovered by Coverity Signed-off-by: Rob Crittenden --- diff --git a/src/getcert.c b/src/getcert.c index 9d015ad..a4e4029 100644 --- a/src/getcert.c +++ b/src/getcert.c @@ -4063,6 +4063,7 @@ thumbprint(const char *s, SECOidTag tag, int bits) } length = strlen(t); if (length == 0) { + free(t); goto done; } u = malloc(length + 1); From 08b9baee53e228224901fa38dc09ab2fb6008a1e Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: May 14 2021 18:53:02 +0000 Subject: [PATCH 4/8] Free the error message when returning Since the submit label may be called multiple times free error_message before returning. Discovered by Coverity Signed-off-by: Rob Crittenden --- diff --git a/src/ipa.c b/src/ipa.c index c47a6e3..5c32310 100644 --- a/src/ipa.c +++ b/src/ipa.c @@ -670,12 +670,14 @@ submit: "add", 1); if (!json_req) { cm_log(0, "json_pack_ex() failed: %s\n", j_error.text); + free(error_message); return CM_SUBMIT_STATUS_UNCONFIGURED; } json_str = json_dumps(json_req, 0); json_decref(json_req); if (!json_str) { cm_log(0, "json_dumps() failed\n"); + free(error_message); return CM_SUBMIT_STATUS_UNCONFIGURED; } @@ -733,6 +735,8 @@ submit: * point. Randomly dropping arguments is not * really an extensible solution, though. */ issuer = NULL; + free(error_message); + error_message = NULL; goto submit; } if ((i == 3005) && (profile != NULL)) { @@ -741,6 +745,8 @@ submit: * point. Randomly dropping arguments is not * really an extensible solution, though. */ profile = NULL; + free(error_message); + error_message = NULL; goto submit; } printf("Server at %s denied our request, " From a13b7ed11c0844c6d83a8929a4fb9984448fe34c Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: May 14 2021 18:53:02 +0000 Subject: [PATCH 5/8] Fix compiler warnings These range from: - unused variables - missing switch options - missing default in switch - logging with known NULL variables - non-void function with no return Signed-off-by: Rob Crittenden --- diff --git a/src/casave.c b/src/casave.c index 26b67cb..6e5554e 100644 --- a/src/casave.c +++ b/src/casave.c @@ -90,7 +90,6 @@ cm_casave_main_n(int fd, struct cm_store_ca *ca, struct cm_store_entry *e, CERTCertTrust trust; CERTCertDBHandle *certdb; PK11SlotInfo *slot = NULL; - SECItem *items[2]; PRUint32 flags; const char *es, *ttrust; char *package, *p; diff --git a/src/certread-n.c b/src/certread-n.c index 502d685..b44420c 100644 --- a/src/certread-n.c +++ b/src/certread-n.c @@ -64,7 +64,6 @@ cm_certread_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, { int status = CM_SUB_STATUS_INTERNAL_ERROR, readwrite, ec; char *pin; - PLArenaPool *arena; SECStatus error; NSSInitContext *ctx; PK11SlotInfo *slot = NULL; diff --git a/src/csrgen-o.c b/src/csrgen-o.c index 41b4f01..e2c59ad 100644 --- a/src/csrgen-o.c +++ b/src/csrgen-o.c @@ -200,9 +200,8 @@ cm_csrgen_o_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, (strlen(entry->cm_key_next_marker) > 0)) { filename = util_build_next_filename(entry->cm_key_storage_location, entry->cm_key_next_marker); if (filename == NULL) { - cm_log(1, "Error opening key file \"%s\" " - "for reading: %s.\n", - filename, strerror(errno)); + cm_log(1, "Error opening key file for reading: %s.\n", + strerror(errno)); _exit(CM_SUB_STATUS_INTERNAL_ERROR); } } else { diff --git a/src/dogtag.c b/src/dogtag.c index 388ad15..6b82ffd 100644 --- a/src/dogtag.c +++ b/src/dogtag.c @@ -121,7 +121,7 @@ main(int argc, const char **argv) const char *serial = NULL, *template = NULL; const char *uid = NULL, *pwd = NULL, *pwdfile = NULL; const char *udn = NULL, *pin = NULL, *pinfile = NULL; - char *csr = NULL, *csre = NULL; + char *csr = NULL; char *poptarg; struct { char *name; diff --git a/src/local.c b/src/local.c index 2f50ac7..a62ebda 100644 --- a/src/local.c +++ b/src/local.c @@ -345,7 +345,7 @@ get_signer_info(void *parent, char *localdir, X509 ***roots, /* Roll the serial number up. */ hexserial = cm_store_increment_serial(parent, hexserial); if (hexserial == NULL) { - cm_log(1, "Error incrementing '%s'.\n", hexserial); + cm_log(1, "Error incrementing serial number.\n"); return CM_SUBMIT_STATUS_UNREACHABLE; } /* Save the next serial number. */ @@ -613,8 +613,7 @@ main(int argc, const char **argv) hexserial = cm_store_increment_serial(parent, hexserial); if (hexserial == NULL) { - cm_log(1, "Error incrementing '%s'.\n", - hexserial); + cm_log(1, "Error incrementing serial number\n"); return CM_SUBMIT_STATUS_UNREACHABLE; } /* Save the next serial number. */ diff --git a/src/pkcs7.c b/src/pkcs7.c index 51b3b0f..b2cccc0 100644 --- a/src/pkcs7.c +++ b/src/pkcs7.c @@ -930,9 +930,8 @@ cm_pkcs7_verify_signed(unsigned char *data, size_t length, PKCS7_SIGNER_INFO *si; BIO *in, *out = NULL; const unsigned char *u; - char *s, buf[LINE_MAX], *p, *q; + char *s, *p, *q; int ret = -1, i; - long error; if (digest != NULL) { *digest = NULL; diff --git a/src/prefs-n.c b/src/prefs-n.c index 28fa606..584bdd7 100644 --- a/src/prefs-n.c +++ b/src/prefs-n.c @@ -38,6 +38,7 @@ cm_prefs_nss_sig_alg(SECKEYPrivateKey *pkey) return SEC_OID_SHA1; break; case cm_prefs_sha256: + case cm_prefs_nodigest: return SEC_OID_SHA256; break; case cm_prefs_sha384: @@ -46,9 +47,10 @@ cm_prefs_nss_sig_alg(SECKEYPrivateKey *pkey) case cm_prefs_sha512: return SEC_OID_SHA512; break; + default: + return SEC_OID_SHA256; + break; } - return SEC_OID_SHA256; - break; case rsaKey: switch (cm_prefs_preferred_digest()) { case cm_prefs_md5: @@ -58,6 +60,7 @@ cm_prefs_nss_sig_alg(SECKEYPrivateKey *pkey) return SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION; break; case cm_prefs_sha256: + case cm_prefs_nodigest: return SEC_OID_PKCS1_SHA256_WITH_RSA_ENCRYPTION; break; case cm_prefs_sha384: @@ -66,9 +69,10 @@ cm_prefs_nss_sig_alg(SECKEYPrivateKey *pkey) case cm_prefs_sha512: return SEC_OID_PKCS1_SHA512_WITH_RSA_ENCRYPTION; break; + default: + return SEC_OID_PKCS1_SHA256_WITH_RSA_ENCRYPTION; + break; } - return SEC_OID_PKCS1_SHA256_WITH_RSA_ENCRYPTION; - break; case rsaPssKey: return SEC_OID_PKCS1_RSA_PSS_SIGNATURE; break; @@ -83,10 +87,13 @@ cm_prefs_nss_sig_alg(SECKEYPrivateKey *pkey) break; case cm_prefs_sha384: case cm_prefs_sha512: + case cm_prefs_nodigest: + return SEC_OID_NIST_DSA_SIGNATURE_WITH_SHA256_DIGEST; + break; + default: + return SEC_OID_NIST_DSA_SIGNATURE_WITH_SHA256_DIGEST; break; } - return SEC_OID_NIST_DSA_SIGNATURE_WITH_SHA256_DIGEST; - break; case ecKey: switch (cm_prefs_preferred_digest()) { case cm_prefs_md5: @@ -94,6 +101,7 @@ cm_prefs_nss_sig_alg(SECKEYPrivateKey *pkey) return SEC_OID_ANSIX962_ECDSA_SHA224_SIGNATURE; break; case cm_prefs_sha256: + case cm_prefs_nodigest: return SEC_OID_ANSIX962_ECDSA_SHA256_SIGNATURE; break; case cm_prefs_sha384: @@ -102,9 +110,10 @@ cm_prefs_nss_sig_alg(SECKEYPrivateKey *pkey) case cm_prefs_sha512: return SEC_OID_ANSIX962_ECDSA_SHA512_SIGNATURE; break; + default: + return SEC_OID_ANSIX962_ECDSA_SHA256_SIGNATURE; + break; } - return SEC_OID_ANSIX962_ECDSA_SHA256_SIGNATURE; - break; default: return SEC_OID_UNKNOWN; break; @@ -122,6 +131,7 @@ cm_prefs_nss_dig_alg(void) return SEC_OID_SHA1; break; case cm_prefs_sha256: + case cm_prefs_nodigest: return SEC_OID_SHA256; break; case cm_prefs_sha384: @@ -130,8 +140,10 @@ cm_prefs_nss_dig_alg(void) case cm_prefs_sha512: return SEC_OID_SHA512; break; + default: + return SEC_OID_SHA256; + break; } - return SEC_OID_SHA256; } unsigned int @@ -153,6 +165,8 @@ cm_prefs_nss_dig_alg_len(void) case SEC_OID_SHA512: return 512 / 8; break; + default: + return 0; + break; } - return 0; } diff --git a/src/prefs-o.c b/src/prefs-o.c index ac68164..4b6ef6f 100644 --- a/src/prefs-o.c +++ b/src/prefs-o.c @@ -44,6 +44,7 @@ cm_prefs_ossl_hash_by_pref(enum cm_prefs_digest digest) return EVP_sha1(); break; case cm_prefs_sha256: + case cm_prefs_nodigest: return EVP_sha256(); break; case cm_prefs_sha384: @@ -52,8 +53,10 @@ cm_prefs_ossl_hash_by_pref(enum cm_prefs_digest digest) case cm_prefs_sha512: return EVP_sha512(); break; + default: + return EVP_sha256(); + break; } - return EVP_sha256(); } const EVP_MD * @@ -73,6 +76,7 @@ cm_prefs_ossl_cipher_by_pref(enum cm_prefs_cipher cipher) return EVP_des_ede3_cbc(); break; case cm_prefs_aes128: + case cm_prefs_nodigest: return EVP_aes_128_cbc(); break; case cm_prefs_aes192: @@ -81,8 +85,10 @@ cm_prefs_ossl_cipher_by_pref(enum cm_prefs_cipher cipher) case cm_prefs_aes256: return EVP_aes_256_cbc(); break; + default: + return EVP_aes_128_cbc(); + break; } - return EVP_aes_128_cbc(); } const EVP_CIPHER * diff --git a/src/scep.c b/src/scep.c index fb5b87e..35d6688 100644 --- a/src/scep.c +++ b/src/scep.c @@ -204,7 +204,8 @@ main(int argc, const char **argv) int prefer_non_renewal = 0, can_renewal = 0; int response_code = 0, response_code2 = 0; enum known_ops op = op_unset; - const char *id = NULL, *cainfo = NULL; + char *id = NULL; + const char *cainfo = NULL; char *message = NULL, *rekey_message = NULL; const char *mode = NULL, *content_type = NULL, *content_type2 = NULL; void *ctx; diff --git a/src/scepgen-o.c b/src/scepgen-o.c index a431815..a341798 100644 --- a/src/scepgen-o.c +++ b/src/scepgen-o.c @@ -320,6 +320,7 @@ build_pkimessage(EVP_PKEY *key, X509 *signer, STACK_OF(X509) *certs, digest = NULL; switch (pref_digest) { case cm_prefs_sha256: + case cm_prefs_nodigest: digest = OBJ_nid2obj(NID_sha256); break; case cm_prefs_sha384: @@ -743,9 +744,8 @@ cm_scepgen_o_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, filename = util_build_next_filename(entry->cm_key_storage_location, entry->cm_key_next_marker); if (filename == NULL) { - cm_log(0, "Error opening key file \"%s\" " - "for reading: %s.\n", - filename, strerror(errno)); + cm_log(0, "Error opening key file for reading: %s.\n", + strerror(errno)); _exit(CM_SUB_STATUS_INTERNAL_ERROR); } new_pkey = key_from_file(filename, entry); diff --git a/src/submit-so.c b/src/submit-so.c index daf11b7..da213d7 100644 --- a/src/submit-so.c +++ b/src/submit-so.c @@ -81,9 +81,8 @@ cm_submit_so_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, (strlen(entry->cm_key_next_marker) > 0)) { filename = util_build_next_filename(entry->cm_key_storage_location, entry->cm_key_next_marker); if (filename == NULL) { - cm_log(1, "Error reading private key from " - "\"%s\": %s.\n", - filename, strerror(errno)); + cm_log(1, "Error reading private key: %s.\n", + strerror(errno)); keyfp = NULL; } else { keyfp = fopen(filename, "r"); diff --git a/src/tdbush.c b/src/tdbush.c index fb81c47..3587f84 100644 --- a/src/tdbush.c +++ b/src/tdbush.c @@ -3690,9 +3690,8 @@ request_modify(DBusConnection *conn, DBusMessage *msg, return DBUS_HANDLER_RESULT_NOT_YET_HANDLED; } } - } else { - return DBUS_HANDLER_RESULT_NOT_YET_HANDLED; } + return DBUS_HANDLER_RESULT_NOT_YET_HANDLED; } /* org.fedorahosted.certmonger.request.resubmit */ From af3264e79f476807cdf18e610704066a25331a17 Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: May 14 2021 18:53:02 +0000 Subject: [PATCH 6/8] clang: Unused variable assignment kret is assigned but unused. --- diff --git a/src/getcert.c b/src/getcert.c index a4e4029..078f5aa 100644 --- a/src/getcert.c +++ b/src/getcert.c @@ -830,7 +830,7 @@ request(const char *argv0, int argc, const char **argv) return 1; } krealm = NULL; - if ((kret = krb5_get_default_realm(kctx, &krealm)) != 0) { + if (krb5_get_default_realm(kctx, &krealm) != 0) { krealm = NULL; } @@ -1926,7 +1926,7 @@ set_tracking(const char *argv0, const char *category, return 1; } krealm = NULL; - if ((kret = krb5_get_default_realm(kctx, &krealm)) != 0) { + if (krb5_get_default_realm(kctx, &krealm) != 0) { krealm = NULL; } From 0bf6d91f576d8601b9213b8de4c0d7b3b91e79c1 Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: May 14 2021 18:53:02 +0000 Subject: [PATCH 7/8] clang: Remove memory leak on failure At this point a number of objects have been allocated. On error be sure to release them. --- diff --git a/src/ipa.c b/src/ipa.c index 5c32310..620c022 100644 --- a/src/ipa.c +++ b/src/ipa.c @@ -1176,6 +1176,9 @@ main(int argc, const char **argv) /* strip off the trailing xml and replace with json */ if ((strlen(xmlrpc_uri) + 1) > sizeof(uri)) { printf(_("xmlrpc_uri is longer than %ld.\n"), sizeof(uri) - 2); + free(profile); + free(issuer); + free(reqprinc); return CM_SUBMIT_STATUS_UNCONFIGURED; } snprintf(uri, strlen(xmlrpc_uri) - 2, "%s", xmlrpc_uri); From 4ae497e9db7604213f266681049f83e3f8d0c035 Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: May 14 2021 18:53:02 +0000 Subject: [PATCH 8/8] clang: free error_message when finding the realm The error message can come from either krb5_get_error_message(), error_message() or a static string. If krb5_get_error_message() is used then krb5_free_error_message() needs to be called to free it. We already strdup'd error_message() but there were some static strings as well. So unify around strdup'ing these strings so we can free() it when the function exits. --- diff --git a/src/ipa.c b/src/ipa.c index 620c022..83b4399 100644 --- a/src/ipa.c +++ b/src/ipa.c @@ -60,14 +60,19 @@ get_error_message(krb5_context ctx, krb5_error_code kcode) { const char *ret; #ifdef HAVE_KRB5_GET_ERROR_MESSAGE - ret = ctx ? krb5_get_error_message(ctx, kcode) : NULL; - if (ret == NULL) { + const char *kret; + kret = ctx ? krb5_get_error_message(ctx, kcode) : NULL; + if (kret == NULL) { ret = error_message(kcode); + } else { + ret = strdup(kret); + krb5_free_error_message(ctx, kret); } + return ret; #else ret = error_message(kcode); -#endif return strdup(ret); +#endif } char * @@ -121,7 +126,7 @@ cm_submit_ccache_realm(char **msg) if (data == NULL) { fprintf(stderr, "Error retrieving principal realm.\n"); if (msg != NULL) { - *msg = "Error retrieving principal realm.\n"; + *msg = strdup("Error retrieving principal realm.\n"); } return NULL; } @@ -129,7 +134,7 @@ cm_submit_ccache_realm(char **msg) if (ret == NULL) { fprintf(stderr, "Out of memory for principal realm.\n"); if (msg != NULL) { - *msg = "Out of memory for principal realm.\n"; + *msg = strdup("Out of memory for principal realm.\n"); } return NULL; }