1869938 - Assertion failure: NS_UsePrivateBrowsing(newChannel) == mRespectPrivacy, at /builds/worker/checkouts/gecko/image/imgLoader.cpp:2446

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Tyson Smith [:tsmith]]

Found with m-c 20231213-63aeef886ee6 (--enable-debug)

This was found by visiting a live website with a debug build.

STR:

• Launch browser and visit site

This issue was triggered by visiting www.pbfenergy.com.

Assertion failure: NS_UsePrivateBrowsing(newChannel) == mRespectPrivacy, at /builds/worker/checkouts/gecko/image/imgLoader.cpp:2446

#0 0x7f2b32c9a6b7 in imgLoader::LoadImage(nsIURI*, nsIURI*, nsIReferrerInfo*, nsIPrincipal*, unsigned long, nsILoadGroup*, imgINotificationObserver*, nsINode*, mozilla::dom::Document*, unsigned int, nsISupports*, nsIContentPolicy::nsContentPolicyType, nsTSubstring<char16_t> const&, bool, bool, unsigned long, imgRequestProxy**) /builds/worker/checkouts/gecko/image/imgLoader.cpp:2446:5
#1 0x7f2b32dbfc33 in nsContentUtils::LoadImage(nsIURI*, nsINode*, mozilla::dom::Document*, nsIPrincipal*, unsigned long, nsIReferrerInfo*, imgINotificationObserver*, int, nsTSubstring<char16_t> const&, imgRequestProxy**, nsIContentPolicy::nsContentPolicyType, bool, bool, unsigned long) /builds/worker/checkouts/gecko/dom/base/nsContentUtils.cpp:4000:21
#2 0x7f2b32ec896c in nsImageLoadingContent::LoadImage(nsIURI*, bool, bool, nsImageLoadingContent::ImageLoadType, unsigned int, mozilla::dom::Document*, nsIPrincipal*) /builds/worker/checkouts/gecko/dom/base/nsImageLoadingContent.cpp:1143:17
#3 0x7f2b32ec9331 in nsImageLoadingContent::LoadImage(nsTSubstring<char16_t> const&, bool, bool, nsImageLoadingContent::ImageLoadType, nsIPrincipal*) /builds/worker/checkouts/gecko/dom/base/nsImageLoadingContent.cpp:1027:10
#4 0x7f2b35d41a65 in mozilla::dom::SVGImageElement::LoadSVGImage(bool, bool) /builds/worker/checkouts/gecko/dom/svg/SVGImageElement.cpp:146:10
#5 0x7f2b35d41e6e in mozilla::dom::SVGImageElement::AfterSetAttr(int, nsAtom*, nsAttrValue const*, nsAttrValue const*, nsIPrincipal*, bool) /builds/worker/checkouts/gecko/dom/svg/SVGImageElement.cpp:215:9
#6 0x7f2b3308f6f9 in mozilla::dom::Element::SetAttrAndNotify(int, nsAtom*, nsAtom*, nsAttrValue const*, nsAttrValue&, nsIPrincipal*, unsigned char, bool, bool, bool, mozilla::dom::Document*, mozAutoDocUpdate const&) /builds/worker/checkouts/gecko/dom/base/Element.cpp:2699:5
#7 0x7f2b3308892a in mozilla::dom::Element::SetAttr(int, nsAtom*, nsAtom*, nsTSubstring<char16_t> const&, nsIPrincipal*, bool) /builds/worker/checkouts/gecko/dom/base/Element.cpp:2555:10
#8 0x7f2b364a7fea in SetAttr /builds/worker/workspace/obj-build/dist/include/mozilla/dom/Element.h:962:12
#9 0x7f2b364a7fea in nsXMLContentSink::AddAttributes(char16_t const**, mozilla::dom::Element*) /builds/worker/checkouts/gecko/dom/xml/nsXMLContentSink.cpp:1366:15
#10 0x7f2b364a54ed in nsXMLContentSink::HandleStartElement(char16_t const*, char16_t const**, unsigned int, unsigned int, unsigned int, bool) /builds/worker/checkouts/gecko/dom/xml/nsXMLContentSink.cpp:949:12
#11 0x7f2b364a5b7a in HandleStartElement /builds/worker/checkouts/gecko/dom/xml/nsXMLContentSink.cpp:895:10
#12 0x7f2b364a5b7a in non-virtual thunk to nsXMLContentSink::HandleStartElement(char16_t const*, char16_t const**, unsigned int, unsigned int, unsigned int) /builds/worker/checkouts/gecko/dom/xml/nsXMLContentSink.cpp
#13 0x7f2b324a4559 in nsExpatDriver::HandleStartElement(rlbox::rlbox_sandbox<rlbox::rlbox_wasm2c_sandbox>&, rlbox::tainted<void*, rlbox::rlbox_wasm2c_sandbox>, rlbox::tainted<char16_t const*, rlbox::rlbox_wasm2c_sandbox>, rlbox::tainted<char16_t const**, rlbox::rlbox_wasm2c_sandbox>) /builds/worker/checkouts/gecko/parser/htmlparser/nsExpatDriver.cpp:477:32
#14 0x7f2b3125d5fa in w2c_rlbox_doContent /builds/worker/workspace/obj-build/security/rlbox/rlbox.wasm.c:56102:5
#15 0x7f2b31242761 in w2c_rlbox_contentProcessor /builds/worker/workspace/obj-build/security/rlbox/rlbox.wasm.c:55060:12
#16 0x7f2b31040a3e in w2c_rlbox_MOZ_XML_ParseBuffer_0 /builds/worker/workspace/obj-build/security/rlbox/rlbox.wasm.c:51128:12
#17 0x7f2b3103ffa2 in w2c_rlbox_MOZ_XML_Parse_0 /builds/worker/workspace/obj-build/security/rlbox/rlbox.wasm.c:50769:12
#18 0x7f2b3103ffa2 in w2c_rlbox_MOZ_XML_Parse /builds/worker/workspace/obj-build/security/rlbox/rlbox.wasm.c:13442:10
#19 0x7f2b324aa738 in impl_invoke_with_func_ptr<XML_Status (XML_ParserStruct *, const char *, int, int), XML_Status (unsigned int, unsigned int, int, int), unsigned int, unsigned int, unsigned int, bool> /builds/worker/workspace/obj-build/dist/include/mozilla/rlbox/rlbox_wasm2c_sandbox.hpp:803:13
#20 0x7f2b324aa738 in INTERNAL_invoke_with_func_ptr<XML_Status (XML_ParserStruct *, const char *, int, int), rlbox::tainted<XML_ParserStruct *, rlbox::rlbox_wasm2c_sandbox> &, rlbox::tainted<const char *, rlbox::rlbox_wasm2c_sandbox>, unsigned long, bool> /builds/worker/workspace/obj-build/dist/include/mozilla/rlbox/rlbox_sandbox.hpp:790:40
#21 0x7f2b324aa738 in nsExpatDriver::ParseChunk(char16_t const*, unsigned int, nsExpatDriver::ChunkOrBufferIsFinal, unsigned int*, unsigned long*) /builds/worker/checkouts/gecko/parser/htmlparser/nsExpatDriver.cpp:1248:14
#22 0x7f2b324ab0ad in ChunkAndParseBuffer /builds/worker/checkouts/gecko/parser/htmlparser/nsExpatDriver.cpp:1204:3
#23 0x7f2b324ab0ad in nsExpatDriver::ResumeParse(nsScanner&, bool) /builds/worker/checkouts/gecko/parser/htmlparser/nsExpatDriver.cpp:1352:5
#24 0x7f2b324b2f3f in nsParser::ResumeParse(bool, bool, bool) /builds/worker/checkouts/gecko/parser/htmlparser/nsParser.cpp:716:24
#25 0x7f2b324b485a in nsParser::OnDataAvailable(nsIRequest*, nsIInputStream*, unsigned long, unsigned int) /builds/worker/checkouts/gecko/parser/htmlparser/nsParser.cpp:1027:12
#26 0x7f2b32cac226 in imgRequest::OnDataAvailable(nsIRequest*, nsIInputStream*, unsigned long, unsigned int) /builds/worker/checkouts/gecko/image/imgRequest.cpp:1073:16
#27 0x7f2b315ec088 in nsBaseChannel::OnDataAvailable(nsIRequest*, nsIInputStream*, unsigned long, unsigned int) /builds/worker/checkouts/gecko/netwerk/base/nsBaseChannel.cpp:849:28
#28 0x7f2b3161b2d8 in nsInputStreamPump::OnStateTransfer() /builds/worker/checkouts/gecko/netwerk/base/nsInputStreamPump.cpp:585:22
#29 0x7f2b3161a9d7 in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) /builds/worker/checkouts/gecko/netwerk/base/nsInputStreamPump.cpp:412:21
#30 0x7f2b3161bd3c in non-virtual thunk to nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) /builds/worker/checkouts/gecko/netwerk/base/nsInputStreamPump.cpp
#31 0x7f2b31359391 in mozilla::NonBlockingAsyncInputStream::RunAsyncWaitCallback(mozilla::NonBlockingAsyncInputStream::AsyncWaitRunnable*, already_AddRefed<nsIInputStreamCallback>) /builds/worker/checkouts/gecko/xpcom/io/NonBlockingAsyncInputStream.cpp:385:13
#32 0x7f2b3135851f in mozilla::NonBlockingAsyncInputStream::AsyncWaitRunnable::Run() /builds/worker/checkouts/gecko/xpcom/io/NonBlockingAsyncInputStream.cpp:33:14
#33 0x7f2b313d9db7 in mozilla::RunnableTask::Run() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:549:16
#34 0x7f2b313cf9c3 in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:876:26
#35 0x7f2b313ce1b7 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:699:15
#36 0x7f2b313ce635 in mozilla::TaskController::ProcessPendingMTTask(bool) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:485:36
#37 0x7f2b313ddd26 in operator() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:211:37
#38 0x7f2b313ddd26 in mozilla::detail::RunnableFunction<mozilla::TaskController::TaskController()::$_0>::Run() /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.h:548:5
#39 0x7f2b313f2e12 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1199:16
#40 0x7f2b313f9f3d in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:480:10
#41 0x7f2b320cb1d5 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:85:21
#42 0x7f2b31fe4d11 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:363:3
#43 0x7f2b31fe4d11 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:345:3
#44 0x7f2b368c46f8 in nsBaseAppShell::Run() /builds/worker/checkouts/gecko/widget/nsBaseAppShell.cpp:148:27
#45 0x7f2b369814a8 in nsAppShell::Run() /builds/worker/checkouts/gecko/widget/gtk/nsAppShell.cpp:470:33
#46 0x7f2b3882ba14 in nsAppStartup::Run() /builds/worker/checkouts/gecko/toolkit/components/startup/nsAppStartup.cpp:296:30
#47 0x7f2b38996005 in XREMain::XRE_mainRun() /builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp:5709:22
#48 0x7f2b38997776 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp:5918:8
#49 0x7f2b38998392 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp:5974:21
#50 0x5576d2c26e87 in do_main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:227:22
#51 0x5576d2c26e87 in main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:445:16
#52 0x7f2b4594dd8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#53 0x7f2b4594de3f in __libc_start_main csu/../csu/libc-start.c:392:3
#54 0x5576d2bfcca8 in _start (/home/worker/build/firefox-bin+0x58ca8) (BuildId: 76ae9b20fe68d09c9d75c1fa641ced1cd3795d82)

