1780822 - Early Hint crashes when preloading stylesheets

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect, P2)

Description

[Marc Leclair [:mleclair]]

On windows 11, when connecting to shopify.com with early hints enable, I get the following crash when launching firefox with shopify in a tab which opens right away:

[Parent 20688, Main Thread] WARNING: NS_ENSURE_SUCCESS(status, status) failed with result 0x804B0002 (NS_BINDING_ABORTED): file C:/mozcentral/mozcentral/netwerk/protocol/http/nsCORSListenerProxy.cpp:1343
Assertion failure: false (Disallowing SystemPrincipal load of stylesheets on HTTP(S).), at C:/mozcentral/mozcentral/dom/security/nsContentSecurityManager.cpp:1184
#01: nsContentSecurityManager::CheckAllowLoadInSystemPrivilegedContext (C:\mozcentral\mozcentral\dom\security\nsContentSecurityManager.cpp:1183)
#02: nsContentSecurityManager::doContentSecurityCheck (C:\mozcentral\mozcentral\dom\security\nsContentSecurityManager.cpp:1384)
#03: mozilla::net::nsHttpChannel::AsyncOpen (C:\mozcentral\mozcentral\netwerk\protocol\http\nsHttpChannel.cpp:5752)
#04: mozilla::net::EarlyHintPreloader::OpenChannel (C:\mozcentral\mozcentral\netwerk\protocol\http\EarlyHintPreloader.cpp:253)
#05: mozilla::net::EarlyHintPreloader::MaybeCreateAndInsertPreload (C:\mozcentral\mozcentral\netwerk\protocol\http\EarlyHintPreloader.cpp:214)
#06: mozilla::net::EarlyHintsService::EarlyHint (C:\mozcentral\mozcentral\netwerk\protocol\http\EarlyHintsService.cpp:47)
#07: mozilla::net::DocumentLoadListener::EarlyHint (C:\mozcentral\mozcentral\netwerk\ipc\DocumentLoadListener.cpp:2849)
#08: mozilla::net::nsHttpChannel::EarlyHint (C:\mozcentral\mozcentral\netwerk\protocol\http\nsHttpChannel.cpp:9734)
#09: mozilla::detail::RunnableFunction<`lambda at C:/mozcentral/mozcentral/netwerk/protocol/http/nsHttpTransaction.cpp:1973:19'>::Run (C:\mozcentral\mozcentral\obj-x86_64-pc-mingw32\dist\include\nsThreadUtils.h:532)
#10: mozilla::RunnableTask::Run (C:\mozcentral\mozcentral\xpcom\threads\TaskController.cpp:539)
#11: mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal (C:\mozcentral\mozcentral\xpcom\threads\TaskController.cpp:851)
#12: mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal (C:\mozcentral\mozcentral\xpcom\threads\TaskController.cpp:683)
#13: mozilla::TaskController::ProcessPendingMTTask (C:\mozcentral\mozcentral\xpcom\threads\TaskController.cpp:461)
#14: mozilla::detail::RunnableFunction<`lambda at C:/mozcentral/mozcentral/xpcom/threads/TaskController.cpp:190:7'>::Run (C:\mozcentral\mozcentral\xpcom\threads\nsThreadUtils.h:532)
#15: nsThread::ProcessNextEvent (C:\mozcentral\mozcentral\xpcom\threads\nsThread.cpp:1209)
#16: NS_ProcessNextEvent (C:\mozcentral\mozcentral\xpcom\threads\nsThreadUtils.cpp:465)
#17: mozilla::ipc::MessagePump::Run (C:\mozcentral\mozcentral\ipc\glue\MessagePump.cpp:107)
#18: MessageLoop::RunHandler (C:\mozcentral\mozcentral\ipc\chromium\src\base\message_loop.cc:374)
#19: MessageLoop::Run (C:\mozcentral\mozcentral\ipc\chromium\src\base\message_loop.cc:356)
#20: nsBaseAppShell::Run (C:\mozcentral\mozcentral\widget\nsBaseAppShell.cpp:152)
#21: nsAppShell::Run (C:\mozcentral\mozcentral\widget\windows\nsAppShell.cpp:613)
#22: nsAppStartup::Run (C:\mozcentral\mozcentral\toolkit\components\startup\nsAppStartup.cpp:296)
#23: XREMain::XRE_mainRun (C:\mozcentral\mozcentral\toolkit\xre\nsAppRunner.cpp:5719)
#24: XREMain::XRE_main (C:\mozcentral\mozcentral\toolkit\xre\nsAppRunner.cpp:5913)
#25: XRE_main (C:\mozcentral\mozcentral\toolkit\xre\nsAppRunner.cpp:5981)
#26: NS_internal_main (C:\mozcentral\mozcentral\browser\app\nsBrowserApp.cpp:406)
#27: wmain (C:\mozcentral\mozcentral\toolkit\xre\nsWindowsWMain.cpp:167)
#28: __scrt_common_main_seh (D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
#29: BaseThreadInitThunk[C:\Windows\System32\KERNEL32.DLL +0x154e0]

This only happens on my local build and I couldn't reproduce on Mac or linux. This is happening on this commit : 696551:04b02c492dab

Comment 1

[Manuel Bucher [:manuel]]

I could reproduce on Linux. Steps to reproduce:

