1102436 - remove PublicKeyPinningService::CheckChainAgainstAllNames

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

Currently we only ever call PublicKeyPinningService::ChainHasValidPins when we have a hostname. We should never call it without one. Thus, CheckChainAgainstAllNames is never used and should be removed.

Comment 1

[Camilo Viecco (:cviecco)]

Keller, this is incorrect, we call Chain has valid pins on every ssl server validation. And in several of those cases we do not have the hostname (for example in getIssuer, which call getChain, which calls verifyChain). If you want to remove it please ensure that all callers of verifyCert when used as SSLServer do have the hostname.

Comment 2

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

Hmmm - looks like nsNSSCertificate::hasValidEVOidTag would cause this to happen, as would nsNSSCertificate::GetChain, but we already know how broken that is. nsNSSCertificateDB::VerifyCertNow should probably take a hostname as an input if it needs to verify a certificate with certificateUsageSSLServer. nsUsageArrayHelper::check also appears to do this.

Comment 3

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

Attachment: MozReview Request: bz://1102436/keeler

/r/7811 - bug 1102436 - remove PublicKeyPinningService::CheckChainAgainstAllNames

Pull down this commit:

hg pull -r 87995909a9acb1dc2003ed00d2960c294d5c6ba3 https://reviewboard-hg.mozilla.org/gecko/

Comment 4

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

Comment on attachment 8598943 [details]
MozReview Request: bz://1102436/keeler

/r/7811 - bug 1102436 - remove PublicKeyPinningService::CheckChainAgainstAllNames

Pull down this commit:

hg pull -r 87995909a9acb1dc2003ed00d2960c294d5c6ba3 https://reviewboard-hg.mozilla.org/gecko/

Comment 5

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

Cykesiopka - comment 4 is more of a feedback request. I realize this is a large patch, so any ideas on how to break it up or make the changes in stages would be appreciated. Also, there's no rush on this, so don't feel like you have to get it all done in the next few days.

Comment 6

[:Cykesiopka]

Hmm, I think splitting out some of the more stand-alone clean-ups into a separate patch 0, or into a separate bug would probably be nice. Thanks.

Comment 7

[:Cykesiopka]

Comment on attachment 8598943 [details]
MozReview Request: bz://1102436/keeler

https://reviewboard.mozilla.org/r/7809/#review6763

I did a brief overview of the changes and I think they look reasonable, so f+.

::: security/manager/ssl/tests/unit/test_pinning_dynamic.js:38
(Diff revision 1)
> -const NON_ISSUED_KEY_HASH = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=";
> +const NON_ISSUED_KEY_HASH = "KHAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAN=";

https://www.youtube.com/watch?v=wRnSnfiUI54#t=18 ?

Comment 8

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

Thanks for the feedback.

(In reply to Cykesiopka from comment #6)
> Hmm, I think splitting out some of the more stand-alone clean-ups into a
> separate patch 0, or into a separate bug would probably be nice. Thanks.

Good call. I'll defer the cleanups to later and focus on the original intent of this bug, which is to remove CheckChainAgainstAllNames.

(In reply to Cykesiopka from comment #7)
> ::: security/manager/ssl/tests/unit/test_pinning_dynamic.js:38
> (Diff revision 1)
> > -const NON_ISSUED_KEY_HASH = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=";
> > +const NON_ISSUED_KEY_HASH = "KHAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAN=";
> 
> https://www.youtube.com/watch?v=wRnSnfiUI54#t=18 ?

Yeah, I couldn't resist.

Comment 9

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

Attachment: MozReview Request: bz://1102436/keeler

/r/8367 - bug 1102436 - remove PublicKeyPinningService::CheckChainAgainstAllNames

Pull down this commit:

hg pull -r ba305ad16ad5f902e929419442c59f41ce4c7f46 https://reviewboard-hg.mozilla.org/gecko/

Comment 10

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

Comment on attachment 8602877 [details]
MozReview Request: bz://1102436/keeler

/r/8367 - bug 1102436 - remove PublicKeyPinningService::CheckChainAgainstAllNames

Pull down this commit:

hg pull -r ba305ad16ad5f902e929419442c59f41ce4c7f46 https://reviewboard-hg.mozilla.org/gecko/

Comment 11

[:Cykesiopka]

Comment on attachment 8602877 [details]
MozReview Request: bz://1102436/keeler

https://reviewboard.mozilla.org/r/8365/#review7137

r+ with comments addressed.

::: security/manager/ssl/public/nsIX509CertDB.idl:367
(Diff revision 1)
> +   *  @param aVerifedChain chain of verification up to the root if success

s/aVerifedChain/aVerifiedChain/

::: security/manager/ssl/src/nsNSSCertificateDB.cpp:1712
(Diff revision 1)
>                                    nsIX509CertList** verifiedChain,

It looks like there are only 4 places in this method to change |verifiedChain| to |aVerifiedChain|, so it probably won't hurt to change them.

::: security/manager/ssl/tests/unit/head_psm.js:130
(Diff revision 1)
> -function checkCertErrorGeneric(certdb, cert, expectedError, usage) {
> +function checkCertErrorGeneric(certdb, cert, expectedError, usage, hostname) {

Would be nice to document that |hostname| is optional.

::: security/manager/ssl/src/nsNSSCertificateDB.cpp:1711
(Diff revision 1)
> +                                  const char* aHostname,

verifyCertNow() in makeCNNICHashes.js hasn't been updated.

::: security/certverifier/CertVerifier.cpp
(Diff revision 1)
> -  if (CERT_LIST_END(rootNode, certList)) {

This is more of a question from me due to being unfamiliar with the CERT_LIST_* stuff: is this equivalent to just checking |!rootNode|? I don't understand what purpose this check serves otherwise...

Comment 12

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

https://reviewboard.mozilla.org/r/8365/#review7191

Thanks for the review!
Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=07aec873111c

> This is more of a question from me due to being unfamiliar with the CERT_LIST_* stuff: is this equivalent to just checking |!rootNode|? I don't understand what purpose this check serves otherwise...

Checking CERT_LIST_END(rootNode, certList) in the original code was essentially a way of determining that the list wasn't empty. Unfortunately this isn't equivalent to !rootNode (CERTCertList is implemented as a circular linked-list, so an empty list has next and prev links that point to itself, but it isn't null). I realized the new code didn't have the appropriate check, so I added it (basically a call to CERT_LIST_EMPTY(certList) higher up in IsChainValid).

Comment 13

[:Cykesiopka]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #12)
> Checking CERT_LIST_END(rootNode, certList) in the original code was
> essentially a way of determining that the list wasn't empty. Unfortunately
> this isn't equivalent to !rootNode (CERTCertList is implemented as a
> circular linked-list, so an empty list has next and prev links that point to
> itself, but it isn't null). I realized the new code didn't have the
> appropriate check, so I added it (basically a call to
> CERT_LIST_EMPTY(certList) higher up in IsChainValid).

Ah, I see. Thanks for the explanation.

Comment 14

[Pulsebot]

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

Comment 15

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

Attachment: patch as landed

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

Comment 16

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

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