1881800 - Make credentialed network fetches with system (chrome) principals opt-in (ie default to `credentials: omit`)

Status: RESOLVED FIXED

(Core :: DOM: Networking, enhancement, P2)

Description

[:Gijs (away/busy until Mar 10)]

Although frontend could potentially try to lint for this instead, it seems better to make fetch/XMLHttpRequest and other http/https network requests automatically omit credentials to defend-in-depth against privacy/auth leakage. Consumers that actually need the credentials should be able to explicitly request them using fetch's credentials option or similar.

Comment 1

[:Gijs (away/busy until Mar 10)]

One alternative that may be easier to deal with in terms of backwards compat / finding breakages would be to explicitly reject fetch requests from system principal that do not specify an explicit credentials mode (I'm assuming we can use webidl wasPassed type stuff to distinguish this from the case where the default of same-origin is used because nothing was passed in the options).

Comment 2

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

This sounds like a great idea. I suspect the more difficult part will be fixing all the tests that depend on this behaviour, but arguably it's worth the effort.

Comment 3

[:Gijs (away/busy until Mar 10)]

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

This sounds like a great idea. I suspect the more difficult part will be fixing all the tests that depend on this behaviour, but arguably it's worth the effort.

For fetch, at least in terms of frontend (mochitest-browser, xpcshell, marionette, mochitest-chrome) tests, it's surprisingly easy. I would not expect any web/platform/wpt tests to depend on privileged fetch calls, but I've been wrong before...

Trying to do this for XHR next. Though admittedly, it's a bit trickier to allow opt-in there - it doesn't appear to use webidl dictionaries and so it's harder to tell if a caller asked for something specific vs. passed no options at all. Either way, though I anticipate more fallout from that, I'll get the fetch patch up to start with.

Unsure if it's straightforward to do something at the channel level that would still allow opt-out. ISTM that the abstractions involved mean that it's hard/impossible to tell at that point if the inclusion of cookies or similar was intentional or not.

Comment 4

[:Gijs (away/busy until Mar 10)]

Attachment: Bug 1881800 - chrome/system-privileged fetch should not use credentials, r?valentin

Comment 5

[:Gijs (away/busy until Mar 10)]

Attachment: Bug 1881800 - chrome/system-privileged XHR should not use credentials, r?#dom-core-reviewers,Mossop!,#extension-reviewers!,#application-update-reviewers,decoder!

Comment 6

[:Gijs (away/busy until Mar 10)]

Realizing I didn't actually touch about pages here, technically, so best update the summary to clarify...

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9da17ae527f3
chrome/system-privileged fetch should not use credentials, r=necko-reviewers,barret,valentin,peterv
https://hg.mozilla.org/integration/autoland/rev/1710585070f0
chrome/system-privileged XHR should not use credentials, r=peterv,extension-reviewers,application-update-reviewers,decoder,mossop,robwu,releng-reviewers,bytesized,jcristau

Comment 8

[Iulian Moraru]

Backed out for causing non unified build bustages on nsIPrincipal.h.

• Push with failures

• Failure log

• Backout link

[task 2024-03-26T12:53:05.768Z] 12:53:05     INFO -  gmake[4]: Entering directory '/builds/worker/workspace/obj-build/dom/xhr'
[task 2024-03-26T12:53:05.771Z] 12:53:05     INFO -  /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ --sysroot /builds/worker/fetches/sysroot-x86_64-linux-gnu -o XMLHttpRequest.o -c  -I/builds/worker/workspace/obj-build/dist/stl_wrappers -I/builds/worker/workspace/obj-build/dist/system_wrappers -include /builds/worker/checkouts/gecko/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fstack-clash-protection -DNDEBUG=1 -DTRIMMED=1 -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -I/builds/worker/checkouts/gecko/dom/xhr -I/builds/worker/workspace/obj-build/dom/xhr -I/builds/worker/checkouts/gecko/dom/base -I/builds/worker/checkouts/gecko/dom/file -I/builds/worker/checkouts/gecko/netwerk/base -I/builds/worker/workspace/obj-build/ipc/ipdl/_ipdlheaders -I/builds/worker/checkouts/gecko/ipc/chromium/src -I/builds/worker/workspace/obj-build/dist/include -I/builds/worker/workspace/obj-build/dist/include/nspr -I/builds/worker/workspace/obj-build/dist/include/nss -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-build/mozilla-config.h -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -fno-aligned-new -fcrash-diagnostics-dir=/builds/worker/artifacts -fno-exceptions -fPIC -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -gdwarf-4 -Xclang -load -Xclang /builds/worker/workspace/obj-build/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O2 -fno-omit-frame-pointer -funwind-tables -Werror -Wall -Wbitfield-enum-conversion -Wdeprecated-this-capture -Wempty-body -Wformat-type-confusion -Wignored-qualifiers -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtautological-constant-in-range-compare -Wtype-limits -Wno-error=tautological-type-limit-compare -Wunreachable-code -Wunreachable-code-return -Wunused-but-set-parameter -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wno-range-loop-analysis -Wc++2a-compat -Wenum-compare-conditional -Wenum-float-conversion -Wno-error=deprecated -Wno-error=deprecated-anon-enum-enum-conversion -Wno-error=deprecated-enum-enum-conversion -Wno-error=deprecated-pragma -Wno-error=deprecated-this-capture -Wcomma -Wimplicit-fallthrough -Wstring-conversion -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wno-error=atomic-alignment -Wno-error=deprecated-builtins -Wformat -Wformat-security -Wno-psabi -Wthread-safety -Wno-error=builtin-macro-redefined -Wno-unknown-warning-option -fno-strict-aliasing -ffp-contract=off  -MD -MP -MF .deps/XMLHttpRequest.o.pp   /builds/worker/checkouts/gecko/dom/xhr/XMLHttpRequest.cpp
[task 2024-03-26T12:53:05.774Z] 12:53:05     INFO -  In file included from /builds/worker/checkouts/gecko/dom/xhr/XMLHttpRequest.cpp:7:
[task 2024-03-26T12:53:05.774Z] 12:53:05     INFO -  In file included from /builds/worker/checkouts/gecko/dom/xhr/XMLHttpRequest.h:13:
[task 2024-03-26T12:53:05.774Z] 12:53:05     INFO -  In file included from /builds/worker/workspace/obj-build/dist/include/mozilla/dom/XMLHttpRequestEventTarget.h:10:
[task 2024-03-26T12:53:05.774Z] 12:53:05     INFO -  In file included from /builds/worker/workspace/obj-build/dist/include/mozilla/DOMEventTargetHelper.h:12:
[task 2024-03-26T12:53:05.774Z] 12:53:05     INFO -  In file included from /builds/worker/workspace/obj-build/dist/include/mozilla/GlobalTeardownObserver.h:15:
[task 2024-03-26T12:53:05.774Z] 12:53:05     INFO -  In file included from /builds/worker/checkouts/gecko/dom/base/nsIGlobalObject.h:15:
[task 2024-03-26T12:53:05.774Z] 12:53:05     INFO -  In file included from /builds/worker/checkouts/gecko/dom/base/nsContentUtils.h:52:
[task 2024-03-26T12:53:05.774Z] 12:53:05     INFO -  In file included from /builds/worker/checkouts/gecko/dom/base/nsPIDOMWindow.h:21:
[task 2024-03-26T12:53:05.774Z] 12:53:05     INFO -  In file included from /builds/worker/workspace/obj-build/dist/include/nsILoadInfo.h:13:
[task 2024-03-26T12:53:05.774Z] 12:53:05     INFO -  In file included from /builds/worker/workspace/obj-build/dist/include/nsIScriptSecurityManager.h:11:
[task 2024-03-26T12:53:05.774Z] 12:53:05    ERROR -  /builds/worker/workspace/obj-build/dist/include/nsIPrincipal.h:348:20: error: inline function 'nsIPrincipal::IsSystemPrincipal' is not defined [-Werror,-Wundefined-inline]
[task 2024-03-26T12:53:05.774Z] 12:53:05     INFO -    348 |        inline bool IsSystemPrincipal() const;
[task 2024-03-26T12:53:05.774Z] 12:53:05     INFO -        |                    ^
[task 2024-03-26T12:53:05.774Z] 12:53:05     INFO -  /builds/worker/checkouts/gecko/dom/xhr/XMLHttpRequest.cpp:57:45: note: used here
[task 2024-03-26T12:53:05.774Z] 12:53:05     INFO -     57 |           (aParams.mMozSystem || principal->IsSystemPrincipal());
[task 2024-03-26T12:53:05.774Z] 12:53:05     INFO -        |                                             ^
[task 2024-03-26T12:53:05.774Z] 12:53:05     INFO -  1 error generated.
[task 2024-03-26T12:53:05.774Z] 12:53:05    ERROR -  gmake[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:690: XMLHttpRequest.o] Error 1
[task 2024-03-26T12:53:05.775Z] 12:53:05     INFO -  gmake[4]: Leaving directory '/builds/worker/workspace/obj-build/dom/xhr'

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dede488daf42
chrome/system-privileged fetch should not use credentials, r=necko-reviewers,barret,valentin,peterv
https://hg.mozilla.org/integration/autoland/rev/bc877b2f6c9e
chrome/system-privileged XHR should not use credentials, r=peterv,extension-reviewers,application-update-reviewers,decoder,mossop,robwu,releng-reviewers,bytesized,jcristau

Comment 10

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/dede488daf42
https://hg.mozilla.org/mozilla-central/rev/bc877b2f6c9e