1685403 - (CVE-2023-29538) onLoadRequest contains translated jar:file:/// URI instead of moz-extension:///

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[[ex-Mozilla] Agi Sferro | :agi]

From github: https://github.com/mozilla-mobile/fenix/issues/17321.

Steps to reproduce

1. Install HTTPS Everywhere for Firefox Android
2. Go to Add-Ons Menu for HTTPS Everywhere
3. Toggle "ON" for Encrypt All Sites Eligible setting
4. Navigate to an insecure page with no HTTPS support, such as http.badssl.com

Expected behavior

Display EASE mode page for insecure sites:

Actual behavior

Redirects to an error page with am unfamiliar prefix that I assume is browser based:

URL:
jar:file:///data/user/0/org.mozilla.fenix/files/mozilla/p4a5m9oi.default/extensions/[e-mail]!/pages/cancel/index.html?originURL=http%3A%2F%2Fhttp.badssl.com%2F

Console Output during this time:
console-export-2021-1-5_16-5-27.txt

When I turn HTTPSE EASE mode off and then back on, this strange error occurs where it tries to navigate to the extension's EASE interstitial but the extension's assets URIs get remapped to the insecure link's URI. Breaking scripts and other assets that need to be loaded for the page:

Console error:

16:06:27.251 Loading failed for the <script> with source “http://http.badssl.com/translation.js”. http.badssl.com:28:1
16:06:27.326 Loading failed for the <script> with source “http://http.badssl.com/util.js”. http.badssl.com:29:1
16:06:27.379 Loading failed for the <script> with source “http://http.badssl.com/ux.js”. http.badssl.com:30:1

console-export-2021-1-5_16-6-48.txt

Device information

Reported across at least two devices, with different builds of Fenix (Stable and Nightly) but I will just give my specs

• Android device:
  Pixel 3a, Android 11, Build RQ1A.201205.003
