In order to do CA pinning (resolve bug 744204, https://bugzilla.mozilla.org/show_bug.cgi?id=744204) the chain validation needs to have yet another validation phase (hopefully before OSCP and other network calls). In order to make this more extensible a callback mechanism was proposed by Brian Smith.

Comment on attachment 633239 [details] [diff] [review] First version of patch.. more for feedback than for review Review of attachment 633239 [details] [diff] [review]: ----------------------------------------------------------------- Hi Camilo, This is something that we've been discussing at Google as well, for use within Chromium. Like you, we're particularly interested in it within the context of pinning (which provides a whitelist of 'good' certs, but collectively operates over a chain), but also in the context of CRLsets (which provides a blacklist of 'bad' certs or issuers+SNs). In the process of working with Chris Palmer, I think we've identified some interesting trouble points with how pinning interacts with PKIX chain building. It might be good to sync up (on IRC, in person, or on an appropriate Mozilla list) to discuss some of these and how they fit within libpkix. I'm all for adding some extension points, I just want to make sure we'll be able to fully support pinning with them (while still being 'pinning agnostic' at the NSS layer)

Comment on attachment 634602 [details] [diff] [review] Version 2 of the patch... better prototype for callback. Review of attachment 634602 [details] [diff] [review]: ----------------------------------------------------------------- I'd agree that yes, this is the proper place to put it for phase two evaluation. pkix_CheckChain is called by pkix_Build_ValidateEntireChain, which is only called when PKIX_PL_Cert_IsCertTrusted has returned true. However, I'm a little concerned with this change as is. This is because it's going to cause some churn in the certificate chain cache, because the cache doesn't know to consider the callback (or any other state associated with the callback). As a result, a failure for cert pinning will eject the entire chain from the cache. See pkix_Build_CheckInCache and pkix_CacheCertChain_Lookup. Note that it /does/ consider the results from PKIX_CertSelector_GetMatchCallback, which contains the bulk of the matching logic for chain building. This may be deemed acceptable, but it seems worth pointing out. ::: security/nss/lib/certdb/certt.h @@ +958,5 @@ > * In NSS 3.12.1 or later. Default is off. > * Value is in value.scalar.b */ > + cert_pi_chainVerifyCallback = 13, > + /* A callback for doing extra validation on the > + * calculated chain. Used for Cert Pinning. I would remove the direct reference to "Cert Pinning", as this can be used for other purposes. @@ +961,5 @@ > + /* A callback for doing extra validation on the > + * calculated chain. Used for Cert Pinning. > + * The iteration over the chain is done where? > + */ > + cert_pi_callbackState = 14, /* where the state/params of the callback will Does it make more sense to combine pi_callbackState & pi_chainVerifyCallback into a structure that takes callback & args? ::: security/nss/lib/certhigh/certvfypkix.c @@ +1555,5 @@ > PKIX_PL_Date *revDate = NULL; > PKIX_RevocationChecker *revChecker = NULL; > > + /*cviecco ahem, seems bad I know*/ > + PKIX_PL_NssContext *NSSContext=(PKIX_PL_NssContext *) plContext; This isn't necessary if you extend libpkix, which seems to me to be the cleaner approach: PKIX_ProcessingParams_SetChainVerifier(...) ::: security/nss/lib/libpkix/pkix/top/pkix_build.c @@ +49,5 @@ > extern PRLogModuleInfo *pkixLog; > > +#ifndef PKIX_FORWARDBUILDERSTATEDEBUG > +#define PKIX_FORWARDBUILDERSTATEDEBUG 1 > +#endif Reminder: I'm guessing this was just for local debug purposes? @@ +149,5 @@ > > PKIX_RETURN(FORWARDBUILDERSTATE); > } > > +/** These sorts of formatting changes seem inappropriate to slip in (they're also not applied consistently - eg: line 55). I'm guessing this was an auto-correction by your editor of choice? It creates a lot of noise in this patch. ::: security/nss/lib/libpkix/pkix/top/pkix_validate.c @@ +819,5 @@ > *pNBIOContext = NULL; > revChecking = *pRevChecking; > > + /*cviecco ahem, seems bad I know*/ > + PKIX_PL_NssContext *NSSContext=(PKIX_PL_NssContext *) plContext; I have the feeling that Brian and I may disagree on this, but I think it is bad to directly cast the NSS types here. Rather than directly doing the conversion within this, I think the more appropriate way to do the callback is: - Define a new callbck type for PKIX_ProcessingParams, that takes PKIX_ types - This function gets the callback from |valparams| (by way of pkix_ExtractParameters) - This function calls the PKIX function with PKIX_ types (PKIX_TrustAnchor_GetTrustedCert, pCertCheckedIndex+certs) Within NSS, define an NSS callback that takes NSS types. When the NSS caller registers for the NSS callback, the NSS->libpkix function registers with libpkix a callback that expects PKIX types. The callback will convert pkix->nss types, then call the NSS caller's callback. This is how all of the existing libpkix interaction is handled. For single certificates, this already exists via the PKIX_CertChainChecker interface, which already exists to allow arbitrary callbacks to be inserted. Unfortunately named, given that it takes only a single certificate, but I think the approach like that would be wholly appropriate, and would 'hide' these details. @@ +822,5 @@ > + /*cviecco ahem, seems bad I know*/ > + PKIX_PL_NssContext *NSSContext=(PKIX_PL_NssContext *) plContext; > + > + /*Now I call my callback*/ > + if(NSSContext->certChainCallback !=NULL){ style nit: This file appears to follow the form of if (arg1 op arg2) { } Note the space between the conditional and open paren, the close paren and the open brace, and spaces between the op and arg1/arg2 (but not between open paren/close paren). Please follow this existing style. @@ +825,5 @@ > + /*Now I call my callback*/ > + if(NSSContext->certChainCallback !=NULL){ > + int (*pfi)(void *,void * ); /*we also send the trust anchor*/ > + CERTCertificate *anchorCert=NULL; > + int callback_return=1; The use of a symbolic type (SECStatus / PRError) seems more appropriate (although in the context of the NSS callback). The PKIX functions would naturally return a PKIX_Error. @@ +840,5 @@ > + certList=CERT_NewCertList(); > + if(NULL==certList){ > + PKIX_ERROR_CREATE(VALIDATE, > + PKIX_MALLOCFAILED, > + pkixErrorResult); You shouldn't be using PKIX_ERROR_CREATE directly. In this case, PKIX_ERROR_ALLOC_ERROR() is correct here (and the other PKIX_MALLOCFAILED instances) See pkix_tools.h for the macro definition - which includes setting the errorResult and calling goto cleanup. @@ +922,5 @@ > + /*version 1, create New PX_ERROR_CREATE*/ > + PKIX_ERROR_CREATE(VALIDATE, > + PKIX_CERTIFICATEREVOKED, > + /*PKIX_CERTISCERTTRUSTEDFAILED,*/ > + pkixErrorResult); You should instead be using PKIX_ERROR(PKIX_SOME_ERROR_CODE_HERE). This will set the error code and call goto cleanup, as appropriate. When PKIX_RETURN is called, it will see that there is an error code on the stack, and construct a PKIX_Error* object and return that.

(In reply to Ryan Sleevi from comment #3) > [A] failure for cert pinning will eject the entire > chain from the cache. [...] This may be > deemed acceptable, but it seems worth pointing out. I agree we should see if we can improve the interaction with the cache here. If it is difficult to do so, perhaps we can move that work to a follow-up bug. > Does it make more sense to combine pi_callbackState & pi_chainVerifyCallback > into a structure that takes callback & args? Sure, I think that seems reasonable. > > + /*cviecco ahem, seems bad I know*/ > > + PKIX_PL_NssContext *NSSContext=(PKIX_PL_NssContext *) plContext; > > This isn't necessary if you extend libpkix, which seems to me to be the > cleaner approach: > > PKIX_ProcessingParams_SetChainVerifier(...) > > +/** > > These sorts of formatting changes seem inappropriate to slip in (they're > also not applied consistently - eg: line 55). I'm guessing this was an > auto-correction by your editor of choice? It creates a lot of noise in this > patch. Camilo made these changes because he was using a JaaDoc-like tool to learn libpkix and these changes helped with that. We should not make these changes in this patch; if it is helpful to have them, then we should file a new bug for them. > > + /*cviecco ahem, seems bad I know*/ > > + PKIX_PL_NssContext *NSSContext=(PKIX_PL_NssContext *) plContext; > > I have the feeling that Brian and I may disagree on this, but I think it is > bad to directly cast the NSS types here. [...] This is how all of the > existing libpkix interaction is handled. IMO, we should simply change the type of plContext from void* to PKIX_PL_NssContext, everywhere in libpkix, in a different bug. I understand your desire for there to be consistency with how things are currently done. But, several months ago we agreed that we were going to try to simplify things from how they are done, to reduce the levels of indirection and wrappers and whatnot, in order to make libpkix easier to maintain. I think the way Camilo is doing things here is much more in the spirit of what we agreed to do there, and given the choice between modifying Camilo's patch to be like the existing code, vs. modifying the existing code to be more like Camilo's patch, I'd prefer to do the latter (in a different bug), unless there's some reason that doing so would create problems. (In the case of problems, should should reopen the discussion on how we will maintain libpkix going forward.)

-removed tracing and doc changes (Ryan Comment 3) -Instead of two parts a single struct to hold callback and callback state -Style cleanup

H (In reply to Ryan Sleevi from comment #3) > Comment on attachment 634602 [details] [diff] [review] > Version 2 of the patch... better prototype for callback. > > Review of attachment 634602 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'd agree that yes, this is the proper place to put it for phase two > evaluation. pkix_CheckChain is called by pkix_Build_ValidateEntireChain, > which is only called when PKIX_PL_Cert_IsCertTrusted has returned true. > > However, I'm a little concerned with this change as is. This is because it's > going to cause some churn in the certificate chain cache, because the cache > doesn't know to consider the callback (or any other state associated with > the callback). As a result, a failure for cert pinning will eject the entire > chain from the cache. I am a little confused here (probably because I dont know enough of the internals os nss). Why is it a problem to eject a chain that is invalid? I dont see an issue with having bad chains recalculated, as they would be rejected anyway. > > See pkix_Build_CheckInCache and pkix_CacheCertChain_Lookup. Note that it > /does/ consider the results from PKIX_CertSelector_GetMatchCallback, which > contains the bulk of the matching logic for chain building. This may be > deemed acceptable, but it seems worth pointing out. > > ::: security/nss/lib/certdb/certt.h > @@ +958,5 @@ > > * In NSS 3.12.1 or later. Default is off. > > * Value is in value.scalar.b */ > > + cert_pi_chainVerifyCallback = 13, > > + /* A callback for doing extra validation on the > > + * calculated chain. Used for Cert Pinning. > > I would remove the direct reference to "Cert Pinning", as this can be used > for other purposes. Agreed > @@ +961,5 @@ > > + /* A callback for doing extra validation on the > > + * calculated chain. Used for Cert Pinning. > > + * The iteration over the chain is done where? > > + */ > > + cert_pi_callbackState = 14, /* where the state/params of the callback will > > Does it make more sense to combine pi_callbackState & pi_chainVerifyCallback > into a structure that takes callback & args? I also agree. The latest patch combines these. > ::: security/nss/lib/certhigh/certvfypkix.c > @@ +1555,5 @@ > > PKIX_PL_Date *revDate = NULL; > > PKIX_RevocationChecker *revChecker = NULL; > > > > + /*cviecco ahem, seems bad I know*/ > > + PKIX_PL_NssContext *NSSContext=(PKIX_PL_NssContext *) plContext; > > This isn't necessary if you extend libpkix, which seems to me to be the > cleaner approach: > > PKIX_ProcessingParams_SetChainVerifier(...) > > ::: security/nss/lib/libpkix/pkix/top/pkix_build.c > @@ +49,5 @@ > > extern PRLogModuleInfo *pkixLog; > > > > +#ifndef PKIX_FORWARDBUILDERSTATEDEBUG > > +#define PKIX_FORWARDBUILDERSTATEDEBUG 1 > > +#endif > > Reminder: I'm guessing this was just for local debug purposes? > Yes. Removed too. > @@ +149,5 @@ > > > > PKIX_RETURN(FORWARDBUILDERSTATE); > > } > > > > +/** > > These sorts of formatting changes seem inappropriate to slip in (they're > also not applied consistently - eg: line 55). I'm guessing this was an > auto-correction by your editor of choice? It creates a lot of noise in this > patch. Agreed and cleaned up. > > ::: security/nss/lib/libpkix/pkix/top/pkix_validate.c > @@ +819,5 @@ > > *pNBIOContext = NULL; > > revChecking = *pRevChecking; > > > > + /*cviecco ahem, seems bad I know*/ > > + PKIX_PL_NssContext *NSSContext=(PKIX_PL_NssContext *) plContext; > > I have the feeling that Brian and I may disagree on this, but I think it is > bad to directly cast the NSS types here. > > Rather than directly doing the conversion within this, I think the more > appropriate way to do the callback is: > > - Define a new callbck type for PKIX_ProcessingParams, that takes PKIX_ types > - This function gets the callback from |valparams| (by way of > pkix_ExtractParameters) > - This function calls the PKIX function with PKIX_ types > (PKIX_TrustAnchor_GetTrustedCert, pCertCheckedIndex+certs) > > Within NSS, define an NSS callback that takes NSS types. When the NSS caller > registers for the NSS callback, the NSS->libpkix function registers with > libpkix a callback that expects PKIX types. The callback will convert > pkix->nss types, then call the NSS caller's callback. > > This is how all of the existing libpkix interaction is handled. > > For single certificates, this already exists via the PKIX_CertChainChecker > interface, which already exists to allow arbitrary callbacks to be inserted. > Unfortunately named, given that it takes only a single certificate, but I > think the approach like that would be wholly appropriate, and would 'hide' > these details. > > @@ +822,5 @@ > > + /*cviecco ahem, seems bad I know*/ > > + PKIX_PL_NssContext *NSSContext=(PKIX_PL_NssContext *) plContext; > > + > > + /*Now I call my callback*/ > > + if(NSSContext->certChainCallback !=NULL){ > > style nit: This file appears to follow the form of > > if (arg1 op arg2) { > } > > Note the space between the conditional and open paren, the close paren and > the open brace, and spaces between the op and arg1/arg2 (but not between > open paren/close paren). Please follow this existing style. > > @@ +825,5 @@ > > + /*Now I call my callback*/ > > + if(NSSContext->certChainCallback !=NULL){ > > + int (*pfi)(void *,void * ); /*we also send the trust anchor*/ > > + CERTCertificate *anchorCert=NULL; > > + int callback_return=1; > > The use of a symbolic type (SECStatus / PRError) seems more appropriate > (although in the context of the NSS callback). The PKIX functions would > naturally return a PKIX_Error. Agreed and Fixed. > @@ +840,5 @@ > > + certList=CERT_NewCertList(); > > + if(NULL==certList){ > > + PKIX_ERROR_CREATE(VALIDATE, > > + PKIX_MALLOCFAILED, > > + pkixErrorResult); > > You shouldn't be using PKIX_ERROR_CREATE directly. > > In this case, PKIX_ERROR_ALLOC_ERROR() is correct here (and the other > PKIX_MALLOCFAILED instances) > > See pkix_tools.h for the macro definition - which includes setting the > errorResult and calling goto cleanup. > Agreed and fixed. > @@ +922,5 @@ > > + /*version 1, create New PX_ERROR_CREATE*/ > > + PKIX_ERROR_CREATE(VALIDATE, > > + PKIX_CERTIFICATEREVOKED, > > + /*PKIX_CERTISCERTTRUSTEDFAILED,*/ > > + pkixErrorResult); > > You should instead be using PKIX_ERROR(PKIX_SOME_ERROR_CODE_HERE). This will > set the error code and call goto cleanup, as appropriate. When PKIX_RETURN > is called, it will see that there is an error code on the stack, and > construct a PKIX_Error* object and return that. Agreed and fixed.

(In reply to Camilo Viecco (:cviecco) from comment #7) > H > (In reply to Ryan Sleevi from comment #3) > > Comment on attachment 634602 [details] [diff] [review] > > Version 2 of the patch... better prototype for callback. > > > > Review of attachment 634602 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > I'd agree that yes, this is the proper place to put it for phase two > > evaluation. pkix_CheckChain is called by pkix_Build_ValidateEntireChain, > > which is only called when PKIX_PL_Cert_IsCertTrusted has returned true. > > > > However, I'm a little concerned with this change as is. This is because it's > > going to cause some churn in the certificate chain cache, because the cache > > doesn't know to consider the callback (or any other state associated with > > the callback). As a result, a failure for cert pinning will eject the entire > > chain from the cache. > I am a little confused here (probably because I dont know enough of the > internals > os nss). Why is it a problem to eject a chain that is invalid? I dont see > an issue > with having bad chains recalculated, as they would be rejected anyway. Different hosts ("foo.example", "bar.example") may share the same chain, based on subjectAltName, but have different pinning requirements. This would primarily be a concern if/when any large CDNs started to look at pinning, since CDNs are often the most egregious in using the same chain for different (independent) logical sites. Beyond that, however, is the possibility of a chain for "foo.example" going A -> B -> C while the chain for "bar.example" going D -> B -> C. As currently implemented, libpkix will attempt to build the chain from A, and the cache will have "B,C" - that is, a cache of how to reach a trust anchor when you start from "B". This cache would generally be usable for D as well (since D is issued by B). While there are edge cases where the cache might be ignored (name constraints, policy verification such as EV, etc), in general, the cache can avoid a good bit of chain walking. However, because the callback returns a chain related status, the caching logic is such that a pinning failure for "foo.example" will cause the cache to be evicted, which would affect the validation of "bar.example" and force the chain to be recomputed. If "B" or "C" had to be discovered by AIA fetching, the invalidation may cause all references to be dropped, forcing an AIA refetch (which, hopefully, will be cached/cachable). Solutions: 1) This may be deemed an acceptable performance trade-off when implementing pinning, and no change. It should be weighed, however, in light of Firefox's performance goals (who I imagine will be the first adopter). 2) It may be that cache entries also take into consideration pins or other client criteria (the context pointer) when computing the hash of the verification results. That would imply that the context needs to be some form of PKIX_PL_Object (since they implement a hash function) OR some form of callback function to determine a hash code provided (which, given that everything uses PKIX_PL_ already, seems even more complex than just using PL objects) 3) Two chain caches - one for when the callback is present, one for when it's absent?

Comment on attachment 638394 [details] [diff] [review] Fixed compilation issue on win of previous patch. Success on try Review of attachment 638394 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/nss/lib/certdb/certt.h @@ +889,5 @@ > > +/* > + * There types are for the validate chain callback param > + */ > +typedef PRBool (*CERTChainVerifyCallBackFunction)(void *,const CERTCertList *); s/CallBack/Callback/, because Callback is one word. Also, add a space before "const". @@ +894,5 @@ > + > +typedef struct { > + CERTChainVerifyCallBackFunction isChainValid; > + void *isChainValidState; > +} CERTVerifyCallBack; This should be called CERTChainVerifyCallback (note case) for consistency with the other things, and because we may have other kinds of VerifyCallbacks in the future @@ +969,5 @@ > * Value is in value.scalar.b */ > + cert_pi_chainVerifyCallback = 13, > + /* The callback container for doing extra > + * validation on the currently > + * calculated chain. Used for Cert Pinning. Remove "Used for Cert Pinning" because the callback may be used for many things in the future. ::: security/nss/lib/certhigh/certvfypkix.c @@ +1554,5 @@ > PKIX_TrustAnchor *trustAnchor = NULL; > PKIX_PL_Date *revDate = NULL; > PKIX_RevocationChecker *revChecker = NULL; > > + /*This casting currently is always valid*/ This comment isn't necessary. @@ +1555,5 @@ > PKIX_PL_Date *revDate = NULL; > PKIX_RevocationChecker *revChecker = NULL; > > + /*This casting currently is always valid*/ > + PKIX_PL_NssContext *NSSContext=(PKIX_PL_NssContext *) plContext; Please use whitespace and camelCase consistently with the rest of the file: PKIX_PL_NssContext *nssContext = (PKIX_PL_NssContext *) plContext; There are several cases where the whitespace convention you are using (especially the lack of whitespace around operators) is different than what is typical in libpkix. Please adjust the added lines of the patch to match the libpkix conventions. Similarly, please use " != NULL" instead of "NULL !=" (etc.) because that is the libpkix convention. Note that different parts of NSS have different conventions (including even the use of tabs vs spaces, and indention levels). To pay homage to the earlier authors of the code, and to prevent future readers of the code from going insane, please use the same conventions that have been used in the past, in each file. The exception is that in whole new blocks of code, you can avoid aligning variable declarations like this: /* BAD */ Foo foo = whatever; FooBar foobar = somethingElse; and instead you can do this: /* GOOD */ Foo foo = whatever; FooBar foobar = somethingElse; If you add variable declarations to existing blocks, then just follow the convention of the block, or do whatever you think the reviewer won't object to. @@ +1730,5 @@ > (PRBool)(param->value.scalar.b != 0), > plContext); > break; > + case cert_pi_chainVerifyCallback: > + NSSContext->certVerifyCallback=param->value.pointer.p; Check that certVerifyCallback->isChainValid is non-NULL and fail here if it is NULL, so that you don't have to do the check below. ::: security/nss/lib/libpkix/pkix/top/pkix_validate.c @@ +43,5 @@ > > #include "pkix_validate.h" > +#include "pkix_pl_cert.h" > +#include "pkix_pl_nsscontext.h" > +#include "pkixt.h" Always have a blank line after #include blocks. @@ +736,5 @@ > PKIX_PL_Cert *cert = NULL; > PKIX_PL_Cert *issuer = NULL; > > + /*Everytime this function is called this is the type */ > + PKIX_PL_NssContext *NSSContext=(PKIX_PL_NssContext *) plContext; Same comments as in the previous function apply here. @@ +748,5 @@ > *pNBIOContext = NULL; > revChecking = *pRevChecking; > > + > + /*Do a callback if the callback exists*/ This comment isn't needed, and neither is the new blank line. @@ +750,5 @@ > > + > + /*Do a callback if the callback exists*/ > + if(NULL != NSSContext->certVerifyCallback && > + NULL != NSSContext->certVerifyCallback-> isChainValid ){ No need for this second check if you do the check in cert_pkixSetParam that I suggested. @@ +752,5 @@ > + /*Do a callback if the callback exists*/ > + if(NULL != NSSContext->certVerifyCallback && > + NULL != NSSContext->certVerifyCallback-> isChainValid ){ > + CERTChainVerifyCallBackFunction pfi; > + PRBool callbackReturn=PR_FALSE; /*assume Failure*/ We usually use "Rv" as a suffix that means "return value" so a good name here would be callbackRv. @@ +758,5 @@ > + CERTCertList *certList = NULL; > + SECStatus rv; > + > + /*The callback expects a certlist that includes the anchor > + *the list starts with the anchor AFAICT, most of the time when we pass and return CERTCertLists containing cert chains, the EE cert is at the beginning of the list and the root certificate is at the end of the list. We should use that same convention here, and we should document the order of the certs in the list near the declarations of the callback types in the public header file. @@ +761,5 @@ > + /*The callback expects a certlist that includes the anchor > + *the list starts with the anchor > + */ > + certList = CERT_NewCertList(); > + if(NULL == certList){ This should be: if (NULL = certList) { PKIX_ERROR_ALLOC_ERROR(); } Note memory allocation failures are handled specially in a way that doesn't require the error handling code to allocate memory (which would fail, obviously). Question for others: why doesn't PKIX_ERROR_ALLOC_ERROR set pkixErrorClass = PKIX_FATAL_ERROR;? @@ +769,5 @@ > + goto cleanup; > + > + } > + > + /*Get the anchors CCERT certificate and add it to the list*/ What does CCERT stand for? @@ +779,5 @@ > + PKIX_PL_Cert_GetCERTCertificate(cert,&tempCert,plContext), > + PKIX_CERTGETCERTCERTIFICATEFAILED); > + > + rv=CERT_AddCertToListTail(certList, tempCert); > + if (rv != SECSuccess) { PKIX_ERROR_ALLOC_ERROR(); @@ +787,5 @@ > + goto cleanup; > + > + } > + > + /*loop the loop to build the rest of chain*/ "loop the loop" is not very clear. I think "Add the end-etity and intermediate certificates" would be clearer. @@ +818,5 @@ > + > + } > + > + /*local asignment for readability*/ > + pfi = NSSContext->certVerifyCallback->isChainValid; why "pfi" instead of "isChainValid"? @@ +822,5 @@ > + pfi = NSSContext->certVerifyCallback->isChainValid; > + > + callbackReturn = (*pfi)(NSSContext->certVerifyCallback->isChainValidState, certList); > + > + /*and cleanup */ All cleanup must (also) be done in the cleanup: section of the function in case you returned early due to some error. @@ +826,5 @@ > + /*and cleanup */ > + if (certList) { > + CERT_DestroyCertList(certList); > + } > + /*OK this might */ what does "this might" mean? @@ +831,5 @@ > + PKIX_DECREF(cert); > + > + /*if it is NOT valid then fail...!*/ > + if(PR_FALSE == callbackReturn ){ > + PKIX_ERROR(PKIX_CERTISCERTTRUSTEDFAILED); Hmm...I wonder if we should use this error code, or a different one? @@ +832,5 @@ > + > + /*if it is NOT valid then fail...!*/ > + if(PR_FALSE == callbackReturn ){ > + PKIX_ERROR(PKIX_CERTISCERTTRUSTEDFAILED); > + goto cleanup; PKIX_ERROR already does "goto cleanup"

Addressing review- at comment#9

Forgot to edit part of a comment. Othervise same at attachment 655780 [details] [diff] [review]

Comment on attachment 655784 [details] [diff] [review] proposed-patch-v3 Review of attachment 655784 [details] [diff] [review]: ----------------------------------------------------------------- Brian asked me to take a look during the morning call. ::: security/nss/lib/certdb/certt.h @@ +895,5 @@ > + * in the last position of the chain. > + * The void parameter is the state of the callback which > + * is an opaque blob from NSS's perspective. > + */ > +typedef PRBool (*CERTChainVerifyCallback)(void *, const CERTCertList *); Should this return a SECStatus instead of a PRBool? Brian likely has more input on this - I do not see it as essential, since the application can track any application-specific errors by using the callback state. @@ +899,5 @@ > +typedef PRBool (*CERTChainVerifyCallback)(void *, const CERTCertList *); > + > +typedef struct { > + CERTChainVerifyCallback isChainValid; > + void *isChainValidState; nit: naming I note within other NSS functions (eg: at the SSL layer), this is simply called "void *arg". I don't feel strongly about this, and while the comment is somewhat clear, I think it may suggest a particular implementation detail that's not required. nit: alignment Even with 8-space tabs, this fails to align properly. @@ +975,5 @@ > * Value is in value.scalar.b */ > + cert_pi_chainVerifyCallback = 13, > + /* The callback container for doing extra > + * validation on the currently > + * calculated chain. nit: trailing whitespace ocd (throughout the files and functions touched by this patch, please ensure that no *new* code has incorrect trailing whitespace) ::: security/nss/lib/certhigh/certvfypkix.c @@ +1555,5 @@ > PKIX_TrustAnchor *trustAnchor = NULL; > PKIX_PL_Date *revDate = NULL; > PKIX_RevocationChecker *revChecker = NULL; > + const CERTVerifyCallback *verifyCallback = NULL; > + PKIX_PL_NssContext *NSSContext = (PKIX_PL_NssContext *) plContext; PKIX_PL_NssContext *NssContext = (PKIX_PL_NssContext *)plContext; @@ +1732,5 @@ > break; > + > + case cert_pi_chainVerifyCallback: > + verifyCallback = param->value.pointer.p; > + if(!verifyCallback){ if (verifyCallback == NULL) { @@ +1733,5 @@ > + > + case cert_pi_chainVerifyCallback: > + verifyCallback = param->value.pointer.p; > + if(!verifyCallback){ > + PORT_SetError(errCode); Why do you use errCode here, but use SEC_ERROR_INVALID_ARGS on 1743? @@ +1737,5 @@ > + PORT_SetError(errCode); > + r = SECFailure; > + break; > + } > + if(!verifyCallback->isChainValid){ if (verifyCallback->isChainValid == NULL) { @@ +1740,5 @@ > + } > + if(!verifyCallback->isChainValid){ > + r = SECFailure; > + PORT_SetError(SEC_ERROR_INVALID_ARGS); > + break; nit: order your PORT_SetError & r assignment consistently between these two conditions. @@ +1742,5 @@ > + r = SECFailure; > + PORT_SetError(SEC_ERROR_INVALID_ARGS); > + break; > + } > + NSSContext->certVerifyCallback = verifyCallback; nit: NssContext->certVerifyCallback = verifyCallback; warning: const conversion here. certVerifyCallback = CERTVerifyCallback * verifyCallback = const CERTVerifyCallback * Please ensure const correctness is properly preserved. ::: security/nss/lib/libpkix/pkix/top/pkix_validate.c @@ +736,5 @@ > void *nbioContext = NULL; > PKIX_PL_Cert *cert = NULL; > PKIX_PL_Cert *issuer = NULL; > + PKIX_PL_NssContext *NSSContext = NULL; > + CERTCertList *certList = NULL; PKIX_PL_NssContext *NssContext = NULL; CERTVerifyCallback *verifyCallback = NULL; CERTCertList *certList = NULL; @@ +748,5 @@ > *pNBIOContext = NULL; > revChecking = *pRevChecking; > + NSSContext = (PKIX_PL_NssContext *)plContext; > + > + if( NSSContext->certVerifyCallback != NULL ){ NssContext = (PKIX_PL_NssContext *)plContext; verifyCallback = NssContext ? NssContext ->certVerifyCallback : NULL; if (verifyCallback != NULL && verifyCallback->isChainValid != NULL) { @@ +750,5 @@ > + NSSContext = (PKIX_PL_NssContext *)plContext; > + > + if( NSSContext->certVerifyCallback != NULL ){ > + CERTChainVerifyCallback isChainValid; > + PRBool callbackrv=PR_FALSE; /*assume Failure*/ nit: PRBool callbackRv = PR_FALSE; /* assume failure */ (spacing and capitalizing) @@ +751,5 @@ > + > + if( NSSContext->certVerifyCallback != NULL ){ > + CERTChainVerifyCallback isChainValid; > + PRBool callbackrv=PR_FALSE; /*assume Failure*/ > + CERTCertificate *tempCert = NULL; It seems like the "correct" include you should be using is "pkix_pl_common.h" (which is what pulls in the NSS headers). This is transitively included by the _pl_ headers you added (lines 45 and 46), but it's generally ideal to include the type you use directly. @@ +758,5 @@ > + /*The callback expects a certlist that includes the anchor > + *the list ends with the anchor > + */ > + certList = CERT_NewCertList(); > + if(certList == NULL){ if (certList == NULL) { @@ +768,5 @@ > + (anchor, &cert, plContext), > + PKIX_TRUSTANCHORGETTRUSTEDCERTFAILED); > + > + PKIX_CHECK( > + PKIX_PL_Cert_GetCERTCertificate(cert,&tempCert,plContext), (cert, &tempCert, plContext) @@ +771,5 @@ > + PKIX_CHECK( > + PKIX_PL_Cert_GetCERTCertificate(cert,&tempCert,plContext), > + PKIX_CERTGETCERTCERTIFICATEFAILED); > + > + rv = CERT_AddCertToListTail(certList, tempCert); You should add a comment here about why it's not necessary to free tempCert (that is, because CERT_AddCertToListTail causes certList to take ownership) This originally threw me, since PKIX_PL_Cert_GetCERTCertificate causes a "copy" (CERT_DupCertificate) to be returned, and so it looked like it was being leaked at first. @@ +778,5 @@ > + } > + > + /*build the rest of chain*/ > + for (j = *pCertCheckedIndex; j < numCerts ; j++) { > + nit: remove blank line @@ +779,5 @@ > + > + /*build the rest of chain*/ > + for (j = *pCertCheckedIndex; j < numCerts ; j++) { > + > + PORT_Assert(cert); Is this necessary? Wouldn't it be better just to induce a fatal error if cert is NULL? @@ +792,5 @@ > + /* check if cert pointer is valid */ > + PORT_Assert(cert); > + if (cert == NULL) { > + continue; > + } Is there any situation in which this can happen? @@ +806,5 @@ > + } > + > + /*local asignment for readability*/ > + isChainValid = NSSContext->certVerifyCallback->isChainValid; > + callbackrv = (*isChainValid)(NSSContext->certVerifyCallback->isChainValidState, certList); callbackRv = (*verifyCallback->isChainValid)(verifyCallback->arg, certList); @@ +808,5 @@ > + /*local asignment for readability*/ > + isChainValid = NSSContext->certVerifyCallback->isChainValid; > + callbackrv = (*isChainValid)(NSSContext->certVerifyCallback->isChainValidState, certList); > + > + /*and cleanup */ nit: unnecessary @@ +812,5 @@ > + /*and cleanup */ > + if (certList) { > + CERT_DestroyCertList(certList); > + certList = NULL; > + } Is there a reason this isn't part of the general cleanup, as per the normal libpkix pattern? @@ +815,5 @@ > + certList = NULL; > + } > + PKIX_DECREF(cert); > + > + /*if it is NOT valid then fail...!*/ nit: unnecessary @@ +816,5 @@ > + } > + PKIX_DECREF(cert); > + > + /*if it is NOT valid then fail...!*/ > + if(callbackrv == PR_FALSE ){ if (callbackRv == PR_FALSE) {

Ryan, thanks for doing the detailed review. (In reply to Ryan Sleevi from comment #12) > > + * in the last position of the chain. > > + * The void parameter is the state of the callback which > > + * is an opaque blob from NSS's perspective. > > + */ > > +typedef PRBool (*CERTChainVerifyCallback)(void *, const CERTCertList *); > > Should this return a SECStatus instead of a PRBool? > > Brian likely has more input on this - I do not see it as essential, since > the application can track any application-specific errors by using the > callback state. I'd like to suggest the following prototype: typedef SECStatus (*CERTChainVerifyCallback)(void *, const CERTCertList *, PRBool /*out*/ * chainOK); With these semantics: rv == SECFailure: The callback encountered a fatal error (out of memory), the verification should immediately fail with the error code from PORT_GetError() without trying more certificates. rv == SECSuccess && !chainOK: certificate chain is bad; try another chain. rv == SECSuccess && chainOK: certificate chain is good. This solves several problems: * We have a way to deal with fatal errors, which the previous prototype didn't handle. * We don't need to worry about what to return if the callback detects multiple errors with the chain * We don't need to worry about the interaction between the result of this function and the verifyLog. The application will be responsible for merging results calculated by this function with the verifyLog. > > +typedef PRBool (*CERTChainVerifyCallback)(void *, const CERTCertList *); > > + > > +typedef struct { > > + CERTChainVerifyCallback isChainValid; > > + void *isChainValidState; > > nit: naming > > I note within other NSS functions (eg: at the SSL layer), this is simply > called "void *arg". I think either "arg" or "isChainValidArg" is OK. > You should add a comment here about why it's not necessary to free tempCert > (that is, because CERT_AddCertToListTail causes certList to take ownership) TIL.

Comment on attachment 655784 [details] [diff] [review] proposed-patch-v3 Review of attachment 655784 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/nss/lib/certdb/certt.h @@ +895,5 @@ > + * in the last position of the chain. > + * The void parameter is the state of the callback which > + * is an opaque blob from NSS's perspective. > + */ > +typedef PRBool (*CERTChainVerifyCallback)(void *, const CERTCertList *); See my comment preceeding this review about the prototype. Name the parameters in the typedef, and then refer to them by name in the comment instead of by type. This will make the documentation clearer. Also, wrap the documentation to 80 chars. @@ +899,5 @@ > +typedef PRBool (*CERTChainVerifyCallback)(void *, const CERTCertList *); > + > +typedef struct { > + CERTChainVerifyCallback isChainValid; > + void *isChainValidState; One space between "void" and "*" @@ +975,5 @@ > * Value is in value.scalar.b */ > + cert_pi_chainVerifyCallback = 13, > + /* The callback container for doing extra > + * validation on the currently > + * calculated chain. In the comment, specify "Value is in value.pointer.verifyCallback". ::: security/nss/lib/certhigh/certvfypkix.c @@ +1731,5 @@ > plContext); > break; > + > + case cert_pi_chainVerifyCallback: > + verifyCallback = param->value.pointer.p; Instead of using "p", extend the pointer union with: CERTVerifyCallback * verifyCallback; to improve type-safety. @@ +1740,5 @@ > + } > + if(!verifyCallback->isChainValid){ > + r = SECFailure; > + PORT_SetError(SEC_ERROR_INVALID_ARGS); > + break; PORT_SEtError should come first, then assignment to rv; @@ +1742,5 @@ > + r = SECFailure; > + PORT_SetError(SEC_ERROR_INVALID_ARGS); > + break; > + } > + NSSContext->certVerifyCallback = verifyCallback; Generally there should be a blank line after "}" in blocks, and in particular there should be one before this line. ::: security/nss/lib/libpkix/pkix/top/pkix_validate.c @@ +755,5 @@ > + CERTCertificate *tempCert = NULL; > + SECStatus rv; > + > + /*The callback expects a certlist that includes the anchor > + *the list ends with the anchor Nit: Use a space after "/*" and "*", in all comments, and space before "*/" in one-line comments, except (optionally) for one-line comments that would need to be wrapped to two-line comments because of the space. @@ +762,5 @@ > + if(certList == NULL){ > + PKIX_ERROR_ALLOC_ERROR(); > + } > + > + /*Get the anchors CERTcertificate and add it to the list*/ whitespace @@ +771,5 @@ > + PKIX_CHECK( > + PKIX_PL_Cert_GetCERTCertificate(cert,&tempCert,plContext), > + PKIX_CERTGETCERTCERTIFICATEFAILED); > + > + rv = CERT_AddCertToListTail(certList, tempCert); e.g. "/* CERT_AddCertToListTail takes ownership */" But, CERT_AddCertToListTail only takes ownership when it succeeds, so you still need to free tempCert when it fails. One way to do that is to set "tempCert = NULL" on line 780, and then always free tempCert in the cleanup: block. This way, you minimize the duplication of the cleanup logic between this block and the cleanup block. @@ +792,5 @@ > + /* check if cert pointer is valid */ > + PORT_Assert(cert); > + if (cert == NULL) { > + continue; > + } In other words, you can assume that PKIX_List_GetItem returned a non-NULL cert if it succeeded, so this check is unnecessary. @@ +812,5 @@ > + /*and cleanup */ > + if (certList) { > + CERT_DestroyCertList(certList); > + certList = NULL; > + } The reason it is important to use the normal cleanup pattern is because there are many hidden "goto cleanups" in the PKIX_CHECK macros, so you would end up leaking various things if you don't centralize (almost) *all* the cleanup logic in the cleanup: section. @@ +818,5 @@ > + > + /*if it is NOT valid then fail...!*/ > + if(callbackrv == PR_FALSE ){ > + PKIX_ERROR(PKIX_CERTISCERTTRUSTEDFAILED); > + goto cleanup; The compiler should be warning you about unreachable code with the "goto cleanup" line, because PKIX_ERROR contains "goto cleanup" on its last line. ::: security/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_nsscontext.h @@ +58,5 @@ > PKIX_UInt32 timeoutSeconds; > PKIX_UInt32 maxResponseLength; > PRTime crlReloadDelay; > PRTime badDerCrlReloadDelay; > + CERTVerifyCallback *certVerifyCallback; const CERTVerifyCallback *

Comment on attachment 655784 [details] [diff] [review] proposed-patch-v3 Review of attachment 655784 [details] [diff] [review]: ----------------------------------------------------------------- Thank you both for your review ::: security/nss/lib/certdb/certt.h @@ +895,5 @@ > + * in the last position of the chain. > + * The void parameter is the state of the callback which > + * is an opaque blob from NSS's perspective. > + */ > +typedef PRBool (*CERTChainVerifyCallback)(void *, const CERTCertList *); OK So I will be using Brians proposal: typedef SECStatus (*CERTChainVerifyCallback)(void *isChainValidArg, const CERTCertList *currentChain, PRBool /*out*/ *chainOK ); where rv!= SECSuccess -> cannot compute(such as bad params)/fatal error and if rv== SECSUCESS -> chainOK == true -> chain is valid chainOK != true -> chain is not valid from the application. @@ +899,5 @@ > +typedef PRBool (*CERTChainVerifyCallback)(void *, const CERTCertList *); > + > +typedef struct { > + CERTChainVerifyCallback isChainValid; > + void *isChainValidState; Space removed. I will rename the void pointer in the struct to isChainValidArg unless there is some issue with that. @@ +975,5 @@ > * Value is in value.scalar.b */ > + cert_pi_chainVerifyCallback = 13, > + /* The callback container for doing extra > + * validation on the currently > + * calculated chain. Ryan: Done, Is there a way to check for spaces locally in the diff? Brian OK, done ::: security/nss/lib/certhigh/certvfypkix.c @@ +1555,5 @@ > PKIX_TrustAnchor *trustAnchor = NULL; > PKIX_PL_Date *revDate = NULL; > PKIX_RevocationChecker *revChecker = NULL; > + const CERTVerifyCallback *verifyCallback = NULL; > + PKIX_PL_NssContext *NSSContext = (PKIX_PL_NssContext *) plContext; Done @@ +1732,5 @@ > break; > + > + case cert_pi_chainVerifyCallback: > + verifyCallback = param->value.pointer.p; > + if(!verifyCallback){ The style around this section uses !foo for X == NULL. I can change it if we agree this is better. (not changed yet) @@ +1733,5 @@ > + > + case cert_pi_chainVerifyCallback: > + verifyCallback = param->value.pointer.p; > + if(!verifyCallback){ > + PORT_SetError(errCode); Because it seems more appropiate from the context. errCode seems to be sent whenever there is a bad parameter passing, and SEC_ERROR_INVALID_ARGS *sometimes* used when the parameters cannot be processed. I think this separation makes sense, but I am willing to change my ways. Code above this section uses both approaches so I really dont care here. @@ +1737,5 @@ > + PORT_SetError(errCode); > + r = SECFailure; > + break; > + } > + if(!verifyCallback->isChainValid){ Same as above, from the context is !foo, But I am willing to change (not done yet) @@ +1740,5 @@ > + } > + if(!verifyCallback->isChainValid){ > + r = SECFailure; > + PORT_SetError(SEC_ERROR_INVALID_ARGS); > + break; ACK. Sorted as your comment. The order that I use was (literally copied) from lines 1578. Should this be filled as a separate bug? @@ +1742,5 @@ > + r = SECFailure; > + PORT_SetError(SEC_ERROR_INVALID_ARGS); > + break; > + } > + NSSContext->certVerifyCallback = verifyCallback; Ryan: good catch. Solved (at least I dont see a warning on gcc-linux now) Brian: OK. Done ::: security/nss/lib/libpkix/pkix/top/pkix_validate.c @@ +748,5 @@ > *pNBIOContext = NULL; > revChecking = *pRevChecking; > + NSSContext = (PKIX_PL_NssContext *)plContext; > + > + if( NSSContext->certVerifyCallback != NULL ){ Like it. (even with my dislike of one liner "if/else" @@ +751,5 @@ > + > + if( NSSContext->certVerifyCallback != NULL ){ > + CERTChainVerifyCallback isChainValid; > + PRBool callbackrv=PR_FALSE; /*assume Failure*/ > + CERTCertificate *tempCert = NULL; Taking a look into it. @@ +778,5 @@ > + } > + > + /*build the rest of chain*/ > + for (j = *pCertCheckedIndex; j < numCerts ; j++) { > + Done @@ +792,5 @@ > + /* check if cert pointer is valid */ > + PORT_Assert(cert); > + if (cert == NULL) { > + continue; > + } OK thanks. Will remove ( Should I remove the assert too?) If we change it we will have to do something similar for line 843 (loop copied from line 830) @@ +808,5 @@ > + /*local asignment for readability*/ > + isChainValid = NSSContext->certVerifyCallback->isChainValid; > + callbackrv = (*isChainValid)(NSSContext->certVerifyCallback->isChainValidState, certList); > + > + /*and cleanup */ Removed @@ +812,5 @@ > + /*and cleanup */ > + if (certList) { > + CERT_DestroyCertList(certList); > + certList = NULL; > + } ACK. @@ +818,5 @@ > + > + /*if it is NOT valid then fail...!*/ > + if(callbackrv == PR_FALSE ){ > + PKIX_ERROR(PKIX_CERTISCERTTRUSTEDFAILED); > + goto cleanup; goto removed ::: security/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_nsscontext.h @@ +58,5 @@ > PKIX_UInt32 timeoutSeconds; > PKIX_UInt32 maxResponseLength; > PRTime crlReloadDelay; > PRTime badDerCrlReloadDelay; > + CERTVerifyCallback *certVerifyCallback; Done

Comment on attachment 655784 [details] [diff] [review] proposed-patch-v3 Review of attachment 655784 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/nss/lib/certdb/certt.h @@ +975,5 @@ > * Value is in value.scalar.b */ > + cert_pi_chainVerifyCallback = 13, > + /* The callback container for doing extra > + * validation on the currently > + * calculated chain. I'm not sure the right way locally. I tend to cheat and setup a git repo overlayed over CVS, at which point git-diff colorizes. You can also view your patch in splinter, which also seems to support highlighting extraneous whitespace. ::: security/nss/lib/certhigh/certvfypkix.c @@ +1555,5 @@ > PKIX_TrustAnchor *trustAnchor = NULL; > PKIX_PL_Date *revDate = NULL; > PKIX_RevocationChecker *revChecker = NULL; > + const CERTVerifyCallback *verifyCallback = NULL; > + PKIX_PL_NssContext *NSSContext = (PKIX_PL_NssContext *) plContext; Note that I gave conflicting advice to Brian's earlier review, and Brian's definitely correct. For internal consistency with the rest of the capitalization, this should be PKIX_PL_NssContext *nssContext (eg: drop the cap on the n), throughout all usages. @@ +1732,5 @@ > break; > + > + case cert_pi_chainVerifyCallback: > + verifyCallback = param->value.pointer.p; > + if(!verifyCallback){ Ugh for this file lacking even internal consistency. You're correct, it seems to mix both == NULL / != NULL ( eg: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certhigh/certvfypkix.c&rev=1.54&mark=1536,1542,1555,1562,1569#1528 ) and the implicit bool conversion ( http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certhigh/certvfypkix.c&rev=1.54&mark=1656,1662,1669,1674,1679#1654 ) I find == NULL to be more descriptive, but I suppose choose the style you think most appropriate. Regardless, please ensure the proper & consistent spacing around the conditional if (condition) { rather than if(condition){ @@ +1733,5 @@ > + > + case cert_pi_chainVerifyCallback: > + verifyCallback = param->value.pointer.p; > + if(!verifyCallback){ > + PORT_SetError(errCode); weird, but you're right, I see the inconsistency. I'm tempted to suggest doing if (!verifyCallback || !verifyCallback->isChainValid) { r = SECFailure; PORT_SetError(errCode); break; } How do you feel about that? @@ +1740,5 @@ > + } > + if(!verifyCallback->isChainValid){ > + r = SECFailure; > + PORT_SetError(SEC_ERROR_INVALID_ARGS); > + break; If the bug is "consistent style within NSS", sure ;-) That said, I suspect that ship has long since sailed, and while some things are important to keep consistent with local conventions (eg: indentation, formatting of ifs, etc), I think actual code should try to read in the most logical form (eg: internally consistent). "Cleaning up" 1578 would be nice, but I think runs contrary to the recommendations for handling style issues within NSS.

removing spaces is more annoying than expected.

Comment on attachment 662680 [details] [diff] [review] proposed-patch-v4.2 (post ryan) Fixed all comments, including formatting.

Comment on attachment 662680 [details] [diff] [review] proposed-patch-v4.2 (post ryan) Review of attachment 662680 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/nss/lib/certdb/certt.h @@ +889,5 @@ > > +/* > + * These types are for the validate chain callback param > + * > + * isChainValidArg is the state of the callback, used by the application but Suggested rewording of this comment: These types are for the validate chain callback param. CERTChainVerifyCallback is an application-supplied callback that can be used to perform application-defined checks. - isValidChainArg [nit: suggest just 'arg', but don't care much] contains the application-provided opaque argument - currentChain is the currently validated chain. It is ordered with the leaf certificate at the head of currentChain and the current trust anchor in the tail. If the application can successfully evaluate the chain, it should assign PR_TRUE or PR_FALSE to *chainOK, and return SECSuccess, indicating the chain was evaluated. Any other return value will be interpreted as a fatal error. You should also highlight this API is only called for (otherwise trustworthy) certs. There's a use case for being able to be called back on all chains (which I don't think is necessary here), but it's at least worth highlighting. Also, it might be worth mentioning what the values of the chain are when the trust anchor is a self-signed cert. Is the length of currentChain 1 (the self-signed cert) or 2 (the self-signed cert + the self signed cert as a trust anchor) @@ +892,5 @@ > + * > + * isChainValidArg is the state of the callback, used by the application but > + * opaque from NSS's perspective. > + * currentChain is the proposed chain to be used for validation, it will be > + * sorted with the end Entity at the head of the chain and the trust anchor in typo: s/end Entity/end-entity/ @@ +894,5 @@ > + * opaque from NSS's perspective. > + * currentChain is the proposed chain to be used for validation, it will be > + * sorted with the end Entity at the head of the chain and the trust anchor in > + * the tail position of the chain. > + * isChainOK is an output paramater that will contain if the prposed chain is typo: s/isChainOK/chainOK/ typo: s/prposed/proposed/ @@ +907,5 @@ > + PRBool *chainOK ); > + > +typedef struct { > + CERTChainVerifyCallback isChainValid; > + void *isChainValidState; nit: You haven't renamed this to match isValidChainArg on line 905 @@ +981,5 @@ > cert_pi_useAIACertFetch = 12, /* Enables cert fetching using AIA extension. > * In NSS 3.12.1 or later. Default is off. > * Value is in value.scalar.b */ > + cert_pi_chainVerifyCallback = 13, > + /* The callback container for doing extra /* Application supplied callback for performing additional * validation on the currently calculated certificate chain. * * Value is in value.pointer.verifyCallback */ ::: security/nss/lib/certhigh/certvfypkix.c @@ +1730,5 @@ > (PRBool)(param->value.scalar.b != 0), > plContext); > break; > + > + case cert_pi_chainVerifyCallback: note: In each of the other parameters, libpkix takes ownership of the supplied argument. Because you're storing the pointer in pkix_pl_nsscontext.h, rather than copying the structure, it means the following is not valid: CERTVerifyCallback cb; cb.isChainValid = &MyClass::MyFunc; cb.isChainValidArg = this; ... CERTValParamInValue mycallback; mycallback.pointer.verifyCallback = &cb; ... CERTValInParam args[] = { { cert_pi_chainVerifyCallback, &mycallback } } My suggestion to remedy this would be to have pkix_pl_nssContext have a CERTVerifyCallback (not "const CERTVerifyCallback*"), and to copy the in params into that structure, rather than taking the pointer of the structure and storing it here. This is perhaps a minor point, but it does make it easier for expectations of how the API behaves. ::: security/nss/lib/libpkix/pkix/top/pkix_validate.c @@ +749,5 @@ > revChecking = *pRevChecking; > + nssContext = (PKIX_PL_NssContext *)plContext; > + verifyCallback = nssContext ? nssContext ->certVerifyCallback : NULL; > + > + if( nssContext->certVerifyCallback != NULL && verifyCallback->isChainValid != NULL ){ Formatting: if (nssContext->certVerifyCallback != NULL && verifyCallback->isChainValid != NULL) { @@ +750,5 @@ > + nssContext = (PKIX_PL_NssContext *)plContext; > + verifyCallback = nssContext ? nssContext ->certVerifyCallback : NULL; > + > + if( nssContext->certVerifyCallback != NULL && verifyCallback->isChainValid != NULL ){ > + PRBool callbackRv = PR_FALSE; /*assume Failure*/ nit: /* assume failure */ @@ +760,5 @@ > + if (certList == NULL) { > + PKIX_ERROR_ALLOC_ERROR(); > + } > + > + /* Get the anchors CERTcertificate and add it to the list */ typo: s/anchors/anchor's/ @@ +773,5 @@ > + rv = CERT_AddCertToListTail(certList, tempCert); > + if (rv != SECSuccess) { > + PKIX_ERROR_ALLOC_ERROR(); > + } > + /*the certList takes ownership of tempCert on success*/ nit: /* the certList ... on success */ (spaces around comments) @@ +799,5 @@ > + PKIX_ERROR_ALLOC_ERROR(); > + } > + > + } > + rv =(*verifyCallback->isChainValid)(verifyCallback->isChainValidState, certList, &callbackRv); rv = (*verifyCallback->isChainValid)(...)

Comment on attachment 662680 [details] [diff] [review] proposed-patch-v4.2 (post ryan) Review of attachment 662680 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/nss/lib/certdb/certt.h @@ +889,5 @@ > > +/* > + * These types are for the validate chain callback param > + * > + * isChainValidArg is the state of the callback, used by the application but Agreeed. What about: //Start--- These types are for the validate chain callback param. CERTChainVerifyCallback is an application-supplied callback that can be used to perform application-defined checks. Two tissues are important to notice: 1. The callback is called only when using libpkix 2. The callback is called once a potential chain is built but before OSCP checks are done. - isValidChainArg [nit: suggest just 'arg', but don't care much] contains the application-provided opaque argument - currentChain is the currently validated chain. It is ordered with the leaf certificate at the head of currentChain and the current trust anchor in the tail. If the application can successfully evaluate the chain, it should assign PR_TRUE or PR_FALSE to *chainOK, and return SECSuccess, indicating the chain was evaluated. Any other return value will be interpreted as a fatal error. //end of comment (for now) I dont know what exactly happens on those cases, about to run experiments to find out. @@ +907,5 @@ > + PRBool *chainOK ); > + > +typedef struct { > + CERTChainVerifyCallback isChainValid; > + void *isChainValidState; oops. Yes my bad. ::: security/nss/lib/certhigh/certvfypkix.c @@ +1730,5 @@ > (PRBool)(param->value.scalar.b != 0), > plContext); > break; > + > + case cert_pi_chainVerifyCallback: OK I makes sense. Will change ::: security/nss/lib/libpkix/pkix/top/pkix_validate.c @@ +749,5 @@ > revChecking = *pRevChecking; > + nssContext = (PKIX_PL_NssContext *)plContext; > + verifyCallback = nssContext ? nssContext ->certVerifyCallback : NULL; > + > + if( nssContext->certVerifyCallback != NULL && verifyCallback->isChainValid != NULL ){ Agreed Fixed. @@ +750,5 @@ > + nssContext = (PKIX_PL_NssContext *)plContext; > + verifyCallback = nssContext ? nssContext ->certVerifyCallback : NULL; > + > + if( nssContext->certVerifyCallback != NULL && verifyCallback->isChainValid != NULL ){ > + PRBool callbackRv = PR_FALSE; /*assume Failure*/ Aggreed Fixed. @@ +760,5 @@ > + if (certList == NULL) { > + PKIX_ERROR_ALLOC_ERROR(); > + } > + > + /* Get the anchors CERTcertificate and add it to the list */ Thanks, fixed @@ +799,5 @@ > + PKIX_ERROR_ALLOC_ERROR(); > + } > + > + } > + rv =(*verifyCallback->isChainValid)(verifyCallback->isChainValidState, certList, &callbackRv); ARG.. (as in pirate, not as in argument). Missed this one.

Comment on attachment 662680 [details] [diff] [review] proposed-patch-v4.2 (post ryan) Review of attachment 662680 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/nss/lib/certdb/certt.h @@ +889,5 @@ > > +/* > + * These types are for the validate chain callback param > + * > + * isChainValidArg is the state of the callback, used by the application but s/tissues/issues/ s/notice/note/ s/potential chain is built/after a candidate chain has been constructed and validated, but before revocation checking has been performed./ s/[nit: ]// (that was just an inline aside) s/currentChain is the currently validated/currentChain is the chain currently being validated/ (suggesting the operation is not yet complete) I would also delete the note 1, since that's implicit (and explicit) in the fact that these are libpkix structures (right?)

Comment on attachment 662680 [details] [diff] [review] proposed-patch-v4.2 (post ryan) Review of attachment 662680 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/nss/lib/certdb/certt.h @@ +889,5 @@ > > +/* > + * These types are for the validate chain callback param > + * > + * isChainValidArg is the state of the callback, used by the application but The results are in: The callback is only run if there is a potential chain anchored to a trustworthy anchor. (Potential means valid pre OSCP checks or name mistmatch checks). If there is for example: invalid times/self signed/ untrusted anchors the callback will not be invoked. However this is not as bad since the caller can determine if the callback was called by putting some state in the state/arg parameter (I do this for cert pinning). Is this what we want?

I'm not sure I follow the question you're asking. Running it after RFC 5280 checks (aka Sections 6.1 and 6.2), but before the revocation checks (eg: 6.3) or RFC 6125 checks is fine. Less than ideal for some use cases (for example, mapping OS X trust settings onto libpkix path building), but that's more like me asking for a unicorn than a pony, as an alternative interface probably makes more sense. For how this is intended to be used (revoking trust, rather than granting it), I think that is fine behaviour. If it's meant to provide applications with a means to do *alternative* revocation checking (eg: disable OCSP & CRLs if the callback is happy, but leave them on otherwise), we'll need more - but I don't think that's quite needed at this point. So yeah, the described behaviour *sounds* right to me.

Comment on attachment 663235 [details] [diff] [review] proposed-patch-v5.3 (post ryan) I think this is it. -Changed the input paramteter handling to copy the struct. - Moved cleanup of certchain and tempCert to fatal section Also lots of minor space cleaups.

Comment on attachment 663228 [details] [diff] [review] proposed-patch-v5 (post ryan) Review of attachment 663228 [details] [diff] [review]: ----------------------------------------------------------------- I think overall this patch looks good. A few nits here or there, but like the past few, nothing functional is changing. I'm not sure if I can r+, but consider this an r+ with qualifications :) ::: security/nss/lib/certdb/certt.h @@ +902,5 @@ > + * certificate at the head of currentChain and the current trust anchor in > + * the tail. > + * - chainOK is the value of the check > + * > + * If the application can successfully evaluate the chain, it should assign *cough* whitespace ;-) "chainOK should contain the application's response to the check" or something like that. I think the point being is that it's not the value (it's a pointer), and it's not set *before* the callback, but set *by* the callback. Or just omit chainOK entirely, letting the bullet points be the "input" arguments and then the sentence following be describing the "output" arguments (or post-conditions) Just a minor tweak, hopefully. ::: security/nss/lib/certhigh/certvfypkix.c @@ +1738,5 @@ > + r = SECFailure; > + break; > + } > + > + nssContext->certVerifyCallback = *verifyCallback; There's a slight risk of us changing the size of the structure at some future point, at which point a new version of NSS (with three members in the struct) could end up reading a structure that was supplied with only two members (since it was compiled and linked against the old headers). I doubt this is a real problem in practice - I don't know how we might extend this structure - but it's at least worth calling out that we *cannot* extend it (unless we made the CERTVerifyCallback an opaque type that they had to create with CERT_CreateVerifyCallback / CERT_FreeVerifyCallback, but that's just layers of ugly abstraction) Maybe worth noting in the header? Maybe I'm just overly paranoid? This same problem existed in the original implementation, albeit in a different form, so it's not something new in this approach, just something I was thinking about while reviewing this. ::: security/nss/lib/libpkix/pkix/top/pkix_validate.c @@ +766,5 @@ > + (anchor, &cert, plContext), > + PKIX_TRUSTANCHORGETTRUSTEDCERTFAILED); > + > + PKIX_CHECK( > + PKIX_PL_Cert_GetCERTCertificate(cert,&tempCert,plContext), nit: whitespace here between arguments I think the fact that it will overflow and thus require wrapping is fine, and consistent with how line 765 was wrapped (which is the fairly common libpkix pattern anyways) @@ +774,5 @@ > + if (rv != SECSuccess) { > + PKIX_ERROR_ALLOC_ERROR(); > + } > + /* the certList takes ownership of tempCert on success */ > + tempCert=NULL; nit: tempCert = NULL @@ +811,5 @@ > + PKIX_DECREF(cert); > + if (callbackRv == PR_FALSE) { > + PKIX_ERROR(PKIX_CERTISCERTTRUSTEDFAILED); > + } > + nit: whitespace ::: security/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_nsscontext.c @@ +86,5 @@ > context->maxResponseLength = PKIX_DEFAULT_MAX_RESPONSE_LENGTH; > context->crlReloadDelay = PKIX_DEFAULT_CRL_RELOAD_DELAY_SECONDS; > context->badDerCrlReloadDelay = > PKIX_DEFAULT_BAD_CRL_RELOAD_DELAY_SECONDS; > + context->certVerifyCallback.isChainValid = NULL; I think it'd be good to set Arg = NULL here as well. I understand it's not strictly necessary (since you'll only call isChainValid is the caller supplied a callback, at which point, you would have overwritten *Arg with their callback's value), but with complete initialization you never have to worry about when the implementation details change.

missed one space on v6

Comment on attachment 663470 [details] [diff] [review] proposed-patch-v6.1 Review of attachment 663470 [details] [diff] [review]: ----------------------------------------------------------------- Getting close. ::: security/nss/lib/certdb/certt.h @@ +887,5 @@ > SECItem inhibitMappingSkipCerts; > } CERTCertificatePolicyConstraints; > > +/* > + *These types are for the validate chain callback param. Leading space before "These" @@ +891,5 @@ > + *These types are for the validate chain callback param. > + * > + * CERTChainVerifyCallback is an application-supplied callback that can be used > + * to perform application-defined checks. Two issues are important to note: > + * 1. The callback is called only when using libpkix I would omit the statement that the callback only works with libpkix because it is self-evident; there is simply no way to pass the callback to any other API. I suggest rewording this as: "CERTChainVerifyCallback is an application-supplied callback that can be used to augment libpkix's certificate chain validation with additional application-specific checks. It may be called multiple times if there are multiple potentially-valid paths for the certificate being validated. This callback is called before revocation checking is done on the certificates in the given chain." @@ +898,5 @@ > + * other cases). > + * > + * - isValidChainArg contains the application-provided opaque argument > + * - currentChain is the currently validated chain. It is ordered with the leaf > + * certificate at the head of currentChain and the current trust anchor in I would omit "current ". @@ +903,5 @@ > + * the tail. > + * > + * If the application can successfully evaluate the chain, it should assign > + * PR_TRUE or PR_FALSE to *chainOK, and return SECSuccess, indicating the chain > + * was evaluated. Any other return value will be interpreted as a fatal error. I would replace this with: "The callback should set *chainOK = PR_TRUE and return SECSuccess if the certificate chain is acceptable. It should set *chainOK = PR_FALSE and return SECSuccess if the chain is unacceptable, to indicate that the given chain is bad and path building should continue. It should return SECFailure to indicate an fatal error that will cause path validation to fail immediately." @@ +905,5 @@ > + * If the application can successfully evaluate the chain, it should assign > + * PR_TRUE or PR_FALSE to *chainOK, and return SECSuccess, indicating the chain > + * was evaluated. Any other return value will be interpreted as a fatal error. > + */ > +typedef SECStatus (*CERTChainVerifyCallback) (void *isChainValidArg, s/CERTChainVerifyCallback/CERTChainVerifyCallbackFunc/ @@ +916,5 @@ > + */ > +typedef struct { > + CERTChainVerifyCallback isChainValid; > + void *isChainValidArg; > +} CERTVerifyCallback; s/CERTVerifyCallback/CERTChainVerifyCallback/ I'm suggesting these renamings because the names should emphasize the "chain" aspect consistently. It is OK for CERTChainVerifyCallbackFunc to have an uglier name because that name is only mostly within libpkix, and it's less likely to be used in the application's code. Also, Ryan had suggested having more callbacks in the future that are not necessarily called on a per-chain basis, so emphasizing "chain" here is a good idea. @@ +992,5 @@ > + cert_pi_chainVerifyCallback = 13, > + /* The callback container for doing extra > + * validation on the currently calculated chain. > + * > + * Value is in value.pointer.verifyCallback */ s/verifyCallback/chainVerifyCallback/ @@ +1234,5 @@ > const char* s; > const CERTCertificate* cert; > const CERTCertList *chain; > const CERTRevocationFlags *revocation; > + const CERTVerifyCallback *verifyCallback; s/verifyCallback/chainVerifyCallback/ ::: security/nss/lib/certhigh/certvfypkix.c @@ +1554,5 @@ > PKIX_PL_Cert *certPkix = NULL; > PKIX_TrustAnchor *trustAnchor = NULL; > PKIX_PL_Date *revDate = NULL; > PKIX_RevocationChecker *revChecker = NULL; > + const CERTVerifyCallback *verifyCallback = NULL; s/verifyCallback/chainVerifyCallback/ NIT: This should be made local to the block that uses it (you'd need to add braces around the body of the case to create a new block). ::: security/nss/lib/libpkix/pkix/top/pkix_validate.c @@ +736,5 @@ > PKIX_PL_Cert *issuer = NULL; > + PKIX_PL_NssContext *nssContext = NULL; > + CERTCertList *certList = NULL; > + const CERTVerifyCallback *verifyCallback = NULL; > + CERTCertificate *tempCert = NULL; Nit: usually "nssCert" is used as the name. @@ +749,5 @@ > revChecking = *pRevChecking; > + nssContext = (PKIX_PL_NssContext *)plContext; > + verifyCallback = &(nssContext->certVerifyCallback); > + > + if (verifyCallback->isChainValid != NULL){ space between ")" and "{". @@ +750,5 @@ > + nssContext = (PKIX_PL_NssContext *)plContext; > + verifyCallback = &(nssContext->certVerifyCallback); > + > + if (verifyCallback->isChainValid != NULL){ > + PRBool callbackRv = PR_FALSE; /*assume failure*/ This should be renamed "chainOK" to match the callback parameter name, and because it doesn't hold any function's return value. @@ +783,5 @@ > + PKIX_DECREF(issuer); > + > + issuer = cert; > + cert = NULL; > + I think you copied this idiom from the existing block below, that is checking revocation. The revocation checking loop needs to have both the cert and the issuer for the call to PKIX_RevocationChecker_Check; that is why it has this awkward structure. I think you can replace the 6 preceding lines with just PKIX_DECREF(cert); @@ +788,5 @@ > + PKIX_CHECK(PKIX_List_GetItem( > + certs, j, (PKIX_PL_Object **)&cert, plContext), > + PKIX_LISTGETITEMFAILED); > + > + PORT_Assert(cert); Is this assertion necessary? I think the contract of PKIX_List_GetItem should be that if it doesn't fail, it must give a non-null result. @@ +802,5 @@ > + /* the certList takes ownership of tempCert on success */ > + tempCert = NULL; > + } > + > + rv = (*verifyCallback->isChainValid)(verifyCallback->isChainValidArg, certList, &callbackRv); Wrap all your added lines to to ~80 characters. (I'd break it before the second opening parenthesis). @@ +804,5 @@ > + } > + > + rv = (*verifyCallback->isChainValid)(verifyCallback->isChainValidArg, certList, &callbackRv); > + if (rv != SECSuccess) { > + PKIX_ERROR_ALLOC_ERROR(); /* XXX: we should map PR_GetError() to the correct error code. */ PKIX_ERROR_FATAL(PKIX_CHAINVERIFYCALLBACKFAILED); @@ +807,5 @@ > + if (rv != SECSuccess) { > + PKIX_ERROR_ALLOC_ERROR(); > + } > + > + PKIX_DECREF(cert); Not necessary if you take my suggestion above ("I think you can replace the 6 preceding lines with just PKIX_DECREF(cert);") @@ +809,5 @@ > + } > + > + PKIX_DECREF(cert); > + if (callbackRv == PR_FALSE) { > + PKIX_ERROR(PKIX_CERTISCERTTRUSTEDFAILED); PKIX_ERROR(PKIX_CHAINVERIFYCALLBACKFAILED)? What do you think Ryan? @@ +911,5 @@ > pkixErrorResult = checkCertError; > pkixErrorCode = pkixErrorResult->errCode; > checkCertError = NULL; > } > +fatal: Blank line before this one.

Comment on attachment 663470 [details] [diff] [review] proposed-patch-v6.1 Review of attachment 663470 [details] [diff] [review]: ----------------------------------------------------------------- Apologies, I keep taking a detailed look at things and finding out I missed patterns from before. I tentatively put this as r-, but I think it's such minor things rather than a "this is wrong" ::: security/nss/lib/certdb/certt.h @@ +893,5 @@ > + * CERTChainVerifyCallback is an application-supplied callback that can be used > + * to perform application-defined checks. Two issues are important to note: > + * 1. The callback is called only when using libpkix > + * 2. The callback is called once a potential chain is built but before OSCP > + * checks are done (ie it will not be called for self-signed certs among note: s/OCSP/revocation/ Gotta consider CRLs ;) @@ +911,5 @@ > + PRBool *chainOK); > + > +/* > + * Since this struct is copied within libpkix it cannot be changed without a change > + * in the NSS version. This isn't what I was meaning on IRC. /* * Note: If extending this structure, it will be necessary * to change the associated CERTValParamInType */ That said, I'm not entirely convinced it needs any note, as I think it's "mostly" obvious, but I felt it worth highlighting, since this is a structure that the caller is allocating (rather than say, using a create-function for). Standard C API/ABI issues. ::: security/nss/lib/libpkix/pkix/top/pkix_validate.c @@ +747,5 @@ > nbioContext = *pNBIOContext; > *pNBIOContext = NULL; > revChecking = *pRevChecking; > + nssContext = (PKIX_PL_NssContext *)plContext; > + verifyCallback = &(nssContext->certVerifyCallback); nit: I didn't think NSS style included the ( ) for address-of operations. verifyCallback = &nssContext->certVerifyCallback; @@ +754,5 @@ > + PRBool callbackRv = PR_FALSE; /*assume failure*/ > + SECStatus rv; > + > + /* The callback expects a certlist that includes the anchor > + * the list ends with the anchor */ nit: This comment doesn't make sense for the code it's documenting. I'd just remove it, I think 764-773 are clear enough to not require such a comment. @@ +781,5 @@ > + for (j = *pCertCheckedIndex; j < numCerts ; j++) { > + PORT_Assert(cert); > + PKIX_DECREF(issuer); > + > + issuer = cert; Why do you hold on to |issuer| here? It's completely unused. (note the lack of whitespace after numCerts) tempCert = NULL; PKIX_DECREF(cert); ... for (j = *pCertCheckedIndex; j < numCerts; j++) { PKIX_CHECK(PKIX_List_GetItem(...)) ... rv = CERT_AddCertToListHead(...); ... PKIX_DECREF(cert); } ... (and then remove Line 811, since it was decref'd above)

Comment on attachment 663470 [details] [diff] [review] proposed-patch-v6.1 Review of attachment 663470 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/nss/lib/libpkix/pkix/top/pkix_validate.c @@ +809,5 @@ > + } > + > + PKIX_DECREF(cert); > + if (callbackRv == PR_FALSE) { > + PKIX_ERROR(PKIX_CERTISCERTTRUSTEDFAILED); Good spot. Yes, I agree.

Comment on attachment 663470 [details] [diff] [review] proposed-patch-v6.1 Review of attachment 663470 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/nss/lib/certdb/certt.h @@ +887,5 @@ > SECItem inhibitMappingSkipCerts; > } CERTCertificatePolicyConstraints; > > +/* > + *These types are for the validate chain callback param. Done @@ +893,5 @@ > + * CERTChainVerifyCallback is an application-supplied callback that can be used > + * to perform application-defined checks. Two issues are important to note: > + * 1. The callback is called only when using libpkix > + * 2. The callback is called once a potential chain is built but before OSCP > + * checks are done (ie it will not be called for self-signed certs among Done. @@ +898,5 @@ > + * other cases). > + * > + * - isValidChainArg contains the application-provided opaque argument > + * - currentChain is the currently validated chain. It is ordered with the leaf > + * certificate at the head of currentChain and the current trust anchor in Done @@ +903,5 @@ > + * the tail. > + * > + * If the application can successfully evaluate the chain, it should assign > + * PR_TRUE or PR_FALSE to *chainOK, and return SECSuccess, indicating the chain > + * was evaluated. Any other return value will be interpreted as a fatal error. Agreed. Done @@ +911,5 @@ > + PRBool *chainOK); > + > +/* > + * Since this struct is copied within libpkix it cannot be changed without a change > + * in the NSS version. Done @@ +916,5 @@ > + */ > +typedef struct { > + CERTChainVerifyCallback isChainValid; > + void *isChainValidArg; > +} CERTVerifyCallback; Since the callback's signature is already named: CERTChainVerifyCallback I think this should be named instead CERTChainVerifyCallbackContainer. (but I dont like that name either). ::: security/nss/lib/certhigh/certvfypkix.c @@ +1554,5 @@ > PKIX_PL_Cert *certPkix = NULL; > PKIX_TrustAnchor *trustAnchor = NULL; > PKIX_PL_Date *revDate = NULL; > PKIX_RevocationChecker *revChecker = NULL; > + const CERTVerifyCallback *verifyCallback = NULL; Done ::: security/nss/lib/libpkix/pkix/top/pkix_validate.c @@ +736,5 @@ > PKIX_PL_Cert *issuer = NULL; > + PKIX_PL_NssContext *nssContext = NULL; > + CERTCertList *certList = NULL; > + const CERTVerifyCallback *verifyCallback = NULL; > + CERTCertificate *tempCert = NULL; OK. Renamed. @@ +747,5 @@ > nbioContext = *pNBIOContext; > *pNBIOContext = NULL; > revChecking = *pRevChecking; > + nssContext = (PKIX_PL_NssContext *)plContext; > + verifyCallback = &(nssContext->certVerifyCallback); Done. @@ +749,5 @@ > revChecking = *pRevChecking; > + nssContext = (PKIX_PL_NssContext *)plContext; > + verifyCallback = &(nssContext->certVerifyCallback); > + > + if (verifyCallback->isChainValid != NULL){ Done. @@ +750,5 @@ > + nssContext = (PKIX_PL_NssContext *)plContext; > + verifyCallback = &(nssContext->certVerifyCallback); > + > + if (verifyCallback->isChainValid != NULL){ > + PRBool callbackRv = PR_FALSE; /*assume failure*/ Agreed. @@ +754,5 @@ > + PRBool callbackRv = PR_FALSE; /*assume failure*/ > + SECStatus rv; > + > + /* The callback expects a certlist that includes the anchor > + * the list ends with the anchor */ Removed @@ +781,5 @@ > + for (j = *pCertCheckedIndex; j < numCerts ; j++) { > + PORT_Assert(cert); > + PKIX_DECREF(issuer); > + > + issuer = cert; Loved this change. Thanks @@ +788,5 @@ > + PKIX_CHECK(PKIX_List_GetItem( > + certs, j, (PKIX_PL_Object **)&cert, plContext), > + PKIX_LISTGETITEMFAILED); > + > + PORT_Assert(cert); Removed @@ +802,5 @@ > + /* the certList takes ownership of tempCert on success */ > + tempCert = NULL; > + } > + > + rv = (*verifyCallback->isChainValid)(verifyCallback->isChainValidArg, certList, &callbackRv); Done @@ +809,5 @@ > + } > + > + PKIX_DECREF(cert); > + if (callbackRv == PR_FALSE) { > + PKIX_ERROR(PKIX_CERTISCERTTRUSTEDFAILED); There is no such error currently. After talking to brian+ryan I will create a new error code. This should land in this bug, but to keep patches sane I will create a second patch for the error code. @@ +911,5 @@ > pkixErrorResult = checkCertError; > pkixErrorCode = pkixErrorResult->errCode; > checkCertError = NULL; > } > +fatal: Done

