1035009 - Stop using CERTCertList in mozilla::pkix

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: stop-using-CERTCertList.patch

The major change in this patch is that BuildCertChain no longer has an output parameter that contains the built chain on success. Instead, if the application wants the constructed chain, then it can save the chain in its IsChainValid method of its TrustDomain. Some uses (especially some anticipated uses outside of Firefox, and also many of our tests) don't need to allocate a CERTCertList for the result. One secondary goal I have is to eliminate all memory allocation that occurs within mozilla::pkix, as part of an effort to ensure safety against use-after-free and similar allocation/deallocation failures.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8451395 [details] [diff] [review]
stop-using-CERTCertList.patch

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

This works, but it seems a little fragile. We should probably guarantee that IsChainValid won't be called again after it has returned SECSuccess once (although, I think the only way of doing this would be to add state to each TrustDomain implementation). We should also document how getting the resulting chain is expected to work in pkix.h or pkixtypes.h or something. In particular, that it's possible to get a handle on the resulting chain in IsChainValid yet have revocation checking fail, causing BuildCertChain to fail.

::: security/apps/AppTrustDomain.cpp
@@ +209,5 @@
> +  return ConstructCERTCertListFromReversedDERArray(certChain, mCertChain);
> +}
> +
> +} } // namespace mozilla::psm
> +

nit: this last blank line is probably unnecessary

::: security/certverifier/CertVerifier.h
@@ +32,5 @@
>                         void* pinArg,
>                         const char* hostname,
>                         Flags flags = 0,
>         /*optional in*/ const SECItem* stapledOCSPResponse = nullptr,
> +      /*optional out*/ ScopedCERTCertList* validationChain = nullptr,

