395093 - pkix_pl_HttpCertStore_ProcessCertResponse is unable to process certs in DER format

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Alexei Volkov]

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)]

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]

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]

Yes, Julien, we did. www.wyomingatwork.com is one of many that do so.

Comment 4

[Alexei Volkov]

Attachment: 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)]

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]

Attachment: 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)]

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]

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]

Attachment: Patch v3

Make so that PKIX_Error is reported to the caller( pkix_pl_HttpCertStore_ProcessCertResponse function).

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

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
NULLCHECK.

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]

Attachment: 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)]

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]

Attachment: Same v4 patch. Diff from nss directory.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

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]

integrated