On Thu, Aug 28, 2008 at 08:22:27PM +0200, Mark Phalan wrote: > > > I've just uploaded a webrev of my resync/pkinit workspace. There still > needs to be some work on pkinit so don't expect the code in > usr/src/lib/krb5/plugins/preauth/pkinit/ to be complete (you can ignore > it for now). I'll post another incremental webrev with any changes I > make to the pkinit code later on. The rest of the changes are resync > changes for MIT 1.6.3. The hg comment needs to be updated, I'll do that > once we get the pkinit PSARC case submitted. > > I've chunked the review up into four pieces as I expect the krb team to > do the review. > > Shawn: Chunk 1 > Peter: Chunk 2 > Glenn: Chunk 3 > Will: Chunk 4 > > I'd like to have this completed by 17th Sept. Let me know if thats a > problem for anyone. > > webrev here: > http://cr.opensolaris.org/~mbp/pkinit/
Have you run this code through the lint -errsecurity=standard security checks? <see http://secprog.sfbay.sun.com/lint/levels.html> Have you compiled with DEBUG defined to make sure the plugin still works? ======================================================================================= Chunk 4 code review: In usr/src/lib/krb5/plugins/preauth/pkinit/pkinit_crypto_openssl.c static int pkinit_oids_refs = 0; and pkinit_init_pkinit_oids(): pkinit_oids_refs++; and pkinit_fini_pkinit_oids(): /* Only call OBJ_cleanup once! */ if (--pkinit_oids_refs == 0) OBJ_cleanup(); - Any thread safety issues here? pkinit_init_req_crypto(): *cryptoctx = ctx; - Other funcs would test an output arg like cryptoctx for NULL and return EINVAL. This should too I guess. out: if (retval) free(ctx); - In earlier funcs ctx was tested for NULL as a condition prior to calling free(). To be safe, that should happen here as well. static krb5_error_code pkinit_init_certs(pkinit_identity_crypto_context ctx) { krb5_error_code retval = ENOMEM; int i; for (i = 0; i < MAX_CREDS_ALLOWED; i++) ctx->creds[i] = NULL; ctx->my_certs = NULL; ctx->cert_index = 0; ctx->my_key = NULL; ctx->trustedCAs = NULL; ctx->intermediateCAs = NULL; ctx->revoked = NULL; retval = 0; return retval; } - The retval assignments are unnecessary (just return 0). cms_signeddata_create(): /* start creating PKCS7 data */ if ((p7 = PKCS7_new()) == NULL) - Are allocation functions like PKCS7_new() allocating zero'ed out structs? If not, do you think they should? *signed_data_len = i2d_PKCS7(p7, NULL); - Again, an instance of an output var. assignment without testing for NULL. This should be handled consistently one way or another. /* fill-in PKCS7_SIGNER_INFO */ if ((p7si = PKCS7_SIGNER_INFO_new()) == NULL) goto cleanup; if (!ASN1_INTEGER_set(p7si->version, 1)) goto cleanup; if (!X509_NAME_set(&p7si->issuer_and_serial->issuer, X509_get_issuer_name(cert))) goto cleanup; - Opportunity for mem leaks in regards to the error path and p7si? cms_envelopeddata_create(): switch (pa_type) { case KRB5_PADATA_PK_AS_REQ: p7->d.enveloped->enc_data->content_type = OBJ_nid2obj(NID_pkcs7_signed); break; case KRB5_PADATA_PK_AS_REP_OLD: case KRB5_PADATA_PK_AS_REQ_OLD: p7->d.enveloped->enc_data->content_type = OBJ_nid2obj(NID_pkcs7_data); break; break; break; break; } - too breaky. pkinit_create_td_dh_parameters(krb5_context context, pkinit_plg_crypto_context plg_cryptoctx, pkinit_req_crypto_context req_cryptoctx, pkinit_identity_crypto_context id_cryptoctx, pkinit_plg_opts *opts, krb5_data **out_data) { krb5_error_code retval = ENOMEM; unsigned int buf1_len = 0, buf2_len = 0, buf3_len = 0, i = 0; unsigned char *buf1 = NULL, *buf2 = NULL, *buf3 = NULL; krb5_typed_data **typed_data = NULL; krb5_data *data = NULL, *encoded_algId = NULL; krb5_algorithm_identifier **algId = NULL; if (opts->dh_min_bits > 4096) goto cleanup; - retval looks wrong in regards to the first goto there. In fact there are several functions in this file that have this problem. crypto_check_cert_eku(): *valid_eku = 0; if (reqctx->received_cert == NULL) goto cleanup; - I don't think "*valid_eku = 0" should occur before the input is checked. The caller must check the return of crypto_check_cert_eku() before checking *valid_eku value. pkinit_octetstring2key(): key_block->magic = 0; - Why not KV5M_KEYBLOCK? client_create_dh(): krb5_error_code retval = KRB5KDC_ERR_PREAUTH_FAILED; unsigned char *buf = NULL; int dh_err = 0; ASN1_INTEGER *pub_key = NULL; if (cryptoctx->dh == NULL) { if ((cryptoctx->dh = DH_new()) == NULL) goto cleanup; - Again retval doesn't seem right for first goto cleanup error path. DH_check(cryptoctx->dh, &dh_err); if (dh_err != 0) { pkiDebug("Warning: dh_check failed with %d\n", dh_err); if (dh_err & DH_CHECK_P_NOT_PRIME) pkiDebug("p value is not prime\n"); if (dh_err & DH_CHECK_P_NOT_SAFE_PRIME) pkiDebug("p value is not a safe prime\n"); if (dh_err & DH_UNABLE_TO_CHECK_GENERATOR) pkiDebug("unable to check the generator value\n"); if (dh_err & DH_NOT_SUITABLE_GENERATOR) pkiDebug("the g value is not a generator\n"); } - Why don't the errors above cause the function to goto cleanup? If they are meaningless then why call DH_check()? client_process_dh(): if (der_decode_data(subjectPublicKey_data, (long)subjectPublicKey_length, &data, &data_len) != 0) { pkiDebug("failed to decode subjectPublicKey\n"); retval = -1; goto cleanup; } - Will a retval = -1 be interpreted/mapped correctly? if ((pub_key = d2i_ASN1_INTEGER(NULL, &p, data_len)) == NULL) goto cleanup; if ((server_pub_key = ASN1_INTEGER_to_BN(pub_key, NULL)) == NULL) goto cleanup; - These don't look like KRB5KDC_ERR_PREAUTH_FAILED errors to me. server_process_dh(): if (!DH_generate_key(dh_server)) goto cleanup; - Seems like the real error may be lost here. i2d_ASN1_INTEGER(pub_key, &p); - should have (void) cast. This occurs in other places as well. pkinit_open_session(): /* Init */ /* Solaris Kerberos XXX */ r = cctx->p11->C_Initialize(NULL); and if ((r = cctx->p11->C_OpenSession(slotlist[i], CKF_SERIAL_SESSION, NULL, NULL, &cctx->session)) != CKR_OK) - Be aware that pkcs11 session handles are not fork safe. Do you know whether the cctx will be used across a fork? See: 5102151 krb contexts are not fork safe for the krb bug relating to this issue. ============================================================================ Stopping here for the day. -- Will Fiveash Sun Microsystems Inc. http://opensolaris.org/os/project/kerberos/
