pkix_pl_LdapCertStore_BuildCrlList should not fail if a crl fails to be decoded

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Alexei Volkov, Assigned: Alexei Volkov)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PKIX)

Attachments

(1 attachment)

1.73 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
Created attachment 291775 [details] [diff] [review]
Patch v1

Function pkix_pl_CRL_CreateToList is modified to return an error if failed to decode der crl and add crl into a list.

There are two callers of this function:
      pkix_pl_HttpCertStore_ProcessCrlResponse - it suppose to fail if received a bad CRL.
      pkix_pl_LdapCertStore_BuildCrlList - which suppose to skip a bad CRL and continue adding others to a list of CRL.

The bug is in pkix_pl_LdapCertStore_BuildCrlList: the function aborts generating crl list if one crl fails to be decoded. 

The attached patch makes the function skip all but fatal errors returned by pkix_pl_CRL_CreateToList.
Attachment #291775 - Flags: review?(nelson)
(Assignee)

Updated

10 years ago
Priority: -- → P2
Whiteboard: PKIX
(Assignee)

Comment 1

10 years ago
The patch has syntax error: 
+                                pkix_pl_CRL_CreateToList(derCrlItem, crlList,
+                                                         plContext);
                                                                   ^^^
and should be:
+                                pkix_pl_CRL_CreateToList(derCrlItem, crlList,
+                                                         plContext),

I patch is approved, the error will be fixed before committing the patch.
Comment on attachment 291775 [details] [diff] [review]
Patch v1

A Patch that doesn't compile is a patch that isn't tested.
Do you have any way to test this patch?  
Can you get a bad CRL to mix with others in a test?
Comment on attachment 291775 [details] [diff] [review]
Patch v1

r=nelson, once the syntax error is fixed.
Attachment #291775 - Flags: review?(nelson) → review+
(Assignee)

Comment 4

10 years ago
We have many ldap tests that are part of libpkix test suite. I believe that some tests try to fetch crl including bad ones.

I've filed the bug 396599 to restore ldap testing of libpkix, but the issue is not yet resolved. 
(Assignee)

Comment 5

10 years ago
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_ldapcertstore.c,v  <--  pkix_pl_ldapcertstore.c
new revision: 1.5; previous revision: 1.4
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.