Closed Bug 1434936 Opened 3 years ago Closed 2 years ago

Use utility functions for certlist segmentation in NSSCertDBTrustDomain::IsChainValid

Categories

(Core :: Security: PSM, enhancement, P1)

60 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: jcj, Assigned: jcj)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

I believe that mozilla::pkix should use utility functions wherever possible to do cert chain segmentation and iteration, rather than having all these CERT_LIST_HEAD, CERT_LIST_NEXT iterators... so I wrote some nice ones a couple months ago in the nsNSSCertList in Bug 1411683.

This bug is to adapt NSSCertDBTrustDomain::IsChainValid and methods it depends
on to use the nsNSSCertList rather than the raw NSS CERTCertList objects.

This is so that I can use those methods for the Symantec Distrust in Bug 1434300 which needs to be patched into IsChainValid.
It looks like I need to use GetSucceededCertChain in this patch set, which gives this a dependency on Bug 731478. I might be able to code around it, so this may change.
Depends on: 731478
Comment on attachment 8947483 [details]
Bug 1434936 - Add method nsNSSCertList::GetRootCertificate

https://reviewboard.mozilla.org/r/217190/#review223382

I think this is a good approach - f+

::: commit-message-9746e:5
(Diff revision 1)
> +Bug 1434936 - Add method nsNSSCertList::GetRootCertificate r?keeler r?fkiefer
> +
> +This adds another utility method to nsNSSCertList to perform CERT_LIST_TAIL
> +on the underlying certificate list and return the last entry -- e.g., the root.
> +This is a convenience method to let other parts of mozilla::pkix continue to

maybe s/mozilla::pkix/the certificate verifier/ ?

::: security/manager/ssl/nsNSSCertificate.h:116
(Diff revision 1)
>    // `aIntermediates` must be empty.
>    nsresult SegmentCertificateChain(/* out */ nsCOMPtr<nsIX509Cert>& aRoot,
>                             /* out */ nsCOMPtr<nsIX509CertList>& aIntermediates,
>                             /* out */ nsCOMPtr<nsIX509Cert>& aEndEntity);
>  
> +  // Obtain the root certificate of a certificate chain. This method does so

Might mention that it removes it from the list, rather than copying (is this really what we want?)

::: security/manager/ssl/tests/gtest/CertListTest.cpp:407
(Diff revision 1)
> +  ASSERT_EQ(rv, NS_OK) << "Should have loaded OK";
> +  rv = AddCertFromStringToList(kCaPem, certList);
> +  ASSERT_EQ(rv, NS_OK) << "Should have loaded OK";
> +
> +  nsCOMPtr<nsIX509Cert> rootCert;
> +  rv = certList->GetRootCertificate(rootCert);

Maybe check that the list is empty now? (again, if that's the behavior we want)
Attachment #8947483 - Flags: review?(dkeeler)
Comment on attachment 8947484 [details]
Bug 1434936 - Use nsNSSCertList in NSSCertDBTrustDomain::IsChainValid

https://reviewboard.mozilla.org/r/217192/#review223384

::: security/certverifier/NSSCertDBTrustDomain.cpp:791
(Diff revision 1)
>    }
>  
> -  CERTCertListNode* rootNode = CERT_LIST_TAIL(certList);
> -  if (!rootNode) {
> +  // Modernization in-progress: Keep certList as a CERTCertList for storage into
> +  // the mBuiltChain variable at the end, but let's use nsIX509CertList for the
> +  // validity calculations.
> +  nsNSSShutDownPreventionLock lock;

You'll need to rebase this on top of bug 1421084 (fingers crossed that doesn't get backed out...)

::: security/certverifier/NSSCertDBTrustDomain.cpp:797
(Diff revision 1)
> +  UniqueCERTCertList certChainCopy =
> +    nsNSSCertList::DupCertList(certList, lock);
> +
> +  nsCOMPtr<nsIX509CertList> x509CertList;
> +  // This adopts the list
> +  x509CertList = new nsNSSCertList(Move(certChainCopy), lock);

nit: we can do declare/assign in one line

