New issue
Advanced search Search tips
Starred by 7 users
Status: Fixed
Owner:
Closed: Jan 2015
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug



Sign in to add a comment
HPKP enforces includeSubDomains even when the directive is not asserted
Reported by scott.he...@gmail.com, Dec 22 2014 Back to list
Chrome Version       : Version 39.0.2171.95 m
OS Version: Windows 8.1 Pro x64
URLs (if applicable) : https://scotthelme.co.uk
Other browsers tested:
Add OK or FAIL after other browsers where you have tested this issue:
     Safari 5: No HPKP Support
  Firefox 4.x: No HPKP Support
     IE 7/8/9: No HPKP Support

What steps will reproduce the problem?
1. Issue a HPKP policy header: add_header Public-Key-Pins "pin-sha256='X3pGTSOuJeEVw989IJ/cEtXUEmy52zs1TZQrU06KUKg='; pin-sha256='MHJYVThihUrJcxW6wcqyOISTXIsInsdj3xK8QrZbHec='; pin-sha256='isi41AizREkLvvft0IRW4u3XMFR2Yg7bvrF7padyCJg='; max-age=10";. Note the lack of the 'includeSubdomains' directive.
2. Visit the site to confirm delivery of the header. 
3. Try to access a subdomain of that site. The HPKP policy is enforced on all subdomains when it should not not be. Subdomains cannot be accessed as a result as their certificates are not included in the HPKP policy. 
4. Remove the HPKP policy header. 
5. Wait the duration of 'max-age' (10 seconds) so that the policy expires. 
6. Try to access a subdomain again. 
7. Subdomain is still inaccessible. Policy does not expire. 

What is the expected result?
The HPKP policy being issued on scotthelme.co.uk should not have any impact on subdomains unless the 'includeSubdomains' directive is included in the policy. The policy should also expire after you have not loaded the site for 10 seconds (max-age). 


What happens instead of that?
The HPKP policy is enforced on subdomains of scotthelme.co.uk rendering them inaccessible due to their certificates not being pinned in the policy. The policy also doesn't expire after the time specified in max-age has elapsed. 

Please provide any additional information below. Attach a screenshot if
possible.

I have included a screenshot showing the policy being delivered from scotthelme.co.uk and a screenshot of one of my subdomains after I have manually cleared the HPKP policy from chrome://net-internals/#hsts so that I could access the subdomain. This shows that the policy is not being delivered when visiting the subdomain so it should not be applied there. 

UserAgentString: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2171.95 Safari/537.36



 
HPKP subdomain browser.png
116 KB View Download
HPKP domain browser.png
103 KB View Download
Labels: TE-NeedFurtherTriage
Labels: -TE-NeedFurtherTriage TE-NeedsFurtherTriage
Cc: palmer@chromium.org rsleevi@chromium.org
Labels: Cr-Internals-Network-SSL
Labels: Needs-Feedback
Can you please attach a net-internals log as detailed in https://dev.chromium.org/for-testers/providing-network-details
I have captured a log of the fault happening and attached it.
net-internals-log.json
973 KB Download
Cc: davidben@chromium.org
Labels: Cr-Security
Summary: HPKP enforces includeSubDomains even when the directive is not asserted (was: HPKP not being enforced properly)
I think I see the bug. We don't actually treat the HSTS and HPKP include_subdomains flags orthogonally. GetDynamicDomainState will accept either HSTS or HPKP include_subdomains and not check again in CheckPublicKeyPins (or ShouldUpgradeToSSL) since it never sees the domain. A quick fix would be to pass the hostname into both and check again, though that would have strange side effects like: if you set HSTS + includeSubdomains on example.com, it would apply to foo.example.com unless foo.example.com sets a HPKP key pin. If foo.example.com then doesn't explicitly set HSTS, it won't have it. Likewise with HSTS and HPKP inverted.

We really should query the two independently and not tie their into the same mechanism. Either separate the structs completely or still return one jumbo struct but query separately within GetDynamicDomainState. The former is probably tidier. I don't think DomainState has any methods that cares about both.

