1066190 - (CVE-2014-1584) if an unknown issuer error is encountered before pinning checks are done, pinning checks never happen

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

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

If a certificate's issuer can't be found, IsChainValid never gets called, which is where we do our pinning checks. As a result, a user can be presented with an overridable error on a site that should be pinned (i.e. we should display a pinning error and not allow an override).

STR:
1. run an https server with a self-signed cert
2. use /etc/hosts to redirect pinningtest.appspot.com to localhost
3. set pinning enforcement to "strict" (2)
4. visit https://pinningtest.appspot.com

expected results: pinning error page, not overridable
actual results: unknown issuer page, overridable

Comment 1

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

Attachment: patch

I'm thinking it might be good to include some more testcases, but I wanted to at least get the fix written.

Comment 2

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

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

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

::: security/certverifier/CertVerifier.cpp
@@ +432,5 @@
> +      if (srv != SECSuccess) {
> +        return SECFailure;
> +      }
> +      certCopy.forget(); // now owned by certList
> +      srv = chainValidationCallback(&callbackState, certList, &chainOK);

Does this mean we are now calling chainValidationCallback twice?

Comment 3

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

[Tracking Requested - why for this release]: this makes it way too easy to bypass our key pinning implementation. Note that the target of an attack using this will still see an untrusted certificate warning, but they'll be able to click through and add an exception, which is definitely not what we want to happen for sites that have pins.

Comment 4

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

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #2)
> Does this mean we are now calling chainValidationCallback twice?

Thanks for the review. Here's my analysis on calling the chain validation callback multiple times: If we did call it and it succeeded, we won't run this code, so that's fine. If we did call it and it failed, either the pinning checks failed or something catastrophic happened. If the pinning checks failed, we won't run this code again and in any case we'll report the correct error. If something catastrophic happened, it may or may not happen again, but in either case we won't return an overridable error. Otherwise, we didn't already call the validation callback, so it would be safe to call it.

Comment 5

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

That sounds fine to me.

Comment 6

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

Attachment: patch v2

I added a couple of other tests to be more thorough. Not significantly different, though, so carrying over r+.

Comment 7

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

This is only a sec-moderate, so:
https://tbpl.mozilla.org/?tree=Try&rev=0674b58e5972
https://hg.mozilla.org/integration/mozilla-inbound/rev/d02e70f0bf3d

Comment 8

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

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

Comment 9

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

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

Approval Request Comment
[Feature/regressing bug #]: bug 744204 (preloaded key pinning)
[User impact if declined]: this provides an easy way to bypass key pinning checks (it still requires a user to add a certificate exception)
[Describe test coverage new/current, TBPL]: has both preexisting tests and new tests added with patch
[Risks and why]: minimal - at worse we could make some overridable errors non-overridable
[String/UUID change made/needed]: none (I actually checked the this time)

Comment 10

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

We're very likely going to be releasing Firefox 32.0.2 this week. Do you want to make the case for this fix to ride along?

Comment 11

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

Given that we wontfix'd bug 1049095 (another pinning bypass) for 32, I don't think it's essential that we take this for 32 either (furthermore, while the patch for this bug applies cleanly to 33-35, I don't think it'll apply as cleanly to 32 (it would be possible to make a branch-specific patch, but still)).

Comment 12

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

Thanks David. Based on comment 11, I'm marking 32 as won't fix.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/46ddbee26635
https://hg.mozilla.org/releases/mozilla-beta/rev/1e3320340bd2

Comment 14

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

IMO, there is a better, simpler, way to fix this: Simply don't allow cert error overrides for any pinned domains. That is what you've effectively done, almost, since most users of HPKP won't be inning their end-entity certificate's key.

Comment 15

[Kamil Jozwiak [:kjozwiak]]

Reproduced the original issue using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-09-09-03-02-07-mozilla-central/

I followed the STR from comment #0 and received the "This Connection is Untrusted" page which was overridable even though pinning enforcement was set to strict(2)

Went through verification using the following builds:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-10-10-03-02-01-mozilla-central/ (changeset: 209717:50b689feab5f)
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-10-10-00-40-01-mozilla-aurora/ (changeset: 218224:72c13d8631ff)
- http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/33.0-candidates/build1/linux-x86_64/ (changeset: 218062:0e0dc4b23c13)

For each channel, I ensured that I received the following error message that was not overridable: (tested on normal/private and e10s windows)

"An error occurred during a connection to pinningtest.appspot.com. The server uses key pinning (HPKP) but no trusted certificate chain could be constructed that matches the pinset. Key pinning violations cannot be overridden. (Error code: mozilla_pkix_error_key_pinning_failure)"