On Wed, Oct 01, 2008 at 05:47:18PM +0200, Mark Phalan wrote:
> 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."
Got it. BTW, it would be nice if you would provide a path/URI to your
hg repos that I could pull from so I could generate a cscope DB to
assist my code review.
> > 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.
It would be good to add comments at these spots like:
/* XXX Not thread safe! */
I also think that we should give back our mods to MIT.
> > 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.
We can give back our changes yes?
> > 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?
The instances are scattered throughout that file.
> > 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.
If the value is supposed to be 0 then there should be a comment as to
why.
> > 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.
Then why do the check in the standard path? Why not bracket this with
#ifdef DEBUG?
> > 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
--
Will Fiveash
Sun Microsystems Inc.
http://opensolaris.org/os/project/kerberos/
