Intent to Ship: Potentially trustworthy data: urls

[Frédéric Wang]

Hello,

Please find below my intent email to make data: urls potentially trustworthy, which is implemented in https://chromium-review.googlesource.com/c/chromium/src/+/2563683

Disclaimer: Handling of data: URLs for security purpose seems a tricky topic and I'm not 100% sure my change is without risk or has unexpected side effect, so I would really welcome feedback from experts in that field before going ahead with that.

Thanks,

-------------------

Contact emails

fw...@chromium.org

Explainer

None

Specification

https://w3c.github.io/webappsec-secure-contexts/#is-url-trustworthy

Design docs

https://github.com/w3c/webappsec-secure-contexts/issues/69
https://chromium-review.googlesource.com/c/chromium/src/+/2563683

Summary

It is often necessary to evaluate whether a URL is secure in order to only enable certain features when minimum standards of authentication and confidentiality are met. For that purpose, web standards rely on the definition of "potentially trustworthy URL", which includes URLs with the "data" scheme in the latest version of the Secure Contexts specification. Blink already treats data: URLs as potentially trustworthy in some cases. The goal is to generalize this to all cases.

Blink component

Blink>Network

TAG review

TAG review status

Not applicable

Risks

Interoperability and Compatibility

This is relaxing security check, so should not break existing websites. Browsers have generally handle "data:" URLs inconsistently and don't necessarily follow the latest Secure context specification. Previous version said data: URLs are not potentially trustworthy, this was only changed early this year, so there is a risk this change is going to make interoperability worse, albeit aligned with the spec.

