1049095 - (CVE-2014-1582) Spdy/Http-2 Coalescing Key Pinning Bypass

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Patrick McManus [:mcmanus]]

I was informed of a potential key pinning bypass attack that I can confirm we are vulnerable to. The attack uses spdy/h2 connection-coalescing to bypass the key pin.

Assume a.example.com is pinned to key-a and b.example.com is pinned to key-b

1] goto host https://a.example.com/  - it has a cert valid for *.example.com on 1.1.1.1 and uses key-a.. it negotiates a spdy connection that stays alive.

2] ask for https://b.example.com/ - it resolves also to 1.1.1.1. Under the spdy/h2 coalescing rules b can use the spdy connection from step 1 because it is the same host (1.1.1.1) and that connection has a valid cert for b.example.com (it was a wildcard) so it can be "joined"

the problem is that b.example.com/ is dispatched on the connection that used key-a and b is supposed to be pinned to key-b.

Before the "joining" we can just check if the connection from step 1 has a full cert chain that satisfies any pins for b.example.com - unfortunately getting the full cert chain requires some work especially with resumed connections (bug 731478)

Realistically, this is probably not a big problem because the configuration is somewhat non-sensical. If these hosts are really eligible for coalescing (which is being done now on <=ff31 which doesn't have any key pinning checks) then you would expect they would be pinned to the same key.

The inclination between keeler, monica, and myself was to try and get this in 34 and ride the trains with the fix.

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

I just told Garrett about this bug and his initial thought was that the stuff that he is doing for ssl error reporting is easy to modify to carry along the cert chain regardless of verification success or failure.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Patrick McManus [:mcmanus] from comment #0)
> 1] goto host https://a.example.com/  - it has a cert valid for *.example.com
> on 1.1.1.1 and uses key-a.. it negotiates a spdy connection that stays alive.
> 
> 2] ask for https://b.example.com/ - it resolves also to 1.1.1.1. Under the
> spdy/h2 coalescing rules b can use the spdy connection from step 1 because
> it is the same host (1.1.1.1) and that connection has a valid cert for
> b.example.com (it was a wildcard) so it can be "joined"
> 
> the problem is that b.example.com/ is dispatched on the connection that used
> key-a and b is supposed to be pinned to key-b.
> 
> Before the "joining" we can just check if the connection from step 1 has a
> full cert chain that satisfies any pins for b.example.com - unfortunately
> getting the full cert chain requires some work especially with resumed
> connections (bug 731478)
> 
> Realistically, this is probably not a big problem because the configuration
> is somewhat non-sensical. If these hosts are really eligible for coalescing
> (which is being done now on <=ff31 which doesn't have any key pinning
> checks) then you would expect they would be pinned to the same key.

Consider a connection to https://attacker.com/, which has a certificate with SANs for both attacker.com and victim.com. The attacker is MitMing your DNS so that DNS resolves to the same IP address for both. Thus, the attacker can now induce Firefox to coalesce victim.com onto attacker.com's connection, regardless of pinning. The attack requires a trusted CA to issue a certificate with SANs for both attacker.com and victim.com, but that's exactly the type of problem (mis-issuance) that key pinning is supposed to protect websites from.

Comment 3

[Martin Thomson [:mt:]]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #2)
> ... that's
> exactly the type of problem (mis-issuance) that key pinning is supposed to
> protect websites from.

Indeed.  How far back does this go?  I'm guessing that this is going to affect SPDY all the way back.  HTTP/2 bothers me less, since it's still off by default.

We should probably consider uplift for SPDY.

Comment 4

[Patrick McManus [:mcmanus]]

(In reply to Martin Thomson [:mt] from comment #3)
m.
> 
> Indeed.  How far back does this go?  

We don't have any key pinning, as I understand it, until firefox-32 (on beta).

Comment 5

[Andrew McCreight [:mccr8]]

David, do you know what security rating this should get? Thanks.
https://wiki.mozilla.org/Security_Severity_Ratings

Comment 6

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

Maybe sec-moderate? (I'm thinking this falls under "Missing Additional Security Controls" since it's a pinning bypass)

Comment 7

[Andrew McCreight [:mccr8]]

Thanks.  Feel free to adjust as appropriate.

Comment 8

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

Attachment: patch

This patch saves the verified cert chain for each TransportSecurityInfo so it can be used later to see if pinning checks would pass for the host that is attempting to join. The unfortunate problem is that for session tickets, we don't have direct access to the previously-validated chain, so we have to add a hash table that keeps track of validated chains from hosts that could possibly initiate a session resumption. Then, we don't want that to grow unboundedly, so we have to cull it periodically (NSS appears to save tickets for 24 hours).

Comment 9

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

Comment on attachment 8475360 [details] [diff] [review]
patch

bsmedberg, this could have a performance impact in that it will in general require more memory. Also, I would appreciate a look at the usage of threads/timers. I realize you have a long review queue, so feel free to redirect this review.

Comment 10

[Benjamin Smedberg]

Comment on attachment 8475360 [details] [diff] [review]
patch

I really don't have the ability to review this. mcmanus is a good reviewer: if you have specific threading or memory-use questions, nathan froyd and maybe nnethercote may be able to help as well.

Comment 11

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8475360 [details] [diff] [review]
patch

Review of attachment 8475360 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/boot/src/PublicKeyPinningService.cpp
@@ +320,5 @@
> +  char* evalHost = const_cast<char*>(hostname);
> +  char* evalPart;
> +  // If there is no '.' in the given hostname, we never enter the while loop,
> +  // which prevents pins for unqualified domain names.
> +  while ((evalPart = strchr(evalHost, '.'))) {

This code is repeated in CheckPinsForHostname, can it be refactored?

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +820,5 @@
>                                                  pinningEnforcementLevel);
>  }
>  
> +/*static*/ CertVerifier::pinning_enforcement_config
> +nsNSSComponent::GetPinningConfig()

GetPinningLevel or GetPinningEnforcementLevel, no need to inherit bad naming from the enum type

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +438,5 @@
> +  CertVerifier::pinning_enforcement_config
> +    pinningLevel = nsNSSComponent::GetPinningConfig();
> +  // Note that verifiedCertChain may be null when ChainHasValidPins is called
> +  // (if, for instance, we resumed a session but couldn't find the chain in the
> +  // ResumedHandshakeInfoTable. This is safe, because ChainHasValidPins will

This is a terrible name, it should be ChainPassesPinningChecks or similar. Also not sure why HostOrSuperdomainHasPins is not part of the check. That way the code is

