1493006 - Make QuotaManager OriginParser understand WebExtension protocol_handlers schemes in support of libdweb

Status: NEW

(Core :: Storage: Quota Manager, enhancement, P3)

Description

[Andrew Sutherland [:asuth] (he/him)]

## Context ##
registerProtocolHandler[1] allows web pages to register scheme handlers for a specific whitelist of schemes plus "web+" prefixes.  The Firefox-specific "protocol_handlers"[2] manifest.json provides similar access and expands the whitelist as well as adding "ext+" prefixes.  Both of these mechanisms result in pages that use the origin of the registering site or extension.

libdweb (https://github.com/mozilla/libdweb), an experimental WebExtension thing, goes a step further and allows registering protocols[3] whose pages have the actual origin that the protocol resembles.  So "dweb://hello/world" would have an effective origin of "dweb://hello".

As of bug 1482812 we started forbidding creation of origin directories that OriginParser can't parse.  (Which is good, because we would break if we tried to parse these new origins.  But obviously bad for people trying to prototype with libdweb.)

1: https://developer.mozilla.org/en-US/docs/Web/API/Navigator/registerProtocolHandler#Permitted_schemes
2: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/protocol_handlers
3: https://github.com/mozilla/libdweb/blob/master/Readme.md#protocol-api

## Enhancement ##
The OriginParser[4] should be made to understand the protocol schemes supported by libdweb.  Unfortunately, it appears the whitelist only exists in JSON at this time at https://searchfox.org/mozilla-central/source/toolkit/components/extensions/schemas/extension_protocol_handlers.json so I think OriginParser may need to duplicate this list.

The list was most recently amended in bug 1428446, previously amended in bug 1385357, and originated in the protocol_handlers support on bug 1310427.

4: https://searchfox.org/mozilla-central/rev/0b8ed772d24605d7cb44c1af6d59e4ca023bd5f5/dom/quota/ActorsParent.cpp#8455

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Little clarification. libdweb does not has a restricted list of protocol schemes. It allows extension / protocol author to pick & and implement one. This also gained enough momentum in the dev community that chrome was pursued to follow our lead:

https://github.com/arewedistributedyet/arewedistributedyet/issues/23
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/29sFh4tTdcs

In fact they even proposing to change standard to use blocklist over the whilelist:

https://github.com/whatwg/html/issues/3998

Could we please reconsider use of white list in favor of block list instead ? Or alternatively I would propose to use protocol handler flag

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIProtocolHandler#Attributes

to determine whether protocol scheme should be supported or not. We already use them for things like determining if document served from that protocol can have secure context or not and things like that.

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Thank you for the clarification and the informative links.

I think it's important to call out that Chrome's intent to implement is explicitly about showing the redirected URL in the address bar so that spoofing isn't a concern for them and that seems to also be presumed in the discussion about the blocklist.  I'd be very interested to know what the strategy is for dealing with spoofing in the libdweb case.  (And as a point of feedback, it seems like the only API that's documented on the project page that seems to have considered the UX ramifications of the powerful APIs is the FileSystem API.  It would be majorly helpful to characterize what the plans are for each API and links to the security UX review/discussion for it being something we're able to ship.  For example, I think my proposal at https://bugzilla.mozilla.org/show_bug.cgi?id=1247628#c41 for exposing the TCPSocket API to WebExtensions was a reasonable start for explaining to users how to mitigate the risk, but that never actually got any security UX sign-off.)

For this bug, the problem right now with using a blocklist instead of the existing safelist (and "web+" and "ext+" extensible variants) is the potential for ambiguous parses of origins via the OriginParser.  "+" is effectively used as a delimiter by the OriginParser, but "+" is allowed by the URL scheme grammar defined at https://url.spec.whatwg.org/#url-scheme-string which is a problem.  This can be enhanced to not be an issue, but it's a much larger enhancement than just expanding the safelist.  Also, without a clear plan signed off on by security UX, it's not clear that the libdweb blocklist-only scheme would happen due to spoofing concerns, so it's not an enhancement that I think we'd want to undertake until we're sure that's the direction Firefox is headed or we have dealt with all the high priority backlog.  (Reiterating that because registerProtocolHandler and WebExtensions' "protocol_handlers" only deal in redirects, they don't create actual origins and don't have the need to change the OriginParser at all.  It's only libdweb affected by this issue.)

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

> (And as a point of feedback, it seems like the only API that's documented on the project page that seems to have considered the UX ramifications of the powerful APIs is the FileSystem API.  It would be majorly helpful to characterize what the plans are for each API and links to the security UX review/discussion for it being something we're able to ship.  For example, I think my proposal at https://bugzilla.mozilla.org/show_bug.cgi?id=1247628#c41 for exposing the TCPSocket API to WebExtensions was a reasonable start for explaining to users how to mitigate the risk, but that never actually got any security UX sign-off.)

Thanks for sharing I'll take a look.

We're just starting to tackle security review. In the first phase of this project we were trying to assess if there would be enough interest in this in first place to justify work on landing these APIs into into Firefox. Given that all protocol authors we were engaged with end up creating implementations that usie these APIs we are now starting to work on roadmap of actually integrating it into firefox.

I'll make sure to capture signeoffs / discussions in the repo as they will happen.


> For this bug, the problem right now with using a blocklist instead of the existing safelist (and "web+" and "ext+" extensible variants) is the potential for ambiguous parses of origins via the OriginParser.  "+" is effectively used as a delimiter by the OriginParser, but "+" is allowed by the URL scheme grammar defined at https://url.spec.whatwg.org/#url-scheme-string which is a problem.  This can be enhanced to not be an issue, but it's a much larger enhancement than just expanding the safelist.

I understand. Is that a larger engineering effort or some other considerations ?

> Also, without a clear plan signed off on by security UX, it's not clear that the libdweb blocklist-only scheme would happen due to spoofing concerns, so it's not an enhancement that I think we'd want to undertake until we're sure that's the direction Firefox is headed or we have dealt with all the high priority backlog.  (Reiterating that because registerProtocolHandler and WebExtensions' "protocol_handlers" only deal in redirects, they don't create actual origins and don't have the need to change the OriginParser at all.  It's only libdweb affected by this issue.)

To address spoofing issues. Only web-extensions would be able to define new protocols, at the same time they are already capable to of hijacking any website without even spoofing so I'm not sure how does this creates higher risk.

That being said I totally understand that doing larger changes to support libdweb use case may not be high priority. So I propose to do following:

1. Expand the whitelist to match the one that was linked originally.
2. Mentor me to make larger enhancement to avoid need for the list.

Does that sounds reasonable ?

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #3)
> I'll make sure to capture signeoffs / discussions in the repo as they will
> happen.

Great!  I've pointed at least one person at libdweb's protocol handler support in https://github.com/w3c/ServiceWorker/issues/822#issuecomment-424197220 which is part of a greater set of ServiceWorker issues where people want to use ServiceWorkers for a use-case that WebExtensions are better suited for.  Being able to show that there's a viable plan to get the implementation into the browser is important.

> I understand. Is that a larger engineering effort or some other
> considerations ?

It's a combination of engineering and testing.  We may be able to simplify this as we drop some of our upgrade-support from very old versions of Firefox.  Each of the profile/STORAGE/{default,temporary}/ORIGIN directories should have .metadata/.metadata-v2 files that explicitly state the origin they cover.  If we no longer support upgrading from profiles that lack .metadata* files, then the OriginParser's ability to unambiguously parse origins no longer matters.

Right now QuotaManager has a history of breaking when weird origins showed up because of the current limitations, so we are extra paranoid right now about that issue.  https://wiki.mozilla.org/DOM/Workers-Storage/projects/WSIF tracks our efforts to improve the situation.

> To address spoofing issues. Only web-extensions would be able to define new
> protocols, at the same time they are already capable to of hijacking any
> website without even spoofing so I'm not sure how does this creates higher
> risk.

As I understand it, we've largely given up on reviewing WebExtensions at any level of depth in the hopes that their limited privileges that are explained to the user via the permission requests documented at https://support.mozilla.org/en-US/kb/permission-request-messages-firefox-extensions helps the user make an informed and safe decision.  (Also, it's worth pointing out that even when we did have a notionally thorough review process, malicious actors slipped through, especially since they're willing to spend money to convert non-malicious extensions into malicious extensions.)

The issue you discuss is covered by the content script permissions, and in particular "Access your data for all websites".  I feel like we do a very bad job on selling how dangerous that permission is.  This is partly due to how most interesting things involve content scripts, so it's quite normal for extensions to acquire this permission.  The documentation I linked to before doesn't cover the risks, and it's quite possible that users would make some assumptions like "of course there's no way that it'd be allowed to look at my banking data!  That's a secure site!" or something else.

But you are absolutely right that making one WebExtension API ultra-secure is no good if there are easier ways for an attacker to accomplish the same nefarious goal.  I think this type of thing is well captured by attack-defense trees.  For a quick example, take a look at https://satoss.uni.lu/projects/atrees/trees/bank_account_attack.pdf from the library of trees at http://satoss.uni.lu/projects/atrees/library.php.  What I would advocate for is that for something like the spoofing concerns that we make sure we characterize the risk of the problem in context and make sure that we consider reasonable options.

For example, requiring the WebExtension to define the protocols that it defines as part of its "manifest.json" seems like a great potential mitigation.  Especially if we augment the addons.mozilla.org quasi-review system so that human intervention is required to approve the use of a previously unseen protocol.  It doesn't take much for a human to realize that "h++ps" is a sketchy looking protocol.  Whereas if we allow WebExtensions to dynamically register *any* protocol they want on the fly, it makes it much more feasible for an attacker to try to implement a phishing extension that looks safe to users who are on the lookout for "Access your data for all websites" permissions.  Or it could be that the extension is itself benign but that it has a bug that lets an attacker trick it into dynamically registering a spoofing protocol, etc.

In general I think the goal for all WebExtension APIs that have great power is to craft CSP-like permissions that constrain the action of the WebExtension.  Like CSP, this benefits both the developer of the WebExtension and the user because they both can better reason about what the WebExtension can do.

> That being said I totally understand that doing larger changes to support
> libdweb use case may not be high priority. So I propose to do following:
> 
> 1. Expand the whitelist to match the one that was linked originally.
> 2. Mentor me to make larger enhancement to avoid need for the list.

These both make sense.  Note that I'm now thinking the "larger enhancement" is much smaller and as I described above, namely for us to stop supporting old profile versions and to change uses of the OriginParser such as used by the consumers of OriginProps[1] to depend on the metadata file to be present and wipe the origin if it's not present.

Changes around startup and cleanup as part of the mentioned-above https://wiki.mozilla.org/DOM/Workers-Storage/projects/WSIF are under way by :ttung.  We're not quite ready for that, but since you're willing to help with the engineering work, I think expanding the whitelist would be a great thing to start with.  To provide context, Telemetry currently shows that something like 130k users are impacted by QuotaManager startup issues, so the dweb issue is arguably a lot lower down the priority scale.

From a testing perspective, you'd want to check out:
- https://searchfox.org/mozilla-central/source/dom/quota/test/unit/test_bad_origin_directory.js
  This test covers the immediate issue your libdweb user saw; it tests that we can create a bad origin.  You'd want to test that we can create one of your safelisted origins.
- https://searchfox.org/mozilla-central/source/dom/quota/test/unit/test_originAttributesUpgrade.js
  This isn't quite what you want, but the basic idea is that you'd want a test that has a zipfile that contains one or more origins from the safelist and verify that QuotaManager successfully starts up with that profile.  This would also provide reassurance that things don't break if we change the QuotaManager schema and trigger an upgrade.  In that case, your test becomes an upgrade test for free!

Tom may have some other changes for OriginParser in flight on other bugs already, so I'd suggest checking with him before getting into any non-trivial implementation changes around OriginParser.

1: symbol:_ZN7mozilla3dom5quota12_GLOBAL__N_122StorageDirectoryHelper11OriginProps4InitEP7nsIFile

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Andrew Sutherland [:asuth] from comment #4)
> 1:
> symbol:
> _ZN7mozilla3dom5quota12_GLOBAL__N_122StorageDirectoryHelper11OriginProps4Init
> EP7nsIFile

I meant to provide the searchfox link, not just the query:
https://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla3dom5quota12_GLOBAL__N_122StorageDirectoryHelper11OriginProps4InitEP7nsIFile&redirect=false