1613609 - system-principal restrictions should check for allowed ui resources rather than blacklisting based on uri scheme

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P3)

Description

[Frederik Braun [:freddy]]

As a follow-up to https://bugzilla.mozilla.org/show_bug.cgi?id=1607483 and https://bugzilla.mozilla.org/show_bug.cgi?id=1513445 we should invert the check for badness (e.g., HTTP, HTTPS, FTP) and instead check for allowed resources like we did in bug 1560178 (e.g., checking for URI_IS_UI_RESOURCE), see https://searchfox.org/mozilla-central/rev/3a0a8e2762821c6afc1d235b3eb3dde63ad3b01a/docshell/base/nsDocShell.cpp#8973-9031

Maybe, this could be a good second bug for a contributor?

Comment 1

[Frederik Braun [:freddy]]

Attachment: Bug 1613609 - transform system principal load restrictions into allow-list r=ckerschb

Comment 2

[Frederik Braun [:freddy]]

Attachment: Bug 1613609 - add 'allowDeprecatedSystemRequests' loadinfo flag r=ckerschb

Depends on D68034

Comment 3

[Frederik Braun [:freddy]]

Attachment: Bug 1613609 - allow requests with new loadinfo flag to succeeed r=ckerschb

Depends on D68035

Comment 4

[Frederik Braun [:freddy]]

Attachment: Bug 1613609 - allow requests with new loadinfo flag to succeeed r=ckerschb

Depends on D68035

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f1a62010e1f
add 'allowDeprecatedSystemRequests' loadinfo flag r=ckerschb

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6c6c67870c37
allow requests with new loadinfo flag to succeeed r=ckerschb

Comment 7

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/4f1a62010e1f
https://hg.mozilla.org/mozilla-central/rev/6c6c67870c37

Comment 8

[Frederik Braun [:freddy]]

Still needs updating the callsites. I'll do that in the same bug.

Comment 9

[Frederik Braun [:freddy]]

Attachment: Bug 1613609 - Add required loadinfo flag requests initiated with SystemPrincipal

Adding the flag to existing channel/loadinfo object for:

• PushServices HTTP, WebSocket
• NetworkGeolocationProvider
• NetUtil.jsm's NewChannel
• NetworkConnectivityService
• OCSP
• Portal Detection
• ProductAddonChecker.jsm
• URLClassifier

Comment 10

[Frederik Braun [:freddy]]

Attachment: Bug 1613609 - prototype patch with whitelist for sysrequest r=ckerschb

Depends on D75063

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/72199fc4ea2b
Add required loadinfo flag requests initiated with SystemPrincipal r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/c593a7296df4
prototype patch with whitelist for sysrequest r=ckerschb

Comment 12

[Natalia Csoregi [:nataliaCs]]

Backed out for multiple failures on nsXPConnect.cpp

backout: https://hg.mozilla.org/integration/autoland/rev/b11b50ab989fef4d9ed7e17806abda0f5cff83e9

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedTaskRun=IVqkLK9zTyK3hG0jCUeirA-0&revision=c593a7296df40184cb02671c2a78911af802ea86&group_state=expanded

failure: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=302714014&repo=autoland&lineNumber=1538