1. enable early hints in about:config: network.early-hints.enabled true
2. start firefox with the shopify url: ./mach run -- https://www.shopify.com/

Comment 2

[Manuel Bucher [:manuel]]

We are currently using the triggeringPrincipal. It seems to be the systemPrincipal when the website loads on startup, which is weird. Is there a principal, that is not the systemPrincipal when loading a website with ./mach run -- https://www.shopify.com/?

Maybe we need to switch to the loadingPrincipal, principalToInherit or some other principal (topLevelPrincipal doesn't seem like the one we are looking for). The principal is needed to call NS_NewChannel. I initially tried to pass nsILoadNode to NS_NewChannel, but I think it isn't available during this stage of the code.

Comment 3

[Manuel Bucher [:manuel]]

both loadingPrincipal and principalToInherit are nullptr on startup when opening the web page with ./mach run -- https://www.shopify.com/.

Comment 4

[Manuel Bucher [:manuel]]

Attachment: Bug 1780822 - Use correct principal on early hint loads r=#necko

Comment 5

[Manuel Bucher [:manuel]]

Even using FindPrincipalToInherit leads to the systemPrincipal (I've tried that in the attached patch)

The triggeringPrincipal is the systemPrincipal for user initiated loads (entering the URL into the URL-bar or starting firefox with that specific url).

Tom, do you know how to refactor the code, that we get the correct (non-system) principal to start the early hint loads?

Comment 6

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Short answer: no. But I might be able to help you fumble around...

I don't know this as a fact, but only as an observation that I've gotten r+ on in the past - document types (which this would be AIUI) don't have a loading principal in the LoadInfo. Instead you get get the channelURI which is a building block of the principals created for the channel (which I think might be done in these functions but I'm really not sure about that.)

The CheckAllowLoadInSystemPrivilegedContext check is to ensure we don't load resources with privileges we shouldn't - it doesn't care about things like OriginAttributes - but a solution that actively ignores OA and just creates a principal using the URI is playing with fire, because if that principal was used for anything else (like constructing a hashkey or whatnot) - it would actively leak content across OA boundaries (e.g. PBM, Containers, ETP, FPI, etc).

I think what you want to do is use nsContentUtils::GetSecurityManager()->GetChannelResultPrincipal() like this to construct a principal from the channel. But I'm really not certain about that - it might seem to work, but be the wrong approach. Someone will need to review it; but I can't do it :)

As an aside, I noticed in the spec that Early Hints can come with CSP headers - do we process/honor those?

Comment 7

[Manuel Bucher [:manuel]]

No, we don't look at the CSP header yet. We are tracking that in Bug 1765289.

Comment 8

[Manuel Bucher [:manuel]]

Attachment: Bug 1780822 - Test that user generated loads don't crash due to using systemPrincipal as loadingPrincipal when processing Early Hints r=#necko

Depends on D152861

Comment 9

[Manuel Bucher [:manuel]]

I think testing this is hard, because the systemprincipal checks seem to be disabled during browser-tests: https://searchfox.org/mozilla-central/rev/5644fae86d5122519a0e34ee03117c88c6ed9b47/dom/security/nsContentSecurityManager.cpp#1134-1151.
I've work around that by enabling security.disallow_non_local_systemprincipal_in_tests during the page load. The test fails before the commit and passes after the commit, so seems to work (aside from a probably unrelated test failure):

netwerk/test/browser/browser_103_user_load.js
  FAIL waiting for vsync to be disabled - timed out after 50 tries. - false == true - JS frame :: chrome://mochikit/content/browser-test.js :: ensureVsyncDisabled :: line 575
Stack trace:
chrome://mochikit/content/browser-test.js:ensureVsyncDisabled:575
  FAIL vsync remained enabled at the end of the test. Is there an animation still running? Consider talking to the performance team for tips to solve this. - false == true - JS frame :: chrome://mochikit/content/browser-test.js :: ensureVsyncDisabled :: line 576
Stack trace:
chrome://mochikit/content/browser-test.js:ensureVsyncDisabled:576

Comment 10

[Manuel Bucher [:manuel]]

It turns out that all browser tests are currently failing on wayland with this error message. So the test failure is not related to my test case and can be ignored.

Comment 11

[Manuel Bucher [:manuel]]

That (unrelated) test failure is tracked in Bug 1783818

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1085415eea61
Use correct principal on early hint loads r=necko-reviewers,ckerschb,kershaw
https://hg.mozilla.org/integration/autoland/rev/7e3f9c4adac2
Test that user generated loads don't crash due to using systemPrincipal as loadingPrincipal when processing Early Hints r=necko-reviewers,kershaw

Comment 13

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/1085415eea61
https://hg.mozilla.org/mozilla-central/rev/7e3f9c4adac2

Comment 14

[Ardelean Oana, Desktop QA [:oardelean]]

I was unable to reproduce this issue on Windows 11 nor on Ubuntu 22.04 on a 2022-07-22 Nightly build. Would you be so kind as to verify the fix on the latest beta/nightly builds?
Thank you.

Comment 15

[Manuel Bucher [:manuel]]

Yeah, this only crashed in debug builds (MOZ_CRASH), so neither in Nightly nor Beta.

Download debug builds from Treeherder:

• downloaded current version from treeherder: doesn't crash
• downloaded parent commit of my patch from treeherder: crashes