233605 - partial CRL decoding skips entry extension checks

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Julien Pierre]

The function cert_check_crl_version in lib/certdb/crl.c is used to check the CRL
version, the extensions in the CRL, as well as the extensions in the CRL entries.

When using partial decoding (CRL_DECODE_SKIP_ENTRIES flag) as we do by default
for CRLs, the entries are decoded at a later time than the rest of the CRL.

But the code that checks entry extensions is never invoked again in
CERT_CompleteCRLDecodeEntries . So the entry extensions are not checked.

The fix is to split cert_check_crl_version into two functions, one to check the
CRL version and its extensions, and another to check the entry extensions, which
can be called from CERT_CompleteCRLDecodeEntries.

Comment 1

[Julien Pierre]

Attachment: Split cert_check_crl_version into separate functions so partial decoding of entries works correctly

Comment 2

[Julien Pierre]

Comment on attachment 141009 [details] [diff] [review]
Split cert_check_crl_version into separate functions so partial decoding of entries works correctly

This patch fixes the core issue of this bug. It also contains some new, more
detailed error codes for CRL processing, as suggested by Nelson today.

Nelson, Wan-Teh, please review.

Comment 3

[Julien Pierre]

Attachment: New error codes to go with the other patch

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 141011 [details] [diff] [review]
New error codes to go with the other patch

Any new error codes need to be added to lib/util/secerr.h
and also to cmd/lib/SECerrs.h  - but I think I volunteered to 
add that for you, so I'll code up a patch for that.

I haven't reviewed the other patch for this bug yet, but I wonder
about the new error SEC_ERROR_CRL_NON_V2_EXTENSION.  I'm wondering
if we need this error code.  If the extension is not critical, 
can't we just ignore it?  If it is critical, isn't 
SEC_ERROR_CRL_UNKNOWN_CRITICAL_EXTENSION the error in that case?

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

After reviewing both patches, it appears to me that error
SEC_ERROR_CRL_NON_V2_EXTENSION means 
"V1 CRLs may not contain critical extensions".  Yes?
I saw no problems on the first pass, but want to review the patches in 
context before I'm done.

Oh, we discussed adding a MISSING_CRL_NEEDED error.  None of these new 
errors seems to be that one.  Are you planning to add that one in a 
separate patch?

Comment 6

[Julien Pierre]

Nelson,

Yes, strictly speaking SEC_ERROR_CRL_UNKNOWN_CRITICAL_EXTENSION would be
adequate, but I think the new error code is more specific as to what the problem
is. It could be renamed to SEC_ERROR_V1_CRITICAL_EXTENSION .

As for MISSING_CRL_NEEDED, this will be in a separate patch indeed. It would be
good if this could get checked in before since it is in the same file.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Attachment: New error codes and strings for error codes

Julien, This patch renames NON_V2 to SEC_ERROR_CRL_V1_CRITICAL_EXTENSION
and adds strings for all new error codes.  I did not change the code in the
.c files to use this error code name.  Please review it.  

I expect you will want to replace all the above patches with one new patch that
includes all 3 files.

Comment 8

[Wan-Teh Chang]

We also need to add these error codes to the error
code table in the SSL Reference before we mark this
bug fixed.

Comment 9

[Julien Pierre]

Attachment: updated patch to use renamed error codes

Comment 10

[Julien Pierre]

Thanks for the review, Nelson.

Checked this in to the tip.

Checking in cmd/lib/SECerrs.h;
/cvsroot/mozilla/security/nss/cmd/lib/SECerrs.h,v  <--  SECerrs.h
new revision: 1.8; previous revision: 1.7
done
Checking in lib/certdb/crl.c;
/cvsroot/mozilla/security/nss/lib/certdb/crl.c,v  <--  crl.c
new revision: 1.41; previous revision: 1.40
done
Checking in lib/util/secerr.h;
/cvsroot/mozilla/security/nss/lib/util/secerr.h,v  <--  secerr.h
new revision: 1.13; previous revision: 1.12
done

Keeping the bug open for the SSL reference work.

Comment 11

[Julien Pierre]

Setting target to 3.10 since this got checked into the tip.
Wan-Teh, what does it take to update the SSL reference ? Does it live only on
mozilla.org or is there also a copy in the cvs repository ?

Comment 12

[Wan-Teh Chang]

Nelson or I can update the SSL reference for you.
(You need a cvs account on gila.mozilla.org, the
cvs repository for www.mozilla.org contents.)

We need the following from you:
- New error codes
- Their numeric values
- Their text messages

Comment 13

[Julien Pierre]

Wan-Teh,

The following new error codes need to be added to the docs for 3.10 :

SEC_ERROR_REVOKED_CERTIFICATE_CRL, -8047,
"Certificate is revoked in issuer's certificate revocation list."

SEC_ERROR_REVOKED_CERTIFICATE_OCSP, -8046
"Issuer's OCSP responder reports certificate is revoked."


SEC_ERROR_CRL_INVALID_VERSION, -8045
"Issuer's Certificate Revocation List has an unknown version number."

SEC_ERROR_CRL_V1_CRITICAL_EXTENSION, -8044
"Issuer's V1 Certificate Revocation List has a critical extension."

SEC_ERROR_CRL_UNKNOWN_CRITICAL_EXTENSION, -8043
"Issuer's V2 Certificate Revocation List has an unknown critical extension."

Please add them to the docs, then mark this bug fixed. Thanks !

Comment 14

[Wan-Teh Chang]

The five new error codes have been added to the SSL Reference
(http://www.mozilla.org/projects/security/pki/nss/ref/ssl/sslerr.html#1039257).
 Marked the bug fixed.