1453452 - Page Info's media preview requests don't trigger webRequest listeners due to system principal use

Status: NEW

(WebExtensions :: Request Handling, defect, P3)

Description

[Jon Wiles]

Attachment: tt.xpi

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180326160923

Steps to reproduce:

1. Install the attached extension that blocks jpg images.
2. Visit http://hi.eewe.us/mary.html
3. Right click where image should be.
4. Click View Image Info.
5. Click the Media Tab if necessary to view the image.


Actual results:

The image appears in Media Preview.


Expected results:

The image should have been blocked in Media Preview.

Comment 1

[Jon Wiles]

Another case:

view-source:http://hi.eewe.us/mary.jpg

shows the image (but should not).  I'm not sure if that's a distinct bug.

Comment 2

[Jon Wiles]

I've attempted to debug this. I think the root cause is that the loading principal of the media preview request, which occurs in pageInfo.js in pageInfo.xul, is SystemPrincipal. So, while WebRequest.jsm observes the request, it does not forward it to ordinary webextensions such as tt.xpi. I'm not sure if the loading principal is wrong, or if a special case should be added for pageInfo.xul.

Comment 3

[Ed DuVall]

Attachment: Bug 1453452 - Media Preview should be filterable.

This change allows webextensions to filter any request for an image,
including requests from the Media Preview window.

Comment 4

[Shane Caraveo (:mixedpuppy)]