• Fenix version:
  Nightly 210104 19:29 (Build #2015785435)
  AC: 71.0.20210104143130, 3790c3ac2
  GV: 86.0a1-20210103092941
  AS: 67.2.0

Note: I am the lead developer on the HTTPS Everywhere project and if there is something that can help guide me to take care of this error on my end, feel free to to speak to that. I am personally stuck debugging this error and can't seem to find the trigger point for this error.

Change performed by the Move to Bugzilla add-on.

Comment 1

[Rob Wu [:robwu]]

Attachment: webRequest-redirect-to-extension-2.zip

It seems that navigations triggered from the location bar are handled incorrectly. The correct resource is loaded, but the loaded resource is associated with the URL of the original load, AND an attempt is made to load the internal (file/resource) URL too (in an external app).

Steps to reproduce (STR)

1. Extract attached extension to a directory, cd that directory.
2. Attach an Android device or start an Android emulator, with Fenix installed.
3. Run web-ext run -t firefox-android to see the device ID.
4. Run web-ext run -t firefox-android --android-device=DEVICE_ID_HERE to run Fenix with the extension (add --firefox-apk org.mozilla.fenix if needed to select a specific Fenix (Nightly) app)
5. Tap on the Menu, "Add-ons" to open the add-on manager. Tap on the "Redirect redir to extension page" extension to view its options. Enable the "Run in private browsing" option.
   (private browsing is not strictly necessary for the STR but will helpfully add another prompt later)
6. Open any web page in a private browsing tab, tap on the Menu, Add-ons. In the submenu that opens, tap on "Redirect redir to extension page" to trigger the extension.
   (the extension has a browserAction.onClicked listener that calls tabs.create to open a new tab)
   (the extension will open https://example.com/redir , intercept the request and redirect to a page bundled with the extension)
7. For comparison, focus the location bar, enter https://example.net/redir and try to visit it.

Expected

• Step 7 should result in exactly the same behavior as step 6. The page's URL should start with moz-extension: and the content area should show

This URL: moz-extension://[uuid]/subdir/test.html
This base URL: moz-extension://[uuid]/subdir/test.html
Subresource loaded: yes

Actual

• Step 7's location bar shows example.com/redir, with a dialog containing following title/text appears: "Open in app? Your activity may no longer be private" jar:file://some/path/to/extension.xpi!/subdir/test.html
• The content area shows that the redirection target is actually loaded in the content area, but with the wrong document URL. The content area contains:

This URL: https://example.com/redir
This base URL: https://example.com/redir
Subresource loaded: no

• Relative URLs in the extension page were resolved relative to the website's base URL. "Subresource loaded: no" in the content area means that the script subresource could not be loaded. With the network tab of the developer tools (via about:debugging from desktop Firefox), I can confirm that the page attempted to load the subresource, relative to https://example.com instead of the moz-extension:-URL).

This is a worrisome issue, as it could potentially result in leakage of static extension information (i.e. paths to extension subresources) (and/or URL parameters) to the domain that was redirected by the extension.

Comment 2

[[ex-Mozilla] Agi Sferro | :agi]

Thanks for the investigation Rob!

I think what's happening here is that AC looks at the jar: URI and thinks it's something the browser cannot load and then tries to send it to an external app, although now I'm wondering if this is the right behavior? I thought we only did that for very specific types of URIs? This sounds like it might be problematic.

:csadilek do you have opinions about this ^ ?

As to fix this we should probably send the moz-extension URI instead of the jar URI in onLoadRequest.

Comment 3

[[ex-Mozilla] Agi Sferro | :agi]

ni for Comment 2

Comment 4

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him from comment #2)

As to fix this we should probably send the moz-extension URI instead of the jar URI in onLoadRequest.

Looks like we're getting this URI from DocumentChannel directly here: https://searchfox.org/mozilla-central/rev/31ddf859c57e812878a5f817e4097efb06de4d97/netwerk/ipc/DocumentLoadListener.cpp#579

So to recap the problem here is that a load involving a moz-extension://... URI triggers a onLoadRequest call with the translated jar:file:/// URI which confuses the front-end.

Would it be possible to keep the moz-extension:... URI here? at least as displaySpec.

Comment 5

[Rob Wu [:robwu]]

(In reply to Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him from comment #4)

Looks like we're getting this URI from DocumentChannel directly here: https://searchfox.org/mozilla-central/rev/31ddf859c57e812878a5f817e4097efb06de4d97/netwerk/ipc/DocumentLoadListener.cpp#579

So to recap the problem here is that a load involving a moz-extension://... URI triggers a onLoadRequest call with the translated jar:file:/// URI which confuses the front-end.

I don't think that this recap is complete. As my STR in comment 1 shows, it's not merely a problem of the app trying to open the internal URL,
but also associating the loaded content with the wrong pre-redirect URL. That is very concerning, bordering on the edge of a sec issue.
If I use window.open to open a same-origin URL, and that URL is redirected to the extension page, then the same-origin URL is able to read the resulting HTML. Such pages were already public anyway via web_accessible_resources, but the content / origin / URL mixup is still very concerning.

FWIW, the internal URL is generated via SubstitutingProtocolHandler::ResolveURI, which calls into ExtensionProtocolHandler.cpp. The actual content is loaded via https://searchfox.org/mozilla-central/rev/014fe72eaba26dcf6082fb9bbaf208f97a38594e/netwerk/protocol/res/ExtensionProtocolHandler.cpp#479

bug 1645264 had added a test case for redirectUrl in webRequest.onBeforeRequest, but that only tested subresources. Document loads have a different code path and should also have unit tests. Navigations triggered via location.href assignment have the same issue, only navigations from browser.tabs.create result in the expected behavior.

Comment 6

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Rob Wu [:robwu] from comment #5)

(In reply to Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him from comment #4)

Looks like we're getting this URI from DocumentChannel directly here: https://searchfox.org/mozilla-central/rev/31ddf859c57e812878a5f817e4097efb06de4d97/netwerk/ipc/DocumentLoadListener.cpp#579

So to recap the problem here is that a load involving a moz-extension://... URI triggers a onLoadRequest call with the translated jar:file:/// URI which confuses the front-end.

I don't think that this recap is complete. As my STR in comment 1 shows, it's not merely a problem of the app trying to open the internal URL,
but also associating the loaded content with the wrong pre-redirect URL. That is very concerning, bordering on the edge of a sec issue.

Oh I missed this part! This is a security issue:

The content area shows that the redirection target is actually loaded in the content area, but with the wrong document URL. The content area contains:

This URL: https://example.com/redir
This base URL: https://example.com/redir
Subresource loaded: no

Am I reading this correctly that the web page thinks it's loaded from https://example.com/redir but it's actually loading from an moz-extension page? Could you do this, say, with google.com or bankingwebsite.com and pretend you're somebody you're not? (and worse, read the cookies?)

Restricting for now.

Comment 7

[Rob Wu [:robwu]]

(In reply to Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him from comment #6)

Am I reading this correctly that the web page thinks it's loaded from https://example.com/redir but it's actually loading from an moz-extension page?

Yes.

Could you do this, say, with google.com or bankingwebsite.com and pretend you're somebody you're not? (and worse, read the cookies?)

I don't know the underlying cause, so I cannot tell. The STR that I use relies on a redirect from extensions to a moz-extension:-URL; redirects to a http(s):-URL work as expected.

Comment 8

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Rob Wu [:robwu] from comment #7)

I don't know the underlying cause, so I cannot tell. The STR that I use relies on a redirect from extensions to a moz-extension:-URL; redirects to a http(s):-URL work as expected.

I mean you can just replace example.com with google.com and just as easily pretend to be a google.com page. It looks like you can access cookies too (via document.cookie) not sure if you can construct an extension that is not supposed to see cookies though and still be able to read document.cookie this way.

Comment 9

[Christian Sadilek [:csadilek]]

I thought we only did that for very specific types of URIs? This sounds like it might be problematic. :csadilek do you have opinions about this ^ ?

App Links functionality in Fenix uses a block list, not an allow list for specific apps. I verified that adding jar to the block list fixes the issue Rob describes above. So, let's do that today as a first step.

The exact flow here is that Fenix doesn't recognize jar as a supported scheme by the engine, verifies it isn't blocked and tries to find an app that supports it. As it doesn't find an app that can handle it, it falls back to loading which it also shouldn't do...and won't once blocked.

As to fix this we should probably send the moz-extension URI instead of the jar URI in onLoadRequest.

Yes, we'd need that as for other recent security issues we decided not to allow load jar:// but we can revisit if that's not feasible for you.

Comment 10

[[ex-Mozilla] Agi Sferro | :agi]

Note: there's no way to exploit this in Fenix stable because we only allow a limited set of trusted extensions.

Comment 11

[Rob Wu [:robwu]]

(In reply to Christian Sadilek [:csadilek] from comment #9)

The exact flow here is that Fenix doesn't recognize jar as a supported scheme by the engine, verifies it isn't blocked and tries to find an app that supports it. As it doesn't find an app that can handle it, it falls back to loading which it also shouldn't do...and won't once blocked.

There is more going on than described here. What you're describing looks like a plausible explanation for the first bullet point of "Actual" in my STR of comment 1 (i.e. the "Open in app? Your activity may no longer be private" dialog).

It does not explain the second+third bullet points from "Actual" in comment 1.

(In reply to Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him from comment #8)

It looks like you can access cookies too (via document.cookie) not sure if you can construct an extension that is not supposed to see cookies though and still be able to read document.cookie this way.

Extensions can only redirect requests if they have host permissions to access the website, so there is no privilege escalation there.

Comment 12

[Rob Wu [:robwu]]

(added reporter of issue on Github to cc so they can follow this bug.)

Extensions can work around this issue in some cases. For requests of top-level documents, cancel the request with {cancel: true} and use browser.tabs.update to replace the document with the moz-extension:-document. Make sure to check that details.frameId is 0 to not inadvertently replace top-level documents for navigations in child frames.

Comment 13

[Christian Sadilek [:csadilek]]

(In reply to Rob Wu [:robwu] from comment #11)

There is more going on than described here. What you're describing looks like a plausible explanation for the first bullet point of "Actual" in my STR of comment 1 (i.e. the "Open in app? Your activity may no longer be private" dialog).

It does not explain the second+third bullet points from "Actual" in comment 1.

Yes, that's because there are a few different things going on here:

1. Fenix shouldn't interpret jar:// URLs as app links (links to be opened in external apps)
2. Fenix shouldn't fall back to loading app links via Gecko in case of errors opening external apps

These two are fixed now on master and will roll into tomorrow's Fenix Nightly. I've verified that the original problem (https://github.com/mozilla-mobile/fenix/issues/17321) is fixed by this and also that the security problem you mentioned above no longer reproduces using your test extension.

3. Load requests for moz-extension:// pages should not be forward to embedders/apps as jar:file:// URLs
4. Load requests "denied" by the embedder/app (to indicate customized hanlding) leave the session in an invalid state

Agi has all context about 3. and 4. :)

Comment 14

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Christian Sadilek [:csadilek] from comment #13)

This is getting a little bit confusing so I'm spinning off 4) from this bug, we can do 3) here.

3. Load requests for moz-extension:// pages should not be forward to embedders/apps as jar:file:// URLs

This is pretty straightforward, onLoadRequest should not contain the internal jar:file:// representation as that doesn't really represent what the browser is trying to load, and moz-extension should be used instead.

4. Load requests "denied" by the embedder/app (to indicate customized hanlding) leave the session in an invalid state

Opened Bug 1686334 for that.

Comment 15

[Rob Wu [:robwu]]

I suppose that we can mark this as fixed by https://github.com/mozilla-mobile/android-components/pull/9381 ?

Comment 16

[Rob Wu [:robwu]]

I didn't mean to remove the dependency. Bugzilla did it.

Comment 17

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Rob Wu [:robwu] from comment #15)

I suppose that we can mark this as fixed by https://github.com/mozilla-mobile/android-components/pull/9381 ?

Even after the AC fix there's still this problem (which is the subject of this bug)

   Load requests for moz-extension:// pages should not be forward to embedders/apps as jar:file:// URLs

This is pretty straightforward, onLoadRequest should not contain the internal jar:file:// representation as that doesn't really represent what the browser is trying to load, and moz-extension should be used instead.

Comment 18

[alexis]

(In reply to Rob Wu [:robwu] from comment #12)

(added reporter of issue on Github to cc so they can follow this bug.)

Extensions can work around this issue in some cases. For requests of top-level documents, cancel the request with {cancel: true} and use browser.tabs.update to replace the document with the moz-extension:-document. Make sure to check that details.frameId is 0 to not inadvertently replace top-level documents for navigations in child frames.

Thank you for mentioning, this. In our workflow, we do set cancel to {cancel: true}, but I will double check the workflow.

To understand what happened.

1. The onLoadRequest that was triggered by the extension
2. Here is where I lose context...but somehow that request was forwarded to load an internal URI
3. And then in turn Fenix tried to load with the extension's sources with the pre-redirect URI

Comment 19

[Neha Kochar [:neha]]

Agi, could you please summarize which parts of this are fixed, and what's left? And is the current state still a sec issue?

Comment 20

[[ex-Mozilla] Agi Sferro | :agi]

The problem that's still a bug here is that onLoadRequest is called with a jar:file: URL instead of the original moz-extension: URL.

It could cause a security vulnerability since it deals with URLs and could confuse the front-end, but the one reported in this bug was fixed.

Comment 21

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Agi,
I talked with Andreas about this. And we thought this may have been fixed by bug 1686334. Could you kindly test and confirm? Thank you.

Comment 22

[[ex-Mozilla] Agi Sferro | :agi]

It's not fixed yet, I can see the jar:file:/// URI here: https://searchfox.org/mozilla-central/rev/ea1234192518e01694a88eac8ff090e4cadf5ca4/widget/android/nsWindow.cpp#1944-1947

05-03 10:37:32.484 22074 22102 D GeckoSession: onLoadRequest jar:file:///data/user/0/org.mozilla.fenix.debug/files/mozilla/65cen61b.default/extensions/[e-mail]!/pages/cancel/index.html?originURL=http%3A%2F%2Fhttp.badssl.com%2F

Comment 23

[Andrew McCreight [:mccr8]]

Nika has some ideas here.

Comment 24

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

I ended up not filling this in 5 months ago, when I originally had the idea, but I have some vague memory of what the specific problem was here, which needs to be fixed. My understanding is that this is probably related to how internal redirects are handled in DocumentLoadListener, and how that informaiton is forwarded to the gecko session. Specifically in https://searchfox.org/mozilla-central/rev/0062309958c212504556ae21a7e3eb120c9bd093/netwerk/ipc/DocumentLoadListener.cpp#2756, we pass down the URI for the new channel in AsyncOnChannelRedirect, but this is too early to learn the actual final URI that the channel is going to have after the redirect. When redirecting within nsBaseChannel (which is what extension URIs use), the OriginalURI of the new channel isn't set until nsBaseChannel::ContinueRedirect (https://searchfox.org/mozilla-central/rev/0062309958c212504556ae21a7e3eb120c9bd093/netwerk/base/nsBaseChannel.cpp#143-145), which is called after AsyncOnChannelRedirect. This means that the "final channel URI" which you see in that function is unfortunately wrong, with no clear way to get the "real" channel URI, as it hasn't been overridden yet.

Fundamentally the code right now is observing implementation details of the moz-extension protocol, and that it is implemented using internal redirects, and then setting OriginalURI to override the final channel URI of the load in the parent process. (extension protocols behave slightly differently in a content process iirc).

I don't necessarily have a solution here off the top of my head, though :valentin might have some insight into what the correct way to handle this kind of thing is.

Comment 25

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

It occurs to me that we actually already ignore internal redirects in the callback (https://searchfox.org/mozilla-central/rev/0062309958c212504556ae21a7e3eb120c9bd093/netwerk/ipc/DocumentLoadListener.cpp#2702-2711). Unfortunately, it seems like the redirect used by nsBaseChannel is inconsistent. If the channel is opened synchronously, it will be a REDIRECT_INTERNAL (https://searchfox.org/mozilla-central/rev/0062309958c212504556ae21a7e3eb120c9bd093/netwerk/base/nsBaseChannel.cpp#643), but if it's opened asynchronously, it'll be a REDIRECT_TEMPORARY (https://searchfox.org/mozilla-central/rev/0062309958c212504556ae21a7e3eb120c9bd093/netwerk/base/nsBaseChannel.cpp#280).

It appears this dates back to bug 464956, where :bz changed it from REDIRECT_INTERNAL to REDIRECT_TEMPORARY in order to change something about how .desktop and .url files are handled. I'm unsure if this is still relevant today.

Comment 26

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

It seems to me that the arguments for bug 464956 hold barely, but only for file channels here and even then it's a shaky premise - that the user's FS is not under their control.
However, the linked bug 464954 has tests in test_CrossSiteXHR.html and that test no longer has jar:file for file:// tests; so we MIGHT be able to change it back to REDIRECT_INTERNAL without major fallback.

Alternatively, could try calling OnRedirectVerifyCallback before calling window->OnLoadRequest, so the URL gets updated, and if that fails, then we could cancel the channel?

Comment 27

[Rob Wu [:robwu]]

I observed an easily reproducible test case with a similar issue (i.e. URL is jar/file instead of moz-extension).
It is not limited to document loads, and also reproducible on desktop.

Could you take a look at bug 1802385 and check if they have the same root cause? Even if the other issue happens to be different, then I'd be interested in knowing what's causing that other bug.

Comment 28

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

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

It seems to me that the arguments for bug 464956 hold barely, but only for file channels here and even then it's a shaky premise - that the user's FS is not under their control.
However, the linked bug 464954 has tests in test_CrossSiteXHR.html and that test no longer has jar:file for file:// tests; so we MIGHT be able to change it back to REDIRECT_INTERNAL without major fallback.

Yeah, I think if it's not an issue anymore, we should switch it back to a REDIRECT_INTERNAL. My impression after looking at that was that it probably would be fine to switch the redirect back to REDIRECT_INTERNAL at this point, so it's probably worth at least trying out?

Alternatively, could try calling OnRedirectVerifyCallback before calling window->OnLoadRequest, so the URL gets updated, and if that fails, then we could cancel the channel?

I'm pretty sure that would break things, even just in DocumentLoadListener::AsyncONChannelRedirect. We very explicitly make sure to fetch the channel creation original URI at the beginning of the function (https://searchfox.org/mozilla-central/rev/0062309958c212504556ae21a7e3eb120c9bd093/netwerk/ipc/DocumentLoadListener.cpp#2685), which would be lost if we updated it before this call.

Comment 29

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

(In reply to Rob Wu [:robwu] from comment #27)

I observed an easily reproducible test case with a similar issue (i.e. URL is jar/file instead of moz-extension).
It is not limited to document loads, and also reproducible on desktop.

Could you take a look at bug 1802385 and check if they have the same root cause? Even if the other issue happens to be different, then I'd be interested in knowing what's causing that other bug.

I think that might be from the same root cause, as the redirect isn't being marked as internal, so the final URI isn't being updated properly, or something like that? I don't know the details of how the final URI for a fetch is determined off the top of my head.

Comment 30

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

I looked into the above thing a bit more because I was surprised by the current behaviour, and it turns out I made some poor assumptions in my original comment explaining this situation, and I think the problem is actually completely different than I was originally implying.

I'll post again soon with an updated explanation of what's going on once I've looked into this a bit more.

Comment 31

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

So it turns out I was completely wrong about the cause. When loading a moz-extension: URI in the parent process, there's actually no nsBaseChannel redirects at all. I've marked my old comments as obsolete to avoid confusing future people coming to this bug with incorrect information.

When a moz-extension URI is loaded in the parent process, it is loaded using a SubstitutingProtocolHandler. This is a class which is used to handle some types of URIs, but the general strategy is used by more protocol handlers than just subclasses of that class.

A substituting-style protocol handler is one which, during nsIProtocolHandler::NewChannel, picks a new URI, opens a channel for that URI, and then returns it. However, it does not want that new channel's URI to be the URI of the loaded document, so it avoids that by first calling SetOriginalURI on the channel to the URI passed to NewChannel, before returning it. (Examples for some protocols: moz-extension/resource, about (1, 2), chrome).

This does the trick, as when code calls NS_GetFinalChannelURI to get the URI being loaded from a channel, it will pull the URI from either GetResultPrincipalURI or GetOriginalURI, not from GetURI. This means that the loaded channel is treated as-if it was a channel for the substituted URI.

Unfortunately, both the example in this bug and the example in comment 27 appear to be places where callers call nsIChannel::GetURI instead of NS_GetFinalChannelURI to determine the URI being loaded by a channel.

1. For this bug, DocumentLoadListener calls AntiTrackingUtils::MaybeGetDocumentUriBeingLoaded, which calls GetURI. I think this should probably be calling NS_GetFinalChannelURI.

2. For bug 1802385, the call appears to be in the FetchDriver, which in a few places (1, 2) calls nsIChannel::GetURI to get the URI being loaded. I'm guessing that these also should probably be using NS_GetFInalChannelURI.

Assuming I'm correct about the above, I'm also a bit worried about other places where nsIChannel::GetURI is called, as it appears like it's probably being used incorrectly somewhat often, likely because it's the obvious method to call to ask "what URI is this channel loading?". And that appears to be mostly true for almost all URIs which come up during normal browsing, but is not correct for internal URIs and moz-extension. (One place which I noticed where NS_GetFinalChannelURI might produce the "wrong" value is for javascript: URIs, which are overridden to return the document URI they're running under: https://searchfox.org/mozilla-central/rev/ce3b4cdbc501cde6176c0330d0d6e8458908dd8a/dom/jsurl/nsJSProtocolHandler.cpp#180-181, but you can't redirect to those anyway, and that's the correct URI for security checks)

The easiest "fix" here is probably to swap some callers over to NS_GetFinalChannelURI, but I worry that won't actually fix issues with people using the wrong method to determine the URI of a channel. We could also simplify the protocol handler for moz-extension to no longer use the substituting approach in the parent process (always using the remote codepath might be good for consistency between in-parent and in-content channels anyway), which would fix most of the visible issues, but still technically doesn't fix issues with internal URIs like resource, chrome, and about.

Changing the meaning or name of nsIChannel::GetURI sounds like it'd be a complete nightmare as well, so that's probably also not a great option :-/

Comment 32

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

It'd be perhaps an inelegant solution, but a bit of me wonders if it would make sense to make everywhere where we currently do this trick instead create a new wrapper channel like we do for content process extension channels, as it would mean that nsIChannel::GetURI does the "right thing" more often. Not sure how much that'd end up breaking, or what the perf/memory hit would be of the extra wrappers though.

Comment 33

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

I should have occured to me that a missing NS_GetFinalChannelURI could be the problem. Sorry I missed it.

The easiest "fix" here is probably to swap some callers over to NS_GetFinalChannelURI, but I worry that won't actually fix issues with people using the wrong method to determine the URI of a channel.

We could audit current uses of nsIChannel::GetURI and add a linter for new uses of it. That would at least force people to think about the correct thing to do.

Comment 34

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

Necko will take this one.

Comment 35

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Thank you very much, Valentin & Kershaw :)

Comment 36

[Kershaw Chang [:kershaw]]

Attachment: Bug 1685403 - Add linter to check using GetURI, r=#necko

Comment 37

[Kershaw Chang [:kershaw]]

Attachment: Bug 1685403 - Use NS_GetFinalChannelURI at some places, r=#necko

Comment 38

[Kershaw Chang [:kershaw]]

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

I should have occured to me that a missing NS_GetFinalChannelURI could be the problem. Sorry I missed it.

The easiest "fix" here is probably to swap some callers over to NS_GetFinalChannelURI, but I worry that won't actually fix issues with people using the wrong method to determine the URI of a channel.

We could audit current uses of nsIChannel::GetURI and add a linter for new uses of it. That would at least force people to think about the correct thing to do.

I am not really sure how to audit the current use of nsIChannel::GetURI, so I just submit a patch to change someplace mentioned in comment #31.
Do you maybe have an idea about how to audit this?
Thanks.

Comment 39

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

I unfortunately don't have an easy rule for figuring out whether or not a call-site should be switched to NS_GetFinalChannelURI. I think it comes down to whether the code is trying to get low-level information about the channel and what it's trying to load, or if it is actually wanting to ask questions about the final URI which is going to be produced by the channel. This probably requires some judgement based on the code being read to figure out what the intention of the code was originally.

Changing callers in JS will also probably be a bit of a pain. Don't know if there's an easy way to get the final URI of the channel from JS (it looks like it might be on ChannelWrapper)? Finding them in JS will also be somewhat painful, because it's just an access to a .URI member, but it does appear that for the most part that is only a property on principals, history entries, or channels (https://searchfox.org/mozilla-central/search?q=%5C.URI%5Cb&path=js&case=true&regexp=true).

Comment 40

[Rob Wu [:robwu]]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #39)

Changing callers in JS will also probably be a bit of a pain. Don't know if there's an easy way to get the final URI of the channel from JS (it looks like it might be on ChannelWrapper)?

ChannelWrapper depends on the extension's WebRequest implementation (bug 1799118), so I recommend to add a utility method or channel method instead of replacing nsIChannel uses with ChannelWrapper.

Comment 41

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

I agree with Nika. It's really difficult to figure out which one you need without thinking about it.
I assume we'll need a new method on nsIChannel that calls NS_GetFinalChannelURI to be accessible from JS.

Comment 42

[Rob Wu [:robwu]]

This bug has morphed from a security issue to a regular bug.
https://bugzilla.mozilla.org/show_bug.cgi?id=1685403#c13 lists the issues associated with this bug.

The original security bug was fixed in multiple ways:

• 1+2 by https://github.com/mozilla-mobile/android-components/pull/9381
• 4 by bug 1686334 (actually bug 1735613)

3 was thought to be fixed by bug 1735613, but it was not:

(In reply to [ex-Mozilla] Agi Sferro | :agi from comment #22)

It's not fixed yet, I can see the jar:file:/// URI here: https://searchfox.org/mozilla-central/rev/ea1234192518e01694a88eac8ff090e4cadf5ca4/widget/android/nsWindow.cpp#1944-1947

05-03 10:37:32.484 22074 22102 D GeckoSession: onLoadRequest jar:file:///data/user/0/org.mozilla.fenix.debug/files/mozilla/65cen61b.default/extensions/[e-mail]!/pages/cancel/index.html?originURL=http%3A%2F%2Fhttp.badssl.com%2F

Nika posted the latest analysis in comment 31 of the root cause, along with how that causes this bug, and (independently) causes bug 1802385. These two specific examples are fixed in the patch from comment 37.
There is a patch to add linter warnings in comment 36. I think that it would make sense to move that to a follow-up bug.

Comment 43

[Kershaw Chang [:kershaw]]

Attachment: Bug 1685403 - Use NS_GetFinalChannelURI at some places, r=#necko

Comment 44

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Use NS_GetFinalChannelURI at some places, r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/6d75b6c4bdf7a2f88110f425352461a1619e3501
https://hg.mozilla.org/mozilla-central/rev/6d75b6c4bdf7

Comment 45

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

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

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

For more information, please visit auto_nag documentation.

Comment 46

[Kershaw Chang [:kershaw]]

I think it's fine not to uplift this patch.

Comment 47

[Oana Horvath [:ohorvath]]

I removed the qe-verify flag after seeing the original issue with STR was fixed somewhere else and it was also covered by automated testing. If QA is still needed here please let us know.

Comment 48

[Tom Ritter [:tjr]]

Attachment: advisory.txt