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

Issue 684306 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

Bypass <script> subresource integrity on xhtml page

Reported by acmesqua...@gmail.com, Jan 24 2017

Issue description

UserAgent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0

Steps to reproduce the problem:
Add bad integrity data to script on an xhtml page

What is the expected behavior?
The script is blocked

What went wrong?
The script runs

Did this work before? No 

Does this work in other browsers? Yes

Chrome version: 57.0.2986.0 dev (64-bit)  Channel: dev
OS Version: 
Flash Version:
 
integrity.xhtml
260 bytes View Download
Labels: Needs-Triage-M57

Comment 2 by phistuck@gmail.com, Jan 24 2017

Here is a test case (yours did not include test.js) -
data:application/xhtml+xml;charset=utf-8,<!DOCTYPE%20html>%0A<html%20xmlns%3D"http%3A%2F%2Fwww.w3.org%2F1999%2Fxhtml"%20xml%3Alang%3D"en">%0A<head>%0A%09<title>xhtml%20%2B%20subresource%20integrity<%2Ftitle>%0A<%2Fhead>%0A<body>%0A%09<p>Serve%20as%20application%2Fxhtml%2Bxml<%2Fp>%0A%09<script%20src%3D"data:text/javascript,alert('')"%20integrity%3D"sha384-junk_hash"><%2Fscript>%0A<%2Fbody>%0A<%2Fhtml>
Going to it should not show an alert dialog.
Status: Untriaged (was: Unconfirmed)
Also, if you go to -
data:text/html;charset=utf-8,<!DOCTYPE%20html>%0A<html%20xmlns%3D"http%3A%2F%2Fwww.w3.org%2F1999%2Fxhtml"%20xml%3Alang%3D"en">%0A<head>%0A%09<title>xhtml%20%2B%20subresource%20integrity<%2Ftitle>%0A<%2Fhead>%0A<body>%0A%09<p>Serve%20as%20application%2Fxhtml%2Bxml<%2Fp>%0A%09<script%20src%3D"data:text/javascript,alert('')"%20integrity%3D"sha384-junk_hash"><%2Fscript>%0A<%2Fbody>%0A<%2Fhtml>

It runs the script until you open the Developer Tools and reload, in which case, it does not run the script and emits a console error.
#2 The script runs
#3 Shows a CORS error because it's a dataURL

Expecting
Failed to find a valid digest in the 'integrity' attribute for resource 'test.js' with computed SHA-256 integrity 'aT6BbccVnoaZ9XEAfrQoLc0H81FovhdUuGdv3qPAz6U='. The resource has been blocked.
test.js
15 bytes View Download
Cc: jww@chromium.org mkwst@chromium.org
jww: can you please triage?

Comment 6 by mkwst@chromium.org, Feb 7 2017

Labels: -Needs-Triage-M57 Security_Impact-Stable M-56 Security_Severity-Low OS-Android OS-Chrome OS-Mac OS-Windows
Status: Available (was: Untriaged)
jww@ left the company, so he's pretty unlikely to triage.

This seems like a bug, we should probably fix it.
The direct cause for this is XMLDocumentParser doesn't use ClassicPendingScript and instead use ScriptResource directly, while the SRI check is implemented in ClassicPendingScript.

Comment 8 by mkwst@chromium.org, Jun 22 2017

Cc: -mkwst@chromium.org -jww@chromium.org
Owner: mkwst@chromium.org
Hrm. I misread this bug on the first pass; I thought we were failing closed, not failing open. Failing open is bad.

hiroshige@: is migrating XMLDocumentParser to ClassicPendingScript on your radar (as part of es6 modules)? Depending on how far out that is, it might make sense to just start failing SRI checks in XHTML documents until we can have a reasonable security story.

Comment 9 by mkwst@chromium.org, Jun 22 2017

Cc: hirosh...@chromium.org
(Actually CCing hiroshige@)
> hiroshige@: is migrating XMLDocumentParser to ClassicPendingScript on your radar (as part of es6 modules)?
It is tracked as a part of Issue 717643 but isn't planned to be done immediately.