(In reply to Jon Wilkes from comment #1)

view-source:http://hi.eewe.us/mary.jpg

shows the image (but should not). I'm not sure if that's a distinct bug.

Yes, view-source would be sorta a separate issue.

Comment 5

[Jon Wiles]

Okay, I created Bug #1588946 for the view-source issue.

Comment 6

[Jon Wiles]

This bug is important to me. Is there anything that I can do to see it addressed?

If you need more information about my use case, I'd be happy to provide that as well.

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I don't see why we shouldn't block the image here. I think Ed's patch might be a bit too broad, maybe Shane can provide some insight when he is back.

Comment 8

[Tomislav Jovanovic :zombie]

(In reply to Jon Wilkes from comment #6)

This bug is important to me.
If you need more information about my use case, I'd be happy to provide that as well.

Can you please explain the important use case?

This doesn't exactly fit in the webRequest model, which is meant to observe and intercept requests made by and for web pages, not the browser's own UI.

A workaround might be to use the proxy api to block it, since that by necessity covers all browser requests.

Comment 9

[Jon Wiles]

This doesn't exactly fit in the webRequest model, which is meant to observe and intercept requests made by and for web pages, not the browser's own UI.

I'm afraid that makes no sense to me. Why is a request inside the main page considered to be "from the user", but a request for an image made inside the media preview pane "from the browser", as if the browser itself were a living thing? The user drives the media prevew pane, just as he drives the browser.

In human terms, the use case is parental controls (or in my case, self-control). A number of people in the NoFap world who use Pluckeye have reported to me that the media preview pane is uh ... detrimental to their mental health. My only response to them is to tell them they have to use Chrome. The only reason I say that is because of this bug.

Comment 10

[kernp25]

Or what do you think of this idea:

If a WebExtension blocks a image from a web page then the image is also blocked in the media tab of the page info window.

Comment 11

[Tomislav Jovanovic :zombie]

Please don't needinfo people needlessly.

Comment 12

[:Gijs (he/him)]

Moving this to page info, which is making these requests.

(In reply to Jon Wilkes from comment #9)

This doesn't exactly fit in the webRequest model, which is meant to observe and intercept requests made by and for web pages, not the browser's own UI.

I'm afraid that makes no sense to me. Why is a request inside the main page considered to be "from the user", but a request for an image made inside the media preview pane "from the browser", as if the browser itself were a living thing?

The browser makes requests that webextensions shouldn't be able to interfere with, like when it checks for updates, updates itself, or updates add-ons or other critical data. That's why we differentiate requests. I'm 100% sure that Chrome also won't let webextensions intercept those kinds of requests (or, if it does, that that's a security issue).

The user drives the media preview pane, just as he drives the browser.

Right, this is the problematic part. The media preview pane shouldn't claim to be requesting these images as "the browser". But there are some annoying edgecases to work out: the images as displayed on the webpage are subject to a number of controls and restrictions from the webpage and the user, like webpage CSP (which may block loads from certain locations), referrer headers (which may cause the webserver providing the image to refuse to provide the image to prevent hotlinking), whether the image is visible right now (may be hidden using CSS or other tools), whether the loads are blocked by tracking protection (like for 1x1 pixel tracking gifs!), whether the image load failed originally for network reasons (but re-requesting it from the preview window may or may not work), and, in the past, whether the user had blocked image loading for the webpage in question.

For all these reasons, plus one of practicality (I think - it's in a parent-process window, not in the website, so it's extra work to figure out the "right" requesting principal), the media preview code used to use the system principal.

Even so, I think the right fix is to fix that, and make the request with the triggering / loading principal set to a plain codebase principal for the opening site (but without CSP), and the referrer policy for the request not sending any referrer.

And in fact, that's pretty close to what it's already trying to do: https://searchfox.org/mozilla-esr78/rev/348e4fefb976ec03f937e2bcd9cccbeb9695c661/browser/base/content/pageinfo/pageInfo.js#883 . Christoph, why isn't this working, ie why is the extension code seeing a system principal for this request?

Comment 13

[Rob Wu [:robwu]]

(In reply to :Gijs (he/him) from comment #12)

And in fact, that's pretty close to what it's already trying to do: https://searchfox.org/mozilla-esr78/rev/348e4fefb976ec03f937e2bcd9cccbeb9695c661/browser/base/content/pageinfo/pageInfo.js#883 . Christoph, why isn't this working, ie why is the extension code seeing a system principal for this request?

The code there is setting the triggeringPrincipal, not the loadingPrincipal.
The loading principal is still the system principal (derived from the media preview document), and consequently the request is hidden from the webRequest API.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

The code there is setting the triggeringPrincipal, not the loadingPrincipal.
The loading principal is still the system principal (derived from the media preview document), and consequently the request is hidden from the webRequest API.

It's quite hard to create a mental model around IsSystemLoad() because there are no comments in that function. Generally the TriggeringPrincipal is the principal which should be used as the number one distinguishing factor for classifying something as a system load.

If we would re-evaluate IsSystemload(), re-order and document things, potentially evaluating the TriggeringPrincipal being Systemprincipal first, then I think it would return false for that request making it show up in the webRequest API.

Comment 15

[Rob Wu [:robwu]]

The objective of that check is to prevent extensions from interfering with critical requests initiated by the browser (=system principal).

Given the lack of comments in IsSystemLoad, I did some code archeology to record the evolution of this logic:

• The check was introduced in bug 1273138, and checked triggeringPrincipal before loadingPrincipal. The motivation for this check is [to]
  "Prevent listening in on requests originating from system principal to prevent tinkering with OCSP, app and addon updates, etc."
  relevant source code part 1 and part 2
• Later, a patch for bug 1318800 swapped the order (loadingPrincipal before triggeringPrincipal), with the comment "If there is no loadingPrincipal, check that the request is not going to inherit a system principal. triggeringPrincipal is the context that initiated the load, but is not necessarily the principal that the request results in, only rely on that if no other principal is available."
  relevant source code
• The current version of the logic (IsSystemLoad()) is the C++ version of the original logic.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #14)

Generally the TriggeringPrincipal is the principal which should be used as the number one distinguishing factor for classifying something as a system load.

If we would re-evaluate IsSystemload(), re-order and document things, potentially evaluating the TriggeringPrincipal being Systemprincipal first, then I think it would return false for that request making it show up in the webRequest API.

This may be the way to go. The order of triggeringPrincipal and loadingPrincipal is often significant and cannot be swapped (e.g. in case of frames), but specifically for testing whether a request ought to be associated with the system principal, I cannot think of a case where the order would yield different results (other than cases like this bug).

Side note, according to the documentation in nsILoadInfo.idl, loadingPrincipal is never null for non-top-level loads. That makes me wonder why we're checking principalToInherit. If the nsLoadInfo documentation is right, then the code should be unreachable.

I suppose that we could swap the order and see if anything breaks in try...

Moving it back to WebExtensions since it seems that the implementation of the extension framework should be changed.

Comment 16

[:Gijs (he/him)]

I'll just add for completeness that I do not believe there's currently an API to change the loading principal of the image XUL element (only the triggering principal), so if we wanted to change this in page info, we'd need to add such an API and then use it from page info...

Comment 17

[Jon Wiles]

Attachment: Page Info's media preview requests should trigger webRequest listeners

In https://bugzilla.mozilla.org/show_bug.cgi?id=1453452 , Christoph said that the triggering principle is probably the principle that should be preferred, as opposed to the loading principle.

Comment 18

[Jon Wiles]

Attachment: Bug 1453452 - Page Info's media preview requests should trigger webRequest listeners

Comment 19

[Shane Caraveo (:mixedpuppy)]

:ckerschb it's unfortunate the comment didn't transfer with the rewrite. The original fix that Rob pointed out has the full comment, reposted here:

+      // If there is no loadingPrincipal, check that the request is not going to
+      // inherit a system principal.  triggeringPrincipal is the context that
+      // initiated the load, but is not necessarily the principal that the
+      // request results in, only rely on that if no other principal is available.
       let {isSystemPrincipal} = Services.scriptSecurityManager;
-
-      data.isSystemPrincipal = (isSystemPrincipal(loadInfo.triggeringPrincipal) ||
-                                isSystemPrincipal(loadInfo.loadingPrincipal));
+      let isTopLevel = !loadInfo.loadingPrincipal && !!data.browser;
+      data.isSystemPrincipal = !isTopLevel &&
+                               isSystemPrincipal(loadInfo.loadingPrincipal ||
+                                                 loadInfo.principalToInherit ||
+                                                 loadInfo.triggeringPrincipal);

The reason IIRC for using triggeringPrincipal last was to ensure it was not a request for a resource being used inside a system principal resource.

That said, I think the question I have is whether any system principal code that sets triggeringPrincipal on a request was expecting that extensions would then be able to intercept and modify the request. Do we know it is safe to allow that? A safer approach may be to wrap this particular request in a content browser (per suggestion from gijs on chat).

Comment 20

[Shane Caraveo (:mixedpuppy)]

To be clear, I'm not against the switch if it is deemed safe, I just don't know that it is safe without a lot of spelunking.

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #19)

That said, I think the question I have is whether any system principal code that sets triggeringPrincipal on a request was expecting that extensions would then be able to intercept and modify the request. Do we know it is safe to allow that?

I guess the answer to the former question is no. I don't think we should allow that.

A safer approach may be to wrap this particular request in a content browser (per suggestion from gijs on chat).

That sounds like the better approach to me.

Comment 22

[Rob Wu [:robwu]]

(In reply to Christoph Kerschbaumer [:ckerschb, back on Jan 4th, 2021]] from comment #21)

(In reply to Shane Caraveo (:mixedpuppy) from comment #19)

A safer approach may be to wrap this particular request in a content browser (per suggestion from gijs on chat).

That sounds like the better approach to me.

Copy-paste Gijs' suggestion from the chat:

the alternative to changing webrequest here is to force all the media preview stuff to use a <browser remote="true"> and load the media in there...
which may be preferable if we want extensions to mess with it, but is more work

Independent of this bug, I think that it's a good idea to load remote content in a content process rather than the main browser process. How would that resolve this bug though? By using the requested resource's URL to construct the loading principal, instead of the system principal?

Comment 23

[:Gijs (he/him)]

(In reply to Rob Wu [:robwu] (will be back in 2021) from comment #22)

Independent of this bug, I think that it's a good idea to load remote content in a content process rather than the main browser process. How would that resolve this bug though? By using the requested resource's URL to construct the loading principal, instead of the system principal?

Pretty much - you can call createAboutBlankContentViewer with the principal you want and then add image/video/whatever tags in there.

We could also construct the channel manually, the way the favicon loader does, stick the data in a blob URI, and load that, either in the parent process or in the content process.

Comment 24

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

The bug assignee didn't login in Bugzilla in the last 7 months.
:mixedpuppy, could you have a look please?
For more information, please visit auto_nag documentation.