To capture some of the Phabricator discussion mentioned above:

Shouldn't we be using openWebLinkIn? I wish we had some documentation for what kind of "trust" openTrustedLinkIn implies.

Essentially this should only be used for a links that are coming from hard coded values within the codebase or from user input.

We should discuss further with Dao and/or someone more familiar with system principals (and any associated security risks).

I think it's reasonable to continue thinking of URL bar URLs as being user-input URLs and therefore use system principal.

Since 5 years ago we've now got more code that assumes URL bar loads will have this principal, so I'm not sure we could easily change this. Flying this by Marco just to be sure but I'm tempted to suggest wontfix.

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

I think it's reasonable to continue thinking of URL bar URLs as being user-input URLs and therefore use system principal.

Since 5 years ago we've now got more code that assumes URL bar loads will have this principal, so I'm not sure we could easily change this. Flying this by Marco just to be sure but I'm tempted to suggest wontfix.

Anything from the urlbar is either something the user has typed, or something from their profile, or something we control (Merino).
But search terms are converted into urls, I wonder if in that case we're taking any additional risk.
One other concern is that openWebLinkIn seems to enforce loading in foreground, that may break some of the urlbar functionality (opening in background tabs), if we'd use it instead of openTrustedLinkIn.

I would love someone with deeper expertise with the principals to evaluate whether the search terms => url transformation, or local protocols like file:/// cause any added risk.
Just trying to be overzealous before calling it non necessary.
Also the Trusted naming may still be confusing overall.

