1250258 - Partially clean up nsNSSCertificateDB.cpp import methods

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[:Cykesiopka]

These methods have some issues we can at least partially fix:
 - Use of deprecated Scoped.h features
 - gotos
 - Variables declared far from where they are actually used
 - Unnecessary variables
 - Manual memory management

Comment 1

[:Cykesiopka]

Attachment: MozReview Request: Bug 1250258 - Partially clean up nsNSSCertificateDB.cpp import methods. r=keeler

Review commit: https://reviewboard.mozilla.org/r/37781/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37781/

Comment 2

[:Cykesiopka]

https://reviewboard.mozilla.org/r/37781/#review34323

::: security/manager/ssl/nsNSSCertificateDB.cpp:668
(Diff revision 1)
> -  SECStatus srv = CERT_FilterCertListByUsage(certList, certUsageAnyCA, true);
> +  SECStatus srv = CERT_FilterCertListByUsage(certList.get(), certUsageAnyCA,
> +                                             true);
>    if (srv != SECSuccess) {
>      return NS_ERROR_FAILURE;
>    }

|srv| can be eliminated by moving the function call to inside the if statement.

Comment 3

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

Comment on attachment 8726047 [details]
MozReview Request: Bug 1250258 - Partially clean up nsNSSCertificateDB.cpp import methods. r=keeler

https://reviewboard.mozilla.org/r/37781/#review34835

This looks good, but I think I've identified some simplifications that might be good to implement directly. Let me know what you think.

::: security/manager/ssl/nsNSSCertificateDB.cpp:516
(Diff revision 1)
>    SECStatus srv = SECFailure;

I think a lot of these can be moved closer to where they're used.

::: security/manager/ssl/nsNSSCertificateDB.cpp:537
(Diff revision 1)
> -  numcerts = certCollection->numcerts;
> +  SECItem** rawArray = (SECItem**)PORT_Alloc(sizeof(SECItem*) * numcerts);

The way getCertsFromPackage interacts with CERT_ImportCerts is unfortunate in that there's this intermediate step of creating a SECItem** rawArray and copying some pointers around. It would be better if getCertsFromPackage just returned the certs in a way that CERT_ImportCerts could use directly. This is probably work for a follow-up bug, though.

