New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 625945 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Buried. Ping if important.
Closed: Jul 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug-Security



Sign in to add a comment

Security: browser history sniffing via HSTS + CSP (bypass previous fix)

Reported by xiaoyi...@outlook.com, Jul 6 2016

Issue description

VULNERABILITY DETAILS
This vulnerability is about bypassing the fix to CVE-2016-1617 ( Issue 544765 , see https://bugs.chromium.org/p/chromium/issues/detail?id=544765). The fix proposed in that bug was to make "img-src http:" === "img-src http: https:" (See comment 19 in that bug).

But there is a flaw in this fix: "img-src http://example.com:80" is not equivalent to "img-src http://example.com:80 https://example.com:443"; rather, it allows http://example.com/ while blocks https://example.com/. So by simply modifying the CSP header, the trick (using HSTS or 301 redirect) works again.

Note that you must list the domain names explicitly in the CSP (i.e. "img-src http://*:80" doesn't block https://example.com:443).

This bug is also reproducible in latest stable version of Firefox.

VERSION
Chrome Version: [51.0.2704.106 m] + [stable]
Operating System: [Windows 10 x64 Version 1511]

REPRODUCTION CASE
Step 1: Download the attached file. 
Step 2: Open incognito window. Then open http://www.npmjs.com/ and then http://www.chase.com/.
Step 3: Open the PoC in the same window. You should see that www.npmjs.com is visited (this is a non-preloaded HSTS domain). Maybe it shows http://www.chase.com/ is visited too (this domain has a 301 redirect to https), but in my tests most of the times Chrome doesn't cache the 301 redirect, and I haven't figured out why, but I don't think this affects the exploitability of this vulnerability.
 
csp probing.html
3.7 KB View Download
Also reported this issue to Mozilla: https://bugzilla.mozilla.org/show_bug.cgi?id=1285003.
Cc: jww@chromium.org
Components: UI>Browser>History Blink>SecurityFeature Privacy
Owner: mkwst@chromium.org
Status: Assigned (was: Unconfirmed)
Foiled yet again by port trickery. :-(

Assigning to OWP folks.

Comment 3 by mkwst@chromium.org, Jul 6 2016

Labels: Security_Severity-Medium ReleaseBlock-Stable
Yup. I'll go drill another hole in CSP that maps `:80` onto `:80` and `:443`, I suppose. Since HSTS doesn't modify non-standard ports, that should address the bulk of the remaining surface area. Does that sound reasonable?

Giving this the same flags as  issue 544765  (though "medium" sounds high, honestly).
It sounds reasonable to me.
mkwst: HSTS affects all ports. ;-)

" This specification also incorporates notions from [JacksonBarth2008]
   in that policy is applied on an "entire-host" basis: it applies to
   HTTP (only) over any TCP port of the issuing host."
https://tools.ietf.org/html/rfc6797#section-1

"if the URI contains an explicit port component that is not
          equal to "80", the port component value MUST be preserved;
          otherwise,"
https://tools.ietf.org/html/rfc6797#section-8.3
Is this Security_Impact-Stable? Also this should have a Milestone.
Labels: -ReleaseBlock-Stable Security_Impact-Stable M-52
Yeah, it probably affects stable. ReleaseBlock-Stable isn't appropriate, since this is not a regression.

However, M52 reaches stable in 3 weeks; I'll let Mike decide if that's feasible. :-)

Comment 8 by mkwst@chromium.org, Jul 7 2016

Cc: jochen@chromium.org
lgarron@: Right. HSTS doesn't _modify_ non-standard ports, it simply changes the protocol. That means that there shouldn't be (??) any other port-related edge cases to worry about after https://github.com/w3c/webappsec-csp/commit/22d08b990290e49f5a666fad08de16d75bb369e7#diff-117d6498d2aa8019cc0abf5eeb87a9fa.

Patch up at https://codereview.chromium.org/2125873003. CCing Jochen on this bug, since I've asked him to review that patch. :)
Project Member

Comment 9 by bugdroid1@chromium.org, Jul 7 2016

Project Member

Comment 10 by sheriffbot@chromium.org, Jul 7 2016

Labels: Pri-1
Cc: dba...@webkit.org wilan...@apple.com ckerschb...@mozilla.com
Labels: Merge-Request-52 Merge-Request-53
Preemptively requesting a merge back to 53 and 52. The patch is small and should be pretty low risk. I don't think it's worth trying to merge into 51, but if things don't explode on Canary tomorrow, I'd like to pull this patch back to beta. 

+{dbates,johnwilander}@webkit.org, ckerschbaumer@mozilla.com who might want to pull similar patches for their respective browsers. I don't have access to the Bugzilla bug or I'd just comment there.

xiaoyin.li@: It's probably worth copy/pasting this report to bugs.webkit.org as well.
> HSTS doesn't _modify_ non-standard ports, it simply changes the protocol

Ah, yes, you're correct, sorry.
The spec explicitly specifies that port 80 is mapped to 443, and nothing else.

The scheme is modified, but I guess we already have that covered.
I just tested the PoC on Microsoft Edge 13. Edge is not vulnerable to this attack, but Microsoft mitigated it in a different way. Simply speaking, Edge follows the http-to-https redirect and it calls the onerror handler after the https request is finished. (From the console, I am sure that the onerror event was triggered due to CSP violation rather than content-type mismatch).

I personally think Edge's behavior is better than the fix proposed here, as 1) it doesn't need to change the CSP spec; 2) it fixes the underlying issue, which is that CSP allows timing intermediate state of a HTTP request; 3) it won't cause any compatibility issue: what if a website really want to allow http and block https, maybe to mitigate DDoS?

