Bug 264355 – Content Security Policy for previous load should not apply to subsequent alternate HTML load

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Michael Catanzaro]

Created attachment 468507 [details]
expected result

To reproduce, first load https://duckduckgo.com/ and then load https://expired.badssl.com/

In Epiphany, the expected result is for an insecure lock icon to be displayed on the TLS error page. But actually, the icon is blocked by DuckDuckGo's Content Security Policy, i.e. the CSP for the *previous* page is still being enforced for the next load, even though the next load is for a different website that has nothing to do with DuckDuckGo. This is probably specific to alternate HTML loads, but I'm not certain.

The TLS error page works fine if I visit https://expired.badssl.com/ directly without first loading https://duckduckgo.com/

(I assume it won't be possible to reproduce the exact same error in Safari as the TLS error page is surely constructed differently, but it seems unlikely that the underlying bug is platform-specific.)

[Michael Catanzaro]

Created attachment 468508 [details]
actual result

Almost forgot to provide the error message from the web inspector:

[Error] Refused to load ephy-resource:///org/gnome/epiphany/page-icons/channel-insecure-symbolic.svg because it does not appear in the img-src directive of the Content Security Policy.

[Patrick Griffis]

Note that the error page isn't a normal navigation, its `WebPage::loadAlternateHTML()`, possibly leaving some state behind.

[Radar WebKit Bug Importer]

<rdar://problem/118411558>

[Michael Catanzaro]

Seems this can also prevent CSS from loading, and even break the "Accept Risk and Proceed" button. See: https://gitlab.gnome.org/GNOME/epiphany/-/issues/2534

[Sebastian Schlatow]

Created attachment 473295 [details]
Page with "Accept risk and continue" Button and open browser console

[Michael Catanzaro]

Here is an inadequate fix:

diff --git a/Source/WebCore/style/StylePendingResources.cpp b/Source/WebCore/style/StylePendingResources.cpp
index 59a00d94a967..05fdc303d062 100644
--- a/Source/WebCore/style/StylePendingResources.cpp
+++ b/Source/WebCore/style/StylePendingResources.cpp
@@ -32,6 +32,7 @@
 #include "CursorData.h"
 #include "CursorList.h"
 #include "DocumentInlines.h"
+#include "DocumentLoader.h"
 #include "FillLayer.h"
 #include "RenderStyleInlines.h"
 #include "SVGURIReference.h"
@@ -53,7 +54,7 @@ static void loadPendingImage(Document& document, const StyleImage* styleImage, c
 
     bool isInUserAgentShadowTree = element && element->isInUserAgentShadowTree();
     ResourceLoaderOptions options = CachedResourceLoader::defaultCachedResourceOptions();
-    options.contentSecurityPolicyImposition = isInUserAgentShadowTree ? ContentSecurityPolicyImposition::SkipPolicyCheck : ContentSecurityPolicyImposition::DoPolicyCheck;
+    options.contentSecurityPolicyImposition = isInUserAgentShadowTree || document.loader()->substituteData().isValid() ? ContentSecurityPolicyImposition::SkipPolicyCheck : ContentSecurityPolicyImposition::DoPolicyCheck;
 
     if (!isInUserAgentShadowTree && document.settings().useAnonymousModeWhenFetchingMaskImages()) {
         switch (loadPolicy) {

This only fixes images, though. There are lots more places that will need to be patched unless we can fix it centrally.

We also probably need to solve bug #272590 at the same time, because I doubt the fix for that will be separate.

[Michael Catanzaro]

Problem is DocumentWriter::begin calls Document::inheritPolicyContainerFrom to inherit security policies from the NavigationAction triggeringAction. I don't understand why, though. Certainly we need to not do that when loading alternate HTML (which WebCore calls "substitute data").

I considered changing DocumentWriter::begin, but it seems cleanest to stop this inside Document::inheritPolicyContainerFrom instead:

diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
index cfe18fba1cb2..2dcc48640115 100644
--- a/Source/WebCore/dom/Document.cpp
+++ b/Source/WebCore/dom/Document.cpp
@@ -7748,6 +7748,11 @@ void Document::initContentSecurityPolicy()
 
 void Document::inheritPolicyContainerFrom(const PolicyContainer& policyContainer)
 {
+    // Substitute data is trusted and not subject to security policies. https://webkit.org/b/264355
+    if (RefPtr loader = this->loader())
+        if (loader->substituteData().isValid())
+            return;
+
     setContentSecurityPolicy(makeUnique<ContentSecurityPolicy>(URL { m_url }, *this));
     SecurityContext::inheritPolicyContainerFrom(policyContainer);
 }

I was hoping this would be enough to fix PDF documents too, but it's not.

Will submit a pull request next week.

Chris, if you have any opinion on this, that would be great, because I see you wrote PolicyContainer and so probably understand how it's expected to work.

[Ryan Reno]

The inheritance behavior comes from this spec algorithm:

https://html.spec.whatwg.org/multipage/browsers.html#determining-navigation-params-policy-container


In particular, a navigation to a local scheme is supposed to inherit from the initiator's policy which is what's happening here. Looks like the user agent is navigating to an about: scheme which has some CSS and/or JS which is being blocked by the pre-existing CSP.

I wonder if maybe the sourceDocument from the spec in this case should be something other than the document being navigated away from. Like maybe there's some other spec interaction I didn't consider that covers this case. At any rate, an exception for the case of loading alternate HTML seems reasonable.

[Michael Catanzaro]

I don't understand the algorithm, because about: pages are controlled by the web browser and are inherently always trusted, right? Well, I suppose we don't need to worry about that, because for whatever reason this isn't any problem for about: pages. We only need to fix substitute data loads. (And also PDF.js documents in bug #264355, though the problem there is different; it's caused by the CSP of the current page, rather than any inherited one.)

[Anne van Kesteren]

The HTML Standard assumes that only about:blank exists. I'm not entirely sure what you mean with inherently trusted, but about:blank typically inherits the origin from its creator.

Are substitute loads always in full control of the browser and result in a cross-origin document? From the perspective of the document starting the navigation? In that case special casing them is probably reasonable.

[Michael Catanzaro]

(In reply to Anne van Kesteren from comment #10)
> Are substitute loads always in full control of the browser

Sort of. The browser could theoretically load untrusted content if it wants to. That would be weird, though. In practice, they're used to create error pages (e.g. network error) and we should assume that's how it will be used.

> and result in a
> cross-origin document? From the perspective of the document starting the
> navigation? In that case special casing them is probably reasonable.

I think the origin would be, say, example.com, but the content of the page might be an error like "Cannot connect, check your internet connection!" with assets provided by the browser.

In my example, the previous document https://duckduckgo.com doesn't actually start the navigation; the navigation gets started by API request when I type the URL https://badssl.example.com into my browser. I would expect the new load to inherit nothing from duckduckgo.com, but this is not true. Since the spec allows for inheriting the policy container from history, this is surely intentional and not a bug, but I'm quite surprised since I had previously expected that a load started by API request would be a fresh start that's totally unrelated to any previously-loaded web content.

[Anne van Kesteren]

I think when the user types in a new address and that address is cross-site with the current document, we should probably do more to sever state.

You might want to double check on the origin as HTML does require a unique origin for error pages and that would seem wise as they are user-agent-controlled. You would not want it to interfere with state from example.com or for that to be possible.

[Patrick Griffis]

Created attachment 473397 [details]
WebKitGTK test case

A simple test case for GTK to reproduce this.

[Michael Catanzaro]

Oh this test case is great, thanks. I hate writing tests.

[Michael Catanzaro]

Pull request: https://github.com/WebKit/WebKit/pull/37456

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #7)
> I considered changing DocumentWriter::begin, but it seems cleanest to stop
> this inside Document::inheritPolicyContainerFrom instead:

After staring a bit more at the code, I changed my mind. The pull request touches DocumentWriter::begin instead.

[EWS]

Committed 288026@main (a2b811f9d215): <https://commits.webkit.org/288026@main>

Reviewed commits have been landed. Closing PR #37456 and removing active labels.