Bug 205979 – REGRESSION (r253662): Large Data URLs are not being handled properly

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

[Brent Fulgham]

The changes in Bug 203825 restricted the size of URLs too much. We need to allow reasonably sized data and other URLs.

[Brent Fulgham]

<rdar://problem/58346124>

[Brent Fulgham]

Created attachment 387173 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 387173 [details]
Patch

How do we know what a "reasonable" data URI size is?

[Simon Fraser (smfr)]

Also having values in hex isn't very helpful.

[Brent Fulgham]

According to <https://bugs.chromium.org/p/chromium/issues/detail?id=44820#c1>, Chrome limits data URI's to 2 MB.

This patch uses 64 MB, which if anything might be a bit large.

[Brent Fulgham]

@Alex Christensen: Does the URL Parser process all of the contents of a data URL? Could I just exclude data URIs from the test?

[Brent Fulgham]

Comment on attachment 387173 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387173&action=review

> Source/WebCore/page/SecurityOrigin.cpp:49
> +constexpr unsigned maximumURLSize = 0x04000000;

This changes from 32 KB to 64 MB, which might be too big for memory-constrained devices. Might be good to keep the limit and just skip the check for data URIs?

Or bump the standard URL limit, but have a much larger limit for data URIs only?

[Brent Fulgham]

(In reply to Brent Fulgham from comment #6)
> @Alex Christensen: Does the URL Parser process all of the contents of a data
> URL? Could I just exclude data URIs from the test?

I created a test case, and confirmed that data URLs are processed the same way, and can run up against this problem too.

[youenn fablet]

Comment on attachment 387173 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387173&action=review

>> Source/WebCore/page/SecurityOrigin.cpp:49
>> +constexpr unsigned maximumURLSize = 0x04000000;
> 
> This changes from 32 KB to 64 MB, which might be too big for memory-constrained devices. Might be good to keep the limit and just skip the check for data URIs?

Should we add a 32KB+1 data URL test?
 
> Or bump the standard URL limit, but have a much larger limit for data URIs only?

I think it might be best to bump to 64 MB.
We do not know what happens for custom schemes for instance.
Also, javascript scheme should probably have the same constraint has data URL.

We could also decide to decrease specific schemes like HTTP/HTTPS/FTP URLs back to 0x8000 if that is adding some kind of protection.

[Brent Fulgham]

Created attachment 387249 [details]
Patch for landing

[Alex Christensen]

(In reply to Brent Fulgham from comment #6)
> @Alex Christensen: Does the URL Parser process all of the contents of a data URL?
It does.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 387249 [details]:

storage/indexeddb/detached-iframe.html bug 206028 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 387249 [details]:

imported/w3c/web-platform-tests/content-security-policy/script-src/scriptnonce-changed-1.html bug 206029 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 387249 [details]
Patch for landing

Clearing flags on attachment: 387249

Committed r254301: <https://trac.webkit.org/changeset/254301>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 387249 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=387249&action=review

> Source/WebCore/page/SecurityOrigin.cpp:49
> +constexpr unsigned maximumURLSize = 0x04000000;

Could we put a comment here explaining the rationale behind this maximum? For example, I know it needs to be "big enough for reasonable data URI sizes" but not how we decided what is reasonable. And I don’t know why it shouldn’t be even bigger. We should just "show our work" here so someone later can make a smart decision about changing this or not changing it.

[Brent Fulgham]

(In reply to Darin Adler from comment #16)
> Comment on attachment 387249 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387249&action=review
> 
> > Source/WebCore/page/SecurityOrigin.cpp:49
> > +constexpr unsigned maximumURLSize = 0x04000000;
> 
> Could we put a comment here explaining the rationale behind this maximum?
> For example, I know it needs to be "big enough for reasonable data URI
> sizes" but not how we decided what is reasonable. And I don’t know why it
> shouldn’t be even bigger. We should just "show our work" here so someone
> later can make a smart decision about changing this or not changing it.

Sure -- I'll add a comment in a follow-up.