Persona is no longer an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 395093 - pkix_pl_HttpCertStore_ProcessCertResponse is unable to process certs in DER format
: pkix_pl_HttpCertStore_ProcessCertResponse is unable to process certs in DER f...
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: All All
: P1 enhancement (vote)
: 3.12
Assigned To: Alexei Volkov
Depends on: 395090 408711
Blocks: 408434
  Show dependency treegraph
Reported: 2007-09-05 16:00 PDT by Alexei Volkov
Modified: 2008-01-10 22:25 PST (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Patch v1. (4.46 KB, patch)
2007-11-28 17:17 PST, Alexei Volkov
no flags Details | Diff | Splinter Review
Patch v2 (4.68 KB, patch)
2007-12-04 10:18 PST, Alexei Volkov
nelson: review-
Details | Diff | Splinter Review
Patch v3 (6.95 KB, patch)
2007-12-10 17:40 PST, Alexei Volkov
nelson: review-
Details | Diff | Splinter Review
Patch v4. (8.60 KB, patch)
2007-12-14 16:01 PST, Alexei Volkov
no flags Details | Diff | Splinter Review
Same v4 patch. Diff from nss directory. (8.61 KB, patch)
2007-12-17 11:36 PST, Alexei Volkov
nelson: review+
Details | Diff | Splinter Review

Description Alexei Volkov 2007-09-05 16:00:33 PDT
This is the limitation of pkix_pl_HttpCertStore_ProcessCertResponse. It refuses process http cert fetch responses that does not contain data in pkcs7-mime time.

Instead of the current code, the function should call CERT_DecodeCertPackage function that implemented in nss/lib/pkcs7/certread.c file. The function can handle data in DER and pkcs7 formats.
Comment 1 Nelson Bolyard (seldom reads bugmail) 2007-09-05 16:14:31 PDT
The present behavior seems EXACTLY backwards.  Since all the code needed to
handle DER is in libNSS already, and the PKCS7 code is not, the PKIX code
should handle DER certs without any problem.  It should be CMS (formerly
known as PKCS7) format that causes the problem.  

Please start by fixing the code so that it handles DER certs without needing
libSMIME or code duplicated from libSMIME.  
Comment 2 Julien Pierre 2007-10-17 17:30:19 PDT
I believe there was a spec somewhere that described that the certs were supposed to be packaged in PKCS7, not just plain DER. I can't remember where. But Richard was looking into it and this is what we had concluded.

Have we found servers that have SIAs that point to HTTP servers that serve plain DER not in PKCS#7 format ?
Comment 3 Alexei Volkov 2007-11-28 17:12:10 PST
Yes, Julien, we did. is one of many that do so.
Comment 4 Alexei Volkov 2007-11-28 17:17:58 PST
Created attachment 290624 [details] [diff] [review]
Patch v1.

Now that we use original CERT_DecodeCertPackage to decode packages, we can drop requirement to only have  pkcs7-mime package. The function is also capable to process plane der certs.
Comment 5 Nelson Bolyard (seldom reads bugmail) 2007-12-03 22:39:39 PST
Comment on attachment 290624 [details] [diff] [review]
Patch v1.

Alexei, with the patch in place, function certCallback looks like this:

> static SECStatus
> certCallback(void *arg, SECItem **secitemCerts, int numcerts)
> {
>         int certsFound = 0;
>         int itemNum = 0;
>         for (; itemNum < numcerts; itemNum++ ) {
>                 error = pkix_pl_Cert_CreateToList(secitemCerts[itemNum],
>                                                   pkixCertList, plContext);
>                 certsFound++;
>                 if (error != NULL) {
>                         PKIX_PL_Object_DecRef
>                                 ((PKIX_PL_Object *)error, plContext);
>                 }
>         }
>         return ((certsFound == numcerts)?SECSuccess:SECFailure);
> }

It appears to me that there is no way for this loop to end prematurely.
There is no break, no goto, no adjusting the loop counter.
Consequently, at the end of this loop, the expression (certsFound == numcerts)
MUST always be true, no?
If that is so, then certsFound serves no purpose.

Should we perhaps only increment certsFound if !error?
Comment 6 Alexei Volkov 2007-12-04 10:18:31 PST
Created attachment 291459 [details] [diff] [review]
Patch v2

Good catch. Propose to continue through out DER cert list regardless to the decoding errors of a particular cert(since we already downloaded certs we better use them for the future validations). Increment certFound count only upon successful decoding.
Comment 7 Nelson Bolyard (seldom reads bugmail) 2007-12-04 14:02:42 PST
Comment on attachment 291459 [details] [diff] [review]
Patch v2

Any function that returns a secstatus must ensure that an NSS error code has been set if/when it returns SECFailure.  Otherwise the caller gets some random error code that misdirects diagnostics.  So, please set an appropriate NSS error code when returning SECFailure.
Comment 8 Alexei Volkov 2007-12-10 16:43:56 PST
I think that in this particular case we should pass received pointer to PKIX_Error to the caller. Later, PKIX_Error stack will be evaluated and NSS error code will be set. Will do appropriate changes.
Comment 9 Alexei Volkov 2007-12-10 17:40:54 PST
Created attachment 292518 [details] [diff] [review]
Patch v3

Make so that PKIX_Error is reported to the caller( pkix_pl_HttpCertStore_ProcessCertResponse function).
Comment 10 Nelson Bolyard (seldom reads bugmail) 2007-12-12 19:18:42 PST
Comment on attachment 292518 [details] [diff] [review]
Patch v3

r-.  I went over this with Alexei yesterday

1. In pkix_pl_HttpCertStore_ProcessCertResponse, cbContext is not
initialized until after potentially 3 calls to PKIX_ERROR, all of
which potentially goto cleanup.  If any of them does goto cleanup,
then cbContext.pkixCertList will be uninitialized at that point.
Please just initialize all of cbContext before getting to the first

2. Maybe pkix_pl_HttpCertStore_DecodeCertPackage shuld be wrapped with
the macro that only catches fatal errors.  Otherwise the code that
follows it will be dead code, I think.
Comment 11 Alexei Volkov 2007-12-14 16:01:19 PST
Created attachment 293231 [details] [diff] [review]
Patch v4.

* Initialize cbContext before first check for an error.
* Check for only fatal errors of pkix_pl_HttpCertStore_DecodeCertPackage or errors that returned as a part of cbContext - fatal errors from certCallback.
Comment 12 Nelson Bolyard (seldom reads bugmail) 2007-12-15 13:19:13 PST
Alexei, bugzilla is unable to diff patch v4 against patch v3.
Maybe this is because the two diffs have different base directories (e.g. 
Index: lib/libpkix/pkix_pl_nss/module/pkix_pl_httpcertstore.c  vs. 
Index: libpkix/pkix_pl_nss/module/pkix_pl_httpcertstore.c
Please make a patch that bugzilla can diff against v3.  Thanks.
Comment 13 Alexei Volkov 2007-12-17 11:36:54 PST
Created attachment 293546 [details] [diff] [review]
Same v4 patch. Diff from nss directory.
Comment 14 Nelson Bolyard (seldom reads bugmail) 2008-01-09 14:06:16 PST
Comment on attachment 293546 [details] [diff] [review]
Same v4 patch. Diff from nss directory.

Please fix this comment:
/* If decRef received, treat it as a fatal error.

I think you meant: "If decRef FAILED ..."

then r=nelson
Comment 15 Alexei Volkov 2008-01-10 22:25:23 PST

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