Gecko: Positive (https://github.com/w3c/webappsec-secure-contexts/issues/69)

WebKit: No signal

Web developers: No signals

Security

In some cases, "data:" should not be treated as secured context, see for example the definition of "potentially trustworthy origin" in https://www.w3.org/TR/powerful-features/#is-origin-trustworthy which does not include "data:". https://chromium-review.googlesource.com/c/chromium/src/+/2563683 only modifies the definition of "potentially trustworthy url", but it's possible legacy use of the helper function actually meant to exclude "data:". For detailed discussion on the spec side, see https://github.com/w3c/webappsec-secure-contexts/issues/69

Is this feature fully tested by web-platform-tests?

No, only partially.

Tracking bug

https://bugs.chromium.org/p/chromium/issues/detail?id=1119740

Link to entry on the Chrome Platform Status

https://chromestatus.com/feature/5634194258526208

This intent message was generated by Chrome Platform Status.

-- 
Frédéric Wang

[Domenic Denicola]

This is a welcome change. The inconsistency where Blink considers data: URLs trustworthy, while Chromium does not, has caused me some headaches when implementing features. It will be great to have them aligned.

[Mike West]

This is the right thing to do from both a spec and implementation perspective. Thanks for tackling the problem.

That said, it's not clear to me what the state of this change is in browsers today. My understanding was that this is more of a bug fix, insofar as we incorrectly consider `data:` to be inherently untrustworthy outside of Blink, but look at the ancestor chain to determine secure-contextness on the Blink side. So `isSecureContext` would be `true` in a `data:` frame within an HTTPS site today, and IDL attributes/methods/interfaces gated on `[SecureContext]` would be instantiated.

That doesn't seem to be Blink's behavior today, however, nor does https://chromium-review.googlesource.com/c/chromium/src/+/2563683 include any changes to web platform tests, so I think I'm missing something about what you actually want to change? What's going to be different for web developers after this intent?

-mike

[Frédéric Wang]

On 01/12/2020 08:07, Mike West wrote:
> This is the right thing to do from both a spec and implementation
> perspective. Thanks for tackling the problem.
>
> That said, it's not clear to me what the state of this change is in
> browsers today. My understanding was that this is more of a bug fix,
> insofar as we incorrectly consider `data:` to be inherently
> untrustworthy outside of Blink, but look at the ancestor chain to
> determine secure-contextness on the Blink side. So `isSecureContext`
> would be `true` in a `data:` frame within an HTTPS site today, and IDL
> attributes/methods/interfaces gated on `[SecureContext]` would be
> instantiated.
>
> That doesn't seem to be Blink's behavior today, however, nor
> does https://chromium-review.googlesource.com/c/chromium/src/+/2563683
> include any changes to web platform tests, so I think I'm missing
> something about what you actually want to change? What's going to be
> different for web developers after this intent?
>
>

Hi Mike,

I was actually not sure either what is going to be web-exposed but the
involved methods are used all over the code, so I thought it would be
safer to open a chromestatus entry and inform blink-dev.

Checking again, from the Blink's point of view, this change is about
accepting "data:" in network::IsUrlPotentiallyTrustworthy, see
https://source.chromium.org/search?q=IsUrlPotentiallyTrustworthy . There
are only a few places directly affected (which of course may in turn
affect others):

content security policy's UpgradeInsecureRequest
security state's GetSecurityLevel
NetworkFetcherImpl::DownloadToFile
CookieChangeSubscription::ShouldObserveChangeTo
ios password manager's WebStateContentIsSecureHtml
sec_header_helper's SetFetchMetadataHeaders and MaybeRemoveSecHeaders
TouchToFillController::Show
InsecureInputTabHelper
RemoteCopyMessageHandler::HandleImage
CookieAccessDelegateImpl::ShouldTreatUrlAsTrustworthy

I'm not sure whether these functions lead to web-exposed changes,
probably we would need to ask to code owners. In any case, as you said
this does not affect any existing tests.

All the other places (including the ones listed on
https://chromium-review.googlesource.com/c/chromium/src/+/2563883 and
https://chromium-review.googlesource.com/c/chromium/src/+/2563683)
already handle "data:" as a special case. IIUC, in the past, Łukasz
Anforowicz tried to remove these special handling and it did break some
tests.

--
Frédéric Wang

[Matt Falkenhagen]

+loading-dev for more visibility. I was asked to review the CL and I'm also unsure if there are any web-exposed changes. Looking at the callsites likely to be web-exposed, I suspect data URLs do not show up there:

UpgradeInsecureRequest -> only operates on http scheme

CookieChangeSubscription::ShouldObserveChangeTo -> only operates on service worker/cookie supported schemes, which is not data:

SetFetchMetadataHeaders -> called from SimpleURLLoader which should not load data URLs

GetSecurityLevel -> this already has a special case for data URLs

It seems likely the CL doesn't cause a web-exposed change but as Frédéric says it does have callsites all over so I wouldn't be surprised if it does.

2020年12月3日(木) 1:31 Frédéric Wang <fw...@igalia.com>:

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/80bbb55d-7011-d7cf-2c6f-f584af0507b4%40igalia.com.

[Frédéric Wang]

Thank you Matt, GetSecurityLevel and UpgradeInsecureRequest sounded the most generic names so it's good that indeed they already handle data: specially. The other cases are less obvious to me, I believe I'll try to find and ask code owners:

NetworkFetcherImpl::DownloadToFile
CookieChangeSubscription::ShouldObserveChangeTo
ios password manager's WebStateContentIsSecureHtml
sec_header_helper's SetFetchMetadataHeaders and MaybeRemoveSecHeaders
TouchToFillController::Show
InsecureInputTabHelper
RemoteCopyMessageHandler::HandleImage
CookieAccessDelegateImpl::ShouldTreatUrlAsTrustworthy

(An alternative or complementary approach would be to add a DCHECK for to ensure these are not called with a "data" scheme and rely on our fuzzers)

-- 
Frédéric Wang

[Yoav Weiss]

It sounds to me that we can consider this not web-exposed (and therefore, not requiring LGTMs), unless you'll find out otherwise, in which case, you'd update this thread.

Does that make sense?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/47469f89-74ad-ca67-215b-b1e98a78ce15%40igalia.com.

[Frédéric Wang]

On 04/12/2020 16:38, Yoav Weiss wrote:
> It sounds to me that we can consider this not web-exposed (and
> therefore, not requiring LGTMs), unless you'll find out otherwise, in
> which case, you'd update this thread.
> Does that make sense?

Hi,

I double-checked the call sites and contacted corresponding code owners
to be sure. I think there could potentially be small behavior change,
but nothing serious. So I think discussion can continue on the CL now.

Thank you.

--
Frédéric Wang

[Frédéric Wang]

On 01/12/2020 08:07, Mike West wrote:

This is the right thing to do from both a spec and implementation perspective. Thanks for tackling the problem.

That said, it's not clear to me what the state of this change is in browsers today. My understanding was that this is more of a bug fix, insofar as we incorrectly consider `data:` to be inherently untrustworthy outside of Blink, but look at the ancestor chain to determine secure-contextness on the Blink side. So `isSecureContext` would be `true` in a `data:` frame within an HTTPS site today, and IDL attributes/methods/interfaces gated on `[SecureContext]` would be instantiated.

That doesn't seem to be Blink's behavior today, however, nor does https://chromium-review.googlesource.com/c/chromium/src/+/2563683 include any changes to web platform tests, so I think I'm missing something about what you actually want to change? What's going to be different for web developers after this intent?

For the record, we have many definitions of secured context ( see https://bugs.chromium.org/p/chromium/issues/detail?id=1153336 ), this intent is only about changing network::IsURLPotentiallyTrustworthy() so that it aligns with the spec regarding data: URLs. It's still possible that other places behave differently though.

-- 
Frédéric Wang