When using CERT_PKIXVerifyCert with a |paramsOut| parameter that contains a cert_po_errorLog (which points to a CERTVerifyLog), the number of errors logged for a given certificate are significantly less than those logged by CERT_VerifyCertificate. CERT_VerifyCertificate ( http://mxr.mozilla.org/security/source/security/nss/lib/certhigh/certvfy.c#1104 ) performs multiple checks against the certificate. When a CERTVerifyLog is passed in, the failure of any given check does not immediately terminate execution - instead, the error is logged and validation continues. These checks include: * Time Validity * Usage validity (KU and EKU) * Trust * Revocation * Plus all of the chain-related issues detected in cert_VerifyCertChainOld ( http://mxr.mozilla.org/security/source/security/nss/lib/certhigh/certvfy.c#488 ), which has a similar behaviour of continuing checks when a log object is provided. When using LibPKIX by calling CERT_PKIXVerifyCert, only a single error is logged. The underlying reason is due to pkix_CertSelector_DefaultMatch, which is called by pkix_Build_InitiateBuildChain ( http://mxr.mozilla.org/security/source/security/nss/lib/libpkix/pkix/top/pkix_build.c?mark=3350,3363#3350 ) The implementation of pkix_CertSelector_DefaultMatch ( http://mxr.mozilla.org/security/source/security/nss/lib/libpkix/pkix/certsel/pkix_certselector.c#1165 ) is quite similar to that of CERT_VerifyCertificate, checking things like basic constraints, policies, validity, naming, usage, and signature. However, the implementation of pkix_CertSelector_DefaultMatch is that it uses the LibPKIX macro of PKIX_CHECK. PKIX_CHECK will terminate the execution of pkix_CertSelector_DefaultMatch the moment one of the checks fails. The check that failed will be added to the CERTVerifyLog (by means of pkix_VerifyNode_* functions), but no additional checks will be executed. Because of this, only a single error will appear in a CERTVerifyLog when a certificate contains multiple errors, such as an expired certificate whose key usage is unacceptable. Applications that allow a user to override certain errors, such as expiration, may have been coded such that they make sure that the ONLY error(s) in the verify log were those that they wish to suppress. If LibPKIX is turned on for these applications, they may fail to catch the key usage error, and may thus allow the user to override additional errors that should not have been. Note that this is a special case that happens when calling CERT_PKIXVerifyCert directly. When using CERT_VerifyCertificate after setting CERT_SetUsePKIXForValidation(PR_TRUE), the initial certificate under inspection WILL have multiple errors set for it in the error log. This is because CERT_VerifyCertificate doesn't hand off to LibPKIX until after it has added all relevant errors to the log for the initial certificate.

Ryan: thank you for reporting and tracking down this bug. This problem of aborting a loop after one thing fails is similar to problem 2 of bug 528743 comment 0. We probably can use PKIX_CHECK_ONLY_FATAL or a variant of it for pkix_CertSelector_DefaultMatch. It seems that we also need to pass pVerifyNode to pkix_CertSelector_DefaultMatch so that pkix_CertSelector_DefaultMatch can call pkix_VerifyNode_Create for each error it encounters. Would you like to give it a try?

FYI: the cert_po_errorLog support for CERT_PKIXVerifyCert was added in two patches in bug 294531: attachment 282175 [details] [diff] [review] and attachment 307775 [details] [diff] [review].

Wan-Teh: Thanks for the references. Yes, I do plan to work on a patch for this specific issue against NSS trunk.

With this bug, PSM's error page and override mechanism will regress when using libPKIX. In order to override a site's cert, PSM must be able to retrieve the full set of errors in one step. Because of this bug, when visiting an SSL site with a certificate that is both expired and untrusted, PSM will only see one error at any time, and the override that PSM creates will never be sufficient (PSM will alternatingly report one or the other error). We must fix this bug, before we can make libpkix the default in Mozilla. Also, fixing this bug is necessary for reporting more details about the real cause of OCSP failures.

Change the VerifyNode API to track a PKIX_List of error objects, rather than a single error. For any step during the path construction or verification, there may be multiple errors associated. While the PKIX_Error interface provides a way to describe both an underlying cause as well as associate supplemental info, these interfaces are not well-suited towards reporting multiple distinct errors, and would require tight coupling between error codes and their supplemental storage mechanism. Instead, let the VerifyNode be able to track the multiple verification failures. This is in anticipation of changing the CertChecker callbacks to receive the VerifyNode, so that they can add as many errors as appropriate.

Please note well the changes to pkix_build.c The code "should" have used pkix_VerifyNode_SetError, which replaces any existing error with a new error code. However, SetError's existing expectations are that verifyNode->error is always NULL, and it's clear that this code in pkix_build.c is not operating on that same assumption. Because of this, AddError seems the correct API call for these - it will create an error list if errorList is NULL, or append the error if it wasn't. Regardless, tests will fail, as expected.

Comment on attachment 644550 [details] [diff] [review] A patch that actually compiles Review of attachment 644550 [details] [diff] [review]: ----------------------------------------------------------------- r=wtc. I think this patch is correct. I ask some questions and suggest some changes below. Please attach a new version of this patch. Thanks. ::: mozilla/security/nss/lib/certhigh/certvfypkix.c @@ +905,3 @@ > children = node->children; > > + if (errorList) { Please confirm that you want to add the errors of the current node to the log first, before adding the errors of the child nodes. @@ -896,3 @@ > children = node->children; > > - if (children == NULL) { The original code seems to imply that for each 'node', at least one of node->children and node->error is NULL. Is that true? @@ +923,1 @@ > #endif Just delete the DEBUG_volkov code. No need to update it. ::: mozilla/security/nss/lib/libpkix/include/pkix_errorstrings.h @@ +1078,4 @@ > PKIX_ERRORENTRY(VERIFYNODEFINDERRORFAILED,pkix_VerifyNode_FindError failed,0), > PKIX_ERRORENTRY(VERIFYNODESETDEPTHFAILED,pkix_VerifyNode_SetDepth failed,0), > PKIX_ERRORENTRY(VERIFYNODESETERRORFAILED,pkix_VerifyNode_SetError failed,0), > +PKIX_ERRORENTRY(VERIFYNODEADDERRORFAILED,pkix_VerifyNode_AddError failed,0), Add this line in sorted order of VERIFYNODEADDERRORFAILED. The PKIX error code names are all caps without underscores, which makes them hard to read. So it would be nice to sort the error code names in this file. ::: mozilla/security/nss/lib/libpkix/pkix/results/pkix_verifynode.c @@ +63,5 @@ > + PKIX_List_Create(&errorList, plContext), > + PKIX_LISTCREATEFAILED); > + PKIX_CHECK( > + PKIX_List_AppendItem(errorList, (PKIX_PL_Object*)error, > + plContext), If 'error' is NULL, should we still create an empty 'errorList'? @@ -685,5 @@ > plContext, > PKIX_FAILUREHASHINGCERT); > > - PKIX_CHECK(PKIX_PL_Object_Hashcode > - ((PKIX_PL_Object *)node->error, Since PKIX_PL_Object_Hashcode throws an error if the first argument is NULL, this seems to imply that node->error cannot be NULL, or that this function has never been called. (This is related to my question above in pkix_VerifyNode_Create.) @@ +964,4 @@ > PKIX_CHECK(pkix_VerifyNode_Create > (original->verifyCert, > original->depth, > + NULL, pkix_VerifyNode_Create should be updated to take an 'errorList' argument instead. You can add a pkix_VerifyNode_Create2 variant to do this in stages, if you don't want to make this patch too big. @@ +1136,5 @@ > + PKIX_RETURN(VERIFYNODE); > +} > + > +/* > + * FUNCTION: PKIX_VerifyNode_SetError Typo: SetError => AddError @@ +1146,5 @@ > + * PARAMETERS: > + * "node" > + * The address of the VerifyNode to be modified. Must be non-NULL. > + * "error" > + * The address of the Error to be stored. Nit: stored => appended or added @@ +1171,5 @@ > + pkixErrorResult = pkix_VerifyNode_SetError(node, error, > + plContext); > + if (pkixErrorResult) { > + pkixErrorClass = pkixErrorResult->errClass; > + pkixErrorCode = pkixErrorResult->errCode; Why don't you use PKIX_CHECK to call pkix_VerifyNode_SetError? The only difference is the value assigned to pkixErrorCode here. Is it important for pkixErrorCode to be pkixErrorResult->errCode here? I think pkixErrorResult->errCode will still be available because the pkix errors seem to be linked by the "cause" pointers. @@ +1251,5 @@ > } > } > } > + > + /* If that fails, try to find an error in the error list. */ Nit: add "of the current level" or "of the current node" at the end of this comment? ::: mozilla/security/nss/lib/libpkix/pkix/top/pkix_build.c @@ +766,5 @@ > + pkixTempResult = pkix_VerifyNode_AddError(\ > + verifyNode, pkixErrorResult, plContext); \ > + if (pkixTempResult) { \ > + PKIX_DECREF(pkixErrorResult); \ > + pkixErrorResult = pkixTempResult; \ I can't find existing code that uses pkixTempResult this way. Could you add a comment to explain this (why do we replace the original error in pkixErrorResult with pkixTempResult)? It seems that we should also update pkixErrorClass since pkixErrorResult->errClass has changed? ... pkixErrorResult = pkixTempResult; \ pkixErrorClass = pkixErrorResult->errClass; \ if (pkixErrorClass == PKIX_FATAL_ERROR) { \ goto cleanup; \ } \ Should we set pkixTempResult to NULL after saving its value in pkixErrorResult?

Comment on attachment 644550 [details] [diff] [review] A patch that actually compiles Review of attachment 644550 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozilla/security/nss/lib/libpkix/pkix/results/pkix_verifynode.c @@ +1264,5 @@ > + PKIX_LISTGETITEMFAILED); > + if (verifyError->plErr) { > + PKIX_INCREF(verifyError); > + *error = verifyError; > + } To preserve the current behavior, perhaps we should use the last error rather than the first error of the error list? pkix_VerifyNode_FindError seems to do exactly that here, because it stays in the for loop after storing verifyError in *error. Speaking of which, I think we need to PKIX_DECREF what was in *error before assigning a new value to *error, right?