Enforce CSP base-uri in HTML preload scanner [chromium/src : main]
Andrew Paseltiner (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Kouhei Ueno
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
What's the threat model here? We don't currently enforce CSP for any of the other directives, and CSP's official threat model doesn't exclude exfiltration (and even if it did, that seems hard to pull off from static HTML that the preload scanner handles).
Related details
- Andrew Paseltiner
- Kouhei Ueno
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Paseltiner (Gerrit)
Andrew Paseltiner added 1 comment![]( "Open in Gerrit")
What's the threat model here? We don't currently enforce CSP for any of the other directives, and CSP's official threat model doesn't exclude exfiltration (and even if it did, that seems hard to pull off from static HTML that the preload scanner handles).
IIUC, we actually do enforce CSP for the other directives, as they go through `BaseFetchContext::CheckCSPForRequest`. It's just that in this case, the URLs are eagerly resolved against the document's base URL, instead of being resolved at the time the request is actually being made, meaning that the base-uri directive is never being considered. The linked bug has more details.
Related details
- Kouhei Ueno
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
Andrew PaseltinerWhat's the threat model here? We don't currently enforce CSP for any of the other directives, and CSP's official threat model doesn't exclude exfiltration (and even if it did, that seems hard to pull off from static HTML that the preload scanner handles).
IIUC, we actually do enforce CSP for the other directives, as they go through `BaseFetchContext::CheckCSPForRequest`. It's just that in this case, the URLs are eagerly resolved against the document's base URL, instead of being resolved at the time the request is actually being made, meaning that the base-uri directive is never being considered. The linked bug has more details.
Thanks for pushing back!
You're right that the preload request path does end up with (header-based) CSP checks (and I believe CSP <meta> disables preloading, to avoid unauthorized requests in that case). The threat model behind the latter is unclear to me tbh, but that's a topic for a different conversation.
Just to see that I understand why base-uri needs special casing here - we can't enforce base-uri restrictions at part of requestResource as we don't know there is the resource was fetched using a relative URL that relied on base URI?
Related details
- Andrew Paseltiner
- Kouhei Ueno
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Paseltiner (Gerrit)
Andrew Paseltiner added 1 comment![]( "Open in Gerrit")
Andrew PaseltinerWhat's the threat model here? We don't currently enforce CSP for any of the other directives, and CSP's official threat model doesn't exclude exfiltration (and even if it did, that seems hard to pull off from static HTML that the preload scanner handles).
Yoav Weiss (@Shopify)IIUC, we actually do enforce CSP for the other directives, as they go through `BaseFetchContext::CheckCSPForRequest`. It's just that in this case, the URLs are eagerly resolved against the document's base URL, instead of being resolved at the time the request is actually being made, meaning that the base-uri directive is never being considered. The linked bug has more details.
Thanks for pushing back!
You're right that the preload request path does end up with (header-based) CSP checks (and I believe CSP <meta> disables preloading, to avoid unauthorized requests in that case). The threat model behind the latter is unclear to me tbh, but that's a topic for a different conversation.
Just to see that I understand why base-uri needs special casing here - we can't enforce base-uri restrictions at part of requestResource as we don't know there is the resource was fetched using a relative URL that relied on base URI?
Correct. Consider this document being preloaded:
```html
<base href="https://foo.example">
<img src="/bar">
```
Without this CL, `/bar` is eagerly resolved to `https://foo.example/bar` and then fetched, meaning we've lost knowledge of whether there was a base URL at all by the time CSP comes into play. The alternative (Option 2 from the commit description) would be to avoid eager resolution at all (which I actually prefer from a code deduplication perspective) and instead let the fetch itself handle that, but IIUC it complicates the preload scanner's logic. If the reviewers push back though, we could consider it.
Related details
- Kouhei Ueno
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
Andrew PaseltinerWhat's the threat model here? We don't currently enforce CSP for any of the other directives, and CSP's official threat model doesn't exclude exfiltration (and even if it did, that seems hard to pull off from static HTML that the preload scanner handles).
Yoav Weiss (@Shopify)IIUC, we actually do enforce CSP for the other directives, as they go through `BaseFetchContext::CheckCSPForRequest`. It's just that in this case, the URLs are eagerly resolved against the document's base URL, instead of being resolved at the time the request is actually being made, meaning that the base-uri directive is never being considered. The linked bug has more details.
Andrew PaseltinerThanks for pushing back!
You're right that the preload request path does end up with (header-based) CSP checks (and I believe CSP <meta> disables preloading, to avoid unauthorized requests in that case). The threat model behind the latter is unclear to me tbh, but that's a topic for a different conversation.
Just to see that I understand why base-uri needs special casing here - we can't enforce base-uri restrictions at part of requestResource as we don't know there is the resource was fetched using a relative URL that relied on base URI?
Correct. Consider this document being preloaded:
```html
<base href="https://foo.example">
<img src="/bar">
```Without this CL, `/bar` is eagerly resolved to `https://foo.example/bar` and then fetched, meaning we've lost knowledge of whether there was a base URL at all by the time CSP comes into play. The alternative (Option 2 from the commit description) would be to avoid eager resolution at all (which I actually prefer from a code deduplication perspective) and instead let the fetch itself handle that, but IIUC it complicates the preload scanner's logic. If the reviewers push back though, we could consider it.
I'm not opposed to copying in the CSP directives, as it can enable genuine support for <meta> CSP in the future (given that apparently we think it's important).
Thinking through this, while I'm not sure there's a security risk, the spec does seem to require these requests are not sent to servers. Given that this is web-exposed, should we also add a WPT here?
Related details
- Andrew Paseltiner
- Antonio Sartori
- Kouhei Ueno
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Paseltiner (Gerrit)
Andrew Paseltiner added 1 comment![]( "Open in Gerrit")
Andrew PaseltinerWhat's the threat model here? We don't currently enforce CSP for any of the other directives, and CSP's official threat model doesn't exclude exfiltration (and even if it did, that seems hard to pull off from static HTML that the preload scanner handles).
Yoav Weiss (@Shopify)IIUC, we actually do enforce CSP for the other directives, as they go through `BaseFetchContext::CheckCSPForRequest`. It's just that in this case, the URLs are eagerly resolved against the document's base URL, instead of being resolved at the time the request is actually being made, meaning that the base-uri directive is never being considered. The linked bug has more details.
Andrew PaseltinerThanks for pushing back!
You're right that the preload request path does end up with (header-based) CSP checks (and I believe CSP <meta> disables preloading, to avoid unauthorized requests in that case). The threat model behind the latter is unclear to me tbh, but that's a topic for a different conversation.
Just to see that I understand why base-uri needs special casing here - we can't enforce base-uri restrictions at part of requestResource as we don't know there is the resource was fetched using a relative URL that relied on base URI?
Yoav Weiss (@Shopify)Correct. Consider this document being preloaded:
```html
<base href="https://foo.example">
<img src="/bar">
```Without this CL, `/bar` is eagerly resolved to `https://foo.example/bar` and then fetched, meaning we've lost knowledge of whether there was a base URL at all by the time CSP comes into play. The alternative (Option 2 from the commit description) would be to avoid eager resolution at all (which I actually prefer from a code deduplication perspective) and instead let the fetch itself handle that, but IIUC it complicates the preload scanner's logic. If the reviewers push back though, we could consider it.
I'm not opposed to copying in the CSP directives, as it can enable genuine support for <meta> CSP in the future (given that apparently we think it's important).
Thinking through this, while I'm not sure there's a security risk, the spec does seem to require these requests are not sent to servers. Given that this is web-exposed, should we also add a WPT here?
I was wondering about WPT myself, but I'm not sure how preloading works with it exactly. Any suggestions?
Related details
- Antonio Sartori
- Kouhei Ueno
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
is_valid_base_url = false;IIUC, this won't skip preloading, but will preload resources while assuming that a base URL is not defined. That doesn't seem like the correct behavior
Andrew Paseltiner (Gerrit)
Andrew Paseltiner added 1 comment![]( "Open in Gerrit")
is_valid_base_url = false;IIUC, this won't skip preloading, but will preload resources while assuming that a base URL is not defined. That doesn't seem like the correct behavior
IIUC that actually is the correct behavior: https://html.spec.whatwg.org/multipage/semantics.html#set-the-frozen-base-url.
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 2 comments![]( "Open in Gerrit")
What's the threat model here? We don't currently enforce CSP for any of the other directives, and CSP's official threat model doesn't exclude exfiltration (and even if it did, that seems hard to pull off from static HTML that the preload scanner handles).
Yoav Weiss (@Shopify)IIUC, we actually do enforce CSP for the other directives, as they go through `BaseFetchContext::CheckCSPForRequest`. It's just that in this case, the URLs are eagerly resolved against the document's base URL, instead of being resolved at the time the request is actually being made, meaning that the base-uri directive is never being considered. The linked bug has more details.
Andrew PaseltinerThanks for pushing back!
You're right that the preload request path does end up with (header-based) CSP checks (and I believe CSP <meta> disables preloading, to avoid unauthorized requests in that case). The threat model behind the latter is unclear to me tbh, but that's a topic for a different conversation.
Just to see that I understand why base-uri needs special casing here - we can't enforce base-uri restrictions at part of requestResource as we don't know there is the resource was fetched using a relative URL that relied on base URI?
Yoav Weiss (@Shopify)Correct. Consider this document being preloaded:
```html
<base href="https://foo.example">
<img src="/bar">
```Without this CL, `/bar` is eagerly resolved to `https://foo.example/bar` and then fetched, meaning we've lost knowledge of whether there was a base URL at all by the time CSP comes into play. The alternative (Option 2 from the commit description) would be to avoid eager resolution at all (which I actually prefer from a code deduplication perspective) and instead let the fetch itself handle that, but IIUC it complicates the preload scanner's logic. If the reviewers push back though, we could consider it.
Andrew PaseltinerI'm not opposed to copying in the CSP directives, as it can enable genuine support for <meta> CSP in the future (given that apparently we think it's important).
Thinking through this, while I'm not sure there's a security risk, the spec does seem to require these requests are not sent to servers. Given that this is web-exposed, should we also add a WPT here?
I was wondering about WPT myself, but I'm not sure how preloading works with it exactly. Any suggestions?
What you can do is add a WPT with a blocking script that document.write()s an HTML comment, and then have HTML that loads resources after that. Those resources would get preloaded, but never actually loaded.
Then you can use resource timing to see what loads were triggers and assert that the behavior matches what you'd expect.
I guess you could also just verify that resources from an unallowed base-uri origin are never loaded (again using resource timing)
is_valid_base_url = false;Andrew PaseltinerIIUC, this won't skip preloading, but will preload resources while assuming that a base URL is not defined. That doesn't seem like the correct behavior
IIUC that actually is the correct behavior: https://html.spec.whatwg.org/multipage/semantics.html#set-the-frozen-base-url.
Huh! That's.. an interesting choice. I guess that makes sense for XSS script dynamically trying to change the page's base.
Related details
- Andrew Paseltiner
- Antonio Sartori
- Kouhei Ueno
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Antonio Sartori (Gerrit)
Antonio Sartori added 1 comment![]( "Open in Gerrit")
if (!CSPDirectiveListAllowFromSource(
*policy, nullptr, CSPDirectiveName::BaseURI, document_url_, url,
url, ResourceRequest::RedirectStatus::kNoRedirect,
ReportingDisposition::kSuppressReporting)) {I am a slightly unhappy with calling CSPDirectiveListAllowFromSource directly. The design idea of the CSP checks was that you would only call methods from content_security_policy.h using a `ContentSecurityPolicy` object.
Maybe it would make sense to add into content_security_policy.h a static method for checking base-uri without reporting. In this way we can add unit tests there and it's easier to make sure that everything keeps working even with a null ContentSecurityPolicy.
Related details
- Andrew Paseltiner
- Kouhei Ueno
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Paseltiner (Gerrit)
Andrew Paseltiner voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andrew Paseltiner
| Commit-Queue | +1 |
1 comment
if (!CSPDirectiveListAllowFromSource(
*policy, nullptr, CSPDirectiveName::BaseURI, document_url_, url,
url, ResourceRequest::RedirectStatus::kNoRedirect,
ReportingDisposition::kSuppressReporting)) {I am a slightly unhappy with calling CSPDirectiveListAllowFromSource directly. The design idea of the CSP checks was that you would only call methods from content_security_policy.h using a `ContentSecurityPolicy` object.
Maybe it would make sense to add into content_security_policy.h a static method for checking base-uri without reporting. In this way we can add unit tests there and it's easier to make sure that everything keeps working even with a null ContentSecurityPolicy.
- Antonio Sartori
- Kouhei Ueno
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Antonio Sartori (Gerrit)
Antonio Sartori added 1 comment![]( "Open in Gerrit")
const KURL& document_url) {I think `document_url` does nothing, and you should be able to remove this parameter and pass an empty url below.
Related details
- Andrew Paseltiner
- Kouhei Ueno
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Paseltiner (Gerrit)
Andrew Paseltiner voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andrew Paseltiner
| Commit-Queue | +1 |
1 comment
I think `document_url` does nothing, and you should be able to remove this parameter and pass an empty url below.
- Antonio Sartori
- Kouhei Ueno
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Antonio Sartori (Gerrit)
Antonio Sartori voted and added 1 comment![]( "Open in Gerrit")
Votes added by Antonio Sartori
| Code-Review | +1 |
1 comment
Thanks! The CSP logic makes sense to me, it's a bit unfortunate that it is a bit hacky because of the cross-thread issue but I don't see much better options.
I'll defer to the other owners for the PreloadScanner, of which I have only limited understanding.
Related details
- Andrew Paseltiner
- Kouhei Ueno
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) voted and added 2 comments![]( "Open in Gerrit")
Votes added by Yoav Weiss (@Shopify)
| Code-Review | +1 |
2 comments
LGTM % nit
I really think we want WPTs here, but maybe we can add them in a followup.
document->GetExecutionContext()->GetContentSecurityPolicy()) {Maybe
```
if (ExecutionContext* context = document->GetExecutionContext()) {
if (ContentSecurityPolicy* csp = context->GetContentSecurityPolicy()) {
...
}
}
```
Related details
- Andrew Paseltiner
- Kouhei Ueno
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Paseltiner (Gerrit)
Andrew Paseltiner voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andrew Paseltiner
| Commit-Queue | +2 |
1 comment
document->GetExecutionContext()->GetContentSecurityPolicy()) {Maybe
```
if (ExecutionContext* context = document->GetExecutionContext()) {
if (ContentSecurityPolicy* csp = context->GetContentSecurityPolicy()) {
...
}
}
```
- Kouhei Ueno
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Paseltiner (Gerrit)
Andrew Paseltiner voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andrew Paseltiner
| Commit-Queue | +2 |
What's the threat model here? We don't currently enforce CSP for any of the other directives, and CSP's official threat model doesn't exclude exfiltration (and even if it did, that seems hard to pull off from static HTML that the preload scanner handles).
Yoav Weiss (@Shopify)IIUC, we actually do enforce CSP for the other directives, as they go through `BaseFetchContext::CheckCSPForRequest`. It's just that in this case, the URLs are eagerly resolved against the document's base URL, instead of being resolved at the time the request is actually being made, meaning that the base-uri directive is never being considered. The linked bug has more details.
Andrew PaseltinerThanks for pushing back!
You're right that the preload request path does end up with (header-based) CSP checks (and I believe CSP <meta> disables preloading, to avoid unauthorized requests in that case). The threat model behind the latter is unclear to me tbh, but that's a topic for a different conversation.
Just to see that I understand why base-uri needs special casing here - we can't enforce base-uri restrictions at part of requestResource as we don't know there is the resource was fetched using a relative URL that relied on base URI?
Yoav Weiss (@Shopify)Correct. Consider this document being preloaded:
```html
<base href="https://foo.example">
<img src="/bar">
```Without this CL, `/bar` is eagerly resolved to `https://foo.example/bar` and then fetched, meaning we've lost knowledge of whether there was a base URL at all by the time CSP comes into play. The alternative (Option 2 from the commit description) would be to avoid eager resolution at all (which I actually prefer from a code deduplication perspective) and instead let the fetch itself handle that, but IIUC it complicates the preload scanner's logic. If the reviewers push back though, we could consider it.
Andrew PaseltinerI'm not opposed to copying in the CSP directives, as it can enable genuine support for <meta> CSP in the future (given that apparently we think it's important).
Thinking through this, while I'm not sure there's a security risk, the spec does seem to require these requests are not sent to servers. Given that this is web-exposed, should we also add a WPT here?
Yoav Weiss (@Shopify)I was wondering about WPT myself, but I'm not sure how preloading works with it exactly. Any suggestions?
What you can do is add a WPT with a blocking script that document.write()s an HTML comment, and then have HTML that loads resources after that. Those resources would get preloaded, but never actually loaded.
Then you can use resource timing to see what loads were triggers and assert that the behavior matches what you'd expect.
I guess you could also just verify that resources from an unallowed base-uri origin are never loaded (again using resource timing)
- Andrew Paseltiner
- Kouhei Ueno
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Enforce CSP base-uri in HTML preload scanner
The Blink HTML preload scanner failed to validate <base href> tags
against the Content Security Policy 'base-uri' directive. This allowed
an attacker with HTML injection capabilities to redirect speculative
subresource fetches to an external origin, potentially leaking sensitive
relative resource paths and the page URL.
This patch updates TokenPreloadScanner to enforce the 'base-uri' CSP
directive when processing <base> tags.
Two approaches were considered for this fix:
1. Scanner-side validation (Chosen): Validate the <base> tag immediately
when the scanner encounters it. This ensures the scanner's internal
base URL state is always CSP-compliant. It prevents all consumers
of the scanner's base URL (including CSSPreloadScanner) from using
a malicious origin. This approach requires copying CSP policies to
the background scanner thread via CachedDocumentParameters.
2. Fetch-side validation: Defer validation until PreloadRequest::Start()
on the main thread. While this avoids copying CSP data, it would
result in redundant CSP checks for every relative subresource
discovered, and it would leave the scanner in a potentially
vulnerable internal state that could be exploited by future scanner
features.
Scanner-side validation was chosen for its architectural consistency
with how the main parser handles <base> tags and for its superior
performance characteristics by avoiding redundant checks during resource
resolution.
- M third_party/blink/renderer/core/frame/csp/content_security_policy.cc
- M third_party/blink/renderer/core/frame/csp/content_security_policy.h
- M third_party/blink/renderer/core/frame/csp/content_security_policy_test.cc
- M third_party/blink/renderer/core/html/parser/html_preload_scanner.cc
- M third_party/blink/renderer/core/html/parser/html_preload_scanner.h
- M third_party/blink/renderer/core/html/parser/html_preload_scanner_test.cc
Code-Review: +1 by Yoav Weiss (@Shopify), +1 by Antonio Sartori
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |