1267905 - Replace uses of ScopedCERTCertList with UniqueCERTCertList

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[:Cykesiopka]

ScopedCERTCertList is based on Scoped.h, which is deprecated. We should use the more standard UniquePtr equivalent.

Comment 1

[:Cykesiopka]

Attachment: MozReview Request: Bug 1267905 - Replace uses of ScopedCERTCertList with UniqueCERTCertList. r=keeler

ScopedCERTCertList is based on Scoped.h, which is deprecated in favour of the
standardised UniquePtr.

Also changes CERTCertList parameters of various functions to make ownership more
explicit.

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

Comment 2

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

Comment on attachment 8748506 [details]
MozReview Request: Bug 1267905 - Replace uses of ScopedCERTCertList with UniqueCERTCertList. r=keeler

https://reviewboard.mozilla.org/r/50323/#review47525

This looks good. Just a few comments.

::: security/apps/AppTrustDomain.h:24
(Diff revision 1)
>  class AppTrustDomain final : public mozilla::pkix::TrustDomain
>  {
>  public:
>    typedef mozilla::pkix::Result Result;
>  
> -  AppTrustDomain(ScopedCERTCertList&, void* pinArg);
> +  AppTrustDomain(/*out*/ UniqueCERTCertList& certChain, void* pinArg);

nit: I'm not sure I agree with the /* out */ comment here, since normally (I claim without actually taking a rigorous look) out parameters for a function have been populated as soon as the function has returned. Since this is the constructor and the certChain doesn't actually get populated until a later call to BuildCertChain, I don't think the annotation fits in this case.

::: security/apps/AppTrustDomain.cpp:52
(Diff revision 1)
>  
>  StaticMutex AppTrustDomain::sMutex;
>  UniquePtr<unsigned char[]> AppTrustDomain::sDevImportedDERData;
>  unsigned int AppTrustDomain::sDevImportedDERLen = 0;
>  
> -AppTrustDomain::AppTrustDomain(ScopedCERTCertList& certChain, void* pinArg)
> +AppTrustDomain::AppTrustDomain(/*out*/ UniqueCERTCertList& certChain,

(same here)

::: security/certverifier/NSSCertDBTrustDomain.h:66
(Diff revision 1)
>                         uint32_t certShortLifetimeInDays,
>                         CertVerifier::PinningMode pinningMode,
>                         unsigned int minRSABits,
>                         ValidityCheckingMode validityCheckingMode,
>                         CertVerifier::SHA1Mode sha1Mode,
> -                       ScopedCERTCertList& builtChain,
> +               /*out*/ UniqueCERTCertList& builtChain,

Same idea here. Maybe we should use a new annotation? Like "eventual out"? Eh, that's not great. Something like that, anyway.

::: security/manager/ssl/SSLServerCertVerification.cpp:1378
(Diff revision 1)
>    // constructor.
>    // We can safely skip checking if NSS has already shut down here since we're
>    // in the middle of verifying a certificate.
>    nsNSSShutDownPreventionLock lock;
> -  CERTCertList* peerCertChainCopy = nsNSSCertList::DupCertList(peerCertChain, lock);
> +  UniqueCERTCertList peerCertChainCopy =
> +    nsNSSCertList::DupCertList(peerCertChain, lock);

Let's also add a null-check for duplicating the cert list (we can set an out-of-memory error and return in that case).

::: security/manager/ssl/SSLServerCertVerification.cpp:1538
(Diff revision 1)
>        PR_SetError(PR_INVALID_STATE_ERROR, 0);
>        return SECFailure;
>    }
>  
>    // Get the peer certificate chain for error reporting
> -  ScopedCERTCertList peerCertChain(SSL_PeerCertificateChain(fd));
> +  UniqueCERTCertList peerCertChain(SSL_PeerCertificateChain(fd));

We should probably null-check this too.

::: security/manager/ssl/nsNSSCertificate.cpp:887
(Diff revision 1)
>        // keep going
>      }
>    }
>  
>    if (!nssChain) {
> -    // There is not verified path for the chain, howeever we still want to 
> +    // There is not verified path for the chain, however we still want to 

nit: trailing space here too

::: security/manager/ssl/nsNSSCertificate.cpp:1624
(Diff revision 1)
>    }
>  
> -  CERTCertListNode* node;
> -  for (node = CERT_LIST_HEAD(aCertList); !CERT_LIST_END(node, aCertList);
> +  for (CERTCertListNode* node = CERT_LIST_HEAD(certList);
> +       !CERT_LIST_END(node, certList);
> -                                              node = CERT_LIST_NEXT(node)) {
> +       node = CERT_LIST_NEXT(node)) {
>      CERTCertificate* cert = CERT_DupCertificate(node->cert);

Yes, we should null-check cert and check the return value of CERT_AddCertToListTail (I'm thinking they can only fail for out-of-memory conditions, which is ok - we can just return a "null" UniqueCERTCertList (and make sure that all callers of DupCertList do the right thing). (Also make sure we don't leak the cert if it did get dup'd.)

Comment 3

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

Comment on attachment 8748506 [details]
MozReview Request: Bug 1267905 - Replace uses of ScopedCERTCertList with UniqueCERTCertList. r=keeler

https://reviewboard.mozilla.org/r/50323/#review47567

::: security/manager/ssl/TransportSecurityInfo.cpp:1086
(Diff revision 1)
>    nsNSSShutDownPreventionLock lock;
>    if (isAlreadyShutDown()) {
>      return NS_ERROR_NOT_AVAILABLE;
>    }
>  
>    nsCOMPtr<nsIX509CertList> comCertList;

Huh - just noticed this when working on something else. Looks like it was a mistake from bug 1029155. Might be nice to remove.

Comment 4

[:Cykesiopka]

https://reviewboard.mozilla.org/r/50323/#review47525

Thanks!

> nit: I'm not sure I agree with the /* out */ comment here, since normally (I claim without actually taking a rigorous look) out parameters for a function have been populated as soon as the function has returned. Since this is the constructor and the certChain doesn't actually get populated until a later call to BuildCertChain, I don't think the annotation fits in this case.

Yes, that makes sense. I can't think of decent annotation right now, so I'll drop the relevant /*out*/ comments for now.

Comment 5

[:Cykesiopka]

https://reviewboard.mozilla.org/r/50323/#review47567

> Huh - just noticed this when working on something else. Looks like it was a mistake from bug 1029155. Might be nice to remove.

Yeah, looks like something from an earlier version of the patch that didn't get removed. I'll remove it.

Comment 6

[:Cykesiopka]

Comment on attachment 8748506 [details]
MozReview Request: Bug 1267905 - Replace uses of ScopedCERTCertList with UniqueCERTCertList. r=keeler

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

Comment 7

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

Comment on attachment 8748506 [details]
MozReview Request: Bug 1267905 - Replace uses of ScopedCERTCertList with UniqueCERTCertList. r=keeler

https://reviewboard.mozilla.org/r/50323/#review47601

Comment 8

[:Cykesiopka]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7162c3d8cadd62444d29280524a5280d6e8e3df6

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6fc34759465e

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6fc34759465e