1560178 - Make it impossible to load untrusted (non-chrome:/resource:/about: ) URLs into docshells in the parent process

Status: RESOLVED FIXED

(Firefox :: Security, defect, P1)

Description

[:Gijs (out for now; he/him)]

We've effectively removed innerHTML-style XSS attacks on chrome from JS by passing all things through the sanitizer. In 68, we've done something similar with DTDs as well. We're working on removing eval() and similar untrusted JS on a per-script basis (bug 1473549). So we're gaining confidence in the trustworthiness of what we run in existing chrome:/resource:

One of the more recent sandbox escapes (cf. bug 1559845 / bug 1559858) has involved loading untrusted content as a separate window in the parent process. Obviously we're changing the IPC messages that let that happen, but I'd like to suggest that we do two things to combat that:

1. make the JS engine/loader code refuse to load scripts with non-chrome/resource: associated URIs in the parent process. (To be clear, this is a bit different from using CSP because CSP won't affect documents we don't ship, and because for historical reasons it's not straightforward to protect browser.xhtml and other shipped docs with CSP that includes unsafe-eval.) (Edit: I've split this off to bug 1562221)
2. refuse to load/construct documents with non-chrome/resource:/about: URIs in the parent process.

This will break data: URI'd scripts/windows in the parent, which might need some test changes (but hopefully not too many).

Ehsan, Bobby, do these ideas seem feasible? Any particular things to look out for in how we would execute on those ideas?

Comment 1

[:Gijs (out for now; he/him)]

It seems like in terms of DOM content loads, we're still relying on redirecting http/https/... loads through the parent process first, ie we create docshells and call InternalLoad() on them with public URLs (at least) when opening links from the parent process (e.g. in about:preferences/about:addons, or parent process extensions, for which we still run automated tests). Nika, is that right? How hard would it be to stop short of creating the docshell and starting a load in the parent for those cases?

Comment 2

[:Gijs (out for now; he/him)]

Talking a bit with both Nick and Boris, and poking at a bunch of docshell stuff, it seems to me that our shouldLoadURI infrastructure is just... not great.

Basically, docshell currently looks for an nsIWebBrowserChrome3, and only if it finds one will it call ShouldLoadURI on it to figure out if loading a given URI in a given docshell (and moreso, process) actually makes sense.

Specifically, for the direct openWindow calls, we never call any of this.

The current setup is problematic for other reasons, notably that GeckoView had to reinvent this particular wheel (which is somewhat shared between browser.js's shouldLoadURI and E10SUtils.jsm's shouldLoadURI). We should probably move some of that code into libxul and ensure we call it directly from docshell, independently from whether we're in a desktop tabbrowser or whatever.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Yeah, there is a ton of work which needs to happen there around shouldLoadURI. As part of fission we're planning to rewrite almost all of that logic, but there's also more we can do now.

If we don't have shouldLoadURI available from nsIWebBrowserChrome3 we don't have any way to change which process we're loading in, so we can only block loads in that case. We should probably whitelist which types of URLs are allowed to start loading in parent-process docshells which have the mUseRemoteTabs bit set, at least for now.

I've got some ideas for how we can do that, but the complexity of the solution I'd throw together would depend on how much we want to uplift this solution. I'm guessing we would want to uplift (at least to Beta?)

Comment 4

[:Gijs (out for now; he/him)]