Comment 1

[Tyson Smith [:tsmith]]

A Pernosco session is available here: https://pernos.co/debug/bRlyg3X2akE4qE5tzexZiQ/index.html

Comment 2

[Timothy Nikkel (:tnikkel)]

(In reply to Tyson Smith [:tsmith] from comment #0)

STR:

• Launch browser and visit site

Also need to be in private browsing window.

Comment 3

[Timothy Nikkel (:tnikkel)]

Not a recent regression, reproduces back to 2020-06-01.

Comment 4

[Timothy Nikkel (:tnikkel)]

Hmm, this potentially allows a page to escape out of private browsing mode so I'm going to hide this and let our security people determine the severity.

It looks like the favicon is an svg, which contains an svg image element, the load of that image seems to happen without respectively private browsing mode.

Comment 5

[Timothy Nikkel (:tnikkel)]

Attachment: sfav.html

Here's a reduced testcase.

It's just an svg document as the favicon expressed as a data uri, and inside it there is an svg image element whose src is a data uri for a png.

Comment 6

[Timothy Nikkel (:tnikkel)]

The principal we are using to load the favicons is the system principal. For the favicon load itself the channel is set to private browsing mode because it is able to get the private browsing mode from the loadcontext (aka dochsell I believe). But for the load of the png inside the svg favicon we can't access the loadcontext/docshell (because inside the svg we don't have a docshell), so the only way for the private browsing mode to make it's way inside the svg is via the principal, which is again just the system principal (with no private browsing annotation, not sure if that's possible).

