1721448 - Stop parsing arbitrary-length data URLs (or stop sending them to/from the parent process via IPC)

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[:Gijs (he/him)]

(Not convinced if this needs to be sec-sensitive, but erring on the side of caution)

From bug 1698221:

(In reply to Daniel Veditz [:dveditz] from bug 1698221 comment #10)

In all cases the real crash reason is MOZ_CRASH(IPC message size is too large) which is an unexploitable termination of the offending tab. This should be filed as a separate bug -- the sending code should be checking the message size before it gets that far and handling it sensibly as it does for non-data: URLs. If I replace the location setting line with the equally long w.location = "https://example.com/#"+Array(100000000); then I get the expected NS_ERROR_MALFORMED_URI and no crash. Need to test whether the crash is unique to data: urls or if the non-crash is special handling in the HTTP channel.

So one of the many minor problems that the meandering conversation in that bug hits is that data: URIs do not appear to have a practical length limit (or, if they do, it is higher than the IPC size limit). As a result, it is possible for the child to tell its browsing context to navigate to a giant data URI.

AIUI, in the "good" case, the IPC message limit is broken in the child, we safely crash the child, and spammy webpages don't get to spam. Good!

In the "bad" case, the IPC message limit is not broken in the child, but in the parent, when the almost-but-not-quite-too-long URI is sent back to the child in a message that includes it multiple times, or as part of a bigger object (e.g. nsIPrincipals, but I'm sure there's others). We crash the parent. Bad! E.g.: https://crash-stats.mozilla.org/report/index/e8febbdc-0098-4ed9-a310-322580210720 . (created by loading https://eviltrap.site/trap/iframe-window.location-dos/ in my win7 VM)

Dan points out that http URIs do not suffer from this problem.

I think we should come up with a solution. At the moment, the max IPC message size appears to be 256 mb. I'm inclined to suggest we max out data URIs at some point well below that (maybe 32mb?), and that we make the relevant URI parser throw if that limit is exceeded. I expect this has web compat ramifications - which may make it impossible...

The other solutions I can think of are some form of memory sharing (but not sure if we even have infrastructure for content-written memory sharing right now? It feels like that could be super dodgy...), or keeping the full URI child-side (sending only a unique identifier and the mimetype to the parent; but I'm not sure if that's easily feasible, either (e.g. not sure how session restore would work, or if we'd need the full URI in the parent for networking purposes)).

Anne, can you clarify where this stands in terms of web specs, and/or what other browsers do?

Comment 1

[Anne (:annevk)]

Web standards typically do not specify OOM limits (or what to do when you reach them) and data: URLs are no exception. I vaguely recall we used to support them well into gibibytes (or maybe that was aspirational), but that seems unlikely now given the stated IPC message size limit. I tried to find the limit in browsers using https://software.hixie.ch/utilities/js/live-dom-viewer/saved/9498, but that does not seem like a good approach.

Comment 2

[Kershaw Chang [:kershaw]]

I assume this bug can wait until Valentin's back.

Comment 3

[Kershaw Chang [:kershaw]]

Valentin, do you have time to take a look?
Thanks.

Comment 4

[Valentin Gosu [:valentin] (he/him)]

So, the reason we throw for http URLs and not for data URLs is that we have the network.standard-url.max-length pref added in bug 1135354 which limits standardURLs to 1Mb.
For data URLs we have no such limit, mostly because people use them to store large image/memory blobs. Arguably they should move toward using blob URLs which do not have the same OOM problem.

The other solutions I can think of are some form of memory sharing (but not sure if we even have infrastructure for content-written memory sharing right now? It feels like that could be super dodgy...), or keeping the full URI child-side (sending only a unique identifier and the mimetype to the parent; but I'm not sure if that's easily feasible, either (e.g. not sure how session restore would work, or if we'd need the full URI in the parent for networking purposes)).

I don't know if this would work. This is almost like reimplementing a form of blob URLs - and at time we need to do URI manipulations on the parent side, and those operations are usually sync. Shared memory is way too complex and would implicitly block if memory is not synced - and anything else would need some logic change.

Indeed, it seems like the best solution would be to limit the size of URLs (I think we can do it in NS_NewURI to make sure it applies consistently to all URL types). I'm thinking we set the limit to 32Mb on EARLY_BETA_OR_EARLIER then if no big breakage is reported for one or two cycles we let it ride to release.

I'd also prefer it if the IPC would safely return an error code for too big messages, instead of just panicking, but I assume that is a larger project 🙂

Comment 5

[Anne (:annevk)]

Adding Nika for the IPC bits.

Do we know what limit Chrome and Safari have? If we can agree on a common limit that might be nice, we could even standardize it.

Comment 6

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1721448 - Add a max-length for all URLs r=#necko

Comment 7

[Valentin Gosu [:valentin] (he/him)]

(In reply to Anne (:annevk) from comment #5)

Do we know what limit Chrome and Safari have? If we can agree on a common limit that might be nice, we could even standardize it.

Testing on Mac OS 11.6 (M1) with the following test case:
new URL("data:text/plain,"+"a".repeat(1024*1024*limit))
Chrome 93 starts failing at 512 Mb. (Invalid string length).
Safari 15.0 starts failing at 2048 Mb (Range error: Out of memory)

Both of these are larger than our IPC limit.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:valentin, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 9

[Valentin Gosu [:valentin] (he/him)]

I was mostly waiting on some feedback regarding the lower limit.
Let's land this (it's early beta & nightly anyway) and see if anyone complains.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Add a max-length for all URLs r=necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/a82ecde63dec0c79bd96361528c2198ba498c021
https://hg.mozilla.org/mozilla-central/rev/a82ecde63dec

Comment 11

[Brindusa Tot, Desktop QA]

Gijs, do you consider we can manualy verify this? If yes, could you please help us with some STR?

Comment 12

[:Gijs (he/him)]

(In reply to Brindusa Tot[:brindusat] from comment #11)

Gijs, do you consider we can manualy verify this? If yes, could you please help us with some STR?

I don't know. We can trivially verify that new URL("data:text/html," + "a".repeat(32 * 1024 * 1024)) now throws an exception, but I think ideally we'd have a testcase that used to reliably crash the parent due to IPC message size issues and now doesn't.

The link from comment 0 ( https://eviltrap.site/trap/iframe-window.location-dos/ ) doesn't crash the parent reliably for me anymore, unfortunately (it crashes the tab instead of the parent on older builds that I've tried running). I guess it can still help because on fixed builds it shows an error in the console after an initial hang, instead of crashing either the tab or parent, and that's something...

Valentin, do you have a better suggestion?

Comment 13

[Valentin Gosu [:valentin] (he/him)]

(In reply to :Gijs (he/him) from comment #12)

Valentin, do you have a better suggestion?

I don't have any better ideas.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider for ESR91 uplift?

Comment 15

[Valentin Gosu [:valentin] (he/him)]

Considering the pref landed as #if defined(EARLY_BETA_OR_EARLIER) I'd say we don't need to uplift to ESR just yet.
But I haven't seen any fallout yet in Nightly/Beta, so it might be worth removing that restriction and then uplifting.
Gijs? What do you think?

Comment 16

[:Gijs (he/him)]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #15)

Considering the pref landed as #if defined(EARLY_BETA_OR_EARLIER) I'd say we don't need to uplift to ESR just yet.
But I haven't seen any fallout yet in Nightly/Beta, so it might be worth removing that restriction and then uplifting.
Gijs? What do you think?

I'm in two minds; I don't know how easy it would be to track issues down to this ticket, also given it's marked security-sensitive, and given that any resulting issues may appear intermittent to users (ie small files rendered as data URIs work and large files do not). So not sure if we should try to rely on beta/nightly to shake much of anything out here, in which case just flipping this on for 95 seems OK.

OTOH 95 beta has also only been out for about 2 weeks yet, what with update lag that's not a lot of time to collect issues.

So I'm a little tempted to have this ride no earlier than the 96 cycle, but we could also try to do a pref experiment with 95 release (ie ship the block to N% of users) if we thought that valuable. What's your sense of this?

I also wonder if we could make this bug public and then relnote this change, that might be a question for Dan or Tom...

Comment 17

[Valentin Gosu [:valentin] (he/him)]

(In reply to :Gijs (he/him) from comment #16)

So I'm a little tempted to have this ride no earlier than the 96 cycle, but we could also try to do a pref experiment with 95 release (ie ship the block to N% of users) if we thought that valuable. What's your sense of this?

I'm not sure what metrics would be impacted by the pref change - it might increase the number of webcompat reports that users send, but it's unclear what our expectations should be.
My suggestion would be to make the bug public, put this into release-notes/dev-notes for 95 with a plan to ship the pref change in 96 or 97 and see if anyone cares enough to complain.

Comment 18

[Daniel Veditz [:dveditz]]

This doesn't need to be hidden: it's a DOS but not otherwise exploitable.

Comment 19

[:Gijs (he/him)]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #17)

(In reply to :Gijs (he/him) from comment #16)

So I'm a little tempted to have this ride no earlier than the 96 cycle, but we could also try to do a pref experiment with 95 release (ie ship the block to N% of users) if we thought that valuable. What's your sense of this?

I'm not sure what metrics would be impacted by the pref change - it might increase the number of webcompat reports that users send, but it's unclear what our expectations should be.

Yeah, fair, it'd be hard for people to recognize breakage...

My suggestion would be to make the bug public, put this into release-notes/dev-notes for 95 with a plan to ship the pref change in 96 or 97 and see if anyone cares enough to complain.

SGTM!

Comment 20

[Valentin Gosu [:valentin] (he/him)]

We need to update https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/Data_URIs with the new limit (also limits I found in comment 6)

I filed a new bug to let the pref ride the trains targeting Firefox 97.

Comment 21

[:Gijs (he/him)]

(In reply to Valentin Gosu [:valentin] (he/him) <<Parental leave March 1 - August 1>> from comment #20)

We need to update https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/Data_URIs with the new limit (also limits I found in comment 6)

I filed a new bug to let the pref ride the trains targeting Firefox 97.

I submitted an MDN pull request at https://github.com/mdn/content/pull/14910 to indicate the new limits.