1575356 - Audit usage of nsIDocShellTreeItem in nsMixedContentBlocker::ShouldLoad and nsDocShell::GetAllowMixedContentAndConnectionData

Reporter

Description

•

5 years ago

https://searchfox.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#830

https://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#5432

The nsMixedContentBlocker::ShouldLoad is the only caller of GetAllowMixedContentAndConnectionData. The latter method might have to disappear or be rewritten completely as a result of fixing ::ShouldLoad.

The GetAllowMixedContentAndConnectionData method returns three pieces of information:

Whether the root docshell has a secure connection.
Whether the root docshell allows mixed content and connection data.
Whether this docshell is the root docshell.

For out-of-process root documents, this method will return incorrect results. The implementation of this method reaches into the root docshell's document to retrieve the URI, principal, and channel.

This information cannot be replicated in the BrowsingContext, so I suspect fixing this will require some sync IPC.

For the caller (ShouldLoad), we also retrieve the root docshell, as well as traversing the parent docshell chain to the root, pulling information out from the contained document (URIs, etc.) to check for https schemes.

This implementation will need to be rewritten to use IPC to forward the lookup when an in-process root is reached. It looks like ShouldLoad is synchronous in nature and isn't amenable to making async. Its users are ShouldLoadCachedImage (synchronously coded), AsyncOnChannelRedirect (easier to adapt to async ShouldLoad implementation), and ShouldProcess (sync implementation).

Turning this code async to avoid blocking on an IPC call seems like it's a lot of work.

One thing to consider is caching this info in BrowsingContext - namely, a flag on every BrowsingContext which simply indicates whether any item in the tree up to the root is https. I'm not sure if that's considered a potential information leak via SPECTRE, but it seems like inconsequential information to expose.

Doing this would allow the entire walk up the docshell tree to be eliminated. The remaining logic surrounding parent docshells seems to all relate to the root docshell, and should be fixable by performing one IPC request to the root-document process (if needed).

Neha Kochar [:neha]

Comment 1

•

5 years ago

NI'ing Nika in case she has ideas.

Flags: needinfo?(nika)

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

Comment 2

•

5 years ago

(In reply to Kannan Vijayan [:djvj] from comment #0)

The GetAllowMixedContentAndConnectionData method returns three pieces of information:

Whether the root docshell has a secure connection.

Whether the root docshell allows mixed content and connection data.

Mixed content & secure connection status might be reasonable to store as a synced BrowsingContext field, but I'm not 100% sure. Given that this stuff is effectively inherited, this might be reasonable to propagate using TabContext or similar.

Whether this docshell is the root docshell.

This should be fairly straightforward to check using the BrowsingContext tree, by checking for GetBrowsingContext() == GetBrowsingContext()->Top() or similar.

One thing to consider is caching this info in BrowsingContext - namely, a flag on every BrowsingContext which simply indicates whether any item in the tree up to the root is https. I'm not sure if that's considered a potential information leak via SPECTRE, but it seems like inconsequential information to expose.

I don't think secure browsing & mixed content status is information which we should be worried about, so that seems reasonable.

Doing this would allow the entire walk up the docshell tree to be eliminated. The remaining logic surrounding parent docshells seems to all relate to the root docshell, and should be fixable by performing one IPC request to the root-document process (if needed).

Based on your summary above, I think we can get away with no IPC here, which would be ideal :-).

Flags: needinfo?(nika)

Neha Kochar [:neha]

Updated

•

5 years ago

Fission Milestone: --- → M5

Priority: -- → P2

Kannan Vijayan [:djvj]

Reporter

Updated

•

5 years ago

Whiteboard: [rm-docshell-tree-item:sync-state]

Kannan Vijayan [:djvj]

Reporter

Updated

•

5 years ago

Depends on: 1588178

Neha Kochar [:neha]

Updated

•

5 years ago

Fission Milestone: M5 → Future

Chris Peterson [:cpeterson]

Comment 3

•

5 years ago