(In reply to :Nika Layzell (busy - slow to reply) from comment #3)

I've got some ideas for how we can do that, but the complexity of the solution I'd throw together would depend on how much we want to uplift this solution. I'm guessing we would want to uplift (at least to Beta?)

Yes for beta. I have a WIP patch that just adds protocol checks after the webbrowserchrome/shouldloaduri checks (which is hacky, but as you said, reduces the amount of rewriting), which sounds roughly like what you proposed (I just checked XRE_IsE10sParentProcess). My main problem right now is tests, some of which try to load http(s) things (where usually I can just switch to the equivalent chrome: URI for mochitests), some of which try to load javascript:/data: URIs (which is a bit trickier).

I'm intending to try to whittle down the broken tests a bit more before I add a test-only pref to bypass this (ie I'd like to avoid that but in terms of impact it might be more feasible if there's too many tests I need to update otherwise).

Comment 5

[:Gijs (out for now; he/him)]

Based on the dep Paul added, it looks like about:addons on 68 will rely on being able to do this for the discovery pane, so that's a problem... :-(

Comment 6

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Do we also need to whitelist moz-extension: for privileged extensions? I might be wrong but it looks to me like we run privileged moz-extension://61c7159b-8a30-4554-bcdc-d5c91d094f91/privileged/FirefoxMonitor.jsm in the parent. This would make sense as part of the point of privileged extension scripts need to do things like write prefs, which is only allowed in the parent.

Comment 7

[:Gijs (out for now; he/him)]

(In reply to Paul Theriault [:pauljt] (needinfo pls) from comment #6)

Do we also need to whitelist moz-extension: for privileged extensions? I might be wrong but it looks to me like we run privileged moz-extension://61c7159b-8a30-4554-bcdc-d5c91d094f91/privileged/FirefoxMonitor.jsm in the parent.

I think you're right, but atm I'm working specifically on docshell/document loads; script loads should also be tackled but it's somewhat of a separate problem. I suspect (but have not gathered evidence to prove) that we're less likely to have IPC APIs that trigger arbitrary script loads than arbitrary document loads in the parent, so I started with docshell. I also just know less about the JS engine.

For docshells, I believe we can avoid loading moz-extension things in the parent unless the extensions.webextensions.remote pref is false (or e10s is off altogether).

Comment 8

[:Gijs (out for now; he/him)]

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

Based on the dep Paul added, it looks like about:addons on 68 will rely on being able to do this for the discovery pane, so that's a problem... :-(

OK, so I was wrong in the sense that we've disabled this by default for 68, but we probably need to not break the disco pane completely in case we need to flip that flag back for some reason.

Comment 9

[:Gijs (out for now; he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7fd695dbcc6538a39dbc894184b99862d77f3e1

(this after https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6248374e225e01a2578c1d0d292e008326ba1e9 which was... uh, interesting.)

I might have to just move the docshell checks to nsContentSecurityManager.cpp or something? Returning where I am seems to make things Very Sad (in the original openWindow thing, the parent window is permanently stuck in a "I have an open modal" state without an actual modal being present, and in general we don't show an error page or anything like that). Not 100% sure why, or if doing it in nsCSM would make it better.

I did notice that the extant checks there (from bug 1513445) actually don't complain about loading data: URIs with a system principal. :-(

Comment 10

[:Gijs (out for now; he/him)]

Attachment: Bug 1560178 - fix/remove about:addons tests that load discovery pane in the parent, r?aswan

Comment 11

[:Gijs (out for now; he/him)]

Attachment: Bug 1560178 - adjust webextension tests that rely on loading untrusted URIs in the parent process when remote webextensions are turned off, r?aswan

Depends on D36308

Comment 12

[:Gijs (out for now; he/him)]

Attachment: Bug 1560178 - fix miscellaneous tests to allow them to keep working when disallowing remote content in the parent process, r?aswan!,ckerschb!

Depends on D36309

Comment 13

[:Gijs (out for now; he/him)]

Attachment: Bug 1560178 - fix devtools tests that load untrusted URIs in the parent, r?jdescottes

Depends on D36310

Comment 14

[:Gijs (out for now; he/him)]

Attachment: Bug 1560178 - disallow unsafe loads in the parent, r?bzbarsky

Depends on D36311

Comment 16

[:Gijs (out for now; he/him)]

I filed bug 1565545 and bug 1565279 and bug 1565276 as followups for some of the tests that I fixed by flipping prefs instead of changing the tests "properly".

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1ff8a41dd3c112cfce8b3911eefbeb84f5dec859

Comment 17

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

Landed as:
https://hg.mozilla.org/integration/autoland/rev/fc2be64e4ece2c9a92947cee728f72e1b783c6aa
https://hg.mozilla.org/integration/autoland/rev/45fca0f1b6755c16f210996590594a9784b7ca8a
https://hg.mozilla.org/integration/autoland/rev/b1dfc0b5a14cf24aaffe4717c7334f09cfbb74e1
https://hg.mozilla.org/integration/autoland/rev/4aaeda889656fb1bcdb27466707f5f76ac627abc
https://hg.mozilla.org/integration/autoland/rev/1ff8a41dd3c112cfce8b3911eefbeb84f5dec859

Backed out for Browser-chrome failures in builds/worker/workspace/build/src/dom/base/Document.cpp:

https://hg.mozilla.org/integration/autoland/rev/5e1f4a7ca7882cb5f8594cbb8f16ecece492998e

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedJob=256213430&resultStatus=retry%2Ctestfailed%2Cbusted%2Cexception&revision=1ff8a41dd3c112cfce8b3911eefbeb84f5dec859
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=256213417&repo=autoland

[task 2019-07-12T14:59:03.495Z] 14:59:03 INFO - TEST-START | dom/base/test/browser_bug593387.js
[task 2019-07-12T14:59:03.495Z] 14:59:03 INFO - GECKO(4295) | Chrome file doesn't exist: /builds/worker/workspace/build/tests/mochitest/browser/dom/base/test/head.js
[task 2019-07-12T14:59:03.531Z] 14:59:03 INFO - GECKO(4295) | ++DOCSHELL 0x7f3b46c5e000 == 2 [pid = 4426] [id = {a84a6312-6537-439b-8f43-a067c310eb04}]
[task 2019-07-12T14:59:03.531Z] 14:59:03 INFO - GECKO(4295) | ++DOMWINDOW == 3 (0x7f3b4703bd40) [pid = 4426] [serial = 12] [outer = (nil)]
[task 2019-07-12T14:59:03.559Z] 14:59:03 INFO - GECKO(4295) | [Child 4426, Main Thread] WARNING: No active window: file /builds/worker/workspace/build/src/js/xpconnect/src/XPCJSContext.cpp, line 662
[task 2019-07-12T14:59:03.601Z] 14:59:03 INFO - GECKO(4295) | [Parent 4295, Main Thread] WARNING: Constructing RangeBoundary with invalid value: 'mRef || aOffset == 0', file /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RangeBoundary.h, line 79
[task 2019-07-12T14:59:03.605Z] 14:59:03 INFO - GECKO(4295) | [Parent 4295, Main Thread] WARNING: Constructing RangeBoundary with invalid value: 'mRef || aOffset == 0', file /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RangeBoundary.h, line 79
[task 2019-07-12T14:59:03.614Z] 14:59:03 INFO - GECKO(4295) | [Child 4426, Main Thread] WARNING: No active window: file /builds/worker/workspace/build/src/js/xpconnect/src/XPCJSContext.cpp, line 662
[task 2019-07-12T14:59:03.623Z] 14:59:03 INFO - GECKO(4295) | ++DOMWINDOW == 4 (0x7f3b46cd3000) [pid = 4426] [serial = 13] [outer = 0x7f3b4703bd40]
[task 2019-07-12T14:59:03.680Z] 14:59:03 INFO - GECKO(4295) | [Child 4426, Main Thread] WARNING: NS_ENSURE_TRUE(aCSP) failed: file /builds/worker/workspace/build/src/dom/security/FramingChecker.cpp, line 167
[task 2019-07-12T14:59:03.681Z] 14:59:03 INFO - GECKO(4295) | ++DOMWINDOW == 5 (0x7f3b46cdf000) [pid = 4426] [serial = 14] [outer = 0x7f3b4703bd40]
[task 2019-07-12T14:59:03.720Z] 14:59:03 INFO - GECKO(4295) | [Child 4426, Main Thread] WARNING: NS_ENSURE_SUCCESS(mStatus, this) failed with result 0x80004005: file /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIURIMutator.h, line 489
[task 2019-07-12T14:59:03.725Z] 14:59:03 INFO - GECKO(4295) | [Child 4426, StreamTrans #1] WARNING: 'NS_FAILED(rv)', file /builds/worker/workspace/build/src/modules/libjar/nsJARChannel.cpp, line 371
[task 2019-07-12T14:59:03.773Z] 14:59:03 INFO - GECKO(4295) | [Child 4426, StreamTrans #1] WARNING: 'NS_FAILED(rv)', file /builds/worker/workspace/build/src/modules/libjar/nsJARChannel.cpp, line 371
[task 2019-07-12T14:59:03.781Z] 14:59:03 INFO - GECKO(4295) | [Child 4426, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file /builds/worker/workspace/build/src/modules/libjar/nsJARChannel.cpp, line 994
[task 2019-07-12T14:59:03.804Z] 14:59:03 INFO - GECKO(4295) | Assertion failure: foundDefaultSrc (about: page must contain a CSP including default-src), at /builds/worker/workspace/build/src/dom/base/Document.cpp:7206
[task 2019-07-12T14:59:03.804Z] 14:59:03 INFO - GECKO(4295) | #01: nsHtml5TreeOpExecutor::DidBuildModel(bool) [parser/html/nsHtml5TreeOpExecutor.cpp:210]
[task 2019-07-12T14:59:03.805Z] 14:59:03 INFO -
[task 2019-07-12T14:59:03.806Z] 14:59:03 INFO - GECKO(4295) | #02: nsHtml5TreeOpExecutor::RunFlushLoop() [parser/html/nsHtml5TreeOpExecutor.cpp:518]
[task 2019-07-12T14:59:03.806Z] 14:59:03 INFO -
[task 2019-07-12T14:59:03.806Z] 14:59:03 INFO - GECKO(4295) | #03: nsHtml5ExecutorReflusher::Run() [parser/html/nsHtml5TreeOpExecutor.cpp:0]
[task 2019-07-12T14:59:03.806Z] 14:59:03 INFO -
[task 2019-07-12T14:59:03.807Z] 14:59:03 INFO - GECKO(4295) | #04: mozilla::SchedulerGroup::Runnable::Run() [xpcom/threads/SchedulerGroup.cpp:295]
[task 2019-07-12T14:59:03.807Z] 14:59:03 INFO -
[task 2019-07-12T14:59:03.807Z] 14:59:03 INFO - GECKO(4295) | #05: nsThread::ProcessNextEvent(bool, bool) [xpcom/threads/nsThread.cpp:1214]
[task 2019-07-12T14:59:03.808Z] 14:59:03 INFO -
[task 2019-07-12T14:59:03.809Z] 14:59:03 INFO - GECKO(4295) | #06: NS_ProcessNextEvent(nsIThread*, bool) [xpcom/threads/nsThreadUtils.cpp:486]
[task 2019-07-12T14:59:03.809Z] 14:59:03 INFO -
[task 2019-07-12T14:59:03.809Z] 14:59:03 INFO - GECKO(4295) | #07: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:88]
[task 2019-07-12T14:59:03.810Z] 14:59:03 INFO -
[task 2019-07-12T14:59:03.811Z] 14:59:03 INFO - GECKO(4295) | #08: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:315]
[task 2019-07-12T14:59:03.813Z] 14:59:03 INFO -
[task 2019-07-12T14:59:03.814Z] 14:59:03 INFO - GECKO(4295) | #09: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:291]
[task 2019-07-12T14:59:03.814Z] 14:59:03 INFO -
[task 2019-07-12T14:59:03.815Z] 14:59:03 INFO - GECKO(4295) | #10: nsBaseAppShell::Run() [widget/nsBaseAppShell.cpp:139]
[task 2019-07-12T14:59:03.817Z] 14:59:03 INFO -
[task 2019-07-12T14:59:03.818Z] 14:59:03 INFO - GECKO(4295) | #11: XRE_RunAppShell() [toolkit/xre/nsEmbedFunctions.cpp:919]
[task 2019-07-12T14:59:03.819Z] 14:59:03 INFO -
[task 2019-07-12T14:59:03.820Z] 14:59:03 INFO - GECKO(4295) | #12: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:238]
[task 2019-07-12T14:59:03.821Z] 14:59:03 INFO -
[task 2019-07-12T14:59:03.822Z] 14:59:03 INFO - GECKO(4295) | #13: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:315]
[task 2019-07-12T14:59:03.822Z] 14:59:03 INFO -
[task 2019-07-12T14:59:03.823Z] 14:59:03 INFO - GECKO(4295) | #14: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:291]
[task 2019-07-12T14:59:03.823Z] 14:59:03 INFO -
[task 2019-07-12T14:59:03.825Z] 14:59:03 INFO - GECKO(4295) | #15: XRE_InitChildProcess(int, char**, XREChildData const*) [toolkit/xre/nsEmbedFunctions.cpp:754]
[task 2019-07-12T14:59:03.825Z] 14:59:03 INFO -
[task 2019-07-12T14:59:04.112Z] 14:59:04 INFO - GECKO(4295) | #16: content_process_main(mozilla::Bootstrap*, int, char**) [ipc/contentproc/plugin-container.cpp:57]
[task 2019-07-12T14:59:04.113Z] 14:59:04 INFO -
[task 2019-07-12T14:59:04.114Z] 14:59:04 INFO - GECKO(4295) | #17: main [browser/app/nsBrowserApp.cpp:267]
[task 2019-07-12T14:59:04.114Z] 14:59:04 INFO -
[task 2019-07-12T14:59:04.115Z] 14:59:04 INFO - GECKO(4295) | #18: libc.so.6 + 0x20830
[task 2019-07-12T14:59:04.115Z] 14:59:04 INFO -
[task 2019-07-12T14:59:04.115Z] 14:59:04 INFO - GECKO(4295) | #19: _start

https://searchfox.org/mozilla-central/rev/15be167a5b436b57fef944b84eef061d24c1af8c/dom/base/test/browser_bug593387.js

Comment 18

[:Gijs (out for now; he/him)]

Ugh. Clearly I should have been slower in answering Christoph in https://bugzilla.mozilla.org/show_bug.cgi?id=1497203 ;-)

Anyway, some pref flips will cure that... there is actually a CSP, but the test turns off CSP because it's adding an http: resource to the page which wouldn't normally be allowed, and the assert that's failing here doesn't check for the CSP pref so it just breaks... I'm going to turn off the assert for that test with another pref flip. Also, fix the support files usage for that test...

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/e95cad095128
https://hg.mozilla.org/mozilla-central/rev/3093028bdd97
https://hg.mozilla.org/mozilla-central/rev/e76e9e4790fb
https://hg.mozilla.org/mozilla-central/rev/6071c24be566
https://hg.mozilla.org/mozilla-central/rev/2236a18eb0a9

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Is this something we wanted to backport to Beta and ESR68 after some bake time?

Comment 21

[:Gijs (out for now; he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #20)

Is this something we wanted to backport to Beta and ESR68 after some bake time?

Yes, I think so. I can do trypushes for beta and esr to see if anything obvious shows up that'd be cause for concern - at least for beta I don't expect obvious testing problems given I wrote most of this while 69 was still nightly, but esr is less of a given. Also, the prettier reformatting will not be my friend for the uplifts...

In terms of bake time, I think we should probably wait at least until Friday if not Monday. On the other hand, beta is likely to get us more feedback quicker than nightly - our test coverage is good enough for the 99% case still working, it's the 1% that might break that we'd want to find out about and that's gonna be harder on nightly than on beta...

Comment 22

[Ryan VanderMeulen [:RyanVM]]

FWIW, ESR68 is going to be reformatted tomorrow IIRC. Maybe we can aim for Thursday's b6 build if you think we're more likely to get useful feedback from Beta?

Comment 23

[:Gijs (out for now; he/him)]

beta trypush: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d1253c0b859a255aa6bfd4a1e61ee35df07b698

Comment 24

[:Gijs (out for now; he/him)]

Comment on attachment 9074757 [details]
Bug 1560178 - disallow unsafe loads in the parent, r?bzbarsky

Beta/Release Uplift Approval Request

• User impact if declined: Larger potential for problematic sandbox escapes by convincing the parent process to load untrusted content
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce: n/a
• List of other uplifts needed: Bug 1565965
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): I can't pretend making a change like this in docshell isn't at least a bit risky, but it is quite early in beta still, and I'd rather know sooner about issues in this space than later. It's also something where there's a fair amount of understanding that we're enforcing something that shouldn't be happening in the first place.

We probably want to uplift 1565965 as well to avoid the layout debugger breaking (though that's not code we'll ship, anyone who will have to debug layout issues on beta will thank us, I assume). There's also a tier-2 failure in an android test in bug 1565908. I don't know if that blocks uplift, I've commented in https://bugzilla.mozilla.org/show_bug.cgi?id=1565908#c5 as to what I think the cause is, but I'm not an expert, so it might make sense to hold off on approving for beta until we've got more confirmation about the seriousness of that issue (or lack thereof). I'm just filing the approval request now because of the suggestion we want this to make b6 on Thursday.

• String changes made/needed: n/a

Comment 25

[:Gijs (out for now; he/him)]

Although the automated test failures are only in tier2 items so technically might not block uplift, and AIUI we do not intend to ever ship fennec things off 69 (current m-b) I think it might be less messy to wait for bug 1565794 and bug 1565908 to be resolved (and particularly bug 1565794 Comment 24) before uplifting to beta.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9074753 [details]
Bug 1560178 - fix/remove about:addons tests that load discovery pane in the parent, r?aswan

Fixes a possible sandbox escape vector. Approved for 69.0b7.

Comment 27

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

https://hg.mozilla.org/releases/mozilla-beta/rev/20dec11dae3c
https://hg.mozilla.org/releases/mozilla-beta/rev/823a920c3127
https://hg.mozilla.org/releases/mozilla-beta/rev/73297d7f6db9
https://hg.mozilla.org/releases/mozilla-beta/rev/a188fb7e00d9
https://hg.mozilla.org/releases/mozilla-beta/rev/d5c1f040691a

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Per discussion with Gijs and dveditz, this isn't ready for ESR uplift yet given the new regressions still being reported so we're going to punt on trying to uplift it for 68.1esr. We'll revisit this next cycle to see where things shake out with Fx69 on release.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Should we proceed with ESR uplift here?

Comment 31

[:Gijs (out for now; he/him)]

I think so, but I think I need to pull together all the patches we needed and check if/how they need rebasing. Leaving ni for that.

Comment 32

[:Gijs (out for now; he/him)]

OK, so I have grafted patches for esr68. Conflicts were very doable in the end.

In addition to the patches from this bug, for the regressions I made some evaluations:

bug 1565794 - fixed by bug 1566957, which was already uplifted to esr68
bug 1565908 - another android harness fix which bug 1565908 comment 15 suggests we don't run on esr68, so not included
bug 1565965 - included
bug 1569135 - included
bug 1571342 - included, though note that the web console output saving was only implemented in bug 1517728 which isn't present on esr68, so it's not affected here (so some chunks are "missing" in the esr68 version compared to beta)
bug 1572793 - included

and for the dependency tree:
bug 1567848 was already uplifted
bug 1568013 - included; this is test-only code and doesn't run on try, but given that these are unit tests rather than perf tests, on balance it seemed like it'd be better to include it.

(there are no other regressions for either of those sets of bugs, nor are there closed see also / dep / block bugs that had patches)

This nets us 5 patches from this bug and 5 patches from deps, for a total of 10. Most of the follow-ups are very small, so uplift to esr68 should be doable... I've attempted to run a trypush (this is take #2 (StaticPrefList.h syntax changed :-( ) so not all of the commits are showing up in treeherder): https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a93d9957a49d8ada3f6a79349d769618f6fa2cc

Assuming that's green (if not I'll fix and keep updating the bug) and assuming approval is granted, please pull/import from the trypush for uplift.

Comment 33

[:Gijs (out for now; he/him)]

Comment on attachment 9074757 [details]
Bug 1560178 - disallow unsafe loads in the parent, r?bzbarsky

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: defense-in-depth (ie preventing arbitrary payload evaluation in the parent) against sandbox escapes for sec-high/sec-crit bugs
• User impact if declined: see above
• Fix Landed on Version: 70, uplifted to 69
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): I mean, I'd like to put "low" but asking to uplift a 10-patch set to esr68 I don't think anyone would take it seriously... Yes, there's risk, because it's ESR so there's no "runway", really (beyond fennec nightly, which is mostly not affected by these changes).

On the flip side, we have already shipped this on 69, which has been out for nearly a month at this point, and are not aware of regressions linked to this change - so far it seems we did OK in catching things while they were on beta. So that helps with confidence here.
The other point is that out of the 5 csets on this bug, 4 of them are basically "fix a bunch of tests". Even for the uplift request for the other 5 csets, 3 out of 5 don't impact anything user-facing. So it's not actually 10 things, more like 1, with 2 regression fixes, which sounds more manageable.

Unfortunately there aren't less risky options that do the same thing here. On the whole, I'm cautiously optimistic that really, this will all be fine, given that 69 went without a hitch (as far as I'm aware). :-)

• String or UUID changes made by this patch: No

Comment 34

[Ryan VanderMeulen [:RyanVM]]

This for putting this all together, Gijs. I agree that it makes to take this now as a defense in depth given where we are in the current lifecycle of ESR68. Approving this and dependent bugs for 68.2esr.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/4561a44e4bcf
https://hg.mozilla.org/releases/mozilla-esr68/rev/98571a8ebaa3
https://hg.mozilla.org/releases/mozilla-esr68/rev/d2a5577794a5
https://hg.mozilla.org/releases/mozilla-esr68/rev/a59884d5f957
https://hg.mozilla.org/releases/mozilla-esr68/rev/3c604f43b572

Comment 36

[Frederik Braun [:freddy]]

Comment 37

[Frederik Braun [:freddy]]

Gijs, I was looking at a related bug to restrict URLs loading in the parent (bug 1560423) and went back to understand the restrictions you added and the reasoning for the individual carve-outs.
I noticed some exceptions were asked to be moved inside an IsInAutomation() condition but ended up without. Can you elaborate what's preventing us from doing so?

Comment 38

[:Gijs (out for now; he/him)]

(In reply to Frederik Braun [:freddyb] from comment #37)

Gijs, I was looking at a related bug to restrict URLs loading in the parent (bug 1560423) and went back to understand the restrictions you added and the reasoning for the individual carve-outs.
I noticed some exceptions were asked to be moved inside an IsInAutomation() condition but ended up without. Can you elaborate what's preventing us from doing so?

Phabricator isn't the best at associating comments with the lines they were originally made on, but if you click the "[<<]" buttons to see them in the original context, you will see that Boris' comments were about the security.allow_unsafe_parent_loads pref (and here). That happened: https://hg.mozilla.org/mozilla-central/rev/2236a18eb0a9#l1.104 .

There's some stupid goop for about:addons that we can get rid of when we remove the "old style" about:addons , which is bug 1565606 / bug 1337627 / bug 1558982. Was there some other part you were hoping to put behind an IsInAutomation() check?

Comment 39

[Frederik Braun [:freddy]]

Your comment regarding about:addons loading the discovery pane remotely says this isn't used anymore but still tested. This read to me as if we could put put this behind IsInAutomation().

Comment 40

[:Gijs (out for now; he/him)]

(In reply to Frederik Braun [:freddyb] from comment #39)

Your comment regarding about:addons loading the discovery pane remotely says this isn't used anymore but still tested. This read to me as if we could put put this behind IsInAutomation().

It's used in Thunderbird, and they use this docshell code, so it would brick XUL about:addons. Basically, toolkit supports it but it isn't used in Firefox (it's behind prefs - prefs which we check in that docshell code). If we remove support for the prefs, we can remove support for the check in docshell, but trying to separately harden that check further right now doesn't seem worth it - we should just remove the old stuff. I'll ping the add-on manager bugs again.