Could Chrome match the Edge's behavior as a fix to this issue? What do you think?
I made a mistake in the previous comment, DDoS is irrelevant here, since in Edge, the https request is still sent to the server; it just doesn't display it on the page.
xiaoyin.l@: I don't think the proposed mitigation helps.

First, you can improve your PoC by using the `securitypolicyviolation` event, which will certainly trigger if we block the request to the upgraded URL. That is, this isn't a timing issue, it's a fundamental issue with the interaction between CSP's current behavior and HSTS. If we allow the one to block the other, then we're vulnerable to the information leakage you're describing.

Second, CSP ought to be blocking outgoing requests if it's to be effective at doing the work it's trying to do. That is, if we don't change the existing behavior, then it's not at all clear to me that allowing requests to go through despite violating the policy is a reasonable thing to do.

Third, I think I'm philosophically opposed to allowing a site to lock itself into insecure transport, if for no other reason than the fact that it would present a barrier to eventual migration to TLS. :)
Project Member

Comment 17 by sheriffbot@chromium.org, Jul 8 2016

Status: Fixed (was: Assigned)
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
@Mike West: You are right. Thanks for your explanation!
Project Member

Comment 19 by sheriffbot@chromium.org, Jul 9 2016

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: reward-topanel
Before we approve merge to M52, Could you please confirm whether this change is baked/verified in Canary and safe to merge?

Also is this change applicable to all OS or any specific OS?

Comment 22 by mkwst@chromium.org, Jul 15 2016

Labels: OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
> Before we approve merge to M52, Could you please confirm whether this change is baked/verified in Canary and safe to merge?

Patch landed a week ago and hasn't exploded yet. The bulk of the change is ~2 lines in CSPSource that are fairly straightforward to visually confirm. I think it's safe to merge.

> Also is this change applicable to all OS or any specific OS?

All OSs that use Blink (that is, everything except iOS). I've marked the OS checkboxes accordingly.
Cc: awhalley@chromium.org
Thank you  mkwst@@. 

+awhalley@ to decide whether we can take this merge in for M52 as it is only baked in canary (not baked in dev/beta yet).
Externally medium merging to Beta (just!) so we're good here; and I agree the diffs look reasonable.
Labels: -Merge-Request-52 -Merge-Request-53 Merge-Approved-53 Merge-Approved-52
Approving merge to M53 branch 2785 and M52 branch 2743 based on comment #24. Please merge ASAP (possibly before 5:00 PM PST today, Friday or latest by 4:00 PM PST on Monday).

Comment 26 Deleted

Project Member

Comment 27 by bugdroid1@chromium.org, Jul 15 2016

Labels: -merge-approved-53 merge-merged-2785
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/17f5f3aa721a2b0069174f29e38d7d48cea05945

commit 17f5f3aa721a2b0069174f29e38d7d48cea05945
Author: Mike West <mkwst@google.com>
Date: Fri Jul 15 18:50:45 2016

CSP: Allow ':80' to match ':443' in source expressions.

https://github.com/w3c/webappsec-csp/commit/22d08b990290e49f5a666fad08de16d75bb369e7#diff-117d6498d2aa8019cc0abf5eeb87a9fa
updated CSP to allow insecure ports to match secure ports in source
expressions. This is a refinement of the change that landed in
https://codereview.chromium.org/1455973003 to address Sniffly.

BUG= 625945 
R=jochen@chromium.org

Review-Url: https://codereview.chromium.org/2125873003
Cr-Commit-Position: refs/heads/master@{#404127}
(cherry picked from commit e6d181417ea462ac221d768c960a21018266a4a8)

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

Cr-Commit-Position: refs/branch-heads/2785@{#161}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/17f5f3aa721a2b0069174f29e38d7d48cea05945/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp
[modify] https://crrev.com/17f5f3aa721a2b0069174f29e38d7d48cea05945/third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp

Project Member

Comment 28 by bugdroid1@chromium.org, Jul 15 2016

Labels: -merge-approved-52 merge-merged-2743
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6ebff0f8de665d2ea2a7d109ef1aeca753993396

commit 6ebff0f8de665d2ea2a7d109ef1aeca753993396
Author: Mike West <mkwst@google.com>
Date: Fri Jul 15 18:56:00 2016

CSP: Allow ':80' to match ':443' in source expressions.

https://github.com/w3c/webappsec-csp/commit/22d08b990290e49f5a666fad08de16d75bb369e7#diff-117d6498d2aa8019cc0abf5eeb87a9fa
updated CSP to allow insecure ports to match secure ports in source
expressions. This is a refinement of the change that landed in
https://codereview.chromium.org/1455973003 to address Sniffly.

BUG= 625945 
R=jochen@chromium.org

Review-Url: https://codereview.chromium.org/2125873003
Cr-Commit-Position: refs/heads/master@{#404127}
(cherry picked from commit e6d181417ea462ac221d768c960a21018266a4a8)

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

Cr-Commit-Position: refs/branch-heads/2743@{#650}
Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939}

[modify] https://crrev.com/6ebff0f8de665d2ea2a7d109ef1aeca753993396/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp
[modify] https://crrev.com/6ebff0f8de665d2ea2a7d109ef1aeca753993396/third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp

Labels: Release-0-M52
Labels: CVE-2016-5137
Labels: reward-1000 reward-unpaid
Our panel has decided to award $1,000 for this report!  A member of our finance team will be in touch in the next few weeks.
Labels: -reward-topanel
Thank you everyone for the quick fix, and thank you for the bounty!
Labels: -reward-unpaid reward-inprocess
Project Member

Comment 36 by sheriffbot@chromium.org, Oct 14 2016

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: CVE_description-submitted

Sign in to add a comment