Kannan says replacing nsIDocShellTreeItem calls should block enabling Fission in Nightly (M6).

Fission Milestone: Future → M6

Kannan Vijayan [:djvj]

Reporter

Updated

•

5 years ago

Blocks: fission-replicated-fields

Chris Peterson [:cpeterson]

Comment 4

•

4 years ago

Christoph, can someone on your team please audit this use of the nsIDocShellTreeItem interface in nsMixedContentBlocker?

With Fission enabled, Documents and nsDocShells for related frames, such as subframes and parent documents, may not be available within the current process and the corresponding nsIDocShellTreeItem methods will return null.

If this code is broken with Fission, fixing it blocks enabling Fission is Nightly and your team should prioritize (or delegate) the fix accordingly.

If this code works as-is with Fission, we don't need to remove this usage of nsIDocShellTreeItem until when we remove nsIDocShellTreeItem entirely (bug 1607591) after we ship Fission MVP.

Fission documentation about replacing nsIDocShellTree Item:
https://wiki.mozilla.org/Project_Fission/DocShell_Tree_Replace

:farre's presentation with examples of replacing nsIDocShellTreeItem with BrowsingContext, WindowContext, SyncedContexts, and BrowsingContextGroup:
https://docs.google.com/presentation/d/1K4j6ngty64TZjJNS5qH-MBoOm3TI2dJedVsbH8jUhKE/edit#slide=id.g6e35225e5d_1_264

Component: DOM: Navigation → DOM: Security

Flags: needinfo?(ckerschb)

Priority: P2 → --

Summary: Fix usage of nsIDocShellTreeItem in nsMixedContentBlocker::ShouldLoad and nsDocShell::GetAllowMixedContentAndConnectionData → Audit usage of nsIDocShellTreeItem in nsMixedContentBlocker::ShouldLoad and nsDocShell::GetAllowMixedContentAndConnectionData

Christoph Kerschbaumer [:ckerschb]

Assignee

Updated

•

4 years ago

Assignee: nobody → ckerschb

Blocks: 1584157

Status: NEW → ASSIGNED

Flags: needinfo?(ckerschb)

Priority: -- → P2

Whiteboard: [rm-docshell-tree-item:sync-state] → [rm-docshell-tree-item:sync-state][domsecurity-active]

Christoph Kerschbaumer [:ckerschb]

Assignee

Comment 5

•

4 years ago

Attached file Bug 1575356: Update Mixed Content Blocker to rely on BrowsingContext instead of nsIDocShellTreeItem. r=smaug,baku — Details

Christoph Kerschbaumer [:ckerschb]

Assignee

Updated

•

4 years ago

Blocks: 1633338

Pulsebot

Comment 6

•

4 years ago

Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/autoland/rev/14568f3c84b6
Update Mixed Content Blocker to rely on BrowsingContext instead of nsIDocShellTreeItem. r=baku,smaug

Natalia Csoregi [:nataliaCs]

Comment 7

•

4 years ago

Backed out for failures on test_iframe_referrer.html

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

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=linux%2C18.04%2Cx64%2Cquantumrender%2Cdebug%2Cmochitests%2Cwith%2Cfission%2Cenabled%2Ctest-linux1804-64-qr%2Fdebug-mochitest-plain-fis-e10s-16%2Cm-fis%2816%29&revision=14568f3c84b6b7f1c15b940aee0b2a28725530d9

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=299736335&repo=autoland&lineNumber=7266

