1255014 - CERT_DecodeCertPackage reads only a single certificate from supplied PEM data

Status: NEW

(NSS :: Libraries, defect, P3)

Description

[Mark Goodwin [:mgoodwin]]

The comments for CERT_DecodeCertPackage suggest that a whole chain will be read from the supplied package. This is the case when a cert sequence is passed in - but not when multiple individual PEM encoded certificates are provided.

Comment 1

[Mark Goodwin [:mgoodwin]]

If / when this is resolved, we should remove the PEM parsing bits of bug 1252882.

Comment 2

[Franziskus Kiefer [:franziskus]]

Attachment: CERT_DecodeCertPackage.patch

This adds a simple gtest for CERT_DecodeCertPackage with DER and PEM certificate chains. It further makes CERT_DecodeCertPackage parse certificate chains in PEM. 

While this changes behaviour of a public API it should be acceptable as it is what the function advertises and consumers should be easily able to adapt to this change if necessary.

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8732171 [details] [diff] [review]
CERT_DecodeCertPackage.patch

Review of attachment 8732171 [details] [diff] [review]:
-----------------------------------------------------------------

::: external_tests/pkcs7_gtest/pkcs7_CERT_DecodeCertPackage_unittest.cc
@@ +782,5 @@
> +
> +class PKC7DecodeCertPackageTest : public ::testing::Test {
> +
> +public:
> +  void TestDecodeCertPackage(char* data, uint32_t len, int expectedNumCerts, const char **expectedSubjects) {

You can (and probably should) pass the subjects as a std::vector, then you get size and length together.  Initializing is less pleasant, but it reads better.

Accepting a SECItem for data+len might also make it easier.  (Lengths should be size_t, but I see that we're constrained here by the NSS API.)

@@ +812,5 @@
> +      if (certChain->numcerts != expectedNumCerts) {
> +        return;
> +      }
> +      // get the actual certs from certChain and compare the subject
> +      while(expectedNumCerts--) {

did you run clang-format?

@@ +828,5 @@
> +    CERTCertificate *aCert =
> +        CERT_DecodeCertFromPackage((char *)currItem->data, currItem->len);
> +    if (!aCert) {
> +      // we were not able to construct the cert
> +      EXPECT_TRUE(false);

You can do this:

EXPECT_TRUE(false) << "unable to construct a certificate";

@@ +831,5 @@
> +      // we were not able to construct the cert
> +      EXPECT_TRUE(false);
> +      return;
> +    }
> +    // printf(" >>> Subject: %s\n", CERT_NameToAscii(&aCert->subject));

remove please

I think that EXPECT_EQ accepts string arguments, which would be a good way to address this debugging complaint.  You might have to use std::string(CERT_NameToAscii(...)) though.

::: external_tests/pkcs7_gtest/pkcs7_gtest.cc
@@ +1,1 @@
> +#include "nspr.h"

Is it possible to link the same object file with different sets of objects to produce binaries?  This is just duplicating code from other directories now.

::: lib/pkcs7/certread.c
@@ +396,2 @@
>          rv = SECFailure;
> +        goto loser;

This diff is hard to follow.  Is rietveld easier?

(I'm not well qualified to review these changes, so let me know if you need a good review of these, I don't have time to check this for correctness now.)

@@ +401,5 @@
> +
> +    cp = (unsigned char*)certChain;
> +
> +    /* walking through certificates */
> +    while(cl) {

clang-format?

@@ +454,5 @@
> +        /* we found a cert, let's read it */
> +        if (certbegin && certend && certCount < 32) {
> +            unsigned int binLen;
> +            unsigned char *decodedCert;
> +            SECItem *certSecItem = (SECItem *)PORT_Alloc(sizeof(SECItem));

Don't allocate on the heap like this unless you have to (and you don't).  Just allocate binaryCerts as an array of SECItem objects.

As it is, you leak these because you pass false to SECITEM_FreeItem below.

Comment 4

[Franziskus Kiefer [:franziskus]]

Attachment: CERT_DecodeCertPackage.patch

Thanks for the comments mt. I also put it up here https://codereview.appspot.com/291320043. Let's hope Wan-Teh can review the certread changes.

> ::: external_tests/pkcs7_gtest/pkcs7_gtest.cc
> @@ +1,1 @@
> > +#include "nspr.h"
> 
> Is it possible to link the same object file with different sets of objects to produce binaries?  This is just duplicating code from other directories now.

Bug 1242565 tracks reworking the way we run gtests (not that anything happened there, but I'll get on to this next week). If anything lands there before this one, I can change this. Otherwise we shouldn't mix things here.

> > +        /* we found a cert, let's read it */
> > +        if (certbegin && certend && certCount < 32) {
> > +            unsigned int binLen;
> > +            unsigned char *decodedCert;
> > +            SECItem *certSecItem = (SECItem *)PORT_Alloc(sizeof(SECItem));
> 
> Don't allocate on the heap like this unless you have to (and you don't).  Just allocate binaryCerts as an array of SECItem objects.

I'm not sure if that's possible. We have to hand a SECItem** to the callback (at least if we don't want to break compatibility). So taking an array of SECItem objects doesn't work.

> As it is, you leak these because you pass false to SECITEM_FreeItem below.
right, that should be true.

Comment 5

[Martin Thomson [:mt:]]

Comment on attachment 8737205 [details] [diff] [review]
CERT_DecodeCertPackage.patch

Review of attachment 8737205 [details] [diff] [review]:
-----------------------------------------------------------------

Please, when you are writing new code like this, consider the person coming after you.

::: external_tests/manifest.mn
@@ +11,5 @@
> +    util_gtest \
> +    pkcs7_gtest \
> +    pk11_gtest \
> +    ssl_gtest \
> +    $(NULL)

You reindented all of these for no reason, but failed to remote the trailing whitespace on line 1.

::: external_tests/pkcs7_gtest/pkcs7_CERT_DecodeCertPackage_unittest.cc
@@ +753,5 @@
> +const char* subj9 = "CN=VeriSign Universal Root Certification Authority,OU=\"(c) 2008 VeriSign, Inc. - For authorized use only\",OU=VeriSign Trust Network,O=\"VeriSign, Inc.\",C=US";
> +const char* subjects2[] = {subj1, subj2, subj3, subj4, subj5, subj6, subj7, subj8, subj9};
> +
> +/* helper function to collect certs from a cert chain */
> +SECStatus collect_certs(void *arg, SECItem **certs, int numcerts) {

Why not SECItemArray *certs ?

@@ +764,5 @@
> +  collectArgs->numcerts = numcerts;
> +  collectArgs->rawCerts = (SECItem *)PORT_ArenaZAlloc(
> +      collectArgs->arena, sizeof(SECItem) * numcerts);
> +  if (!collectArgs->rawCerts)
> +    return (SECFailure);

Drop the parentheses.  And my preference is to use braces for all if() statements.

@@ +787,5 @@
> +                             const std::vector<const char *> subjects) {
> +    PLArenaPool *arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
> +    if (!arena) {
> +      // we didn't get memory so something is really wrong.
> +      EXPECT_TRUE(false) << "we didn't get memory so something is really wrong";

No need for comments that duplicate the statement.

@@ +834,5 @@
> +      EXPECT_TRUE(false) << "unable to construct a certificate";
> +      return;
> +    }
> +    EXPECT_TRUE(strncmp(CERT_NameToAscii(&aCert->subject), expected,
> +                        strlen(expected)) == 0)

This doesn't test that the subject name is the same length as what was expected.

Why aren't you using EXPECT_EQ() ?

@@ +842,5 @@
> +};
> +
> +/* DER encoded */
> +TEST_F(PKC7DecodeCertPackageTest, TestGoodDERCertChain) {
> +  const std::vector<const char *> subjs = {&subjects1[0], &subjects1[0] + 3};

don't use a constant like `+ 3`.  PR_ARRAY_SIZE should work well enough.

::: lib/pkcs7/certread.c
@@ +224,5 @@
>      SECItem der;
>  
>      rv = ATOB_ConvertAsciiToItem(&der, certstr);
>      if (rv != SECSuccess)
> +        return NULL;

Did you convert tabs to spaces in this file?  I am finding it hard to find the changed code with the gratuitous whitespace changes.

@@ +245,5 @@
>  SECStatus
>  CERT_DecodeCertPackage(char *certbuf,
> +                       int certlen,
> +                       CERTImportCertificateFunc f,
> +                       void *arg)

I can't review this.

https://en.wikipedia.org/wiki/Cyclomatic_complexity

@@ +443,5 @@
> +                /* skip all blank lines */
> +                while (cl && (*cp == '\n' || *cp == '\r')) {
> +                    cp++;
> +                    cl--;
> +                }

Please avoid code duplication.