::: security/certverifier/NSSCertDBTrustDomain.cpp:817
(Diff revision 1)
> +  UniqueCERTCertificate root(rootCert->GetCert());
>    if (!root) {
>      return Result::FATAL_ERROR_LIBRARY_FAILURE;
>    }
>    bool isBuiltInRoot = false;
> -  rv = IsCertBuiltInRoot(root, isBuiltInRoot);
> +  rv = IsCertBuiltInRoot(root.get(), isBuiltInRoot);

Let's use rootCert->GetIsBuiltInRoot

(note that there's a bit of an inefficiency here - nsNSSCertificate::GetIsBuiltInRoot calls IsCertBuiltInRoot with the underlying CERTCertificate, which in debug builds calls nsNSSComponent::IsCertTestBuiltInRoot, which constructs a new nsNSSCertificate :(
)
Attachment #8947484 - Flags: review?(dkeeler)
Comment on attachment 8947485 [details]
Bug 1434936 - Rework ChainHasValidPins to use nsNSSCertList

https://reviewboard.mozilla.org/r/217194/#review223390

::: security/manager/ssl/PublicKeyPinningService.cpp:259
(Diff revision 1)
>      return NS_ERROR_INVALID_ARG;
>    }
>  
> +  // This is a temporary duplication until EvalChain is rewritten to use
> +  // nsNSSCertList.
> +  nsNSSShutDownPreventionLock lock;

Again, bug 1421084.

::: security/manager/ssl/PublicKeyPinningService.cpp:264
(Diff revision 1)
> +  nsNSSShutDownPreventionLock lock;
> +  UniqueCERTCertList tempCertList =
> +    UniqueCERTCertList(certList->GetRawCertList());
> +  UniqueCERTCertList rawCertList =
> +    nsNSSCertList::DupCertList(tempCertList, lock);
> +  tempCertList.release();

If I'm reading things correctly, the result of nsNSSCertList::GetRawCertList needs to be stored in a *non-owning* type (it just returns the raw pointer without any addref-ing)
(Note that yes, this is different from nsNSSCertificate::GetCert, which *does* need an owner. Yaaaay for consistency.)

