Restrict <base> to ~sane schemes.

[mikewest]

Apparently, Chrome is happy to support <base href="data:/randomness/">, which doesn't seem like a good idea. WDYT about restricting document base URLs to network schemes (and, I suppose, the scheme of the current document's URL's origin... file: comes to mind).

[annevk]

What's the reason for restricting it?

If we do this, we need to update quite a number of /url tests that rely on manipulating the base element to work.

[lweichselbaum]

If you can inject a base tag with data:, you can control all script tags with relative paths (in Chrome) and bypass nonce based CSPs.
E.g.: <script src=x.js nonce=random-123></script>

While I think it makes sense to fix this, it must be noted that the more generic form of this attack (using an attacker controlled domain) would still work.

<script src=x.js nonce=random-123></script>

[annevk]

Okay, so the assumption is that an attacker can inject a data URL <base>, but not an HTTP URL <base>?

[mikewest]

I don't think this change has ~any value in preventing XSS. As you both note, the attacker can inject <base href='https://evil.com/'>. We accept the risk of the one (which we have to, because I'm pretty sure that cross-origin <base> is too widely used to discard), so we accept the risk of the other.

That said, it's not clear to me that it makes sense to include non-network schemes (and particularly non-hierarchical schemes) as potential document base URLs. I'm not suggesting that we remove the resolution rules set up in the URL spec; I am suggesting that we poke at the contours of what we allow for HTML documents. data:/path is a totally reasonable (for some definition of "reasonable") URL, and it should have resolution rules. That doesn't mean it should also be usable as a document base URL. :)

[mikewest]

The specific thing that triggered this suggestion is actually a mismatch between the way the URL parser handles data: URLs, and the way Chrome handles extracting a body from data: URLs. That is, data:/,alert(1) is parsed as a URL with a scheme of data: and a path of /,alert(1), but turns into a resource with a body of alert(1) and a type of / when fetched. That's somewhat surprising.

[mikewest]

I've landed https://codereview.chromium.org/2626243002 in Chrome to gather some metrics.

While we're here, it might also be reasonable to mitigate some dangling markup issues in target by restricting its character set to exclude things like < and \r\n. Let's see what the numbers look like.

[mikewest]

@annevk, any thoughts on #2249 (comment)? Based on early numbers from the last week or so of Chrome's beta (which is, admittedly not a truly representative sample), data: appeared exactly zero times as a base URL.

Also, newlines in target appeared 5 times, and < 16 times. I'd like to restrict those as well. :)

[annevk]

Not really, that it works seems expected from how the system is designed. Though data URL parsing/interpretation could use some better interoperability and standard here and there.

As for adding defense-in-depth restrictions, it seems reasonable if we can get everyone to implement them. Bit ambivalent on the whole thing myself.

[mikewest]

FWIW, based on quick experimentation with https://output.jsbin.com/zucihoraca:

• Safari matches Chrome's behavior
• Edge ignores data: inside <base> (it tries to request the original resource)
• Firefox allows data: URLs in <base>, but doesn't request the resulting resource. I haven't dug into the details of why.

I'd like to change Chrome's behavior to align with either Edge or Firefox. Edge's behavior makes more sense to me. If you're ambivalent, that's what I'd prefer to do.

[annevk]

You will also need to test multiple <base> elements to see if its ignored or used but then ignored.

[mikewest]

All browsers seem to ignore all but the first <base> element, as specified.

I should be more clear about what I mean when I say "ignored". The patch to HTML that I'd propose would be something like changing step 3 of https://html.spec.whatwg.org/multipage/semantics.html#set-the-frozen-base-url to:

3. Set element's frozen base URL to document's fallback base URL, if urlRecord is failure, or urlRecord's scheme is data, or running "Is base allowed for Document?" on the resulting URL record and document returns "Blocked", and to urlRecord otherwise.

That is, we'd treat <base href="data:whatever"> just like <base href="http::::///////not-valid">. It would have a frozen base url of the document's fallback base url, and we'd ignore any subsequent <base> tags.

