From 52cfcf7f292d3989c5e489cb59a32d7054c43652 Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Tue, 2 Nov 2010 18:24:45 -0400 Subject: [PATCH 104/150] - comments --- src/plugins/preauth/pkinit/pkinit_crypto_nss.c | 95 +++++++++++++++++------- 1 files changed, 67 insertions(+), 28 deletions(-) diff --git a/src/plugins/preauth/pkinit/pkinit_crypto_nss.c b/src/plugins/preauth/pkinit/pkinit_crypto_nss.c index 4847835..cf7b7ba 100644 --- a/src/plugins/preauth/pkinit/pkinit_crypto_nss.c +++ b/src/plugins/preauth/pkinit/pkinit_crypto_nss.c @@ -105,11 +105,11 @@ struct _pkinit_req_crypto_context { struct _pkinit_identity_crypto_context { PLArenaPool *pool; - SECMODModule *pem_module; - SECMODModule **id_modules; - PK11SlotInfo **id_userdbs; - PK11SlotInfo *id_p12_slot; - PK11GenericObject **id_objects; + SECMODModule *pem_module; /* used for FILE: and DIR: */ + SECMODModule **id_modules; /* used for PKCS11: */ + PK11SlotInfo **id_userdbs; /* used for NSS: */ + PK11SlotInfo *id_p12_slot; /* used for PKCS12: */ + PK11GenericObject **id_objects; /* used with FILE: and DIR: */ CERTCertList *id_certs, *ca_certs; CERTCertificate *id_cert; struct { @@ -566,6 +566,7 @@ crypto_pwcb(PK11SlotInfo *slot, PRBool retry, void *arg) name, answer, 1, &prompt); answer = NULL; if ((ret == 0) && (reply.data != NULL)) { + /* The result will be freed with PR_Free, so return a copy. */ answer = PR_Malloc(reply.length + 1); memcpy(answer, reply.data, reply.length); answer[reply.length] = '\0'; @@ -585,6 +586,7 @@ crypto_pwcb(PK11SlotInfo *slot, PRBool retry, void *arg) return answer; } +/* Make sure we're using our callback, and set up the callback data. */ static void * crypto_pwcb_prep(pkinit_identity_crypto_context id_cryptoctx, krb5_context context) @@ -631,6 +633,8 @@ pkinit_fini_identity_crypto(pkinit_identity_crypto_context id_cryptoctx) { int i; pkiDebug("%s\n", __FUNCTION__); + /* The order of cleanup here is intended to ensure that nothing gets + * freed before anything that might have a reference to it. */ if (id_cryptoctx->id_cert != NULL) { CERT_DestroyCertificate(id_cryptoctx->id_cert); } @@ -1168,7 +1172,9 @@ oakley_get_group(PLArenaPool *pool, int minimum_prime_size) if (params == NULL) { return NULL; } - for (i = 0; i < sizeof(oakley_groups) / sizeof(oakley_groups[0]); i++) { + for (i = 0; + i < sizeof(oakley_groups) / sizeof(oakley_groups[0]); + i++) { if (oakley_groups[i].bits >= minimum_prime_size) { if (oakley_parse_group(pool, &oakley_groups[i], ¶ms) == 0) { @@ -1969,6 +1975,8 @@ crypto_load_pkcs11(krb5_context context, return status; } +/* Return the slot which we'll use for holding PEM items. Open the module if + * we need to, first. */ static PK11SlotInfo * crypto_get_pem_slot(struct _pkinit_identity_crypto_context *id) { @@ -1999,6 +2007,8 @@ crypto_get_pem_slot(struct _pkinit_identity_crypto_context *id) return slot; } +/* Return the slot which we'll use for holding imported PKCS12 certificates + * and keys. Open the module if we need to, first. */ static PK11SlotInfo * crypto_get_p12_slot(struct _pkinit_identity_crypto_context *id) { @@ -2010,10 +2020,12 @@ crypto_get_p12_slot(struct _pkinit_identity_crypto_context *id) return id->id_p12_slot; } +/* Resolve any ambiguities from having a duplicate nickname in the PKCS12 + * bundle and in the database. */ static SECItem * crypto_nickname_c_cb(SECItem *old_nickname, PRBool *cancel, void *arg) { - pkiDebug("%s: warning: nickname collision on \"%.*s\"\n", + pkiDebug("%s: warning: nickname collision on \"%.*s\", skipping\n", __FUNCTION__, old_nickname->len, old_nickname->data); *cancel = PR_TRUE; return NULL; @@ -2098,6 +2110,7 @@ crypto_load_pkcs12(krb5_context context, return SECSuccess; } +/* Helper to fill out a CK_ATTRIBUTE. */ static void crypto_set_attributes(CK_ATTRIBUTE *attr, CK_ATTRIBUTE_TYPE type, @@ -2110,6 +2123,7 @@ crypto_set_attributes(CK_ATTRIBUTE *attr, attr->ulValueLen = ulValueLen; } +/* Load keys, certs, and/or CRLs from files. */ static SECStatus crypto_load_files(krb5_context context, pkinit_plg_crypto_context plg_cryptoctx, @@ -2154,6 +2168,10 @@ crypto_load_files(krb5_context context, if ((certfile == NULL) && (crlfile == NULL)) { return SECFailure; } + /* If we're told to load a key, then we know for sure that it's a + * key+cert combination, so go ahead and try to load the key first. + * That way, if we're just guessing that there's a key, and we're + * wrong, we'll just skip the cert. */ status = SECSuccess; if (keyfile != NULL) { n_attrs = 0; @@ -2163,7 +2181,7 @@ crypto_load_files(krb5_context context, &cktrue, sizeof(cktrue)); crypto_set_attributes(&attrs[n_attrs++], CKA_LABEL, (char *) keyfile, strlen(keyfile) + 1); - permanent = PR_FALSE; + permanent = PR_FALSE; /* set lifetime to "session" */ obj = PK11_CreateGenericObject(slot, attrs, n_attrs, permanent); if (obj == NULL) { pkiDebug("%s: error loading key \"%s\"\n", @@ -2199,6 +2217,8 @@ crypto_load_files(krb5_context context, } } + /* If we loaded a key, or there wasn't one, see if we were told to + * load a cert. */ if ((status == SECSuccess) && (certfile != NULL)) { before = PK11_ListCertsInSlot(slot); n_attrs = 0; @@ -2211,7 +2231,7 @@ crypto_load_files(krb5_context context, cktrust = cert_mark_trusted ? CK_TRUE : CK_FALSE; crypto_set_attributes(&attrs[n_attrs++], CKA_TRUST, &cktrust, sizeof(cktrust)); - permanent = PR_FALSE; + permanent = PR_FALSE; /* set lifetime to "session" */ obj = PK11_CreateGenericObject(slot, attrs, n_attrs, permanent); if (obj == NULL) { pkiDebug("%s: error loading %scertificate \"%s\"\n", @@ -2250,9 +2270,9 @@ crypto_load_files(krb5_context context, } } /* Add any certs which are in the slot now, but which weren't - * before, to the right list of certs. (We don't have an API - * to get the certificate from the generic object that we just - * created, so we have to do it the hard way.) */ + * before, to the right list of certs. (I don't see an API to + * get the certificate from the generic object that we just + * created, so we do it the hard way.) */ after = PK11_ListCertsInSlot(slot); if (after != NULL) { for (anode = CERT_LIST_HEAD(after); @@ -2290,7 +2310,7 @@ crypto_load_files(krb5_context context, status = SECFailure; } } else { - /* Don't lose the ref. */ + /* Don't just lose the ref. */ CERT_DestroyCertificate(cert); } } @@ -2302,6 +2322,8 @@ crypto_load_files(krb5_context context, } } + /* If we succeeded to this point, or more likely didn't do anything + * yet, cache a CRL. */ if ((status == SECSuccess) && (crlfile != NULL)) { /* FIXME: cache a CRL from the named file */ } @@ -2343,13 +2365,13 @@ crypto_load_dir(krb5_context context, while ((ent = readdir(dir)) != NULL) { i = strlen(ent->d_name); /* Skip over anything that isn't named ".crt" or - * ".crl", whichever we want. */ + * ".crl", whichever we want at the moment. */ if ((i < 5) || (strcmp(ent->d_name + i - 4, suffix) != 0)) { pkiDebug("%s: skipping candidate \"%s/%s\"\n", __FUNCTION__, dirname, ent->d_name); continue; } - /* Construct full path to the file. */ + /* Construct a path to the file. */ certcrl = PORT_Alloc(strlen(dirname) + 1 + i + 1); if (certcrl == NULL) { continue; @@ -2416,7 +2438,8 @@ crypto_load_certdb(krb5_context context, j = strlen(p); for (i = 0; configdir[i] != '\0'; i++) { if (configdir[i] == '\'') { - p[j++] = '\\'; + p[j++] = '\\'; /* Is this the right way to do + * escaping? */ } p[j++] = configdir[i]; } @@ -2555,7 +2578,7 @@ crypto_free_cert_info(krb5_context context, pkinit_req_crypto_context req_cryptoctx, pkinit_identity_crypto_context id_cryptoctx) { - return 0; /* FIXME: maybe should we nuke some lists here? */ + return 0; /* Maybe should we nuke the id_certs list here? */ } /* Count how many candidate "self" certificates and keys we have. We could as @@ -2688,7 +2711,7 @@ cert_get_ku_bits(krb5_context context, CERTCertificate *cert) } static unsigned int -cert_get_eku_bits(krb5_context context, CERTCertificate *cert, int kdc) +cert_get_eku_bits(krb5_context context, CERTCertificate *cert, PRBool kdc) { PLArenaPool *pool; SECItem *ext, **oids; @@ -2759,7 +2782,7 @@ crypto_cert_get_matching_data(krb5_context context, md->issuer_dn = strdup(cert_handle->cert->issuerName); /* FIXME: these are not RFC2253 */ md->ku_bits = cert_get_ku_bits(context, cert_handle->cert); - md->eku_bits = cert_get_eku_bits(context, cert_handle->cert, 0); + md->eku_bits = cert_get_eku_bits(context, cert_handle->cert, PR_FALSE); if (cert_retrieve_cert_sans(context, cert_handle->cert, &md->sans, &md->sans, NULL) != 0) { free(md->subject_dn); @@ -2894,45 +2917,51 @@ crypto_load_cas_and_crls(krb5_context context, /* Figure out what we're doing here. */ switch (catype) { case CATYPE_ANCHORS: - /* Mark certs we load as trusted roots. */ + /* Screen out source types we can't use. */ switch (idtype) { case IDTYPE_FILE: case IDTYPE_DIR: case IDTYPE_NSS: + /* We only support these sources. */ break; default: return EINVAL; break; } + /* Mark certs we load as trusted roots. */ cert_self = PR_FALSE; cert_mark_trusted = PR_TRUE; load_crl = PR_FALSE; break; case CATYPE_INTERMEDIATES: - /* Hang on to certs as reference material. */ + /* Screen out source types we can't use. */ switch (idtype) { case IDTYPE_FILE: case IDTYPE_DIR: case IDTYPE_NSS: + /* We only support these sources. */ break; default: return EINVAL; break; } + /* Hang on to certs as reference material. */ cert_self = PR_FALSE; cert_mark_trusted = PR_FALSE; load_crl = PR_FALSE; break; case CATYPE_CRLS: - /* FIXME: Load CRLs. */ + /* Screen out source types we can't use. */ switch (idtype) { case IDTYPE_FILE: case IDTYPE_DIR: + /* We only support these sources. */ break; default: return EINVAL; break; } + /* No certs, just CRLs. */ cert_self = PR_FALSE; cert_mark_trusted = PR_FALSE; load_crl = PR_TRUE; @@ -3004,6 +3033,7 @@ pkinit_get_kdc_cert(krb5_context context, return 0; } +/* Create typed-data with sets of acceptable DH parameters. */ krb5_error_code pkinit_create_td_dh_parameters(krb5_context context, pkinit_plg_crypto_context plg_cryptoctx, @@ -3016,7 +3046,7 @@ pkinit_create_td_dh_parameters(krb5_context context, SECItem tmp, *oid; krb5_algorithm_identifier id[sizeof(oakley_groups) / sizeof(oakley_groups[0])]; - const krb5_algorithm_identifier *ids[sizeof(id) / sizeof(id[0]) + 1]; + const krb5_algorithm_identifier *ids[(sizeof(id) / sizeof(id[0])) + 1]; unsigned int i, j; krb5_data *data; krb5_typed_data typed_datum; @@ -3074,6 +3104,8 @@ pkinit_create_td_dh_parameters(krb5_context context, return code; } +/* Parse typed-data with sets of acceptable DH parameters and return the + * minimum prime size that the KDC will accept. */ krb5_error_code pkinit_process_td_dh_params(krb5_context context, pkinit_plg_crypto_context plg_cryptoctx, @@ -3107,6 +3139,7 @@ pkinit_process_td_dh_params(krb5_context context, return 0; } +/* Create typed-data with the client cert that we didn't like. */ krb5_error_code pkinit_create_td_invalid_certificate(krb5_context context, pkinit_plg_crypto_context plg_cryptoctx, @@ -3114,6 +3147,7 @@ pkinit_create_td_invalid_certificate(krb5_context context, pkinit_identity_crypto_context id_cryptoctx, krb5_data **edata) { + CERTCertificate *invalid; krb5_external_principal_identifier id; const krb5_external_principal_identifier *ids[2]; struct issuer_and_serial_number isn; @@ -3123,16 +3157,18 @@ pkinit_create_td_invalid_certificate(krb5_context context, const krb5_typed_data *typed_data[2]; krb5_error_code code; - /* We didn't trust the peer's certificate. */ + /* We didn't trust the peer's certificate. FIXME: or was it a + * certificate that was somewhere in its certifying chain? */ if (req_cryptoctx->peer_cert == NULL) { return ENOENT; } + invalid = req_cryptoctx->peer_cert; /* Fill in the identifier. */ memset(&id, 0, sizeof(id)); if (req_cryptoctx->peer_cert->keyIDGenerated) { - isn.issuer = req_cryptoctx->peer_cert->derIssuer; - isn.serial = req_cryptoctx->peer_cert->serialNumber; + isn.issuer = invalid->derIssuer; + isn.serial = invalid->serialNumber; if (SEC_ASN1EncodeItem(req_cryptoctx->pool, &item, &isn, issuer_and_serial_number_template) != &item) { return ENOMEM; @@ -3140,7 +3176,7 @@ pkinit_create_td_invalid_certificate(krb5_context context, id.issuerAndSerialNumber.data = item.data; id.issuerAndSerialNumber.length = item.len; } else { - item = req_cryptoctx->peer_cert->subjectKeyID; + item = invalid->subjectKeyID; id.subjectKeyIdentifier.data = item.data; id.subjectKeyIdentifier.length = item.len; } @@ -3164,6 +3200,7 @@ pkinit_create_td_invalid_certificate(krb5_context context, return code; } +/* Create typed-data with a list of certifiers that we would accept. */ krb5_error_code pkinit_create_td_trusted_certifiers(krb5_context context, pkinit_plg_crypto_context plg_cryptoctx, @@ -3199,6 +3236,7 @@ pkinit_create_td_trusted_certifiers(krb5_context context, PR_FALSE, crypto_pwcb_prep(id_cryptoctx, context)); if (slist == NULL) { + CERT_DestroyCertList(clist); return ENOENT; } @@ -3206,7 +3244,7 @@ pkinit_create_td_trusted_certifiers(krb5_context context, i = 0; status = SECSuccess; for (sle = slist->head; sle != NULL; sle = sle->next) { - /* Skip over slots we would need to log in to use. */ + /* Skip over slots we would still need to log in to use. */ if (!PK11_IsLoggedIn(sle->slot, crypto_pwcb_prep(id_cryptoctx, context)) && PK11_NeedLogin(sle->slot)) { @@ -3390,6 +3428,7 @@ pkinit_identity_set_prompter(pkinit_identity_crypto_context id_cryptoctx, return 0; } +/* Convert a DH secret to a keyblock. */ krb5_error_code pkinit_octetstring2key(krb5_context context, krb5_enctype etype, -- 1.7.6.4