Last Comment Bug 448324 - ocsp checker returns incorrect error code on request with invalid signing cert
: ocsp checker returns incorrect error code on request with invalid signing cert
Status: RESOLVED FIXED
PKIX
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.12
: All All
: P1 normal (vote)
: 3.12.1
Assigned To: Alexei Volkov
:
Mentors:
Depends on:
Blocks: 444404
  Show dependency treegraph
 
Reported: 2008-07-28 14:44 PDT by Alexei Volkov
Modified: 2008-08-11 18:47 PDT (History)
0 users
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 - fix reported error (7.47 KB, patch)
2008-07-28 15:34 PDT, Alexei Volkov
nelson: review-
Details | Diff | Review
Patch v2 - simplifyed version (2.95 KB, patch)
2008-08-10 22:12 PDT, Alexei Volkov
nelson: review+
Details | Diff | Review
Patch v3 (4.68 KB, patch)
2008-08-11 14:27 PDT, Alexei Volkov
nelson: review+
Details | Diff | Review

Description Alexei Volkov 2008-07-28 14:44:06 PDT
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.
Comment 1 Alexei Volkov 2008-07-28 15:34:24 PDT
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 2 Nelson Bolyard (seldom reads bugmail) 2008-08-04 18:11:41 PDT
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).
Comment 3 Alexei Volkov 2008-08-10 22:12:04 PDT
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 4 Nelson Bolyard (seldom reads bugmail) 2008-08-11 12:25:27 PDT
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).
Comment 5 Alexei Volkov 2008-08-11 14:27:52 PDT
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 6 Nelson Bolyard (seldom reads bugmail) 2008-08-11 15:09:44 PDT
Comment on attachment 333284 [details] [diff] [review]
Patch v3

r=nelson
Comment 7 Alexei Volkov 2008-08-11 18:47:08 PDT
(In reply to comment #6)
> (From update of attachment 333284 [details] [diff] [review])
checked in

Note You need to log in before you can comment on or make changes to this bug.