pkix_pl_HttpCertStore_ProcessCertResponse is unable to process certs in DER format

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P1
enhancement
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Alexei Volkov, Assigned: Alexei Volkov)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PKIX NSS312B2)

Attachments

(1 attachment, 4 obsolete attachments)

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

Description

10 years ago
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.
(Assignee)

Updated

10 years ago
Depends on: 395090
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.  
Priority: -- → P1
(Assignee)

Updated

10 years ago
Whiteboard: PKIX

Comment 2

10 years ago
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 ?
Whiteboard: PKIX → PKIX NSS312B2
Version: 3.12 → trunk
(Assignee)

Comment 3

10 years ago
Yes, Julien, we did. www.wyomingatwork.com is one of many that do so.
(Assignee)

Comment 4

10 years ago
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.
Attachment #290624 - Flags: review?(nelson)
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?
(Assignee)

Comment 6

10 years ago
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.
Attachment #290624 - Attachment is obsolete: true
Attachment #291459 - Flags: review?(nelson)
Attachment #290624 - Flags: review?(nelson)
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.
Attachment #291459 - Flags: review?(nelson) → review-
(Assignee)

Comment 8

10 years ago
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.
(Assignee)

Comment 9

10 years ago
Created attachment 292518 [details] [diff] [review]
Patch v3

Make so that PKIX_Error is reported to the caller( pkix_pl_HttpCertStore_ProcessCertResponse function).
Attachment #291459 - Attachment is obsolete: true
Attachment #292518 - Flags: review?(nelson)
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.
Attachment #292518 - Flags: review?(nelson) → review-
(Assignee)

Comment 11

10 years ago
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.
Attachment #292518 - Attachment is obsolete: true
Attachment #293231 - Flags: review?(nelson)
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.
(Assignee)

Comment 13

10 years ago
Created attachment 293546 [details] [diff] [review]
Same v4 patch. Diff from nss directory.
Attachment #293231 - Attachment is obsolete: true
Attachment #293546 - Flags: review?(nelson)
Attachment #293231 - Flags: review?(nelson)
Depends on: 408711
(Assignee)

Updated

10 years ago
Blocks: 408434
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
Attachment #293546 - Flags: review?(nelson) → review+
(Assignee)

Comment 15

9 years ago
integrated
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.