(In reply to Marco Bonardo [:mak] from comment #5)

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

I think it's reasonable to continue thinking of URL bar URLs as being user-input URLs and therefore use system principal.

Since 5 years ago we've now got more code that assumes URL bar loads will have this principal, so I'm not sure we could easily change this. Flying this by Marco just to be sure but I'm tempted to suggest wontfix.

Anything from the urlbar is either something the user has typed, or something from their profile, or something we control (Merino).
But search terms are converted into urls, I wonder if in that case we're taking any additional risk.

I don't think so. The search URLs are always http/https URLs, right? If there aren't enough safeguards in place to ensure that part, let's add some defense-in-depth in that code. But assuming that checks exist there, most principals would do as triggering navigation to those URLs, and using system doesn't really present additional risk, I believe.

One other concern is that openWebLinkIn seems to enforce loading in foreground, that may break some of the urlbar functionality (opening in background tabs), if we'd use it instead of openTrustedLinkIn.

Right. I think honestly the gist of this bug is for URL bar code to pass any non-system principal, so it could also e.g. pass a custom principal to openLinkIn directly, if that was more helpful.

I would love someone with deeper expertise with the principals to evaluate whether the search terms => url transformation, or local protocols like file:/// cause any added risk.
Just trying to be overzealous before calling it non necessary.

OK, let's doublecheck with Nika, but on the whole given the source of URLs etc. I'm reasonably confident that changing this is a lot of work for little to no gain, so we should wontfix this bug.

Also the Trusted naming may still be confusing overall.

I'd like to hear more about this though! What do you think would be a better name, and/or in what way is "trusted" confusing? The original thinking here was that for URLs that come directly from web content (untrusted!) we use "web", and for URLs provided by the user / OS, we need something else... I'm fine renaming the API but I'm not sure what would be a better description. :-)

Maybe, in case it helps the thinking here, I should expand a bit on how I think about this:

We use principals for 2 things, and that is part of what's leading to the confusion.

1. We use them to establish what privileges a document and/or script has. So (very very abridged version:) system principal can do whatever it likes, and null principal can do as close to nothing as possible.

2. We use them to identify an actor/agent/source for a resource load. So for a link from web content, we expect the principal for loading that link to identify the source of the link, which in practice means the principal should be the one associated with the document that contained the link. This is then used to (a) restrict what things are allowed to open and (b) to some degree, determine what the principal for the resulting load is given (1), especially for documents that inherit the principal that triggered the load.

This bug is part of an effort to try to minimize the cases of (2) where we use system principal, because of the risk of then inheriting that (very powerful) principal as the determinant of what the resulting document can do under (1).

I think this ends up messy because "system" is the correct identifier for the source of the resource load when that source is e.g. a bookmark or the URL bar. But we don't want to inherit that principal because that might allow the loaded content to do bad things.

I think the correct solution there is to prevent principal inheritance of system principal for such loads, for which we have a number of mechanisms. But I think trying to change the triggering principal as this bug suggests likely isn't going to lead to the right results.

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

I don't think so. The search URLs are always http/https URLs, right? If there aren't enough safeguards in place to ensure that part, let's add some defense-in-depth in that code. But assuming that checks exist there, most principals would do as triggering navigation to those URLs, and using system doesn't really present additional risk, I believe.

Yes, engines can only point to http(s) uris. So it sounds like the original concern is addressed already.
Add-ons may hook up in the urlbar and convert any search string into whatever they wish (omnibox API), though the user must have installed the add-on first.

Also the Trusted naming may still be confusing overall.

I'd like to hear more about this though! What do you think would be a better name, and/or in what way is "trusted" confusing?

I was reasoning around the "We should only ever use this function when the user has instigated the action to load the URL with certainty they wanted to load it" concept from OP. That's a lot more precise than "This comes from a trusted document (e.g. chrome)". It sounds more like openUserActivatedLinkFromChromeIn than Trusted. Of course that verbose naming may not be great. Not sure it's be worth the bikeshed coloring discussion anyway. Let's concentrate on ensuring we're doing the best, security-wise.

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

I would love someone with deeper expertise with the principals to evaluate whether the search terms => url transformation, or local protocols like file:/// cause any added risk.
Just trying to be overzealous before calling it non necessary.

OK, let's doublecheck with Nika, but on the whole given the source of URLs etc. I'm reasonably confident that changing this is a lot of work for little to no gain, so we should wontfix this bug.

Using the system principal (with inheritance disabled) for loads triggered on behalf of the user (e.g. through bookmarks or entering the URL in the URL bar) makes sense, and is what other code is expecting to happen. I don't think it poses any serious risk.

Based on the discussion in this bug, the main risk would be if these APIs are used to load an arbitrary URL string which was generated by a semi-trusted or untrusted entity like a WebExtension or a Content Process. We shouldn't allow e.g. a webextension using the omnibox API to convert a search string into a URL which that WebExtension wouldn't otherwise be allowed to load (e.g. a WebExtension presumably shouldn't be allowed to load about:config in response to a search - though I would imagine that the output from the omnibox API would be limited to only certain schemes?). We also shouldn't ever load a URL through this API using a URL sent by an untrusted content process, as that could potentially be abused as part of a sandbox escape.

Ok, for omnibox results, when the user picks a result the urlbar calls handleInputEntered and then the add-on has to take care of the load.
So I'm passing the ball to Luca and with that we should be done with the investigation (thanks everyone for the help).

(In reply to Marco Bonardo [:mak] from comment #10)

Ok, for omnibox results, when the user picks a result the urlbar calls handleInputEntered and then the add-on has to take care of the load.
So I'm passing the ball to Luca and with that we should be done with the investigation (thanks everyone for the help).

The assessment related to the interactions with the extensions using the omnibox API looks correct, the extension itself will be notified when the user have selected a suggestion coming from the particular extension (through the omnibox.onInputEntered API event which will get notified the content of the suggestion entry + the details related to the content disposition to be aware of where the extension should be eventually opening a tab, see https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/omnibox/onInputEntered#addlistener_syntax), and in response to the API event I'd expect that in most of the cases the extension will be then eventually be using the browser.tabs.update/browser.tabs.create API methods to load an url in the current active tab or in a new background or foreground one.

I'm also forwarding the needinfo to Rob, Rob re-reviewed WebExtensions internals uses of principals not long ago while working on the DNR WebExtensions API and so he may have some thoughts to add.

(In reply to Marco Bonardo [:mak] from comment #8)

Add-ons may hook up in the urlbar and convert any search string into whatever they wish (omnibox API), though the user must have installed the add-on first.

To clarify, the omnibox API can only offer search suggestions but not directly feed URLs to the urlbar. When the user chooses the suggested result, the responsibility of handling it is passed to the extension through the omnibox.onInputEntered event. Extensions typically use the browser.tabs.create or browser.tabs.update APIs to perform the actual load. These methods already validate that the extension is allowed to load the URL AND use the extension's content principal (moz-extension:) as the triggering principal.

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

We shouldn't allow e.g. a webextension using the omnibox API to convert a search string into a URL which that WebExtension wouldn't otherwise be allowed to load

This is not possible.

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #11)

(In reply to Marco Bonardo [:mak] from comment #10)

Ok, for omnibox results, when the user picks a result the urlbar calls handleInputEntered and then the add-on has to take care of the load.
So I'm passing the ball to Luca and with that we should be done with the investigation (thanks everyone for the help).

I'm also forwarding the needinfo to Rob, Rob re-reviewed WebExtensions internals uses of principals not long ago while working on the DNR WebExtensions API and so he may have some thoughts to add.

Since we have established that extensions do not affect the loaded URL, you don't have to be concerned about extensions inputting arbitrary URLs.

There is a relevant consideration from a different angle though:

• Extensions can redirect http(s) requests with the webRequest or declarativeNetRequest APIs, if the extension has the permission to modify the request.
• The choice for loadingPrincipal does affect the extension's ability to modify the request: when a system principal is used, extensions can't see the request.
• The choice of triggeringPrincipal does not affect the extension's ability to modify the request (despite misleading code, see the write-up at bug 1825881).

(in relation with Gijs' comment 7, loadingPrincipal is case 1, triggeringPrincipal is case 2)

I think we can resolve the bug as WORKSFORME:

1. the search service only supports http(s) urls, as such they will get an appropriate principal
2. add-ons get notified of user picks, and will use other WebExt APIs to react, those APIs have already been audited

Thank you again for the help investigating the branching into other components.