Looks like this is now called "builtChain" in the implementation - it would be nice to be consistent.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +572,4 @@
>         mCheckChainCallback));
>  
> +  if (!mBuiltChain && !mCheckChainCallback) {
> +    // No need to create a CERTCertList for builtChain.

mBuiltChain?

::: security/manager/ssl/src/ScopedNSSTypes.h
@@ +90,4 @@
>  MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE(ScopedCERTCertList,
>                                            CERTCertList,
>                                            CERT_DestroyCertList)
> +// defined in nsNSSCertificate.cpp

Maybe this should be declared in nsNSSCertificate.h

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +1507,5 @@
>  }
>  
> +namespace mozilla {
> +
> +// XXX: It seems like we only construct CERTCertLists for the purpose of

File a bug? Maybe Cykesiopka or Harsh can work on it.

@@ +1526,5 @@
> +  size_t i = certArray.GetLength();
> +  while (i > 0) {
> +    --i;
> +    SECItem* certDER(const_cast<SECItem*>(certArray.GetDER(i)));
> +    ScopedCERTCertificate cert(CERT_NewTempCertificate(certDB, certDER,

We need to document that this function can only be called in between NSS init/shutdown, right?

@@ +1533,5 @@
> +      return SECFailure;
> +    }
> +    // certArray is ordered with the root first, but we want the resulting
> +    // certList to have the root last.
> +    if (CERT_AddCertToListTail(certList, cert) != SECSuccess) {

Why not loop over the array with an increasing index and use CERT_AddCertToListHead?

@@ +1536,5 @@
> +    // certList to have the root last.
> +    if (CERT_AddCertToListTail(certList, cert) != SECSuccess) {
> +      return SECFailure;
> +    }
> +    cert.forget(); // cert is now owned by *mBuiltChain

certList, not mBuiltChain

::: security/pkix/lib/pkixbuild.cpp
@@ +241,5 @@
> +      Result rv = chain.Append(cert->GetDER());
> +      if (rv != Success) {
> +        // If we have this PR_NOT_REACHED here, the
> +        // pkixbuild.MaxAcceptableCertChainLength test cannot run in debug
> +        // mode.

Is this comment still accurate? We could just increase NonOwningDERArray::MAX_LENGTH, right?

@@ +261,5 @@
>    if (endEntityOrCA == EndEntityOrCA::MustBeCA) {
>      // Avoid stack overflows and poor performance by limiting cert chain
>      // length.
>      static const unsigned int MAX_SUBCA_COUNT = 6;
> +    static_assert(1 + MAX_SUBCA_COUNT + 1 == NonOwningDERArray::MAX_LENGTH,

We should probably document the "1 + MAX_SUBCA_COUNT + 1". I'm assuming 1 for end-entity, 1 for the root?

::: security/pkix/test/gtest/pkixbuild_tests.cpp
@@ +162,5 @@
>    {
>      return SECSuccess;
>    }
>  
> +  virtual SECStatus IsChainValid(const DERArray&)

See the comment on the IsChainValid in pkixcert_extension_tests.cpp regarding the expected length of the DERArray.

::: security/pkix/test/gtest/pkixcert_extension_tests.cpp
@@ +102,5 @@
>    {
>      return SECSuccess;
>    }
>  
> +  virtual SECStatus IsChainValid(const DERArray&)

Are all of the successful test chains we come up with going to be of length 2? If so, we should probably ASSERT_EQ(2, derArray.GetLength()) to give us some confidence we're getting something like what we're expecting.

@@ +181,3 @@
>                                   &unknownNonCriticalExtension, key));
>    ASSERT_TRUE(cert);
>    ScopedCERTCertList results;

Shouldn't this be removed as well?

@@ +209,5 @@
>    };
>    const char* certCN = "CN=Cert With Critical Wrong OID Extension";
>    ScopedSECKEYPrivateKey key;
>    // cert is owned by the arena
>    const SECItem* cert(CreateCert(arena.get(), certCN, 

if you feel like addressing it, there's a trailing space on this line

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

Hey Garrett, all your dreams are coming true.

Comment 3

[Garrett Robinson [:grobinson]]

brb reevaluating my dreams

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8451395 [details] [diff] [review]
stop-using-CERTCertList.patch

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

::: security/apps/AppTrustDomain.cpp
@@ +209,5 @@
> +  return ConstructCERTCertListFromReversedDERArray(certChain, mCertChain);
> +}
> +
> +} } // namespace mozilla::psm
> +

nixed.

::: security/certverifier/CertVerifier.h
@@ +32,5 @@
>                         void* pinArg,
>                         const char* hostname,
>                         Flags flags = 0,
>         /*optional in*/ const SECItem* stapledOCSPResponse = nullptr,
> +      /*optional out*/ ScopedCERTCertList* validationChain = nullptr,

fixed.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +572,4 @@
>         mCheckChainCallback));
>  
> +  if (!mBuiltChain && !mCheckChainCallback) {
> +    // No need to create a CERTCertList for builtChain.

fixed.

::: security/manager/ssl/src/ScopedNSSTypes.h
@@ +90,4 @@
>  MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE(ScopedCERTCertList,
>                                            CERTCertList,
>                                            CERT_DestroyCertList)
> +// defined in nsNSSCertificate.cpp

OK. But, now interdiff won't work!

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +1507,5 @@
>  }
>  
> +namespace mozilla {
> +
> +// XXX: It seems like we only construct CERTCertLists for the purpose of

bug 1036065.

@@ +1526,5 @@
> +  size_t i = certArray.GetLength();
> +  while (i > 0) {
> +    --i;
> +    SECItem* certDER(const_cast<SECItem*>(certArray.GetDER(i)));
> +    ScopedCERTCertificate cert(CERT_NewTempCertificate(certDB, certDER,

We should be assuming that about any function that takes CERT* parameters already.

@@ +1533,5 @@
> +      return SECFailure;
> +    }
> +    // certArray is ordered with the root first, but we want the resulting
> +    // certList to have the root last.
> +    if (CERT_AddCertToListTail(certList, cert) != SECSuccess) {

Done.

@@ +1536,5 @@
> +    // certList to have the root last.
> +    if (CERT_AddCertToListTail(certList, cert) != SECSuccess) {
> +      return SECFailure;
> +    }
> +    cert.forget(); // cert is now owned by *mBuiltChain

Fixed.

::: security/pkix/lib/pkixbuild.cpp
@@ +241,5 @@
> +      Result rv = chain.Append(cert->GetDER());
> +      if (rv != Success) {
> +        // If we have this PR_NOT_REACHED here, the
> +        // pkixbuild.MaxAcceptableCertChainLength test cannot run in debug
> +        // mode.

I removed the comment. Originally I added the comment because I misunderstood my own code, and once I fixed the bug that was causing the test failure, I put the PR_NOT_REACHED back but forgot to remove the comment.

@@ +261,5 @@
>    if (endEntityOrCA == EndEntityOrCA::MustBeCA) {
>      // Avoid stack overflows and poor performance by limiting cert chain
>      // length.
>      static const unsigned int MAX_SUBCA_COUNT = 6;
> +    static_assert(1 + MAX_SUBCA_COUNT + 1 == NonOwningDERArray::MAX_LENGTH,

done.

::: security/pkix/test/gtest/pkixbuild_tests.cpp
@@ +162,5 @@
>    {
>      return SECSuccess;
>    }
>  
> +  virtual SECStatus IsChainValid(const DERArray&)

I disagree, because this is not what  this test is testing. If we want to test that IsChainValid is passed the correct and complete chain in the correct order, it makes sense to ask for a new test, but we shouldn't obfuscate a test of a different thing.

::: security/pkix/test/gtest/pkixcert_extension_tests.cpp
@@ +102,5 @@
>    {
>      return SECSuccess;
>    }
>  
> +  virtual SECStatus IsChainValid(const DERArray&)

See my previous comment. If we want to test what we're passing to IsChainValid then we can do so with a separate test. If you want to block landing this patch on that new test, we can do so, though I don't think it is strictly* necessary because we already have the test_getChains.js test for this.

*Is there any other kind of necessary?

@@ +181,3 @@
>                                   &unknownNonCriticalExtension, key));
>    ASSERT_TRUE(cert);
>    ScopedCERTCertList results;

Done.

@@ +209,5 @@
>    };
>    const char* certCN = "CN=Cert With Critical Wrong OID Extension";
>    ScopedSECKEYPrivateKey key;
>    // cert is owned by the arena
>    const SECItem* cert(CreateCert(arena.get(), certCN, 

I already fixed this in the patch I checked in for bug 1035008.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: stop-using-CERTCertList.patch

Asking for re-review since I changed so much stuff. I will highlight some changes to pay particular attention to.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8452654 [details] [diff] [review]
stop-using-CERTCertList.patch

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

::: security/certverifier/CertVerifier.cpp
@@ +408,5 @@
>                                    PRTime time,
>                       /*optional*/ void* pinarg,
>                                    const char* hostname,
>                                    bool saveIntermediatesInPermanentDatabase,
> +                 /*optional out*/ ScopedCERTCertList* builtChain,

In addition to addressing your review comments, I made the naming of builtChain consistent with VerifyCert.

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +1523,5 @@
> +
> +  CERTCertDBHandle* certDB(CERT_GetDefaultCertDB()); // non-owning
> +
> +  size_t numCerts = certArray.GetLength();
> +  for (size_t i = 0; i < numCerts; ++i) {

As you ordered.

::: security/pkix/include/pkix/pkixtypes.h
@@ +226,5 @@
> +  // which case the application must not assume anything about the validity of
> +  // the last certificate chain passed to IsChainValid; especially, it would be
> +  // very wrong to assume that the certificate chain is valid.
> +  //
> +  // certChain.GetDER(0) is the trust anchor.

The overview comment in your review indicated quite a bit of uncertainty about IsChainValid. Let me know if this documentation is sufficient to clarify things.

::: security/pkix/test/gtest/pkixcert_extension_tests.cpp
@@ +103,5 @@
>    {
>      return SECSuccess;
>    }
>  
> +  virtual SECStatus IsChainValid(const DERArray&)

Let me know if you think we need unit tests for IsChainValid, or whether the test_GetChain.js tests are good enough. I could go either way.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8452654 [details] [diff] [review]
stop-using-CERTCertList.patch

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

I reviewed a diff of the patches, which seemed to work. Looks good to me. I don't think we need additional IsChainValid tests at the moment.

::: security/manager/ssl/src/nsNSSCertificate.h
@@ +79,5 @@
>  {
>    static const bool value = true;
>  };
> +
> +// defined in nsNSSCertificate.cpp

not sure this comment is necessary anymore

@@ +81,5 @@
>  };
> +
> +// defined in nsNSSCertificate.cpp
> +SECStatus ConstructCERTCertListFromReversedDERArray(
> +            const mozilla::pkix::DERArray&,

maybe it would be nice to give this parameter a name? (even though it is pretty self-explanatory...)

::: security/pkix/include/pkix/pkixtypes.h
@@ +226,5 @@
> +  // which case the application must not assume anything about the validity of
> +  // the last certificate chain passed to IsChainValid; especially, it would be
> +  // very wrong to assume that the certificate chain is valid.
> +  //
> +  // certChain.GetDER(0) is the trust anchor.

Great - this is just what I was looking for.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ed88d692f42

Thanks for the reviews!

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0ed88d692f42