https://code.google.com/p/chromium/codesearch#chromium/src/net/http/transport_security_state.cc&cl=GROK&l=847&gsn=GetDynamicDomainState
Would this also apply to the value of the max-age directive then? The max-age on my HPKP policy is set to 10, yet doesn't expire. I do have a max-age value of 31536000 on my HSTS policy. 
Yeah, I think the same mix-up is happening with max-age too. CheckPublicKeyPins doesn't re-check max-age.
Owner: davidben@chromium.org
Status: Assigned
I've started fiddling with this, so I'll go take the bug.
Status: Started
I did an initial pass at fixing this here:
https://codereview.chromium.org/826423009/
Linking since these seem related: https://code.google.com/p/chromium/issues/detail?id=412866. In addition, I'm not sure this is related, but it looks like the set of pins that are cached is not cleared for each valid response (each valid Public-Key-Pinning header is supposed to clear out the old entries and set it to the values in the latest response): https://code.google.com/p/chromium/issues/detail?id=412866#c9. 

Hrm, yeah, I can see how it ends up appending the old entries. I think my patch actually ended up fixing that too. I'll update it to include an explicit test.
Cc: agl@chromium.org
 Issue 412866  has been merged into this issue.
Project Member Comment 15 by bugdroid1@chromium.org, Jan 15 2015
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ffd3a3bf5a45013052f6ae319983a7a249f4db38

commit ffd3a3bf5a45013052f6ae319983a7a249f4db38
Author: davidben <davidben@chromium.org>
Date: Thu Jan 15 21:04:45 2015

Treat HSTS and HPKP state independently.

Although we have historically, and in static preloads, treated HSTS and HPKP as
part of the same underlying mechanism, the new headers consider them completely
orthogonal. Our current implementation has bugs where, particular where
includeSubdomains is involved, HPKP and HSTS entries get mixed together. This
CL does the following:

- Include separate domain strings for HPKP and HSTS in the output of
  GetDynamicDomainState. This allows net-internals to report on the two
  separately.

- Switch tests to query TransportSecurityState's public API rather than
  manipulate DomainState directly, to reduce dependency on it.

- Make AddHSTSHeader, AddHSTS, etc., follow the same codepath. Notably the
  header variants called GetDynamicDomainState to get the template which means
  an includeSubdomains HPKP state on a parent domain would get copied over.

- AddHPKPHeader no longer appends the old pins to the new set.

- Make DeleteAllDynamicDataSince clear STS and PKP state independently.
  Notably, the old version would almost never drop DomainState entries because
  pkp.last_observed would be uninitialized and never pass the check.

- Make GetDynamicDomainState stitch together the appropriate STS and PKP
  results to form its output DomainState. This avoids includeSubdomains and
  expiration from one mechanism interacting with that of another.

- Add tests for all this.

We should remove DomainState altogether and leave PKPState and STSState as
separate entities (with some consideration for how they were historically
stored on disk), but this CL leaves that alone for now.

BUG= 444511 

Review URL: https://codereview.chromium.org/826423009

Cr-Commit-Position: refs/heads/master@{#311734}

[modify] http://crrev.com/ffd3a3bf5a45013052f6ae319983a7a249f4db38/chrome/browser/resources/net_internals/hsts_view.js
[modify] http://crrev.com/ffd3a3bf5a45013052f6ae319983a7a249f4db38/chrome/browser/ui/webui/net_internals/net_internals_ui.cc
[modify] http://crrev.com/ffd3a3bf5a45013052f6ae319983a7a249f4db38/chrome/test/data/webui/net_internals/hsts_view.js
[modify] http://crrev.com/ffd3a3bf5a45013052f6ae319983a7a249f4db38/net/http/http_security_headers.cc
[modify] http://crrev.com/ffd3a3bf5a45013052f6ae319983a7a249f4db38/net/http/http_security_headers_unittest.cc
[modify] http://crrev.com/ffd3a3bf5a45013052f6ae319983a7a249f4db38/net/http/transport_security_state.cc
[modify] http://crrev.com/ffd3a3bf5a45013052f6ae319983a7a249f4db38/net/http/transport_security_state.h
[modify] http://crrev.com/ffd3a3bf5a45013052f6ae319983a7a249f4db38/net/http/transport_security_state_unittest.cc

Labels: M-42
Status: Fixed
And with that, I think it should be fixed. (Expect it to be in the next canary if you want to test it out.)
Sign in to add a comment