Comment on attachment 664177 [details] [diff] [review] proposed-patch-v7.1 Missed one space. And pending the change of the error code.

Comment on attachment 664219 [details] [diff] [review] proposed-patch-v7.2 Review of attachment 664219 [details] [diff] [review]: ----------------------------------------------------------------- r+ with nits addressed. Ryan or Wan-Teh, do you have any opinions on the error message text for the new error code, or the naming of said codes? ::: security/nss/lib/certdb/certt.h @@ +898,5 @@ > + * the given chain. > + * > + * - isValidChainArg contains the application-provided opaque argument > + * - currentChain is the currently validated chain. It is ordered with the leaf > + * certificate at the head of currentChain and the trust anchor in the tail. Nit: should be "... at the head and the trust anchor at the tail." @@ +993,5 @@ > * Value is in value.scalar.b */ > + cert_pi_chainVerifyCallback = 13, > + /* The callback container for doing extra > + * validation on the currently calculated chain. > + * remove this blank comment line. ::: security/nss/lib/certhigh/certvfypkix.c @@ +1730,5 @@ > plContext); > break; > + > + case cert_pi_chainVerifyCallback: > + { Nit: un-indent this block by four spaces, including the braces. @@ +1732,5 @@ > + > + case cert_pi_chainVerifyCallback: > + { > + const CERTChainVerifyCallback *chainVerifyCallback = NULL; > + chainVerifyCallback = param->value.pointer.chainVerifyCallback; Nit: It doesn't make sense to assign NULL to chainVerifyCallback and then assign param->value.pointer.chainVerifyCallback to it on the next line. You can do this: const CERTChainVerifyCallback *chainVerifyCallback = param->value.pointer.chainVerifyCallback; ::: security/nss/lib/libpkix/pkix/top/pkix_validate.c @@ +767,5 @@ > + PKIX_CHECK( > + PKIX_PL_Cert_GetCERTCertificate(cert, &nssCert, plContext), > + PKIX_CERTGETCERTCERTIFICATEFAILED); > + > + rv = CERT_AddCertToListTail(certList, nssCert); Nit: Use CERT_AddCertToListHead here so that it is clearer that this block of code is doing exactly the same thing as the code within the for loop. (At some point, we should probably factor out this "add cert to head of a CERTCertList" logic that is (or will be soon) repeated in libpkix, into a separate function. You don't need to do it now though. @@ +797,5 @@ > + > + rv = (*chainVerifyCallback->isChainValid) > + (chainVerifyCallback->isChainValidArg, certList, &chainOK); > + if (rv != SECSuccess) { > + PKIX_ERROR_ALLOC_ERROR(); PKIX_ERROR_FATAL(PKIX_CHAINVERIFYCALLBACKFAILED); @@ +800,5 @@ > + if (rv != SECSuccess) { > + PKIX_ERROR_ALLOC_ERROR(); > + } > + > + if (chainOK == PR_FALSE) { if (!chainOK) { (Do not do comparisons == or != true or false.)

No, no preferences. I interpret your question as asking both about the libpkix error code and how that is mapped from libpkix to NSS/NSPR, right? For libpkix, the error code is likely inconsequential, since they're so verbose to begin with. For NSS, I suspect it should just map into trust failed, right? Or are you proposing a new error for NSS to indicate *application* trust failed? I think regardless, the error will be tricky, and will/should be known to the application already (since it rejected it to begin with), so I'm not sure how important it is to add a new error.

I adjusted some of the wording of the comments and adjusted the indention of the "break" statement to match the indention of another break statement in the same switch.

Checking in lib/certdb/certt.h; /cvsroot/mozilla/security/nss/lib/certdb/certt.h,v <-- certt.h new revision: 1.57; previous revision: 1.56 done Checking in lib/certhigh/certvfypkix.c; /cvsroot/mozilla/security/nss/lib/certhigh/certvfypkix.c,v <-- certvfypkix.c new revision: 1.55; previous revision: 1.54 done Checking in lib/libpkix/include/pkix_errorstrings.h; /cvsroot/mozilla/security/nss/lib/libpkix/include/pkix_errorstrings.h,v <-- pkix_errorstrings.h new revision: 1.40; previous revision: 1.39 done Checking in lib/libpkix/pkix/top/pkix_validate.c; /cvsroot/mozilla/security/nss/lib/libpkix/pkix/top/pkix_validate.c,v <-- pkix_validate.c new revision: 1.19; previous revision: 1.18 done Checking in lib/libpkix/pkix_pl_nss/module/pkix_pl_nsscontext.c; /cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_nsscontext.c,v <-- pkix_pl_nsscontext.c new revision: 1.7; previous revision: 1.6 done Checking in lib/libpkix/pkix_pl_nss/module/pkix_pl_nsscontext.h; /cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_nsscontext.h,v <-- pkix_pl_nsscontext.h new revision: 1.7; previous revision: 1.6 done Checking in lib/util/SECerrs.h; /cvsroot/mozilla/security/nss/lib/util/SECerrs.h,v <-- SECerrs.h new revision: 1.27; previous revision: 1.26 done Checking in lib/util/secerr.h; /cvsroot/mozilla/security/nss/lib/util/secerr.h,v <-- secerr.h new revision: 1.34; previous revision: 1.33 done

Comment on attachment 666107 [details] [diff] [review] cviecco's last patch, updated to fix some nits Brian: I have a question about the error message for SEC_ERROR_APPLICATION_CALLBACK_ERROR in mozilla/security/nss/lib/util/SECerrs.h: >+ER3(SEC_ERROR_APPLICATION_CALLBACK_ERROR, (SEC_ERROR_BASE + 178), >+"The certificate was rejected by extra checks in the application.") In last week's NSS conference call, you said that SEC_ERROR_APPLICATION_CALLBACK_ERROR is intended to be a generic error code for any NSS application callback error. That was why I accepted the error code name. Otherwise I would have asked for a more specific error code name such as SEC_ERROR_CHAIN_VERIFY_CALLBACK_ERROR. But its error message is specific to the CERTChainVerifyCallbackFunc. Could you clarify this? Should we change the error message? Thanks.

I agree that the error message text is not generic enough. I suggest we use the message "The operation failed due to an application callback error." or similar. Maybe you are helping your localization team with the new error messages. In this case, it is important that the application never report this error to the user. Instead, the application should always report the actual error message. But, I suppose that localization teams will want to translate it regardless.

Comment on attachment 673116 [details] [diff] [review] Make the error message for SEC_ERROR_APPLICATION_CALLBACK_ERROR more generic r=wtc. Thanks. It would be nice to point out the actual error is not one defined by NSS. Perhaps something like "The application callback failed due to an application-defined error." or "The operation failed due to an application-specific error." (I was not helping someone translate the error messages. I reviewed the diffs between BETA1 and RC1 and notice this.) We should change the error message in NSS 3.14.1. Therefore, it's better to open a new bug report for this. I also suggest that you add a comment to secerr.h about not reporting this error code to the user.

Marked the bug fixed because NSS 3.14 has been released. We can probably just use this bug for the error message patch because it's just a tweak, or open a new bug report for it.

(In reply to Wan-Teh Chang from comment #53) > We can probably just use this bug for the error message patch > because it's just a tweak, or open a new bug report for it. Did this ever happen?

(In reply to Florian Bender from comment #54) > (In reply to Wan-Teh Chang from comment #53) > > We can probably just use this bug for the error message patch > > because it's just a tweak, or open a new bug report for it. > > Did this ever happen? Ping :cviecco as assignee.

This is closed. I filed up bug 1003948 to track the make error message more generic.

Thanks!