[task 2020-05-18T09:53:27.011Z] 09:53:27 INFO - INFO | runtests.py | TSan using symbolizer at /builds/worker/workspace/build/application/firefox/llvm-symbolizer
[task 2020-05-18T09:53:27.015Z] 09:53:27 INFO - runtests.py | SSL tunnel pid: 1253
[task 2020-05-18T09:53:27.112Z] 09:53:27 INFO - greprefs.js:2149: prefs parse error: expected ';' after ')'
[task 2020-05-18T09:53:27.115Z] 09:53:27 INFO - ThreadSanitizer:DEADLYSIGNAL
[task 2020-05-18T09:53:27.115Z] 09:53:27 INFO - ==1247==ERROR: ThreadSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f3a32365327 bp 0x7fff1fc15690 sp 0x7fff1fc155c0 T1247)
[task 2020-05-18T09:53:27.115Z] 09:53:27 INFO - ==1247==The signal is caused by a WRITE memory access.
[task 2020-05-18T09:53:27.116Z] 09:53:27 INFO - ==1247==Hint: address points to the zero page.
[task 2020-05-18T09:53:27.475Z] 09:53:27 INFO - #0 xpc::ReadOnlyPage::Init() /builds/worker/checkouts/gecko/js/xpconnect/src/nsXPConnect.cpp:1181:3 (libxul.so+0x1993327)
[task 2020-05-18T09:53:27.475Z] 09:53:27 INFO - #1 nsXPConnect::InitStatics() /builds/worker/checkouts/gecko/js/xpconnect/src/nsXPConnect.cpp:143:3 (libxul.so+0x1993106)
[task 2020-05-18T09:53:27.475Z] 09:53:27 INFO - #2 xpcModuleCtor() /builds/worker/checkouts/gecko/js/xpconnect/src/XPCModule.cpp:11:3 (libxul.so+0x1968f5f)
[task 2020-05-18T09:53:27.475Z] 09:53:27 INFO - #3 nsLayoutModuleInitialize() /builds/worker/checkouts/gecko/layout/build/nsLayoutModule.cpp:108:7 (libxul.so+0x4f80047)
[task 2020-05-18T09:53:27.475Z] 09:53:27 INFO - #4 nsComponentManagerImpl::Init() /builds/worker/checkouts/gecko/xpcom/components/nsComponentManager.cpp:482:5 (libxul.so+0xadfa11)
[task 2020-05-18T09:53:27.476Z] 09:53:27 INFO - #5 NS_InitXPCOM /builds/worker/checkouts/gecko/xpcom/build/XPCOMInit.cpp:445:51 (libxul.so+0xb3c7a4)
[task 2020-05-18T09:53:27.476Z] 09:53:27 INFO - #6 XRE_XPCShellMain(int, char**, char**, XREShellData const*) /builds/worker/checkouts/gecko/js/xpconnect/src/XPCShellImpl.cpp:1204:10 (libxul.so+0x1976c1d)
[task 2020-05-18T09:53:27.476Z] 09:53:27 INFO - #7 mozilla::BootstrapImpl::XRE_XPCShellMain(int, char**, char**, XREShellData const*) /builds/worker/checkouts/gecko/toolkit/xre/Bootstrap.cpp:54:12 (libxul.so+0x63e10ab)
[task 2020-05-18T09:53:27.480Z] 09:53:27 INFO - #8 main /builds/worker/checkouts/gecko/js/xpconnect/shell/xpcshell.cpp:66:27 (xpcshell+0xc7c16)
[task 2020-05-18T09:53:27.528Z] 09:53:27 INFO - #9 __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310 (libc.so.6+0x21b96)
[task 2020-05-18T09:53:27.528Z] 09:53:27 INFO - #10 _start <null> (xpcshell+0x30970)
[task 2020-05-18T09:53:27.528Z] 09:53:27 INFO - ThreadSanitizer can not provide additional info.
[task 2020-05-18T09:53:27.528Z] 09:53:27 INFO - SUMMARY: ThreadSanitizer: SEGV /builds/worker/checkouts/gecko/js/xpconnect/src/nsXPConnect.cpp:1181:3 in xpc::ReadOnlyPage::Init()

Comment 13

[Natalia Csoregi [:nataliaCs]]

Also Crash: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=302714348&repo=autoland&lineNumber=1367

Comment 14

[Frederik Braun [:freddy]]

The TSAN stack in comment 12 looks scary, but it's just a forgotten semicolon when adding a pref.
Maybe we ought to have a linter for that.

The geckoview crash from comment 13 looks unrelated, but I'll try to repro locally.

Comment 15

[Frederik Braun [:freddy]]

Comment 13 is a different codepath to reading preferences (due to geckoview working differently) but it's ultimately caused by the same bug: A missing semicolon in all.js.

I'm feeling lucky. Landing again with the added semicolon.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5bcb7b13a4ad
Add required loadinfo flag requests initiated with SystemPrincipal r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/dd6e395dc342
prototype patch with whitelist for sysrequest r=ckerschb

Comment 17

[Alexandru Michis [:malexandru]]

Backed out 2 changesets for causing bustages.

Backout link: https://hg.mozilla.org/integration/autoland/rev/514a51a0cac1ccb14850ea8984eb1608ab48930c

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=%28run&tochange=514a51a0cac1ccb14850ea8984eb1608ab48930c&fromchange=70102e3f7b282236de8e9bd6863bc78b53856aed

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=302745382&repo=autoland&lineNumber=542

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ca644917ae5a
Add required loadinfo flag requests initiated with SystemPrincipal r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/be2d763a80d8
prototype patch with whitelist for sysrequest r=ckerschb

Comment 19

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ca644917ae5a
https://hg.mozilla.org/mozilla-central/rev/be2d763a80d8

Comment 20

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/522db850edb3
Backed out changeset be2d763a80d8 for making nightlies not being able to connecto to sites fi add-ons are installed. a=backout DONTBUILD

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/53e1ca235403
prototype patch with whitelist for sysrequest r=ckerschb

Comment 22

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/53e1ca235403

Comment 23

[Jeff Muizelaar [:jrmuizel]]

So it looks like this also broke updates. See bug 1639979.

Comment 24

[Frederik Braun [:freddy]]

Thanks for the pointer. The push from May 20th has this shipped with the pref set to disable the check, while we iterate on reducing breakage.
My sincere apologies.