if (pinningLevel != disabled && !ChainPassesPinningChecks) {
  // Don't join

Comment 12

[Patrick McManus [:mcmanus]]

this will be item #1 on wed.. fwiw

Comment 13

[Camilo Viecco (:cviecco)]

Comment on attachment 8475360 [details] [diff] [review]
patch

Review of attachment 8475360 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +444,5 @@
> +  nsCOMPtr<nsIX509CertList> verifiedCertChain;
> +  GetVerifiedCertChain(getter_AddRefs(verifiedCertChain));
> +  mozilla::pkix::Time now = mozilla::pkix::Now();
> +  if (pinningLevel != CertVerifier::pinningDisabled &&
> +      PublicKeyPinningService::HostOrSuperdomainHasPins(hostnameFlat.get(), now) &&

I dont see the need for HostOrSuperdomainHasPins. If verifiedCertChain->GetRawCertList() will return null then just call ChainHasValidPins with a non null empty CERTCertList.
That way we are guaranteed that we only implement the search logic once (a plus once we have dynamic pins)

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8475360 [details] [diff] [review]
patch

Review of attachment 8475360 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/nsNSSCallbacks.h
@@ +226,5 @@
>    static void initTable();
>    static SEC_HttpClientFcn sNSSInterfaceTable;
>  };
>  
> +class ResumedHandshakeInfoTable

Why not just finish the work I started for bug 731478? Then you would be able to use SSL_PeerCertificateChain to get the cert chain the libssl fd in JoinConnection for resumed sessions too, without all this complexity.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +438,5 @@
> +  CertVerifier::pinning_enforcement_config
> +    pinningLevel = nsNSSComponent::GetPinningConfig();
> +  // Note that verifiedCertChain may be null when ChainHasValidPins is called
> +  // (if, for instance, we resumed a session but couldn't find the chain in the
> +  // ResumedHandshakeInfoTable. This is safe, because ChainHasValidPins will

Why not just call unconditionally call CertVerifier::VerifySSLServerCert here, perhaps in a new "local-only" (no OCSP fetching) mode? It seems like the root cause of this problem is that we tried to do a subset of the work that CertVerifier::VerifySSLServerCert is doing here, but we didn't keep this subset in sync with what CertVerifier::VerifySSLServerCert does. It seems likely that in the future, similar problems will come up as we extend the behavior of CertVerifier to do more kinds of checks.

Comment 15

[Patrick McManus [:mcmanus]]

Comment on attachment 8475360 [details] [diff] [review]
patch

Review of attachment 8475360 [details] [diff] [review]:
-----------------------------------------------------------------

I've built and tested the patch and, other than the assert mentioned below, we still join connections on google.com which is in the pin list. cool.

A chief concern here is the unbounded size of the cert chain cache in the resumed handshake info table (it is bounded by time, but not size). Do we have any sense of mean cert chain sizes and the distribution of how many people collect in 24 hrs?

I do think you can effectively bound it with an lru. Just store all the entries as lru-list with a hash index for quick lookups and when a new entry makes the list exceed a configured max size (variable by platform), drop the lru to make room. At time of use in joinConnection() if the cert chain can't be found, pessimistically don't join the connection. It would be nice to have data, but I bet you will have a high hit rate while knowing the max size. You can still walk the list periodically to toss out things over a day old.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +825,5 @@
> +{
> +  // Default pinning enforcement level is disabled.
> +  CertVerifier::pinning_enforcement_config pinningLevel =
> +    static_cast<CertVerifier::pinning_enforcement_config>
> +      (Preferences::GetInt("security.cert_pinning.enforcement_level",

this isn't the main thread :(

#0  0x00007f98d0678d8d in nanosleep () at ../sysdeps/unix/syscall-template.S:81
#1  0x00007f98d0678c24 in __sleep (seconds=0) at ../sysdeps/unix/sysv/linux/sleep.c:137
#2  0x00007f98cab3d8ad in ah_crap_handler (signum=11) at ../../../gecko-dev/toolkit/xre/nsSigHandlers.cpp:88
#3  0x00007f98cab1b731 in nsProfileLock::FatalSignalHandler (signo=11, info=0x7f98b75fdd70, context=0x7f98b75fdc40) at /home/mcmanus/src/mozilla2/wd/gecko-dev/profile/dirserviceprovider/nsProfileLock.cpp:185
#4  0x00007f98cb97b42f in AsmJSFaultHandler (signum=11, info=0x7f98b75fdd70, context=0x7f98b75fdc40) at ../../../gecko-dev/js/src/asmjs/AsmJSSignalHandlers.cpp:978
#5  <signal handler called>
#6  mozilla::Preferences::InitStaticMembers () at ../../../gecko-dev/modules/libpref/Preferences.cpp:443
#7  0x00007f98c6f9e682 in mozilla::Preferences::GetInt (aPref=0x7f98cc9af349 <.L.str41> "security.cert_pinning.enforcement_level", aResult=0x7f98b75fe0f0) at ../../../gecko-dev/modules/libpref/Preferences.cpp:1395
#8  0x00007f98c6f328b5 in mozilla::Preferences::GetInt (aPref=0x7f98cc9af349 <.L.str41> "security.cert_pinning.enforcement_level", aDefault=0) at ../../dist/include/mozilla/Preferences.h:115
#9  0x00007f98ca9459d9 in nsNSSComponent::GetPinningConfig () at ../../../../../gecko-dev/security/manager/ssl/src/nsNSSComponent.cpp:829
#10 0x00007f98ca90bf5e in nsNSSSocketInfo::JoinConnection (this=0x7f9895eb8380, npnProtocol=..., hostname=..., port=443, _retval=0x7f98b75fe41f) at ../../../../../gecko-dev/security/manager/ssl/src/nsNSSIOLayer.cpp:446
#11 0x00007f98ca90c175 in non-virtual thunk to nsNSSSocketInfo::JoinConnection(nsACString_internal const&, nsACString_internal const&, int, bool*) () at /home/mcmanus/src/mozilla2/wd/ccache/data/tmp/nsNSSIOLay.tmp.ds9.842.ii:467

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +438,5 @@
> +  CertVerifier::pinning_enforcement_config
> +    pinningLevel = nsNSSComponent::GetPinningConfig();
> +  // Note that verifiedCertChain may be null when ChainHasValidPins is called
> +  // (if, for instance, we resumed a session but couldn't find the chain in the
> +  // ResumedHandshakeInfoTable. This is safe, because ChainHasValidPins will

I think brian's suggestion here about using VerifySSLServerCert is very good.

Comment 16

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

Attachment: patch v2

This patch is much better. I realized that as long as we're saving intermediates from successful connections, we'll be able to re-verify certificates from successful connections. (Unfortunately, this won't always work for private-browsing connections, since we don't save those intermediates.)

Comment 17

[Patrick McManus [:mcmanus]]

(In reply to David Keeler (:keeler) [use needinfo?] from comment #16)
> Created attachment 8476050 [details] [diff] [review]
> patch v2
> 
> This patch is much better. I realized that as long as we're saving
> intermediates from successful connections, we'll be able to re-verify
> certificates from successful connections. 

wow.

Comment 18

[Patrick McManus [:mcmanus]]

Comment on attachment 8476050 [details] [diff] [review]
patch v2

Review of attachment 8476050 [details] [diff] [review]:
-----------------------------------------------------------------

I tested this and it wfm. I tested the fail path by messing with the intermediate cert directly in gdb and watching the join fail. Other joins succeed where expected.

Comment 19

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

Patrick, do you have suggestions for how to write an automated test for this? I see there are some spdy xpcshell tests already, but is there a way to make sure that the test exercises joining connections?

Comment 20

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

Here's a try run, in any case: https://tbpl.mozilla.org/?tree=Try&rev=00be5800bbaf

Comment 21

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

Comment on attachment 8476050 [details] [diff] [review]
patch v2

We're trying to get this ready to be checked in and uplifted. Camilo, if Brian can't get to this today, would you mind reviewing it?

Comment 22

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8476050 [details] [diff] [review]
patch v2

Review of attachment 8476050 [details] [diff] [review]:
-----------------------------------------------------------------

fwiw, lgtm.

Comment 23

[Richard Barnes [:rbarnes]]

Comment on attachment 8476050 [details] [diff] [review]
patch v2

Review of attachment 8476050 [details] [diff] [review]:
-----------------------------------------------------------------

The only thing that concerns me is this:

> (although not during private browsing mode)

Just to confirm, in private browsing mode:
-- CertVerifier will not find the intermediate certs
-- So the verification will fail
-- So the connections will not coalesce (they'll just continue independently)

If that's correct, LGTM.

Comment 24

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

Yes, that's what I believe will happen.

Comment 25

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8476050 [details] [diff] [review]
patch v2

Review of attachment 8476050 [details] [diff] [review]:
-----------------------------------------------------------------

This patch is much better.

::: security/certverifier/CertVerifier.cpp
@@ +431,5 @@
>                                    Time time,
>                       /*optional*/ void* pinarg,
>                                    const char* hostname,
>                                    bool saveIntermediatesInPermanentDatabase,
> +                                  bool localOnly,

It is better to pass the flags the same way that they are passed to CertVerifier::VerifyCert, so that when you are reading the calling code, you see "FLAG_LOCAL_ONLY" instead of "true", which is much clearer.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +758,5 @@
>    SECOidTag evOidPolicy;
>    rv = certVerifier.VerifySSLServerCert(cert, stapledOCSPResponse,
>                                          time, infoObject,
>                                          infoObject->GetHostNameRaw(),
> +                                        saveIntermediates, false, nullptr,

I suggest s/false/CertVerifier::FLAG_LOCAL_ONLY/

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +433,5 @@
>    }
>  
> +  // Attempt to verify the joinee's certificate using the joining hostname.
> +  // This ensures that any key pins specified for the joining host are
> +  // satisfied by the joinee's certificate chain.

You should generalize this comment, e.g. by saying "This ensures that any hostname-specific verification logic (e.g. key pinning) is satisfied by the joinee's end-entity certificate."

Please add this comment, and consider its implications:

"XXX(bug XXXXXXX): The certificate chain built by this verification may be different than the certificate chain originally built during the joined connection's TLS handshake. Consequently, we may report a wrong and/or misleading certificate chain for HTTP transactions coalesced onto this connection. This may become problematic in the future. For example, if/when we begin relying on intermediate certificates being stored in the securityInfo of a cached HTTPS response, that cached certificate chain may actually be the wrong chain. We should consider having JoinConnection return the certificate chain built here, so that the calling Necko code can associate the correct certificate chain with the HTTP transactions it is trying to join onto this connection."

@@ +436,5 @@
> +  // This ensures that any key pins specified for the joining host are
> +  // satisfied by the joinee's certificate chain.
> +  // This only works because we permanently save the intermediates from
> +  // successfully-verified connections in the certificate database (although
> +  // not during private browsing mode). Additionally, we make sure this

David, whether or not we save certificates into the permanent database actually doesn't matter here. We are concerned about what CERT_CreateSubjectCertList does. CERT_CreateSubjectCertList looks for certificates in two places: (1) the permanent database, and (2) the set of (transient) CERTCertificate instances that have been created.

Bug 731478 is relevant because, with that bug fixed, we are guaranteed to have a CERTCertificate instance for every cert in the chain, for both resumed and full handshakes. (Please verify this claim yourself.) This will be true whether or not we're in private browsing mode. (In fact, the fact that this is true in private browsing mode is actually a bug, because we're not supposed to be sharing things across connections like this in private browsing mode.)

In other words, your comment is misleading.

@@ +437,5 @@
> +  // satisfied by the joinee's certificate chain.
> +  // This only works because we permanently save the intermediates from
> +  // successfully-verified connections in the certificate database (although
> +  // not during private browsing mode). Additionally, we make sure this
> +  // verification only uses local information. Since we're on the network

NIT: s/. Since/information; since/

@@ +440,5 @@
> +  // not during private browsing mode). Additionally, we make sure this
> +  // verification only uses local information. Since we're on the network
> +  // thread, we would be blocking on ourselves if we attempted any network i/o.
> +  nsAutoCString hostnameFlat(PromiseFlatCString(hostname));
> +  RefPtr<SharedCertVerifier> certVerifier(GetDefaultCertVerifier());

GetDefaultCertVerifier can return nullptr, and you should handle that.

Comment 26

[Patrick McManus [:mcmanus]]

(In reply to David Keeler (:keeler) [use needinfo?] from comment #19)
> Patrick, do you have suggestions for how to write an automated test for
> this? I see there are some spdy xpcshell tests already, but is there a way
> to make sure that the test exercises joining connections?

We would be a little ways away from that..

the spdy and h2 tests use self signed certs and an override - and we never join onto connections with exceptions.

So we would need a xpcshell test that annotates the trust anchor list instead.. and a dns shim to give localhost 3 names.. and then at that point the back end can make assertions about the number of streams that use a particular connection in the same way the multiplexing tests already do.

maybe nick has an easier thought

Comment 27

[Camilo Viecco (:cviecco)]

Comment on attachment 8476050 [details] [diff] [review]
patch v2

Review of attachment 8476050 [details] [diff] [review]:
-----------------------------------------------------------------

Brian's comments are spot on.

Comment 28

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

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #25)
> Comment on attachment 8476050 [details] [diff] [review]
> ::: security/manager/ssl/src/SSLServerCertVerification.cpp
> @@ +758,5 @@
> >    SECOidTag evOidPolicy;
> >    rv = certVerifier.VerifySSLServerCert(cert, stapledOCSPResponse,
> >                                          time, infoObject,
> >                                          infoObject->GetHostNameRaw(),
> > +                                        saveIntermediates, false, nullptr,
> 
> I suggest s/false/CertVerifier::FLAG_LOCAL_ONLY/

Well, except we don't want local only for this verification, right? (So the flags would be 0 here.)

> ::: security/manager/ssl/src/nsNSSIOLayer.cpp
> @@ +433,5 @@
> Please add this comment, and consider its implications:

Filed bug 1056935.
 
> @@ +436,5 @@
> David, whether or not we save certificates into the permanent database
> actually doesn't matter here. We are concerned about what
> CERT_CreateSubjectCertList does. CERT_CreateSubjectCertList looks for
> certificates in two places: (1) the permanent database, and (2) the set of
> (transient) CERTCertificate instances that have been created.
> 
> Bug 731478 is relevant because, with that bug fixed, we are guaranteed to
> have a CERTCertificate instance for every cert in the chain, for both
> resumed and full handshakes. (Please verify this claim yourself.) This will
> be true whether or not we're in private browsing mode. (In fact, the fact
> that this is true in private browsing mode is actually a bug, because we're
> not supposed to be sharing things across connections like this in private
> browsing mode.)

Ah, I see. Thanks for the explanation.

Comment 29

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

Attachment: patch v2.1

Addressed comments, carrying over r+.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4d1c0041347

Comment 30

[u408661]

(In reply to AFK Aug 22- Sep 01 Patrick McManus [:mcmanus] from comment #26)
> So we would need a xpcshell test that annotates the trust anchor list
> instead.. and a dns shim to give localhost 3 names.. and then at that point
> the back end can make assertions about the number of streams that use a
> particular connection in the same way the multiplexing tests already do.
> 
> maybe nick has an easier thought

Nope, we discussed this pretty thoroughly back when we were first adding the SPDY tests, and came to this same conclusion. We also came to the conclusion that that was *way* too much work, when we already had a hack for the certificate stuff.

That said, it's certainly functionality that I would *like* to be able to test.

Comment 31

[Patrick McManus [:mcmanus]]

I g(In reply to Nicholas Hurley [:hurley] from comment #30)
> (In reply to AFK Aug 22- Sep 01 Patrick McManus [:mcmanus] from comment #26)
> > So we would need a xpcshell test that annotates the trust anchor list
> > instead.. and a dns shim to give localhost 3 names.. and then at that point
> > the back end can make assertions about the number of streams that use a
> > particular connection in the same way the multiplexing tests already do.
> > 
> > maybe nick has an easier thought
> 
> Nope, we discussed this pretty thoroughly back when we were first adding the
> SPDY tests, and came to this same conclusion. We also came to the conclusion
> that that was *way* too much work, when we already had a hack for the
> certificate stuff.
> 
> That said, it's certainly functionality that I would *like* to be able to
> test.

I guess we could make the "don't join on exceptions" code preffable and have the test bypass that particular rule so it could test the other ones.

Comment 32

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

I don't think it would be too much work to set up those tests so that the certificate verifies without any exceptions/overrides. In fact, I did much (all?) of the work already in bug 846581.

Comment 33

[Patrick McManus [:mcmanus]]

(In reply to David Keeler (:keeler) [use needinfo?] from comment #32)
> I don't think it would be too much work to set up those tests so that the
> certificate verifies without any exceptions/overrides. In fact, I did much
> (all?) of the work already in bug 846581.

oh that's great. I made bug 1056987 for a new test and had it block on 846581. A new test would have also had some serious dns pains, but we added a mechanism to specify by pref a whole bunch of aliases for localhost sometime between introducing coalescing and now.

Comment 34

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

[Tracking Requested - why for this release]: Key pinning on twitter could be bypassed by this.

Comment 35

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

Attachment: patch for beta

Here's a patch for beta. The one issue is that if the user has disabled mozilla::pkix and is using classic verification, the LOCAL_ONLY flag isn't respected (yay NSS). I thought the safe thing to do would be to pessimistically say all connections can't be joined in that case.

Approval Request Comment
[Feature/regressing bug #]: key pinning / spdy connection joining
[User impact if declined]: pinning on twitter will be bypassable
[Describe test coverage new/current, TBPL]: minimal (see earlier comments in this bug)
[Risks and why]: see above - could result in degraded user experience if the user has disabled mozilla::pkix (which requires them going into about:config, so...)
[String/UUID change made/needed]: none

Comment 36

[Lawrence Mandel [:lmandel] (use needinfo)]

We're really late in beta and this being a sec-moderate I think we should consider whether we really need this in 32 or we can live with this until 33. This also hasn't hit m-c yet and according to comment 35 has received minimal testing.

David - How badly do we need to take this in 32? If this had been discovered after beta9, would you argue for inclusion the RC? Would you argue that we chemspill for this issue?

Comment 37

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

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

Comment 38

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

The effect of not taking this for 32 is that users don't really get the benefit of twitter and amo being pinned until 33. So, it's not worse than the status quo. I wouldn't argue for a chemspill. If we had better automated test coverage, I might argue for inclusion in the RC, but as it is, I can't claim with great confidence that it will be more beneficial than harmful, particularly considering the performance impact of a user disabling mozilla::pkix.

Comment 39

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8477072 [details] [diff] [review]
patch for beta

Given comment 38, I'm going to deny this for beta. Let's wait until 33 for this change.

Comment 40

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

Attachment: patch for aurora

This is patch v2.1 rebased for Aurora.

Comment 41

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

Comment on attachment 8478701 [details] [diff] [review]
patch for aurora

Approval Request Comment
[Feature/regressing bug #]: pinning / SPDY
[User impact if declined]: pinning won't actually be useful
[Describe test coverage new/current, TBPL]: minimal, although it has landed in Nightly (see previous comments)
[Risks and why]: low risk - haven't seen any problems on Nightly yet
[String/UUID change made/needed]: none

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d8e146e60d24

Comment 43

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking this as needing verification so that I have it tracked. Sounds like verification should be covered by the automated tests in bug 1056987 and bug 846581.

Comment 44

[Al Billings [:abillings - ex-MoCo]]

(In reply to Patrick McManus [:mcmanus] from comment #0)
> I was informed of a potential key pinning bypass attack that I can confirm
> we are vulnerable to.

I'm writing an advisory for this fix/issue. If you didn't discover it, who is it that I should credit in the advisory?

Comment 45

[Patrick McManus [:mcmanus]]

(In reply to Al Billings [:abillings] from comment #44)
> (In reply to Patrick McManus [:mcmanus] from comment #0)
> > I was informed of a potential key pinning bypass attack that I can confirm
> > we are vulnerable to.
> 
> I'm writing an advisory for this fix/issue. If you didn't discover it, who
> is it that I should credit in the advisory?

it came privately from a chrome developer - they aren't expecting credit.

Comment 46

[Al Billings [:abillings - ex-MoCo]]

(In reply to Patrick McManus [:mcmanus] from comment #45)

> it came privately from a chrome developer - they aren't expecting credit.

I guess you get it then! (I have to put someone down.)

Comment 47

[Matt Wobensmith [:mwobensmith][:matt:]]

Marking [qe-verify-] as creating the scenario that exercises this functionality would take us more time than we have allotted for bug verification. Also, the tests referenced on comment 43 have yet to be completed. If that is incorrect and more testing is needed on this, please let me know and I'm happy to do what I can.