Closed Bug 1066190 (CVE-2014-1584) Opened 7 years ago Closed 7 years ago

if an unknown issuer error is encountered before pinning checks are done, pinning checks never happen


(Core :: Security: PSM, defect)

Not set



Tracking Status
firefox32 + wontfix
firefox33 + verified
firefox34 + verified
firefox35 + verified
firefox-esr31 --- unaffected


(Reporter: keeler, Assigned: keeler)



(Keywords: sec-moderate, Whiteboard: [adv-main33+])


(1 file, 1 obsolete file)

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).

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

expected results: pinning error page, not overridable
actual results: unknown issuer page, overridable
Attached patch patch (obsolete) — Splinter Review
I'm thinking it might be good to include some more testcases, but I wanted to at least get the fix written.
Attachment #8488092 - Flags: review?(mmc)
Comment on attachment 8488092 [details] [diff] [review]

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?
Attachment #8488092 - Flags: review?(mmc) → review+
[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.
(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.
That sounds fine to me.
Attached patch patch v2Splinter Review
I added a couple of other tests to be more thorough. Not significantly different, though, so carrying over r+.
Attachment #8488092 - Attachment is obsolete: true
Attachment #8488823 - Flags: review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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)
Attachment #8488823 - Flags: approval-mozilla-beta?
Attachment #8488823 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(dkeeler)
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)).
Flags: needinfo?(dkeeler)
Thanks David. Based on comment 11, I'm marking 32 as won't fix.
Attachment #8488823 - Flags: approval-mozilla-beta?
Attachment #8488823 - Flags: approval-mozilla-beta+
Attachment #8488823 - Flags: approval-mozilla-aurora?
Attachment #8488823 - Flags: approval-mozilla-aurora+
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.
Whiteboard: [adv-main33+]
Alias: CVE-2014-1584
Reproduced the original issue using the following build:

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:
- (changeset: 209717:50b689feab5f)
- (changeset: 218224:72c13d8631ff)
- (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 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)"
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.