[task 2020-04-28T06:42:58.378Z] 06:42:58 [task 2020-04-28T06:42:58.378Z] 06:42:58 [task 2020-04-28T06:42:58.378Z] 06:42:58 [task 2020-04-28T06:42:58.380Z] 06:42:58 [task 2020-04-28T06:42:58.381Z] 06:42:58 [task 2020-04-28T06:42:58.381Z] 06:42:58 [task 2020-04-28T06:42:58.381Z] 06:42:58 [task 2020-04-28T06:42:58.382Z] 06:42:58 [task 2020-04-28T06:42:58.382Z] 06:42:58 [task 2020-04-28T06:42:58.382Z] 06:42:58 [task 2020-04-28T06:42:58.382Z] 06:42:58 [task 2020-04-28T06:42:58.383Z] 06:42:58 [task 2020-04-28T06:42:58.383Z] 06:42:58 [task 2020-04-28T06:42:58.383Z] 06:42:58 [task 2020-04-28T06:42:58.383Z] 06:42:58 [task 2020-04-28T06:42:58.384Z] 06:42:58 [task 2020-04-28T06:42:58.384Z] 06:42:58 [task 2020-04-28T06:42:58.384Z] 06:42:58 [task 2020-04-28T06:42:58.384Z] 06:42:58 [task 2020-04-28T06:42:58.384Z] 06:42:58 [task 2020-04-28T06:42:58.384Z] 06:42:58 [task 2020-04-28T06:42:58.385Z] 06:42:58 [task 2020-04-28T06:42:58.385Z] 06:42:58 [task 2020-04-28T06:42:58.385Z] 06:42:58 [task 2020-04-28T06:42:58.385Z] 06:42:58 [task 2020-04-28T06:42:58.386Z] 06:42:58 [task 2020-04-28T06:42:58.386Z] 06:42:58 [task 2020-04-28T06:42:58.386Z] 06:42:58 [task 2020-04-28T06:42:58.386Z] 06:42:58 [task 2020-04-28T06:42:58.387Z] 06:42:58 [task 2020-04-28T06:42:58.387Z] 06:42:58 [task 2020-04-28T06:42:58.387Z] 06:42:58 [task 2020-04-28T06:42:58.387Z] 06:42:58 [task 2020-04-28T06:42:58.388Z] 06:42:58 [task 2020-04-28T06:42:58.388Z] 06:42:58 [task 2020-04-28T06:42:58.388Z] 06:42:58 [task 2020-04-28T06:42:58.389Z] 06:42:58 [task 2020-04-28T06:42:58.389Z] 06:42:58 [task 2020-04-28T06:42:58.389Z] 06:42:58 [task 2020-04-28T06:42:58.389Z] 06:42:58 [task 2020-04-28T06:42:58.389Z] 06:42:58 [task 2020-04-28T06:42:58.389Z] 06:42:58 [task 2020-04-28T06:42:58.389Z] 06:42:58 INFO - TEST-PASS | dom/base/test/test_iframe_referrer.html | origin (iframe) with no meta --- origin (http://example.com/)
INFO - {"same-origin-with-origin-in-meta":{"referrer":"http://example.com/tests/dom/base/test/referrer_testserver.sjs?ACTION=generate-iframe-policy-test&NAME=same-origin-with-origin-in-meta&ATTRIBUTE_POLICY=same-origin&META_POLICY=origin","policy":"full","expected":"same-origin"}}
INFO - TEST-PASS | dom/base/test/test_iframe_referrer.html | same-origin-with-origin-in-meta tests have to be performed.
INFO - TEST-PASS | dom/base/test/test_iframe_referrer.html | same-origin with origin in meta --- full (http://example.com/tests/dom/base/test/referrer_testserver.sjs?ACTION=generate-iframe-policy-test&NAME=same-origin-with-origin-in-meta&ATTRIBUTE_POLICY=same-origin&META_POLICY=origin)
INFO - Buffered messages finished
INFO - TEST-UNEXPECTED-FAIL | dom/base/test/test_iframe_referrer.html | Test timed out.
INFO - SimpleTest.ok@SimpleTest/SimpleTest.js:299:16
INFO - reportError@SimpleTest/TestRunner.js:128:22
INFO - TestRunner._checkForHangs@SimpleTest/TestRunner.js:150:18
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
INFO - setTimeout handlerTestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
INFO - TestRunner.runTests/<@SimpleTest/TestRunner.js:420:16
INFO - promise callbackTestRunner.runTests@SimpleTest/TestRunner.js:407:48
INFO - RunSet.runtests@SimpleTest/setup.js:218:14
INFO - RunSet.runall@SimpleTest/setup.js:197:12
INFO - hookupTests@SimpleTest/setup.js:294:12
INFO - parseTestManifest@http://mochi.test:8888/manifestLibrary.js:46:13
INFO - getTestManifest/req.onload@http://mochi.test:8888/manifestLibrary.js:59:28
INFO - EventHandlerNonNullgetTestManifest@http://mochi.test:8888/manifestLibrary.js:55:3
INFO - hookup@SimpleTest/setup.js:270:20
INFO - EventHandlerNonNull*@dom/base/test?autorun=1&closeWhenDone=1&consoleLevel=INFO&testname=tests/dom/base/test/mochitest.ini&manifestFile=tests.json&dumpOutputDirectory=%2Ftmp&cleanupCrashes=true:11:1

Flags: needinfo?(ckerschb)

Christoph Kerschbaumer [:ckerschb]

Assignee

Comment 8

•

4 years ago

(In reply to Natalia Csoregi [:nataliaCs] from comment #7)

Backed out for failures on test_iframe_referrer.html

Ah, that is because we changed the default for fission and this test explicitly pushes the pref for mixed active content. I guess we disable the test for now until we have fixed Bug 1631405.

Flags: needinfo?(ckerschb)

Pulsebot

Comment 9

•

4 years ago

Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/autoland/rev/f4a75756b1b4
Update Mixed Content Blocker to rely on BrowsingContext instead of nsIDocShellTreeItem. r=baku,smaug

Natalia Csoregi [:nataliaCs]

Comment 10

•

4 years ago

Backed out for failures on test_iframe_referrer_invalid.html

backout: https://hg.mozilla.org/integration/autoland/rev/2904c30c38aeeedaa3f3cb629c33c2ac55b36f19

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedTaskRun=FUKOIJoeT1-JY8KAhZnVew-0&searchStr=linux%2C18.04%2Cx64%2Cquantumrender%2Cdebug%2Cmochitests%2Cwith%2Cfission%2Cenabled%2Ctest-linux1804-64-qr%2Fdebug-mochitest-plain-fis-e10s-16%2Cm-fis%2816%29&revision=f4a75756b1b404f010abe91fccfa7a0a4a96df43

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=299745539&repo=autoland&lineNumber=7265

Flags: needinfo?(ckerschb)

Christoph Kerschbaumer [:ckerschb]

Assignee

Comment 11

•

4 years ago

(In reply to Natalia Csoregi [:nataliaCs] from comment #10)

Backed out for failures on test_iframe_referrer_invalid.html

Sorry - looking at it now - thanks!

Flags: needinfo?(ckerschb)

Natalia Csoregi [:nataliaCs]

Comment 12

•

4 years ago

•

Edited

Other failure:

Christoph Kerschbaumer [:ckerschb]

Assignee

Comment 13

•

4 years ago

Third time is a charm:
It seems some tests explicitly flip the security_mixed_content_block_active_content pref which we haven't accounted for in the early return for fission. Instead of updating tests I just added that early return when executing in fission mode to make all of those tests succeed. See:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bfebc7f63b0ac2f02ccee7063102705ca296dc6

Pulsebot

Comment 14

•

4 years ago

Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/autoland/rev/5797e768d878
Update Mixed Content Blocker to rely on BrowsingContext instead of nsIDocShellTreeItem. r=baku,smaug

Arthur Iakab [arthur_iakab]

Comment 15

•

4 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/5797e768d878

Status: ASSIGNED → RESOLVED

Closed: 4 years ago

status-firefox77: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla77

Christoph Kerschbaumer [:ckerschb]

Assignee

Updated

•

4 years ago

Depends on: 1634289

Christoph Kerschbaumer [:ckerschb]

Assignee

Updated

•

4 years ago

Blocks: 1635365