On Mon, 2008-09-29 at 19:20 -0500, Will Fiveash wrote:
> 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>
No. I was relying on the standard ON lint checks. I'll re-run with
"-errsecurity=standard" for the new pkinit code.
>
> Have you compiled with DEBUG defined to make sure the plugin still
> works?
DEBUG works with the latest version of pkinit code (see below).
>
> =======================================================================================
> Chunk 4 code review:
>
> In usr/src/lib/krb5/plugins/preauth/pkinit/pkinit_crypto_openssl.c
Thanks for reviewing this file - unfortunately there are going to be
more changes to this file which will be need to be reviewed later. I did
mention above:
"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."
>
> 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?
Looks like it to me. Probably not an issue as krb5kdc/kinit/pam_krb5 are
not multithreaded applications. May be a problem if krb5kdc becomes
multithreaded.
>
> 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.
Agreed. Fixed.
>
> 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.
I agree from a style point of view. However its valid to feed free() a
NULL and nothing bad will happen.
I'm leaving this as is to keep the delta smaller between MIT's and our
code.
>
> 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).
>
Agreed but leaving as they do no harm (and source code differences
matter more...).
> 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?
In this case I believe that PKCS7_new will allocate and initialize a
structure with sane values (0s mostly)- i.e. the structure won't have
garbage for values after this call.
>
> *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.
>
Agreed. I've added a check for NULL for both signed_data and
signed_data_len.
> /* 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?
>
Fixed.
> 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.
>
Fixed.
> 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.
>
Fixed for pkinit_create_dt_dh_parameters().
Which other functions have this issue?
> 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.
Agreed. Added a check for valid_eku != NULL.
>
> pkinit_octetstring2key():
>
> key_block->magic = 0;
>
> - Why not KV5M_KEYBLOCK?
>
It should be I guess.
Fixed.
> 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.
>
Fixed.
> 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?
Because with the generated dh key here DH_check() always seems to
return DH_NOT_SUITABLE_GENERATOR for the client. I believe this isn't
actually a problem (see this thread:
http://www.mail-archive.com/openssl-users at openssl.org/msg46952.html)
> If they are meaningless then why call DH_check()?
I believe they are pretty much meaningless (but harmless) as the
values used to generate the DH key are ok.
> 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?
>
In this case I don't think its a problem. If client_process_dh()
fails it will eventually return to krb5_do_preauth() with "module_ret"
set to -1. krb5_do_preauth just checks this value against 0.
It may however result in confusing debug error messages.
Changed to "KRB5_PREAUTH_FAILED".
> 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.
>
Do you mean because they are returned to the client (not the KDC)?
Changed to KRB5_PREAUTH_FAILED.
> server_process_dh():
>
> if (!DH_generate_key(dh_server))
> goto cleanup;
>
> - Seems like the real error may be lost here.
>
Agreed. The OpenSSL Err_* functions could be used here to set the
error. Currently on the client side no error messages are printed
if things go wrong with preauth as there may be other preauth
plugins/methods that may succeed. There must be a better way of
doing things as it makes debugging pre-auth failures a PITA. Still,
I'm not sure I want to try and change this as part of this
integration.
> i2d_ASN1_INTEGER(pub_key, &p);
>
> - should have (void) cast. This occurs in other places as well.
Fixed (as part of the lint fixes).
>
> 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.
I'll investigate this further.
Thanks for the review!
I'll send an updated webrev out tomorrow or friday with the latest pkinit plugin
code changes (and the above changes), until then please don't review anything
in the "plugins" dir.
-M