[bzbarsky]

For what it's worth, the way Firefox handles this (in URL spec terms) is by always setting the "cannot-be-a-base-URL" flag on data: URLs.

[bzbarsky]

Wait, why are we changing web platform tests before we've agreed on what the spec should say here?

[annevk]

Yeah I'm not sure why that happened. @jeffcarp that should not have been merged.

[annevk]

I created whatwg/meta#17 to clarify the CONTRIBUTOR guidelines around this so that it hopefully doesn't happen again.

[zcorpan]

I guess the problem here was that https://codereview.chromium.org/2685843003 landed before this PR, and tests are automatically upstreamed to web-platform-tests.

[zcorpan]

I sent a message to blink-dev about this https://groups.google.com/a/chromium.org/d/msg/blink-dev/QIRcSgN9AK4/0JY58rBhCAAJ

[RByers]

Yes, this test change shouldn't have been landed until the spec was updated. Sorry about this, we're still getting used to our new system encouraging blink developers to land test changes along with their blink patches (which then get automatically merged back to WPT). We'll work on improving our docs and guidance to reduce mistakes like this. In the meantime, please feel free to revert anything you feel was landed in error.

We've been talking lots with @jgraham and others about the inevitable conflicts/mistakes like this as we all work to make WPT a first-class part of our engine development process, but we're all optimistic it'll be worth it in the end :-)

/cc @foolip

[foolip]

We've anticipated some mishaps of this sort. Just yesterday I noticed an unexpected test change in servo/servo@8b69e73 and it was quickly dealt with, so let's keep learning how to do this together :)

@mikewest, can you write a PR that matches what you'd like to ship, or is it entangled in a bunch of other questions which we have yet to decide on?

[zcorpan]

@mikewest have you considered Gecko's approach (see #2249 (comment))? That doesn't just affect <base> but also new URL('bar', 'data:/,/foo') etc.

[annevk]

Gecko's approach would require changing the URL Standard and special casing data URLs in it. That does not seem great.

[annevk]

(And would still leave the hole for javascript URLs, unless we also change the URL Standard for them.)

[zcorpan]

Right. Can you explain what is bad about it? It's not clear to me. :-)

[annevk]

Coming back to this, I don't see how the CSS case is a security problem. The <base> case is a marginal security problem as it can lead to script execution, assuming the attacker can inject data: but not https:. Consider that cross-origin scripts could be banned through CSP.

The moment Gecko aligns its URL parser it would also end up being vulnerable to that case.

Given that Chromium and WebKit already implement a mitigation I think we should standardize on that and have Gecko align to prevent it from introducing a novel XSS angle for itself once it aligns its URL parser.

Persuasive enough @zcorpan @mozfreddyb?

(I discovered that a single url/ test in Interop 2023 leads back to this issue. Currently failing in Chromium and WebKit. https://wpt.fyi/results/url/a-element.html%3Fexclude%3D(file%7Cjavascript%7Cmailto) search for <#x> against <data:,>.)

[zcorpan]

My preference would have been to disallow data: etc as base URLs, but it didn't happen. So, at this point, I agree.

For #2249 (comment) maybe we could have a depth limit for @import?

[annevk]

Great, I'll work on a PR.

I wonder why the @import case doesn't result in recursion in WebKit, but it does seem reasonable to add a check if Chromium's behavior is indeed correct as you could have a website hang without script and I vaguely recall us preventing that in other cases.

[zcorpan]

In STP I get this message in the console:

Did not parse stylesheet at 'data:/,%20@import%20url('x%20/**/');' because non CSS MIME types are not allowed for cross-origin stylesheets.

[annevk]

We discussed the CSS issue further on Chat and as a result Simon filed these bugs:

• https://bugs.chromium.org/p/chromium/issues/detail?id=1477672
• https://bugzilla.mozilla.org/show_bug.cgi?id=1850965

No specification change should be needed for that scenario. (The main scenario was fixed in #9665.)