Closed Bug 1429732 Opened 2 years ago Closed 2 years ago

Consider making registerProtocolHandler require SecureContext

Categories

(Core :: DOM: Security, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: jkt, Assigned: jkt)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, sec-want, site-compat, Whiteboard: [domsecurity-backlog1][adv-main60-])

Attachments

(1 file)

Registering a protocol handler is a powerful action in that all types of a certain protocol would be sent to a third party web page the user has approved.

The problem here is that over an insecure connection this will allow a MiTM to detect the URLs that are being sent to the protocol handler.

The protocols permitted are:
    bitcoin, geo, im, irc, ircs, magnet, mailto, mms, news, nntp, sip, sms, smsto, ssh, tel, urn, webcal, wtai, xmpp

And web+...

As far as I'm aware we don't have telemetry on this feature so we would likely have to get some on secure/not secure. I found stats on it's overall usage before of 0.2% however I can't find the source for that stat.
Seems like a good idea. Are you going to land telemetry in this bug or in a separate bug this depends on? I recommend a separate bug so we don't land patches and then leave a bug open.
Flags: needinfo?(jkt)
Keywords: sec-want
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
My current plan is to land in this bug:
- A pref to disable
- A default for nightly to disable
- A warning deprecation for 62 removal

This will give us beta and stable deprecation statistics if we need to extend the period of deprecation. I'm taking this rapid approach due to the design of this API and the expected low usage 0.002836% of pageloads according to Chrome.

Perhaps a follow up task would be to clean up prior handlers to ensure they are HTTPS too.
Flags: needinfo?(jkt)
Assignee: nobody → jkt
Status: NEW → ASSIGNED
Comment on attachment 8948241 [details]
Bug 1429732 - Use a pref to disable registerProtocolHandler in insecure contexts.

https://reviewboard.mozilla.org/r/217758/#review223596

::: dom/base/nsGlobalWindowInner.h:851
(Diff revision 2)
>                        mozilla::dom::CallerType aCallerType,
>                        mozilla::ErrorResult& aError);
>    void SetOuterHeight(JSContext* aCx, JS::Handle<JS::Value> aValue,
>                        mozilla::dom::CallerType aCallerType,
>                        mozilla::ErrorResult& aError);
> +  static bool RegisterProtocolHandlerAllowedForContext(JSContext* /* unused */, JSObject* aObj);

Move this near the other IsFoobarEnabled() static methods.

::: dom/locales/en-US/chrome/dom/dom.properties:356
(Diff revision 2)
>  # LOCALIZATION NOTE: %1$S is the invalid property value and %2$S is the property name.
>  InvalidKeyframePropertyValue=Keyframe property value “%1$S” is invalid according to the syntax for “%2$S”.
>  # LOCALIZATION NOTE: Do not translate "ReadableStream".
>  ReadableStreamReadingFailed=Failed to read data from the ReadableStream: “%S”.
> +# LOCALIZATION NOTE: Do not translate "registerProtocolHandler".
> +RegisterProtocolHandlerInsecureWarning=Use of the registerProtocolHandler for insecure connections will be removed in version 62.

Don't be too precise :) Just say that it will be removed soon.

::: dom/webidl/Navigator.webidl:80
(Diff revision 2)
>  };
>  
>  [NoInterfaceObject]
>  interface NavigatorContentUtils {
>    // content handler registration
> -  [Throws]
> +  [Throws,Func="nsGlobalWindowInner::RegisterProtocolHandlerAllowedForContext"]

Space after ','
Attachment #8948241 - Flags: review?(amarchesini) → review+
Comment on attachment 8948241 [details]
Bug 1429732 - Use a pref to disable registerProtocolHandler in insecure contexts.

https://reviewboard.mozilla.org/r/217758/#review223882
Attachment #8948241 - Flags: review?(dao+bmo) → review+
Comment on attachment 8948241 [details]
Bug 1429732 - Use a pref to disable registerProtocolHandler in insecure contexts.

https://reviewboard.mozilla.org/r/217758/#review223596

> Don't be too precise :) Just say that it will be removed soon.

We agreed over IRC this was consistent with other newer notices.
Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dcb60325f0cf
Use a pref to disable registerProtocolHandler in insecure contexts. r=baku,dao
Backed out for breaking uriloader/exthandler/tests/mochitest/mochitest.ini: runByManifest mode must be enabled to set the `prefs` key (fails at least on Android):

https://hg.mozilla.org/integration/autoland/rev/7f1a4d21cf30b8edce8816f7f752f2345f7e5af9

Push with failures (one 24 has the same failure summary but a different reason in the raw log): https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=dcb60325f0cf4e375a0e2bb36d96e7632a16bdba&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=161116295&repo=autoland&lineNumber=1571

ERROR parsing uriloader/exthandler/tests/mochitest/mochitest.ini: runByManifest mode must be enabled to set the `prefs` key
Flags: needinfo?(jkt)
Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3f96ca5e9d2d
Use a pref to disable registerProtocolHandler in insecure contexts. r=baku,dao
https://hg.mozilla.org/mozilla-central/rev/3f96ca5e9d2d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
I've added an entry for this on our experimental features page:

https://developer.mozilla.org/en-US/Firefox/Experimental_features#Security

Let me know if this reads OK. Thanks!
LGTM thanks Chris!
Flags: needinfo?(jkt)
Whiteboard: [domsecurity-backlog1] → [domsecurity-backlog1][adv-main60-]
Blocks: 1460506
You need to log in before you can comment on or make changes to this bug.