::: security/manager/ssl/nsNSSCertificateDB.cpp:585
(Diff revision 1)
>      SECStatus rv = certVerifier->VerifyCert(node->cert,

This doesn't seem like an incredibly useful check. Sure, it can be helpful to know right away if the certificate won't verify as expected, but it can still fail to verify later, so I'm not even sure why we try.

::: security/manager/ssl/nsNSSCertificateDB.cpp:610
(Diff revision 1)
> -nsNSSCertificateDB::ImportValidCACerts(int numCACerts, SECItem *CACerts, nsIInterfaceRequestor *ctx,  const nsNSSShutDownPreventionLock &proofOfLock)
> +nsNSSCertificateDB::ImportValidCACerts(int numCACerts, SECItem* CACerts,

The bulk of this function seems very similar to ImportEmailCertificate - can we factor the common part out?

::: security/manager/ssl/nsNSSCertificateDB.cpp:681
(Diff revision 1)
>      SECStatus rv = certVerifier->VerifyCert(node->cert,

This check is almost worse than the email cert one - if the list of CAs is in the wrong order, then none of them will verify (except the last), so none of them will be imported. If we get rid of the calls to VerifyCert, I think we can even fold all of the various import functions together into one (note that it looks like CERT_ImportCerts ignores its usage argument, so we don't even have to special case that - see https://dxr.mozilla.org/mozilla-central/rev/e7319545eb3819da67ffe1d4233022ae71e3a9a1/security/nss/lib/certdb/certdb.c#2407).

Comment 4

[:Cykesiopka]

https://reviewboard.mozilla.org/r/37781/#review34835

Yes, the simplifications that I could do made sense and were worth implementing.

> The way getCertsFromPackage interacts with CERT_ImportCerts is unfortunate in that there's this intermediate step of creating a SECItem** rawArray and copying some pointers around. It would be better if getCertsFromPackage just returned the certs in a way that CERT_ImportCerts could use directly. This is probably work for a follow-up bug, though.

Yup. Judging by how I get steadily less sane the more I look at the code in this file, I suspect there will be quite a few follow ups.

> This doesn't seem like an incredibly useful check. Sure, it can be helpful to know right away if the certificate won't verify as expected, but it can still fail to verify later, so I'm not even sure why we try.

x-x509-email-cert and friends strike again! This check is apparently in place to prevent DoS attacks - see Bug 249004.
I'll add comments where appropriate.

> This check is almost worse than the email cert one - if the list of CAs is in the wrong order, then none of them will verify (except the last), so none of them will be imported. If we get rid of the calls to VerifyCert, I think we can even fold all of the various import functions together into one (note that it looks like CERT_ImportCerts ignores its usage argument, so we don't even have to special case that - see https://dxr.mozilla.org/mozilla-central/rev/e7319545eb3819da67ffe1d4233022ae71e3a9a1/security/nss/lib/certdb/certdb.c#2407).

Yeah. One of the callers (handleCACertDownload()) kinda attempts to handle this, but this isn't enforced anywhere else.
I believe the VerifyCert() thing here is also related to Bug 249004, so I'll also just add a comment for now.

Comment 5

[:Cykesiopka]

Comment on attachment 8726047 [details]
MozReview Request: Bug 1250258 - Partially clean up nsNSSCertificateDB.cpp import methods. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37781/diff/1-2/

Comment 6

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

Comment on attachment 8726047 [details]
MozReview Request: Bug 1250258 - Partially clean up nsNSSCertificateDB.cpp import methods. r=keeler

https://reviewboard.mozilla.org/r/37781/#review35933

As a heads up, bug 1255438 might conflict with these changes.
Looks good - let me know what you think about the verification-on-import issue (long story short, if we can stop exposing this functionality to content, I don't think we have to verify on import).

::: security/manager/ssl/nsNSSCertificateDB.cpp:497
(Diff revisions 1 - 2)
> +ImportCertsIntoTempStorage(int numcerts, SECItem* certs,
> +                           const SECCertUsage usage, const bool caOnly,
> +                           const nsNSSShutDownPreventionLock& /*proofOfLock*/,
> +                   /*out*/ const UniqueCERTCertList& filteredCerts)
> +{
> +  NS_ENSURE_ARG(numcerts > 0);

NS_ENSURE_ARG_MIN(numcerts, 1) might be more appropriate here

::: security/manager/ssl/nsNSSCertificateDB.cpp:530
(Diff revisions 1 - 2)
> +      continue;
> +    }
> +
> +    CERTCertificate* cert = CERT_DupCertificate(importedCerts[i]);
> +    if (cert) {
> +      CERT_AddCertToListTail(filteredCerts.get(), cert);

We should check the return value here (if it fails, cert needs to have its refcount decresed again).
(This is also a problem elsewhere in that we hardly ever check the return value from this function.)

::: security/manager/ssl/nsNSSCertificateDB.cpp:615
(Diff revisions 1 - 2)
> -    }
>    }
>  
> -  /* go down the remaining list of certs and verify that they have
> -   * valid chains, then import them.
> -   */
> +  // Iterate through the filtered cert list and import verified certs into
> +  // permanent storage.
> +  // Note: We verify the certs in order to prevent DoS attacks. See Bug 249004.

As described, that attack is only possible with a verification library that doesn't explore all possible paths. Since we now have a library that actually does so, this shouldn't affect Firefox/Thunderbird. (To confirm my theory, I wrote a quick test to verify, and this seems to be correct.)

I suppose someone could mount a DoS attack by just spamming email certs at the user (which we helpfully import automatically with no intervening UI, it seems). I think a safe way to deal with that would be to just remove that auto-import functionality.

Let's do this: keep this verification here for now, remove auto-importing of email certs (and server certs, since that's a no-op) in another bug (they just take up space in Firefox and I don't think navigation is how they're imported in Thunderbird), and then come back and remove this.

::: security/manager/ssl/nsNSSCertificateDB.cpp:675
(Diff revisions 1 - 2)
>      return NS_ERROR_UNEXPECTED;
>    }
>  
> -  /* filter out the certs we don't want */
> -  SECStatus srv = CERT_FilterCertListByUsage(certList.get(), certUsageAnyCA,
> -                                             true);
> +  // Iterate through the filtered cert list and import verified certs into
> +  // permanent storage.
> +  // Note: We verify the certs in order to prevent DoS attacks. See Bug 249004.

Since mozilla::pkix doesn't have the same flaw classic verification does, and since there's a UI prompt before importing CA certificates when navigated to, I don't think we need to do this verification here.

Comment 7

[:Cykesiopka]

https://reviewboard.mozilla.org/r/37781/#review35933

Thanks for the review!

> As described, that attack is only possible with a verification library that doesn't explore all possible paths. Since we now have a library that actually does so, this shouldn't affect Firefox/Thunderbird. (To confirm my theory, I wrote a quick test to verify, and this seems to be correct.)
> 
> I suppose someone could mount a DoS attack by just spamming email certs at the user (which we helpfully import automatically with no intervening UI, it seems). I think a safe way to deal with that would be to just remove that auto-import functionality.
> 
> Let's do this: keep this verification here for now, remove auto-importing of email certs (and server certs, since that's a no-op) in another bug (they just take up space in Firefox and I don't think navigation is how they're imported in Thunderbird), and then come back and remove this.

Sure, this sounds reasonable. I'll file follow ups as necessary.

> Since mozilla::pkix doesn't have the same flaw classic verification does, and since there's a UI prompt before importing CA certificates when navigated to, I don't think we need to do this verification here.

Fair point. I'll defer this to another bug just so it's easier to track.

Comment 8

[:Cykesiopka]

Comment on attachment 8726047 [details]
MozReview Request: Bug 1250258 - Partially clean up nsNSSCertificateDB.cpp import methods. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37781/diff/2-3/

Comment 9

[:Cykesiopka]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d13639723c5
(I got tired of waiting for the Win7 xpcshell jobs to even start after 7+ hours and cancelled them. Looks like TH still thinks the jobs are pending.)

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/df87a62ca357

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/df87a62ca357