Anyway, I think moving the SRI check to ScriptResource will be a better fix (See also Issue 735719).

Cc: -hirosh...@chromium.org mkwst@chromium.org
Components: Blink>HTML>Script
Labels: -OS-Linux -OS-Android -OS-Windows -OS-Chrome -OS-Mac OS-All
Owner: hirosh...@chromium.org
Status: Started (was: Available)
Summary: Bypass <script> subresource integrity on xhtml page (was: Bypass subresource integrity on xhtml page)
Assigning to myself, as introducing PendingScript to XMLDocumentParser turned out to be needed for module-script-related issues.
Draft CL: https://chromium-review.googlesource.com/c/557304 (I expect this is not mergeable though).
Project Member

Comment 12 by bugdroid1@chromium.org, Jun 30 2017

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

commit e7e993356ec37ca2afa5bd515bcaf1c908b55037
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Fri Jun 30 19:14:02 2017

Make PendingScriptClient GarbageCollectedMixin

Preparation for https://chromium-review.googlesource.com/c/557304
where XMLDocumentParser (that is already GarbageCollected) is changed
to be a subclass of PendingScriptClient.

Bug: 686281, 717643, 735719,  684306 
Change-Id: I700095279c4b8ab44fb725d716925663dcfe9ded
Reviewed-on: https://chromium-review.googlesource.com/557302
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#483785}
[modify] https://crrev.com/e7e993356ec37ca2afa5bd515bcaf1c908b55037/third_party/WebKit/Source/bindings/core/v8/ScriptStreamerTest.cpp
[modify] https://crrev.com/e7e993356ec37ca2afa5bd515bcaf1c908b55037/third_party/WebKit/Source/core/dom/PendingScript.h
[modify] https://crrev.com/e7e993356ec37ca2afa5bd515bcaf1c908b55037/third_party/WebKit/Source/core/dom/ScriptLoader.h
[modify] https://crrev.com/e7e993356ec37ca2afa5bd515bcaf1c908b55037/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.h

Project Member

Comment 13 by bugdroid1@chromium.org, Jul 12 2017

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

commit d99cd2ff4053dcd10366499bbc1695dac19a8832
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Wed Jul 12 04:12:38 2017

Use ExecuteScriptBlock() and PendingScript in XMLDocumentParser

Previously, XMLDOcumentParser uses ScriptResource directly.
This CL replaces the ScriptResource with PendingScript, in order to:
- Replace a call of ScriptLoader::ExecuteScript() with
  ExecuteScriptBlock() to unify load/error event handling within
  ExecuteScriptBlock(), as follow-up of
  https://chromium-review.googlesource.com/c/554098 (Issue 686281),
- Prepare for module script support in XHTMLs (Issue 717643), and
- Enable SRI check in XHTMLs ( Issue 684306 ).

Bug: 686281, 717643,  684306 
Change-Id: Ibedded79223d30952dee4c8370fc9eed2201a462
Reviewed-on: https://chromium-review.googlesource.com/557304
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Daniel Vogelheim <vogelheim@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485842}
[modify] https://crrev.com/d99cd2ff4053dcd10366499bbc1695dac19a8832/third_party/WebKit/LayoutTests/http/tests/security/subresourceIntegrity/shared-with-xhtml-expected.txt
[modify] https://crrev.com/d99cd2ff4053dcd10366499bbc1695dac19a8832/third_party/WebKit/LayoutTests/http/tests/security/subresourceIntegrity/shared-with-xhtml.html
[add] https://crrev.com/d99cd2ff4053dcd10366499bbc1695dac19a8832/third_party/WebKit/LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-blocked-xhtml-expected.txt
[add] https://crrev.com/d99cd2ff4053dcd10366499bbc1695dac19a8832/third_party/WebKit/LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-blocked-xhtml.xhtml
[modify] https://crrev.com/d99cd2ff4053dcd10366499bbc1695dac19a8832/third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp
[modify] https://crrev.com/d99cd2ff4053dcd10366499bbc1695dac19a8832/third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.h

Status: Fixed (was: Started)

Sign in to add a comment