Last Comment Bug 407064 - pkix_pl_LdapCertStore_BuildCrlList should not fail if a crl fails to be decoded
: pkix_pl_LdapCertStore_BuildCrlList should not fail if a crl fails to be decoded
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.12
: All All
: P2 normal (vote)
: 3.12
Assigned To: Alexei Volkov
Depends on:
  Show dependency treegraph
Reported: 2007-12-05 15:39 PST by Alexei Volkov
Modified: 2007-12-10 16:04 PST (History)
0 users
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Patch v1 (1.73 KB, patch)
2007-12-05 15:39 PST, Alexei Volkov
nelson: review+
Details | Diff | Splinter Review

Description Alexei Volkov 2007-12-05 15:39:41 PST
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.
Comment 1 Alexei Volkov 2007-12-05 15:52:47 PST
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 2 Nelson Bolyard (seldom reads bugmail) 2007-12-05 18:37:13 PST
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 3 Nelson Bolyard (seldom reads bugmail) 2007-12-06 13:42:05 PST
Comment on attachment 291775 [details] [diff] [review]
Patch v1

r=nelson, once the syntax error is fixed.
Comment 4 Alexei Volkov 2007-12-10 16:02:07 PST
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. 
Comment 5 Alexei Volkov 2007-12-10 16:04:23 PST
/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

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