Closed Bug 1267905 Opened 4 years ago Closed 4 years ago

Replace uses of ScopedCERTCertList with UniqueCERTCertList

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

ScopedCERTCertList is based on Scoped.h, which is deprecated. We should use the more standard UniquePtr equivalent.
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/
Attachment #8748506 - Flags: review?(dkeeler)
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.)
Attachment #8748506 - Flags: review?(dkeeler) → review+
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.
Attachment #8748506 - Flags: review+
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.
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 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/
Attachment #8748506 - Attachment description: MozReview Request: Bug 1267905 - Replace uses of ScopedCERTCertList with UniqueCERTCertList. → MozReview Request: Bug 1267905 - Replace uses of ScopedCERTCertList with UniqueCERTCertList. r=keeler
Attachment #8748506 - Flags: review?(dkeeler)
Comment on attachment 8748506 [details]
MozReview Request: Bug 1267905 - Replace uses of ScopedCERTCertList with UniqueCERTCertList. r=keeler

https://reviewboard.mozilla.org/r/50323/#review47601
Attachment #8748506 - Flags: review?(dkeeler) → review+
https://hg.mozilla.org/mozilla-central/rev/6fc34759465e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1271043
You need to log in before you can comment on or make changes to this bug.