::: security/manager/ssl/PublicKeyPinningService.cpp:280
(Diff revision 1)
>    if (dynamicFingerprints.Length() == 0 && !staticFingerprints) {
>      chainHasValidPins = true;
>      return NS_OK;
>    }
>    if (dynamicFingerprints.Length() > 0) {
> -    return EvalChain(certList, nullptr, &dynamicFingerprints, chainHasValidPins);
> +    return EvalChain(rawCertList, nullptr, &dynamicFingerprints, chainHasValidPins);

We can't just use tempCertList directly?

::: security/manager/ssl/PublicKeyPinningService.cpp:328
(Diff revision 1)
> +      return rv;
> +    }
> +
>      // Only log telemetry if the certificate list is non-empty.
> -    if (!CERT_LIST_END(rootNode, certList)) {
> -      if (!enforceTestModeResult && pinningTelemetryInfo) {
> +    if (rootCert && !enforceTestModeResult && pinningTelemetryInfo) {
> +      CERTCertificate* rootCertObj = rootCert.get()->GetCert();

Heh, so, yeah, this needs an owner (UniqueCERTCertificate)
Attachment #8947485 - Flags: review?(dkeeler)
Comment on attachment 8947483 [details]
Bug 1434936 - Add method nsNSSCertList::GetRootCertificate

https://reviewboard.mozilla.org/r/217190/#review223526

::: security/manager/ssl/tests/gtest/CertListTest.cpp:402
(Diff revision 1)
> +TEST_F(psm_CertList, TestGetRootCertificateChainTwo)
> +{
> +  RefPtr<nsNSSCertList> certList = new nsNSSCertList();
> +
> +  nsresult rv = AddCertFromStringToList(kCaIntermediatePem, certList);
> +  ASSERT_EQ(rv, NS_OK) << "Should have loaded OK";

Expected values come first in gtests (I see that's wrong everywhere here but we can start doing it the right way :)

::: security/manager/ssl/tests/gtest/CertListTest.cpp:409
(Diff revision 1)
> +  ASSERT_EQ(rv, NS_OK) << "Should have loaded OK";
> +
> +  nsCOMPtr<nsIX509Cert> rootCert;
> +  rv = certList->GetRootCertificate(rootCert);
> +  ASSERT_EQ(rv, NS_OK) << "Should have fetched the root OK";
> +  ASSERT_TRUE(rootCert) << "Root cert should be filled in";

This is not an invariant so it should be `EXPECT_TRUE` to give nicer error messages. (Same for all other checks that should test something.)

::: security/manager/ssl/tests/gtest/CertListTest.cpp:420
(Diff revision 1)
> +  nsAutoString rootCn;
> +  ASSERT_TRUE(NS_SUCCEEDED(rootCert->GetCommonName(rootCn))) << "Getters should work.";
> +  ASSERT_TRUE(rootCn.EqualsLiteral("ca")) << "Root CN should match";
> +}
> +
> +TEST_F(psm_CertList, TestGetRootCertificateChainFour)

You could share the code between this and the previous test if you iterate over the certs you add. Not a big deal though.
Attachment #8947483 - Flags: review?(franziskuskiefer)
Comment on attachment 8947484 [details]
Bug 1434936 - Use nsNSSCertList in NSSCertDBTrustDomain::IsChainValid

https://reviewboard.mozilla.org/r/217192/#review223528

::: security/certverifier/NSSCertDBTrustDomain.cpp:793
(Diff revision 1)
> -  CERTCertListNode* rootNode = CERT_LIST_TAIL(certList);
> -  if (!rootNode) {
> +  // Modernization in-progress: Keep certList as a CERTCertList for storage into
> +  // the mBuiltChain variable at the end, but let's use nsIX509CertList for the
> +  // validity calculations.
> +  nsNSSShutDownPreventionLock lock;
> +  UniqueCERTCertList certChainCopy =
> +    nsNSSCertList::DupCertList(certList, lock);

Just FYI, something I don't expect `DupCertList` (or `CERT_DupCertificate` for that matter) to do; This doesn't duplicate the certificate (list) but increases the ref counter on it.

::: security/certverifier/NSSCertDBTrustDomain.cpp:856
(Diff revision 1)
> -         !CERT_LIST_END(node, certList); node = CERT_LIST_NEXT(node)) {
> +    intCertList->ForEachCertificateInChain(
> +      [&foundRequiredIntermediate] (nsCOMPtr<nsIX509Cert> aCert, bool aHasMore,
> +                                    /* out */ bool& aContinue) {
> +        // We need an owning handle when calling nsIX509Cert::GetCert().
> +        UniqueCERTCertificate nssCert(aCert->GetCert());
> -      if (CertMatchesStaticData(
> +        if (CertMatchesStaticData(

nit: This looks like tabs? I think...
Attachment #8947484 - Flags: review?(franziskuskiefer)
Comment on attachment 8947485 [details]
Bug 1434936 - Rework ChainHasValidPins to use nsNSSCertList

https://reviewboard.mozilla.org/r/217194/#review223530

What keeler said :)
Attachment #8947485 - Flags: review?(franziskuskiefer)
Comment on attachment 8947483 [details]
Bug 1434936 - Add method nsNSSCertList::GetRootCertificate

https://reviewboard.mozilla.org/r/217190/#review223526

> This is not an invariant so it should be `EXPECT_TRUE` to give nicer error messages. (Same for all other checks that should test something.)

But I do want the test to stop and not crash if it's false... but good point about the other ones! Adjusted. Thanks, gtest-master!
Comment on attachment 8947484 [details]
Bug 1434936 - Use nsNSSCertList in NSSCertDBTrustDomain::IsChainValid

https://reviewboard.mozilla.org/r/217192/#review223384

> You'll need to rebase this on top of bug 1421084 (fingers crossed that doesn't get backed out...)

oh boy! :D
Comment on attachment 8947484 [details]
Bug 1434936 - Use nsNSSCertList in NSSCertDBTrustDomain::IsChainValid

https://reviewboard.mozilla.org/r/217192/#review223528

Thanks!

> Just FYI, something I don't expect `DupCertList` (or `CERT_DupCertificate` for that matter) to do; This doesn't duplicate the certificate (list) but increases the ref counter on it.

Yeah, it's a bit of a strange mechanism.

> nit: This looks like tabs? I think...

No, it's spaces. ReviewBoard is just weird... :)
Comment on attachment 8947485 [details]
Bug 1434936 - Rework ChainHasValidPins to use nsNSSCertList

https://reviewboard.mozilla.org/r/217194/#review223390

> Heh, so, yeah, this needs an owner (UniqueCERTCertificate)

There doesn't appear to be another way to do this without adding more methods to nssCertList, so I guess this code should get wholesale folded into what's currently the 4th patch of the series. I'm dropping these issues but not going to do the folding yet until you can take a look at that.
Comment on attachment 8947483 [details]
Bug 1434936 - Add method nsNSSCertList::GetRootCertificate

https://reviewboard.mozilla.org/r/217190/#review224138

lgtm
Attachment #8947483 - Flags: review?(franziskuskiefer) → review+
Comment on attachment 8947484 [details]
Bug 1434936 - Use nsNSSCertList in NSSCertDBTrustDomain::IsChainValid

https://reviewboard.mozilla.org/r/217192/#review224130

::: security/manager/ssl/tests/gtest/CertListTest.cpp:488
(Diff revision 2)
> +  mozilla::Unused << rawCertList.release();
> +  // This adopts the copied list
> +  RefPtr<nsNSSCertList> certListCopy = new nsNSSCertList(Move(nssCertListCopy));
> +  ASSERT_TRUE(certListCopy) << "Copy should exist.";
> +  {
> +    nsCOMPtr<nsIX509Cert> rootCert;

You could move this block to its own function and re-use the code.
Attachment #8947484 - Flags: review?(franziskuskiefer) → review+
Comment on attachment 8947483 [details]
Bug 1434936 - Add method nsNSSCertList::GetRootCertificate

https://reviewboard.mozilla.org/r/217190/#review224294
Attachment #8947483 - Flags: review?(dkeeler) → review+
Comment on attachment 8947484 [details]
Bug 1434936 - Use nsNSSCertList in NSSCertDBTrustDomain::IsChainValid

https://reviewboard.mozilla.org/r/217192/#review224322

Seems good to me. See comments about the test, though.

::: security/certverifier/NSSCertDBTrustDomain.cpp:800
(Diff revision 2)
> +  if (!nssCertList) {
>      return Result::FATAL_ERROR_LIBRARY_FAILURE;
>    }
> -  CERTCertificate* root = rootNode->cert;
> +
> +  nsCOMPtr<nsIX509Cert> rootCert;
> +  nssCertList->GetRootCertificate(rootCert);

nit: check return value

::: security/manager/ssl/tests/gtest/CertListTest.cpp:468
(Diff revision 2)
>    nsresult rv = certList->GetRootCertificate(rootCert);
>    EXPECT_EQ(NS_OK, rv) << "Should have again fetched the root OK";
>    EXPECT_FALSE(rootCert) << "Root cert should be empty";
>  }
> +
> +TEST_F(psm_CertList, TestDupCertList)

This whole test seems a little out of place to me. If you want to verify that nsNSSCertList::DupCertList does the right thing, maybe just do that?

::: security/manager/ssl/tests/gtest/CertListTest.cpp:479
(Diff revision 2)
> +  ASSERT_EQ(NS_OK, rv) << "Should have loaded OK";
> +  rv = AddCertFromStringToList(kCaIntermediatePem, certList);
> +  ASSERT_EQ(NS_OK, rv) << "Should have loaded OK";
> +  rv = AddCertFromStringToList(kCaPem, certList);
> +  ASSERT_EQ(NS_OK, rv) << "Should have loaded OK";
> +  mozilla::UniqueCERTCertList rawCertList =

We should explain why we're doing this weird copy (GetRawCertList returns something that *shouldn't* be owned, but to copy the list, we need a UniqueCERTCertList, so use a UniqueCERTCertList but call release to avoid decrementing the refcount).
Alternatively, why not add a copy method to nsNSSCertList? (Or, I guess I'm not sure why we need to test this functionality...)
Or, we could fix the GetRawCertList problem and make it require an owning object... (that might cause more problems, though)
Attachment #8947484 - Flags: review?(dkeeler) → review+
Comment on attachment 8947484 [details]
Bug 1434936 - Use nsNSSCertList in NSSCertDBTrustDomain::IsChainValid

https://reviewboard.mozilla.org/r/217192/#review224322

> nit: check return value

Thanks!

> This whole test seems a little out of place to me. If you want to verify that nsNSSCertList::DupCertList does the right thing, maybe just do that?

I'm going to remove it, since it's not really relevant. Thanks.
Attachment #8947485 - Attachment is obsolete: true
Attachment #8947485 - Flags: review?(franziskuskiefer)
Attachment #8947485 - Flags: review?(dkeeler)
Comment on attachment 8947484 [details]
Bug 1434936 - Use nsNSSCertList in NSSCertDBTrustDomain::IsChainValid

https://reviewboard.mozilla.org/r/217192/#review224400

::: security/manager/ssl/tests/gtest/CertListTest.cpp:7
(Diff revision 3)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "gtest/gtest.h"
> +#include "mozilla/Unused.h"

This appears to be... <ahem> unused.
Comment on attachment 8948844 [details]
Bug 1434936 - Rework ChainHasValidPins to use nsNSSCertList

https://reviewboard.mozilla.org/r/218216/#review224402

Looks good - just the one comment.

::: security/manager/ssl/nsSiteSecurityService.cpp:1140
(Diff revision 2)
>      return NS_ERROR_FAILURE;
>    }
>  
> -  CERTCertListNode* rootNode = CERT_LIST_TAIL(certList);
> -  if (CERT_LIST_END(rootNode, certList)) {
> -    return NS_ERROR_FAILURE;
> +  // This copy to produce an nsNSSCertList should also be removed in Bug #1406854
> +  UniqueCERTCertList certListCopy = nsNSSCertList::DupCertList(certList);
> +  nsCOMPtr<nsIX509CertList> x509CertList = new nsNSSCertList(Move(certListCopy));

As far as I can tell, nothing else uses certList, so we can probably just Move it into the x509CertList without bothering with DupCertList.
Attachment #8948844 - Flags: review?(dkeeler) → review+
Comment on attachment 8947484 [details]
Bug 1434936 - Use nsNSSCertList in NSSCertDBTrustDomain::IsChainValid

https://reviewboard.mozilla.org/r/217192/#review224400

> This appears to be... <ahem> unused.

(•_•) 
( •_•)>⌐■-■ 
(⌐■_■)
Comment on attachment 8948844 [details]
Bug 1434936 - Rework ChainHasValidPins to use nsNSSCertList

https://reviewboard.mozilla.org/r/218216/#review224402

Thanks, both of you!
Comment on attachment 8948844 [details]
Bug 1434936 - Rework ChainHasValidPins to use nsNSSCertList

https://reviewboard.mozilla.org/r/218216/#review224816
Attachment #8948844 - Flags: review?(franziskuskiefer) → review+
Thanks for all the reviews! The reftests problems in this try push [1] look like they're infra issues, not mine, so marking checkin-needed. 

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=645c4eaf234f98e2ec31739151f0421fda5c693d
Keywords: checkin-needed
Comment on attachment 8948844 [details]
Bug 1434936 - Rework ChainHasValidPins to use nsNSSCertList

https://reviewboard.mozilla.org/r/218216/#review224402

> As far as I can tell, nothing else uses certList, so we can probably just Move it into the x509CertList without bothering with DupCertList.

Re-opened and closed issue, hopefully reviewboard will let it through now.
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55a95e2eb1c3
Add method nsNSSCertList::GetRootCertificate r=keeler r=fkiefer
https://hg.mozilla.org/integration/mozilla-inbound/rev/cab650790a71
Use nsNSSCertList in NSSCertDBTrustDomain::IsChainValid r=keeler r=fkiefer
https://hg.mozilla.org/integration/mozilla-inbound/rev/ede9e6ccc610
Rework ChainHasValidPins to use nsNSSCertList r=keeler r=fkiefer
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.