Verifying cert from www.diginotar.nl(need to download cert with vfyserv and verify with vfychain :( ): vfychain -s allow-crl-and-ocsp -d . cert.000 Chain is bad, -8048 = Invalid OCSP signing certificate in OCSP response. Now trying to use libpkix: vfychain -s allow-crl-and-ocsp -d . -pp cert.000 -t cert.001 Chain is bad, -8179 = Peer's Certificate issuer is not recognized. This is the regression in returned codes. Need to fix it for 3.12.1.
Created attachment 331598 [details] [diff] [review] Patch v1 - fix reported error The patch fixes the error that is to be set to OCSP_INVALID_SIGNING_CERT if requests signing cert can not be validated(function pkix_pl_OcspResponse_CallCertVerify). It also fixes ocsp error code delivery mechanism. Prior to the patch, if code in function pkix_pl_OcspResponse_CallCertVerify had received a cert validation error, it threw an error which bypassed most of the error check code that was written to convert one error code to another. One more fix is about incorrect handling error when signature object was not assign(pkix_pl_OcspResponse_VerifySignature function).
Comment on attachment 331598 [details] [diff] [review] Patch v1 - fix reported error In today's review, I think we concluded that a) we want to continue to use a separate plContext when calling response->verifyFcn() in pkix_pl_OcspResponse_CallCertVerify. b) It's not necessary to change the signature of pkix_pl_OcspResponse_CallCertVerify to achieve the goal of setting the NSS error code SEC_ERROR_OCSP_INVALID_SIGNING_CERT when the cert verification reports failure, c) detecting a NULL "signature" in pkix_pl_OcspResponse_VerifySignature is good to do (as this patch does).
Created attachment 333190 [details] [diff] [review] Patch v2 - simplifyed version Set the proper error through PORT_SetError if signature of the response could not be verified.
Comment on attachment 333190 [details] [diff] [review] Patch v2 - simplifyed version In pkix_pl_OcspResponse_VerifySignature there is a call to pkix_pl_OcspResponse_CallCertVerify that is made with the macro PKIX_CHECK. I thought that the "short patch" was going to change that macro to PKIX_CHECK_ONLY_FATAL. Does this patch achieve its goal without doing that? This patch seems harmless. :) It's OK to check in, but I'm not sure it solves the problem(s).
Created attachment 333284 [details] [diff] [review] Patch v3 Previous patch was indeed incomplete. We should not only set an error in CallVerifyCert function, but also should not throw errors if a cert was invalid for response signing.
Comment on attachment 333284 [details] [diff] [review] Patch v3 r=nelson
(In reply to comment #6) > (From update of attachment 333284 [details] [diff] [review]) checked in