From 3da0e186904ad81dd87cf74bfae88270f14bb770 Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: Sep 25 2018 13:08:48 +0000 Subject: [PATCH 1/7] Use the correct slot when saving certificates in NSS Certificates were always stored in the NSS certdb. --- diff --git a/src/certsave-n.c b/src/certsave-n.c index 8e15a18..af176ce 100644 --- a/src/certsave-n.c +++ b/src/certsave-n.c @@ -92,7 +92,11 @@ cm_certsave_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, SECStatus error; SECItem *item, subject; char *p, *q, *pin; + const char *token; const char *es; + PK11SlotList *slotlist; + PK11SlotListElement *sle; + CK_MECHANISM_TYPE mech; NSSInitContext *ctx; CERTCertDBHandle *certdb; CERTCertList *certlist; @@ -192,231 +196,253 @@ cm_certsave_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, } _exit(CM_CERTSAVE_STATUS_INTERNAL_ERROR); } - /* Be ready to count our uses of a PIN. */ - memset(&cb_data, 0, sizeof(cb_data)); - cb_data.entry = entry; - cb_data.n_attempts = 0; - pin = NULL; - if (cm_pin_read_for_key(entry, &pin) != 0) { - cm_log(1, "Error reading PIN for key store, " - "failing to save certificate.\n"); + /* Find the tokens that we might use for cert storage. */ + mech = CKM_RSA_X_509; + slotlist = PK11_GetAllTokens(mech, PR_FALSE, PR_FALSE, NULL); + if (slotlist == NULL) { + cm_log(1, "Error getting list of tokens.\n"); PORT_FreeArena(arena, PR_TRUE); - error = NSS_ShutdownContext(ctx); - if (error != SECSuccess) { + if (NSS_ShutdownContext(ctx) != SECSuccess) { cm_log(1, "Error shutting down NSS.\n"); } - _exit(CM_CERTSAVE_STATUS_AUTH); + _exit(2); } - /* Set a PIN if we're supposed to be using one and aren't using - * one yet in this database. */ - if (PK11_NeedUserInit(PK11_GetInternalKeySlot())) { - PK11_InitPin(PK11_GetInternalKeySlot(), NULL, - pin ? pin : ""); - ec = PORT_GetError(); - if (ec != 0) { - es = PR_ErrorToName(ec); + /* Walk the list looking for the requested slot, or the first one if + * none was requested. */ + if (cm_pin_read_for_cert(entry, &pin) != 0) { + cm_log(1, "Error reading PIN for cert db.\n"); + _exit(CM_SUB_STATUS_ERROR_AUTH); + } + PK11_SetPasswordFunc(&cm_pin_read_for_cert_nss_cb); + for (sle = slotlist->head; + ((sle != NULL) && (sle->slot != NULL)); + sle = sle->next) + { + /* Log the slot's name. */ + token = PK11_GetTokenName(sle->slot); + if (token != NULL) { + cm_log(3, "Found token '%s'.\n", token); } else { - es = NULL; + cm_log(3, "Found unnamed token.\n"); } - if (PK11_NeedUserInit(PK11_GetInternalKeySlot())) { - if (es != NULL) { - cm_log(1, "Key storage slot still " - "needs user PIN to be set: " - "%s.\n", es); - } else { - cm_log(1, "Key storage slot still " - "needs user PIN to be set.\n"); - } + /* If we're looking for a specific slot, and this isn't it, + * keep going. */ + if ((entry->cm_cert_token != NULL) && + ((token == NULL) || + (strcmp(entry->cm_cert_token, token) != 0))) { + if (token != NULL) { + cm_log(1, + "Token is named \"%s\", not \"%s\", " + "skipping.\n", + token, entry->cm_cert_token); + } else { + cm_log(1, + "Token is unnamed, not \"%s\", " + "skipping.\n", + entry->cm_cert_token); + } + goto next_slot; + } + /* Be ready to count our uses of a PIN. */ + memset(&cb_data, 0, sizeof(cb_data)); + cb_data.entry = entry; + cb_data.n_attempts = 0; + pin = NULL; + if (cm_pin_read_for_key(entry, &pin) != 0) { + cm_log(1, "Error reading PIN for key store, " + "failing to save certificate.\n"); PORT_FreeArena(arena, PR_TRUE); error = NSS_ShutdownContext(ctx); if (error != SECSuccess) { cm_log(1, "Error shutting down NSS.\n"); } - switch (ec) { - case PR_NO_ACCESS_RIGHTS_ERROR: /* EACCES or EPERM */ - _exit(CM_CERTSAVE_STATUS_PERMS); - break; - default: - _exit(CM_CERTSAVE_STATUS_AUTH); - break; - } + _exit(CM_CERTSAVE_STATUS_AUTH); } - /* We're authenticated now, so count this as a use of - * the PIN. */ - if ((pin != NULL) && (strlen(pin) > 0)) { - cb_data.n_attempts++; - } - } - /* Log in, if case we need to muck around with the key - * database. */ - PK11_SetPasswordFunc(&cm_pin_read_for_key_nss_cb); - error = PK11_Authenticate(PK11_GetInternalKeySlot(), PR_TRUE, - &cb_data); - ec = PORT_GetError(); - if (error != SECSuccess) { - if (ec != 0) { + if (PK11_NeedUserInit(sle->slot)) { + PK11_InitPin(sle->slot, NULL, pin ? pin : ""); + ec = PORT_GetError(); es = PR_ErrorToName(ec); - } else { - es = NULL; - } - if (es != NULL) { - cm_log(1, "Error authenticating to key store: %s.\n", - es); - } else { - cm_log(1, "Error authenticating to key store.\n"); - } - PORT_FreeArena(arena, PR_TRUE); - error = NSS_ShutdownContext(ctx); - if (error != SECSuccess) { - cm_log(1, "Error shutting down NSS.\n"); - } - _exit(CM_CERTSAVE_STATUS_AUTH); - } - if ((pin != NULL) && - (strlen(pin) > 0) && - (cb_data.n_attempts == 0)) { - cm_log(1, "PIN was not needed to auth to key " - "store, though one was provided. " - "Treating this as an error.\n"); - PORT_FreeArena(arena, PR_TRUE); - error = NSS_ShutdownContext(ctx); - if (error != SECSuccess) { - cm_log(1, "Error shutting down NSS.\n"); - } - _exit(CM_CERTSAVE_STATUS_AUTH); - } - certdb = CERT_GetDefaultCertDB(); - if (certdb != NULL) { - /* Strip the header and footer. */ - p = entry->cm_cert; - q = NULL; - if (p != NULL) { - while (strncmp(p, "-----BEGIN ", 11) == 0) { - p += strcspn(p, "\r\n"); - p += strspn(p, "\r\n"); + if (PK11_NeedUserInit(sle->slot)) { + if (es != NULL) { + cm_log(1, "Key storage slot still " + "needs user PIN to be set: " + "%s.\n", es); + } else { + cm_log(1, "Key storage slot still " + "needs user PIN to be set.\n"); + } + PORT_FreeArena(arena, PR_TRUE); + error = NSS_ShutdownContext(ctx); + if (error != SECSuccess) { + cm_log(1, "Error shutting down NSS.\n"); + } + switch (ec) { + case PR_NO_ACCESS_RIGHTS_ERROR: /* EACCES or EPERM */ + _exit(CM_CERTSAVE_STATUS_PERMS); + break; + default: + _exit(CM_CERTSAVE_STATUS_AUTH); + break; + } } - q = strstr(p, "-----END"); + /* count this as use of the PIN */ + cb_data.n_attempts++; } - if ((q == NULL) || (*p == '\0')) { - cm_log(1, "Unable to parse certificate.\n"); - PORT_FreeArena(arena, PR_TRUE); - if (NSS_ShutdownContext(ctx) != SECSuccess) { - cm_log(1, "Error shutting down NSS.\n"); + if (PK11_NeedLogin(sle->slot)) { + error = PK11_Authenticate(sle->slot, PR_TRUE, &cb_data); + if (error != SECSuccess) { + cm_log(1, "Error authenticating to cert db for token " + "%s.\n", token); + goto next_slot; } - _exit(CM_CERTSAVE_STATUS_INTERNAL_ERROR); + cb_data.n_attempts++; } - /* Handle the base64 decode. */ - item = NSSBase64_DecodeBuffer(arena, NULL, p, q - p); - if (item == NULL) { - cm_log(1, "Unable to decode certificate " - "into buffer.\n"); + if ((pin != NULL) && + (strlen(pin) > 0) && + (cb_data.n_attempts == 0)) { + cm_log(1, "PIN was not needed to auth to key " + "store, though one was provided. " + "Treating this as an error.\n"); PORT_FreeArena(arena, PR_TRUE); - if (NSS_ShutdownContext(ctx) != SECSuccess) { + error = NSS_ShutdownContext(ctx); + if (error != SECSuccess) { cm_log(1, "Error shutting down NSS.\n"); } - _exit(CM_CERTSAVE_STATUS_INTERNAL_ERROR); + _exit(CM_CERTSAVE_STATUS_AUTH); } - /* Do a "shallow" decode to pull out the subject name - * so that we can check for a conflict. */ - memset(&csdata, 0, sizeof(csdata)); - if (SEC_ASN1DecodeItem(arena, &csdata, - CERT_SignedDataTemplate, - item) != SECSuccess) { - cm_log(1, "Unable to decode certificate " - "signed data into buffer.\n"); - PORT_FreeArena(arena, PR_TRUE); - if (NSS_ShutdownContext(ctx) != SECSuccess) { - cm_log(1, "Error shutting down NSS.\n"); + certdb = CERT_GetDefaultCertDB(); + if (certdb != NULL) { + /* Strip the header and footer. */ + p = entry->cm_cert; + q = NULL; + if (p != NULL) { + while (strncmp(p, "-----BEGIN ", 11) == 0) { + p += strcspn(p, "\r\n"); + p += strspn(p, "\r\n"); + } + q = strstr(p, "-----END"); } - _exit(CM_CERTSAVE_STATUS_INTERNAL_ERROR); - } - memset(&cert, 0, sizeof(cert)); - if (SEC_ASN1DecodeItem(arena, &cert, - CERT_CertificateTemplate, - &csdata.data) != SECSuccess) { - cm_log(1, "Unable to decode certificate " - "data into buffer.\n"); - PORT_FreeArena(arena, PR_TRUE); - if (NSS_ShutdownContext(ctx) != SECSuccess) { - cm_log(1, "Error shutting down NSS.\n"); + if ((q == NULL) || (*p == '\0')) { + cm_log(1, "Unable to parse certificate.\n"); + PORT_FreeArena(arena, PR_TRUE); + if (NSS_ShutdownContext(ctx) != SECSuccess) { + cm_log(1, "Error shutting down NSS.\n"); + } + _exit(CM_CERTSAVE_STATUS_INTERNAL_ERROR); } - _exit(CM_CERTSAVE_STATUS_INTERNAL_ERROR); - } - subject = cert.derSubject; - /* Ask NSS if there would be a conflict. */ - have_trust = PR_FALSE; - if (SEC_CertNicknameConflict(entry->cm_cert_nickname, - &subject, - certdb)) { - /* Delete the certificate that's already there - * with the nickname we want, otherwise our - * cert with a different subject name will be - * discarded. */ - certlist = PK11_FindCertsFromNickname(entry->cm_cert_nickname, - NULL); + /* Handle the base64 decode. */ + item = NSSBase64_DecodeBuffer(arena, NULL, p, q - p); + if (item == NULL) { + cm_log(1, "Unable to decode certificate " + "into buffer.\n"); + PORT_FreeArena(arena, PR_TRUE); + if (NSS_ShutdownContext(ctx) != SECSuccess) { + cm_log(1, "Error shutting down NSS.\n"); + } + _exit(CM_CERTSAVE_STATUS_INTERNAL_ERROR); + } + /* Do a "shallow" decode to pull out the subject name + * so that we can check for a conflict. */ + memset(&csdata, 0, sizeof(csdata)); + if (SEC_ASN1DecodeItem(arena, &csdata, + CERT_SignedDataTemplate, + item) != SECSuccess) { + cm_log(1, "Unable to decode certificate " + "signed data into buffer.\n"); + PORT_FreeArena(arena, PR_TRUE); + if (NSS_ShutdownContext(ctx) != SECSuccess) { + cm_log(1, "Error shutting down NSS.\n"); + } + _exit(CM_CERTSAVE_STATUS_INTERNAL_ERROR); + } + memset(&cert, 0, sizeof(cert)); + if (SEC_ASN1DecodeItem(arena, &cert, + CERT_CertificateTemplate, + &csdata.data) != SECSuccess) { + cm_log(1, "Unable to decode certificate " + "data into buffer.\n"); + PORT_FreeArena(arena, PR_TRUE); + if (NSS_ShutdownContext(ctx) != SECSuccess) { + cm_log(1, "Error shutting down NSS.\n"); + } + _exit(CM_CERTSAVE_STATUS_INTERNAL_ERROR); + } + subject = cert.derSubject; + /* Ask NSS if there would be a conflict. */ + have_trust = PR_FALSE; + if (SEC_CertNicknameConflict(entry->cm_cert_nickname, + &subject, + certdb)) { + /* Delete the certificate that's already there + * with the nickname we want, otherwise our + * cert with a different subject name will be + * discarded. */ + certlist = PK11_FindCertsFromNickname(entry->cm_cert_nickname, + NULL); + if (certlist != NULL) { + /* Look for certs with different + * subject names but the same nickname, + * because they've got to go. */ + for (node = CERT_LIST_HEAD(certlist); + (node != NULL) && + !CERT_LIST_EMPTY(certlist) && + !CERT_LIST_END(node, certlist); + node = CERT_LIST_NEXT(node)) { + if (!SECITEM_ItemsAreEqual(&subject, + &node->cert->derSubject)) { + cm_log(3, "Found a " + "certificate " + "with the same " + "nickname but " + "different " + "subject, " + "removing " + "certificate " + "\"%s\" with " + "subject " + "\"%s\".\n", + node->cert->nickname, + node->cert->subjectName ? + node->cert->subjectName : + ""); + /* Get a handle for + * this certificate's + * private key, in case + * we need to remove + * it. */ + privkey = PK11_FindKeyByAnyCert(node->cert, NULL); + privkeys = add_privkey_to_list(privkeys, privkey); + SEC_DeletePermCertificate(node->cert); + } + } + CERT_DestroyCertList(certlist); + } + } else { + cm_log(3, "No duplicate nickname entries.\n"); + } + /* This certificate's subject may already be present + * with a different nickname. Delete those, too. */ + certlist = CERT_CreateSubjectCertList(NULL, certdb, + &subject, + PR_FALSE, + PR_FALSE); if (certlist != NULL) { - /* Look for certs with different - * subject names but the same nickname, - * because they've got to go. */ + /* Look for certs with different nicknames but + * the same subject name, because those have + * got to go. */ + i = 0; for (node = CERT_LIST_HEAD(certlist); (node != NULL) && !CERT_LIST_EMPTY(certlist) && !CERT_LIST_END(node, certlist); node = CERT_LIST_NEXT(node)) { - if (!SECITEM_ItemsAreEqual(&subject, - &node->cert->derSubject)) { + if ((node->cert->nickname != NULL) && + (strcmp(entry->cm_cert_nickname, + node->cert->nickname) != 0)) + { + i++; cm_log(3, "Found a " - "certificate " - "with the same " - "nickname but " - "different " - "subject, " - "removing " - "certificate " - "\"%s\" with " - "subject " - "\"%s\".\n", - node->cert->nickname, - node->cert->subjectName ? - node->cert->subjectName : - ""); - /* Get a handle for - * this certificate's - * private key, in case - * we need to remove - * it. */ - privkey = PK11_FindKeyByAnyCert(node->cert, NULL); - privkeys = add_privkey_to_list(privkeys, privkey); - SEC_DeletePermCertificate(node->cert); - } - } - CERT_DestroyCertList(certlist); - } - } else { - cm_log(3, "No duplicate nickname entries.\n"); - } - /* This certificate's subject may already be present - * with a different nickname. Delete those, too. */ - certlist = CERT_CreateSubjectCertList(NULL, certdb, - &subject, - PR_FALSE, - PR_FALSE); - if (certlist != NULL) { - /* Look for certs with different nicknames but - * the same subject name, because those have - * got to go. */ - i = 0; - for (node = CERT_LIST_HEAD(certlist); - (node != NULL) && - !CERT_LIST_EMPTY(certlist) && - !CERT_LIST_END(node, certlist); - node = CERT_LIST_NEXT(node)) { - if ((node->cert->nickname != NULL) && - (strcmp(entry->cm_cert_nickname, - node->cert->nickname) != 0)) { - i++; - cm_log(3, "Found a " - "certificate with a " + "certificate with a " "different nickname but " "the same subject, " "removing certificate " @@ -426,284 +452,291 @@ cm_certsave_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, node->cert->subjectName ? node->cert->subjectName : ""); - /* Get a handle for this - * certificate's private key, - * in case we need to remove - * it. */ - privkey = PK11_FindKeyByAnyCert(node->cert, NULL); - privkeys = add_privkey_to_list(privkeys, privkey); - SEC_DeletePermCertificate(node->cert); - } else { - /* Same nickname, and we - * already know it has the same - * subject name. Save its - * trust. */ - if (!have_trust) { - if (CERT_GetCertTrust(node->cert, + /* Get a handle for this + * certificate's private key, + * in case we need to remove + * it. */ + privkey = PK11_FindKeyByAnyCert(node->cert, NULL); + privkeys = add_privkey_to_list(privkeys, privkey); + SEC_DeletePermCertificate(node->cert); + } else { + /* Same nickname, and we + * already know it has the same + * subject name. Save its + * trust. */ + if (!have_trust) { + if (CERT_GetCertTrust(node->cert, &trust) == SECSuccess) { - have_trust = PR_TRUE; + have_trust = PR_TRUE; + } } } } - } - if (i == 0) { - cm_log(3, "No duplicate subject name entries.\n"); - } - CERT_DestroyCertList(certlist); - } else { - cm_log(3, "No duplicate subject name entries.\n"); - } - /* Make one more attempt at finding an existing trust - * value. */ - if (!have_trust) { - oldcert = PK11_FindCertFromNickname(entry->cm_cert_nickname, NULL); - if (oldcert != NULL) { - if (CERT_GetCertTrust(oldcert, - &trust) == SECSuccess) { - have_trust = PR_TRUE; + if (i == 0) { + cm_log(3, "No duplicate subject name entries.\n"); } - CERT_DestroyCertificate(oldcert); + CERT_DestroyCertList(certlist); + } else { + cm_log(3, "No duplicate subject name entries.\n"); } - } - /* Import the certificate. */ - returned = NULL; - error = CERT_ImportCerts(certdb, - certUsageUserCertImport, - 1, &item, &returned, - PR_TRUE, - PR_FALSE, - entry->cm_cert_nickname); - ec = PORT_GetError(); - if (error == SECSuccess) { - /* If NSS uses SQL DB storage, CERT_ImportCerts creates - * an incomplete internal state (the cert isn't - * associated with the private key, and calling - * PK11_FindKeyByAnyCert returns no result). - * As a workaround, we import the cert again using - * PK11_ImportCert, which magically fixes the issue. - * See rhbz#1532188 */ - error = PK11_ImportCert(PK11_GetInternalKeySlot(), - returned[0], - CK_INVALID_HANDLE, - returned[0]->nickname, - PR_FALSE); - } - if (error == SECSuccess) { - cm_log(1, "Imported certificate \"%s\", got " - "nickname \"%s\".\n", - entry->cm_cert_nickname, - returned[0]->nickname); - status = 0; - /* Set the trust on the new certificate, - * perhaps matching the trust on an - * already-present certificate with the same - * nickname. */ + /* Make one more attempt at finding an existing trust + * value. */ if (!have_trust) { - memset(&trust, 0, sizeof(trust)); - trust.sslFlags = CERTDB_USER; - trust.emailFlags = CERTDB_USER; - trust.objectSigningFlags = CERTDB_USER; + oldcert = PK11_FindCertFromNickname(entry->cm_cert_nickname, NULL); + if (oldcert != NULL) { + if (CERT_GetCertTrust(oldcert, + &trust) == SECSuccess) { + have_trust = PR_TRUE; + } + CERT_DestroyCertificate(oldcert); + } } - error = CERT_ChangeCertTrust(certdb, - returned[0], - &trust); + /* Import the certificate. */ + returned = NULL; + error = CERT_ImportCerts(certdb, + certUsageUserCertImport, + 1, &item, &returned, + PR_TRUE, + PR_FALSE, + entry->cm_cert_nickname); ec = PORT_GetError(); - if (error != SECSuccess) { + if (error == SECSuccess) { + /* If NSS uses SQL DB storage, CERT_ImportCerts creates + * an incomplete internal state (the cert isn't + * associated with the private key, and calling + * PK11_FindKeyByAnyCert returns no result). + * As a workaround, we import the cert again using + * PK11_ImportCert, which magically fixes the issue. + * See rhbz#1532188 */ + error = PK11_ImportCert(sle->slot, + returned[0], + CK_INVALID_HANDLE, + returned[0]->nickname, + PR_FALSE); + } + if (error == SECSuccess) { + cm_log(1, "Imported certificate \"%s\", got " + "nickname \"%s\".\n", + entry->cm_cert_nickname, + returned[0]->nickname); + status = 0; + /* Set the trust on the new certificate, + * perhaps matching the trust on an + * already-present certificate with the same + * nickname. */ + if (!have_trust) { + memset(&trust, 0, sizeof(trust)); + trust.sslFlags = CERTDB_USER; + trust.emailFlags = CERTDB_USER; + trust.objectSigningFlags = CERTDB_USER; + } + error = CERT_ChangeCertTrust(certdb, + returned[0], + &trust); + ec = PORT_GetError(); + if (error != SECSuccess) { + if (ec != 0) { + es = PR_ErrorToName(ec); + } else { + es = NULL; + } + if (es != NULL) { + cm_log(0, "Error setting trust " + "on certificate \"%s\": " + "%s.\n", + entry->cm_cert_nickname, es); + } else { + cm_log(0, "Error setting trust " + "on certificate \"%s\".\n", + entry->cm_cert_nickname); + } + } + /* Delete any other certificates that are there + * with the same nickname. While NSS's + * database allows duplicates so long as they + * have the same subject name and nickname, + * several APIs and many applications can't + * dependably find the right one among more + * than one. So bye-bye, old certificates. */ + certlist = PK11_FindCertsFromNickname(entry->cm_cert_nickname, + NULL); + if (certlist != NULL) { + /* Look for certs with contents. */ + for (node = CERT_LIST_HEAD(certlist); + (node != NULL) && + !CERT_LIST_EMPTY(certlist) && + !CERT_LIST_END(node, certlist); + node = CERT_LIST_NEXT(node)) { + if (!SECITEM_ItemsAreEqual(item, + &node->cert->derCert)) { + cm_log(3, "Found a " + "certificate " + "with the same " + "nickname and " + "subject, but " + "different " + "contents, " + "removing it.\n"); + /* Get a handle for + * this certificate's + * private key, in case + * we need to remove + * it. */ + privkey = PK11_FindKeyByAnyCert(node->cert, NULL); + privkeys = add_privkey_to_list(privkeys, privkey); + SEC_DeletePermCertificate(node->cert); + } + } + CERT_DestroyCertList(certlist); + } + } else { if (ec != 0) { es = PR_ErrorToName(ec); } else { es = NULL; } if (es != NULL) { - cm_log(0, "Error setting trust " - "on certificate \"%s\": " - "%s.\n", - entry->cm_cert_nickname, es); + cm_log(0, "Error importing certificate " + "into NSSDB \"%s\": %s.\n", + entry->cm_cert_storage_location, + es); } else { - cm_log(0, "Error setting trust " - "on certificate \"%s\".\n", - entry->cm_cert_nickname); + cm_log(0, "Error importing certificate " + "into NSSDB \"%s\".\n", + entry->cm_cert_storage_location); } - } - /* Delete any other certificates that are there - * with the same nickname. While NSS's - * database allows duplicates so long as they - * have the same subject name and nickname, - * several APIs and many applications can't - * dependably find the right one among more - * than one. So bye-bye, old certificates. */ - certlist = PK11_FindCertsFromNickname(entry->cm_cert_nickname, - NULL); - if (certlist != NULL) { - /* Look for certs with contents. */ - for (node = CERT_LIST_HEAD(certlist); - (node != NULL) && - !CERT_LIST_EMPTY(certlist) && - !CERT_LIST_END(node, certlist); - node = CERT_LIST_NEXT(node)) { - if (!SECITEM_ItemsAreEqual(item, - &node->cert->derCert)) { - cm_log(3, "Found a " - "certificate " - "with the same " - "nickname and " - "subject, but " - "different " - "contents, " - "removing it.\n"); - /* Get a handle for - * this certificate's - * private key, in case - * we need to remove - * it. */ - privkey = PK11_FindKeyByAnyCert(node->cert, NULL); - privkeys = add_privkey_to_list(privkeys, privkey); - SEC_DeletePermCertificate(node->cert); - } + switch (ec) { + case PR_NO_ACCESS_RIGHTS_ERROR: /* ACCES/PERM */ + status = CM_CERTSAVE_STATUS_PERMS; + break; + default: + status = CM_CERTSAVE_STATUS_INTERNAL_ERROR; + break; } - CERT_DestroyCertList(certlist); - } - } else { - if (ec != 0) { - es = PR_ErrorToName(ec); - } else { - es = NULL; } - if (es != NULL) { - cm_log(0, "Error importing certificate " - "into NSSDB \"%s\": %s.\n", - entry->cm_cert_storage_location, - es); - } else { - cm_log(0, "Error importing certificate " - "into NSSDB \"%s\".\n", - entry->cm_cert_storage_location); - } - switch (ec) { - case PR_NO_ACCESS_RIGHTS_ERROR: /* ACCES/PERM */ - status = CM_CERTSAVE_STATUS_PERMS; - break; - default: - status = CM_CERTSAVE_STATUS_INTERNAL_ERROR; - break; + /* If we managed to import the certificate, mark its + * key for having its nickname removed. */ + if ((returned != NULL) && (returned[0] != NULL)) { + privkey = PK11_FindKeyByAnyCert(returned[0], NULL); + privkeys = add_privkey_to_list(privkeys, privkey); + CERT_DestroyCertArray(returned, 1); } - } - /* If we managed to import the certificate, mark its - * key for having its nickname removed. */ - if ((returned != NULL) && (returned[0] != NULL)) { - privkey = PK11_FindKeyByAnyCert(returned[0], NULL); - privkeys = add_privkey_to_list(privkeys, privkey); - CERT_DestroyCertArray(returned, 1); - } - /* In case we're rekeying, but failed, mark the - * candidate key for name-clearing or removal, too. */ - if ((entry->cm_key_next_marker != NULL) && - (strlen(entry->cm_key_next_marker) > 0)) { - p = util_build_next_nickname(entry->cm_key_nickname, - entry->cm_key_next_marker); - privkeylist = PK11_ListPrivKeysInSlot(PK11_GetInternalKeySlot(), p, NULL); - if (privkeylist != NULL) { - for (knode = PRIVKEY_LIST_HEAD(privkeylist); - !PRIVKEY_LIST_EMPTY(privkeylist) && - !PRIVKEY_LIST_END(knode, privkeylist); - knode = PRIVKEY_LIST_NEXT(knode)) { - q = PK11_GetPrivateKeyNickname(knode->key); - if ((q != NULL) && - (strcmp(p, q) == 0)) { - privkey = SECKEY_CopyPrivateKey(knode->key); - privkeys = add_privkey_to_list(privkeys, privkey); - break; + /* In case we're rekeying, but failed, mark the + * candidate key for name-clearing or removal, too. */ + if ((entry->cm_key_next_marker != NULL) && + (strlen(entry->cm_key_next_marker) > 0)) { + p = util_build_next_nickname(entry->cm_key_nickname, + entry->cm_key_next_marker); + privkeylist = PK11_ListPrivKeysInSlot(sle->slot, p, NULL); + if (privkeylist != NULL) { + for (knode = PRIVKEY_LIST_HEAD(privkeylist); + !PRIVKEY_LIST_EMPTY(privkeylist) && + !PRIVKEY_LIST_END(knode, privkeylist); + knode = PRIVKEY_LIST_NEXT(knode)) { + q = PK11_GetPrivateKeyNickname(knode->key); + if ((q != NULL) && + (strcmp(p, q) == 0)) { + privkey = SECKEY_CopyPrivateKey(knode->key); + privkeys = add_privkey_to_list(privkeys, privkey); + break; + } } + SECKEY_DestroyPrivateKeyList(privkeylist); } - SECKEY_DestroyPrivateKeyList(privkeylist); } - } - if (privkeys != NULL) { - /* Check if any certificates are still using - * the keys that correspond to certificates - * that we removed. */ - for (i = 0; privkeys[i] != NULL; i++) { - privkey = privkeys[i]; - oldcert = PK11_GetCertFromPrivateKey(privkey); - if (!entry->cm_key_preserve && (oldcert == NULL)) { - /* We're not preserving - * orphaned keys, so remove - * this one. No need to mess - * with its nickname first. */ - PK11_DeleteTokenPrivateKey(privkey, PR_FALSE); - if (error == SECSuccess) { - cm_log(3, "Removed old key.\n"); - } else { - ec = PORT_GetError(); - if (ec != 0) { - es = PR_ErrorToName(ec); + if (privkeys != NULL) { + /* Check if any certificates are still using + * the keys that correspond to certificates + * that we removed. */ + for (i = 0; privkeys[i] != NULL; i++) { + privkey = privkeys[i]; + oldcert = PK11_GetCertFromPrivateKey(privkey); + if (!entry->cm_key_preserve && (oldcert == NULL)) { + /* We're not preserving + * orphaned keys, so remove + * this one. No need to mess + * with its nickname first. */ + PK11_DeleteTokenPrivateKey(privkey, PR_FALSE); + if (error == SECSuccess) { + cm_log(3, "Removed old key.\n"); } else { - es = NULL; + ec = PORT_GetError(); + if (ec != 0) { + es = PR_ErrorToName(ec); + } else { + es = NULL; + } + if (es != NULL) { + cm_log(0, "Failed " + "to remove " + "old key: " + "%s.\n", es); + } else { + cm_log(0, "Failed " + "to remove " + "old key.\n"); + } } - if (es != NULL) { - cm_log(0, "Failed " - "to remove " - "old key: " - "%s.\n", es); - } else { - cm_log(0, "Failed " - "to remove " - "old key.\n"); - } - } - } else { - /* Remove the explicit - * nickname, so that the key - * will have to be found using - * the certificate's nickname, - * and certutil will display - * the matching certificate's - * nickname when it's asked to - * list the keys in the - * database. */ - error = PK11_SetPrivateKeyNickname(privkey, ""); - if (error == SECSuccess) { - cm_log(3, "Removed " - "name from old " - "key.\n"); } else { - ec = PORT_GetError(); - if (ec != 0) { - es = PR_ErrorToName(ec); + /* Remove the explicit + * nickname, so that the key + * will have to be found using + * the certificate's nickname, + * and certutil will display + * the matching certificate's + * nickname when it's asked to + * list the keys in the + * database. */ + error = PK11_SetPrivateKeyNickname(privkey, ""); + if (error == SECSuccess) { + cm_log(3, "Removed " + "name from old " + "key.\n"); } else { - es = NULL; - } - if (es != NULL) { - cm_log(0, "Failed " - "to unname " - "old key: " - "%s.\n", es); - } else { - cm_log(0, "Failed " - "to unname " - "old key.\n"); + ec = PORT_GetError(); + if (ec != 0) { + es = PR_ErrorToName(ec); + } else { + es = NULL; + } + if (es != NULL) { + cm_log(0, "Failed " + "to unname " + "old key: " + "%s.\n", es); + } else { + cm_log(0, "Failed " + "to unname " + "old key.\n"); + } } + SECKEY_DestroyPrivateKey(privkey); + } + if (oldcert != NULL) { + CERT_DestroyCertificate(oldcert); } - SECKEY_DestroyPrivateKey(privkey); - } - if (oldcert != NULL) { - CERT_DestroyCertificate(oldcert); } + free(privkeys); } - free(privkeys); + } else { + cm_log(1, "Error getting handle to default NSS DB.\n"); } - } else { - cm_log(1, "Error getting handle to default NSS DB.\n"); - } - PORT_FreeArena(arena, PR_TRUE); - if (NSS_ShutdownContext(ctx) != SECSuccess) { - cm_log(1, "Error shutting down NSS.\n"); - } - /* Fixup the ownership and permissions on the key and - * certificate databases. */ - util_set_db_entry_key_owner(entry->cm_key_storage_location, entry); - util_set_db_entry_cert_owner(entry->cm_cert_storage_location, entry); - } + PORT_FreeArena(arena, PR_TRUE); + if (NSS_ShutdownContext(ctx) != SECSuccess) { + cm_log(1, "Error shutting down NSS.\n"); + } + /* Fixup the ownership and permissions on the key and + * certificate databases. */ + util_set_db_entry_key_owner(entry->cm_key_storage_location, entry); + util_set_db_entry_cert_owner(entry->cm_cert_storage_location, entry); + break; +next_slot: + if (sle == slotlist->tail) { + break; + } + } /* for slot loop */ + } /* ctx == NULL */ + if (status != 0) { _exit(status); } From c029b32c04a9a5993b9c8715fb82421fee613137 Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: Sep 25 2018 13:08:48 +0000 Subject: [PATCH 2/7] Include the token name when a PIN is provided but is unused This improves the output so the user will know which token the PIN is missing for. Theoretically it should be the token they asked for but this will show certmogner's view of it. --- diff --git a/src/certread-n.c b/src/certread-n.c index f2e78c0..57a38dc 100644 --- a/src/certread-n.c +++ b/src/certread-n.c @@ -259,9 +259,9 @@ cm_certread_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, if ((pin != NULL) && (strlen(pin) > 0) && (cb_data.n_attempts == 0)) { - cm_log(1, "PIN was not needed to auth to cert " - "db, though one was provided. " - "Treating this as an error.\n"); + cm_log(1, "PIN was not needed to auth to token " + "%s, though one was provided. " + "Treating this as an error.\n", token); goto next_slot; } } diff --git a/src/keygen-n.c b/src/keygen-n.c index 8078a52..84b0bbd 100644 --- a/src/keygen-n.c +++ b/src/keygen-n.c @@ -400,8 +400,8 @@ next_slot: (strlen(pin) > 0) && (cb_data.n_attempts == 0)) { cm_log(1, "PIN was not needed to auth to key " - "store, though one was provided. " - "Treating this as an error.\n"); + "store token %s, though one was provided. " + "Treating this as an error.\n", token); PK11_FreeSlotList(slotlist); error = NSS_ShutdownContext(ctx); if (error != SECSuccess) { From f396b19b2c222fa0a50e9bb9704059af4578e678 Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: Sep 25 2018 13:08:48 +0000 Subject: [PATCH 3/7] Add utility function to get the internal token name The NSS internal token is the default if no token is specified for the cert or the key. --- diff --git a/src/certread-n.c b/src/certread-n.c index 57a38dc..1d9217c 100644 --- a/src/certread-n.c +++ b/src/certread-n.c @@ -190,6 +190,9 @@ cm_certread_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, cm_log(1, "Error reading PIN for cert db.\n"); _exit(CM_SUB_STATUS_ERROR_AUTH); } + if (entry->cm_cert_token == NULL) { + entry->cm_cert_token = util_internal_token_name(); + } PK11_SetPasswordFunc(&cm_pin_read_for_cert_nss_cb); for (sle = slotlist->head; ((sle != NULL) && (sle->slot != NULL)); @@ -253,7 +256,8 @@ cm_certread_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, } error = PK11_Authenticate(sle->slot, PR_TRUE, &cb_data); if (error != SECSuccess) { - cm_log(1, "Error authenticating to cert db.\n"); + cm_log(1, "certread-n: Error authenticating to cert db " + "slot %s.\n", PK11_GetTokenName(sle->slot)); goto next_slot; } if ((pin != NULL) && diff --git a/src/certsave-n.c b/src/certsave-n.c index af176ce..193309c 100644 --- a/src/certsave-n.c +++ b/src/certsave-n.c @@ -214,6 +214,9 @@ cm_certsave_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, _exit(CM_SUB_STATUS_ERROR_AUTH); } PK11_SetPasswordFunc(&cm_pin_read_for_cert_nss_cb); + if (entry->cm_cert_token == NULL) { + entry->cm_cert_token = util_internal_token_name(); + } for (sle = slotlist->head; ((sle != NULL) && (sle->slot != NULL)); sle = sle->next) diff --git a/src/keygen-n.c b/src/keygen-n.c index 84b0bbd..f7fdf6c 100644 --- a/src/keygen-n.c +++ b/src/keygen-n.c @@ -272,6 +272,9 @@ cm_keygen_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, cm_log(1, "Error locating token for key generation.\n"); _exit(CM_SUB_STATUS_ERROR_NO_TOKEN); } + if (entry->cm_cert_token == NULL) { + entry->cm_cert_token = util_internal_token_name(); + } /* Walk the list looking for the requested slot, or the first one if * none was requested. */ slot = NULL; diff --git a/src/keyiread-n.c b/src/keyiread-n.c index 89913aa..b8408bf 100644 --- a/src/keyiread-n.c +++ b/src/keyiread-n.c @@ -152,6 +152,9 @@ cm_keyiread_n_get_keys(struct cm_store_entry *entry, int readwrite) _exit(CM_SUB_STATUS_ERROR_AUTH); } PK11_SetPasswordFunc(&cm_pin_read_for_cert_nss_cb); + if (entry->cm_key_token == NULL) { + entry->cm_key_token = util_internal_token_name(); + } n_tokens = 0; pubkey = NULL; /* In practice, the internal slot is either a non-storage slot (in diff --git a/src/submit-n.c b/src/submit-n.c index 872153e..da07d25 100644 --- a/src/submit-n.c +++ b/src/submit-n.c @@ -346,6 +346,9 @@ cm_submit_n_decrypt_envelope(const unsigned char *envelope, cm_log(1, "Error reading PIN for key storage.\n"); goto done; } + if (args->entry->cm_key_token == NULL) { + args->entry->cm_key_token = util_internal_token_name(); + } PK11_SetPasswordFunc(&cm_pin_read_for_cert_nss_cb); n_tokens = 0; /* In practice, the internal slot is either a non-storage slot (in @@ -402,7 +405,7 @@ cm_submit_n_decrypt_envelope(const unsigned char *envelope, } error = PK11_Authenticate(slot, PR_TRUE, &cb_data); if (error != SECSuccess) { - cm_log(1, "Error authenticating to token " + cm_log(1, "submit-n: Error authenticating to token " "\"%s\".\n", token); goto done; } diff --git a/src/util-n.c b/src/util-n.c index 7805e58..293e258 100644 --- a/src/util-n.c +++ b/src/util-n.c @@ -287,3 +287,9 @@ util_set_db_entry_cert_owner(const char *dbdir, struct cm_store_entry *entry) util_set_db_owner_perms(dbdir, secmoddb, entry->cm_cert_owner, entry->cm_cert_perms); } + +char * +util_internal_token_name() +{ + return strdup(PK11_GetTokenName(PK11_GetInternalKeySlot())); +} diff --git a/src/util-n.h b/src/util-n.h index 8a918d5..637fd4b 100644 --- a/src/util-n.h +++ b/src/util-n.h @@ -29,5 +29,6 @@ void util_set_db_entry_key_owner(const char *dbdir, struct cm_store_entry *entry); void util_set_db_entry_cert_owner(const char *dbdir, struct cm_store_entry *entry); +char * util_internal_token_name(); #endif From 6ebe5695a626c6cd254b249bbebf9846bcb936c0 Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: Sep 25 2018 13:08:48 +0000 Subject: [PATCH 4/7] Only de-duplicate certificates within the same token certmonger may not have read/write access to tokens other than the one it is examining so don't try to de-duplicate certificates on other tokens. --- diff --git a/src/certsave-n.c b/src/certsave-n.c index 193309c..d0152ca 100644 --- a/src/certsave-n.c +++ b/src/certsave-n.c @@ -391,8 +391,9 @@ cm_certsave_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, !CERT_LIST_EMPTY(certlist) && !CERT_LIST_END(node, certlist); node = CERT_LIST_NEXT(node)) { - if (!SECITEM_ItemsAreEqual(&subject, - &node->cert->derSubject)) { + if ((!SECITEM_ItemsAreEqual(&subject, + &node->cert->derSubject)) && + (sle->slot == node->cert->slot)) { cm_log(3, "Found a " "certificate " "with the same " @@ -441,7 +442,8 @@ cm_certsave_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, node = CERT_LIST_NEXT(node)) { if ((node->cert->nickname != NULL) && (strcmp(entry->cm_cert_nickname, - node->cert->nickname) != 0)) + node->cert->nickname) != 0) && + (sle->slot == node->cert->slot)) { i++; cm_log(3, "Found a " From 697dd085e7b2ce15eefc454509987270131d7f1e Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: Sep 25 2018 13:08:48 +0000 Subject: [PATCH 5/7] Ensure that an OpenSSL random seed file exists when testing Otherwise some openssl command-line invocations will fail and because of the way the tests are done the error message is not shown. --- diff --git a/tests/Makefile.am b/tests/Makefile.am index 4e40743..fe368dc 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -433,6 +433,9 @@ subdirs += \ endif check: all + if [ ! -e $$HOME/.rnd ] ; then \ + openssl rand -writerand $$HOME/.rnd; \ + fi for required in certutil cmsutil pk12util openssl diff cmp mktemp \ dos2unix unix2dos dbus-launch ; do \ which $$required || exit 1; \ From e93ecadec7c868f4227e084ffb65c70a6efd7314 Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: Sep 25 2018 13:08:48 +0000 Subject: [PATCH 6/7] Log test failures of bad pin Previously this would show a "don't know why" failure. --- diff --git a/tests/tools/certsave.c b/tests/tools/certsave.c index ac0f73e..fd86a4c 100644 --- a/tests/tools/certsave.c +++ b/tests/tools/certsave.c @@ -106,6 +106,11 @@ main(int argc, char **argv) printf("Failed to save (%s:%s), " "filesystem permissions error.\n", ctype, entry->cm_cert_storage_location); + } else + if (cm_certsave_pin_error(state) == 0) { + printf("Failed to save (%s:%s), " + "pin error.\n", + ctype, entry->cm_cert_storage_location); } else { printf("Failed to save (%s:%s), " "don't know why.\n", From 15d406ee3afbb52832d5c61a1afb735724d109a2 Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: Sep 25 2018 13:08:48 +0000 Subject: [PATCH 7/7] Use only PK11_ImportCert to import certs, not CERT_ImportCerts CERT_ImportCerts always imports a given certificate into the certificate database, whether a token is requested or not. Using PK11_ImportCert will import the cert, associate the key properly and will only add the certificate to the appropriate token. --- diff --git a/src/certsave-n.c b/src/certsave-n.c index d0152ca..fcb4314 100644 --- a/src/certsave-n.c +++ b/src/certsave-n.c @@ -100,7 +100,7 @@ cm_certsave_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, NSSInitContext *ctx; CERTCertDBHandle *certdb; CERTCertList *certlist; - CERTCertificate **returned, *oldcert, cert; + CERTCertificate *oldcert, *newcert, cert; CERTCertTrust trust; CERTSignedData csdata; CERTCertListNode *node; @@ -497,33 +497,18 @@ cm_certsave_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, } } /* Import the certificate. */ - returned = NULL; - error = CERT_ImportCerts(certdb, - certUsageUserCertImport, - 1, &item, &returned, - PR_TRUE, - PR_FALSE, - entry->cm_cert_nickname); - ec = PORT_GetError(); - if (error == SECSuccess) { - /* If NSS uses SQL DB storage, CERT_ImportCerts creates - * an incomplete internal state (the cert isn't - * associated with the private key, and calling - * PK11_FindKeyByAnyCert returns no result). - * As a workaround, we import the cert again using - * PK11_ImportCert, which magically fixes the issue. - * See rhbz#1532188 */ + newcert = CERT_DecodeCertFromPackage((char *)item->data, item->len); + if (newcert != NULL) { error = PK11_ImportCert(sle->slot, - returned[0], + newcert, CK_INVALID_HANDLE, - returned[0]->nickname, + entry->cm_cert_nickname, PR_FALSE); } if (error == SECSuccess) { - cm_log(1, "Imported certificate \"%s\", got " + cm_log(1, "Imported certificate with " "nickname \"%s\".\n", - entry->cm_cert_nickname, - returned[0]->nickname); + entry->cm_cert_nickname); status = 0; /* Set the trust on the new certificate, * perhaps matching the trust on an @@ -536,7 +521,7 @@ cm_certsave_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, trust.objectSigningFlags = CERTDB_USER; } error = CERT_ChangeCertTrust(certdb, - returned[0], + newcert, &trust); ec = PORT_GetError(); if (error != SECSuccess) { @@ -621,10 +606,10 @@ cm_certsave_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry, } /* If we managed to import the certificate, mark its * key for having its nickname removed. */ - if ((returned != NULL) && (returned[0] != NULL)) { - privkey = PK11_FindKeyByAnyCert(returned[0], NULL); + if (newcert != NULL) { + privkey = PK11_FindKeyByAnyCert(newcert, NULL); privkeys = add_privkey_to_list(privkeys, privkey); - CERT_DestroyCertArray(returned, 1); + CERT_DestroyCertificate(newcert); } /* In case we're rekeying, but failed, mark the * candidate key for name-clearing or removal, too. */