We have this code for "triggering principal" which was added to give the favicon load in the parent process the correct (non-system) principal, but it does not seem to be activating. So probably the next step is to investigate why that is not happening and fix that since I do not think we want to be loading content provided urls in the parent process with the system principal.

Comment 7

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #6)

We have this code for "triggering principal" which was added to give the favicon load in the parent process the correct (non-system) principal, but it does not seem to be activating. So probably the next step is to investigate why that is not happening and fix that since I do not think we want to be loading content provided urls in the parent process with the system principal.

Looks like bug 1277803 was where that was added.

Looks like the principal should come from the iconloadingprincipal attribute, which is set here

https://searchfox.org/mozilla-central/rev/8b5d93d099b5b501cf5bd2b606c59afcf88bef1c/browser/base/content/tabbrowser.js#1006

unfortunately every call of that function passes null for the loading principal (except for the one in sessionstore, so I don't think that one can ever be non-null because it would require someone else to set it to non-null first so that it was in the saved data). I'm not sure when we stopped passing a principal into there.

Bug 1277803 added several tests, I'm not sure why they aren't failing.

Comment 8

[Timothy Nikkel (:tnikkel)]

Ah, so I found

https://bugzilla.mozilla.org/show_bug.cgi?id=1453751
https://hg.mozilla.org/integration/mozilla-inbound/rev/40ed437da7ae

where we load the favicon in the child process and convert it to the data uri before passing to the parent. That explains why the loading principal was removed from the setIcon calls because they should all be data uris.

Comment 9

[Timothy Nikkel (:tnikkel)]

If I make a content page set a favicon as a data uri to an svg, and have that svg include an image whose source is remote (ie wikipedia.org), then that data uri makes it to the parent process image element intact and we do get as far as creating a channel for that remote uri with private browsing not set.

However, I think there is something that blocks the load of the remote image at some point after that because the image does not show up in the favicon when loaded in a non-debug build (to avoid hitting this assert).

Comment 10

[Timothy Nikkel (:tnikkel)]

FWIW, I can fix this assert by passing the principal here

https://searchfox.org/mozilla-central/rev/8b5d93d099b5b501cf5bd2b606c59afcf88bef1c/browser/actors/LinkHandlerParent.sys.mjs#153

ie replace that line with

gBrowser.setIcon(tab, iconURL, originalURL, lazy.E10SUtils.serializePrincipal(browser.contentPrincipal));

Comment 11

[Timothy Nikkel (:tnikkel)]

And this is what blocks the remote load

https://searchfox.org/mozilla-central/rev/8b5d93d099b5b501cf5bd2b606c59afcf88bef1c/dom/base/nsDataDocumentContentPolicy.cpp#108

So this is probably not a big deal then. But we want to keep this assert and avoid false positives, so it'd be good to find a way to make this channel have private browsing set on it.

Comment 12

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #11)

And this is what blocks the remote load

https://searchfox.org/mozilla-central/rev/8b5d93d099b5b501cf5bd2b606c59afcf88bef1c/dom/base/nsDataDocumentContentPolicy.cpp#108

So this looks like it would allow javascript, blob, and data uris. And the principal we are loading with here is the system principal. So that would mean that we could potentially load any blob from any origin? That doesn't seem good.

Comment 13

[Timothy Nikkel (:tnikkel)]

I can fix the assert of this bug by getting the private browsing bit from the channel of the owner doc if there is no loading context like so

https://hg.mozilla.org/try/rev/b2431f384c474e726adef0fab2961b2075eac98c

Comment 14

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #12)

(In reply to Timothy Nikkel (:tnikkel) from comment #11)

And this is what blocks the remote load

https://searchfox.org/mozilla-central/rev/8b5d93d099b5b501cf5bd2b606c59afcf88bef1c/dom/base/nsDataDocumentContentPolicy.cpp#108

So this looks like it would allow javascript, blob, and data uris. And the principal we are loading with here is the system principal. So that would mean that we could potentially load any blob from any origin? That doesn't seem good.

I verified that I can show the blob image of a different page with a different origin in the favicon of another page (different origin) using this technique. Not sure how useful this is for an attacker since they will still need to get the blob uri somehow. Using comment 10 mitigates this. I'll file a bug so the right eyes can evaluate this. I'll keep this bug focused on the original assert.

Comment 15

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #14)

I verified that I can show the blob image of a different page with a different origin in the favicon of another page (different origin) using this technique. Not sure how useful this is for an attacker since they will still need to get the blob uri somehow. Using comment 10 mitigates this. I'll file a bug so the right eyes can evaluate this. I'll keep this bug focused on the original assert.

-> filed bug 1870596

Comment 16

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1869938. For docs that don't have a load context/docshell, try to inherit private browsing from the doc's channel. r?#necko-reviewers

This is how we decide which imgLoader to use (private or not) when we load an image: https://searchfox.org/mozilla-central/rev/91cc8848427fdbbeb324e6ca56a0d08d32d3c308/dom/base/nsContentUtils.cpp#3898

Depends on D196719

Comment 17

[Timothy Nikkel (:tnikkel)]

I think this is an S3 because I believe the channels where we can fail to set the private browsing bit are limited to URIs that are either javascript, blob, or data, for which I don't think private browsing will make much of a difference (correct me if any of this is wrong).

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a0075cc44476
For docs that don't have a load context/docshell, try to inherit private browsing from the doc's channel. r=necko-reviewers,kershaw

Comment 19

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/mozilla-central/rev/a0075cc44476

Comment 20

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

The patch landed in nightly and beta is affected.
:tnikkel, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox122 to wontfix.

For more information, please visit BugBot documentation.

Comment 21

[Timothy Nikkel (:tnikkel)]

Mainly to keep the assert from firing so if it fires we know it might be a real issue (as opposed to this bug).