Closed Bug 287850 Opened 19 years ago Closed 18 years ago

chain validation returns ambiguous error codes when OCSP enabled

Categories

(NSS :: Libraries, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
3.11.3

People

(Reporter: nelson, Assigned: nelson)

Details

(Whiteboard: Sun CR 6420859)

Attachments

(1 file, 1 obsolete file)

The error code SEC_ERROR_BAD_SIGNATURE is ambiguous when OCSP is enabled.
The caller of CERT_VerifyCertificate cannot tell whether that error 
indicates that the cert's own signature was invalid, or the OCSP response's
signature was invalid.  That's because CERT_VerifyCertificate returns
CERT_CheckOCSPStatus's error code as its own error code, and
CERT_CheckOCSPStatus returns SEC_ERROR_BAD_SIGNATURE for an invalid 
signature on the OCSP response.  

This problem affects CERT_VerifyCert and CERT_VerifyCertificate. 

SEC_ERROR_BAD_SIGNATURE is one of SEVERAL AMBIGUOUS error codes, where 
the OCSP client uses the same error codes for OCSP failures as the cert 
chain validation code uses for cert failures.  Other ambiguous errors include
SEC_ERROR_BAD_DER, SEC_ERROR_INVALID_TIME, SEC_ERROR_EXTENSION_NOT_FOUND (aargh!).  

There are several ways this could be fixed.  I suggest that the OCSP code
never return an error code that is one of the ones also returned by the 
cert chain validity code when OCSP is not in use.  That may mean the 
creation of some new error codes, or may mean overloading some existing 
OCSP specific error codes.  

I might suggest using 

SEC_ERROR_OCSP_MALFORMED_RESPONSE instead of SEC_ERROR_BAD_DER 
SEC_ERROR_CRL_EXPIRED             instead of SEC_ERROR_INVALID_TIME
SEC_ERROR_CRL_BAD_SIGNATURE       instead of SEC_ERROR_BAD_SIGNATURE

SEC_ERROR_EXTENSION_NOT_FOUND should never be returned by CERT_VerifyCert,
and should be replaced with SEC_ERROR_CERT_BAD_ACCESS_LOCATION or,
in the case where the EKU extension is not found, it should return 
SEC_ERROR_INADEQUATE_KEY_USAGE (however, see bug 58507 about this last point).
The approach that is currently in the tree for CRLs is that most errors that
relate to the validity of the CRLs end up being considered as grounds for
considering the certificate itself revoked. The logic is that if the
user/administrator installed a CRL, he intended for a revocation check to be
done, and if it couldn't be, then the cert is considered revoked . Thus, the
CRLs error you list aren't actually returned by CERT_VerifyCert*.

The story for OCSP is different, as it is possible for an application to call an
OCSP checking API. CRL checking functions are not currently exposed from libnss.

I believe there is an existing bug about this ambiguity for OCSP, but I can't
find out at the moment, or I would mark it as duplicate.
QA Contact: bishakhabanerjee → jason.m.reid
Assignee: julien.pierre.bugs → wtchang
OS: Windows XP → All
Hardware: PC → All
Status: NEW → ASSIGNED
Reassigning back to me. Sorry, I was click-happy.
Assignee: wtchang → julien.pierre.bugs
Status: ASSIGNED → NEW
Priority: -- → P3
Target Milestone: --- → 3.12
QA Contact: jason.m.reid → libraries
Taking.  

The issue of Cert chain validation returning SEC_ERROR_EXTENSION_NOT_FOUND
is becoming a hot issue at Sun.  I examined the OCSP code today and found
6 places where it happens, representing 6 ways it can happen in 
http://lxr.mozilla.org/security/source/security/nss/lib/certhigh/ocsp.c
Here are the 6 ways, and what I propose to do about them.

1. static Function ocsp_AddServiceLocatorExtension fails to find an AIA 
    extension in the cert. (line 848) It then goes on to take remedial 
    action and this is not considered a failure.  It sets RV to SECSUccess,
    but it leaves this error code set.  IT SHOULD NOT LEAVE THIS ERROR SET!!
    Called from ocsp_CreateSingleRequestList() at line 938, which is 
    called from CERT_CreateOCSPRequest         at line 1035
    called from CERT_GetEncodedOCSPResponse    at line 2354
    called from CERT_CheckOCSPStatus           at line 3525
        ---> change to error 0

2. static Function ocsp_CertIsOCSPSigner fails to find EKU extension. 
   (line 2420)
    called from ocsp_AuthorizedResponderForCertID at line 3015,
    called from ocsp_VerifySingleResponse         at line 3152
    called from CERT_GetOCSPStatusForCertID       at line 3637
    called from CERT_CheckOCSPStatus              at line 3579
    I propose to change this to set one of these other two error codes:
	---> SEC_ERROR_OCSP_INVALID_SIGNING_CERT
        ---> SEC_ERROR_INADEQUATE_KEY_USAGE   (but this seems ambiguous, too).

3. Function CERT_GetOCSPAuthorityInfoAccessLocation fails to find the AIA
    extension in the cert (line 3227)
    called from ocsp_GetResponderLocation   at line 3341
    called from CERT_CheckOCSPStatus        at line 3481
	---> SEC_ERROR_CERT_BAD_ACCESS_LOCATION

4. Function CERT_GetOCSPAuthorityInfoAccessLocation finds the AIA extension
    but it did not include an OCSP method, (line 3253)
    called from ocsp_GetResponderLocation   at line 3341
    called from CERT_CheckOCSPStatus        at line 3481
	---> SEC_ERROR_CERT_BAD_ACCESS_LOCATION

5. Function CERT_GetOCSPAuthorityInfoAccessLocation finds the AIA extension
    and it includes an OCSP method, but that method contains no URI. 
    (line 3271)
    called from ocsp_GetResponderLocation   at line 3341
    called from CERT_CheckOCSPStatus        at line 3481
	---> SEC_ERROR_CERT_BAD_ACCESS_LOCATION

6. When ocsp_GetResponderLocation() returns SEC_ERROR_EXTENSION_NOT_FOUND, 
   function CERT_CheckOCSPStatus returns SECSuccess, but leaves the error 
   code set.  (line 3471) 
        ---> change to error 0
Assignee: julien.pierre.bugs → nelson
Priority: P3 → P2
Whiteboard: Sun CR 6420859
Target Milestone: 3.12 → 3.11.3
Attachment #229616 - Flags: review?(julien.pierre.bugs)
In writing the above patch, I found a path that returns SECFailure without
setting any error code.  It occured in ocsp_CertIsOCSPSigner() if the cert
contains an EKU extension, but the EKU does not include the special 
OCSP_RESPONDER OID.  My patch now sets SEC_ERROR_OCSP_INVALID_SIGNING_CERT 
in that case, also.
Status: NEW → ASSIGNED
The previous patch turned all errors (including NO_MEMORY) in one functin into BAD_ACCESS_LOCATION.  This patch is more exacting.
Attachment #229616 - Attachment is obsolete: true
Attachment #229618 - Flags: review?(julien.pierre.bugs)
Attachment #229616 - Flags: review?(julien.pierre.bugs)
Attachment #229618 - Flags: review?(julien.pierre.bugs) → review+
Checking in ocsp.c; new revision: 1.21.2.7; previous revision: 1.21.2.6
Checking in ocsp.c; new revision: 1.26;     previous revision: 1.25

Marking Resolved/Fixed.

Thanks for the quick review, Julien.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: