1717833 - Pref to defend against unwanted protocol registration inoperative

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[[e-mail]]

An experiment or application can't defend itself against unwanted protocol registration because the relevant preference doesn't get checked until after a runtime assertion.

https://searchfox.org/mozilla-central/rev/9975889f5c0d5c59bd22121a454beba774cbae71/dom/base/Navigator.cpp#970-987

Either the assertion should be downgraded back into an exception or it should be checked after the preference.

Comment 1

[:Gijs (out for now; he/him)]

What's the concrete problem here? AIUI this code is only reachable for schemes in the "safe" list ( https://searchfox.org/mozilla-central/rev/9975889f5c0d5c59bd22121a454beba774cbae71/dom/base/Navigator.cpp#864-871 ), and/or prefixed with web+, given the block at https://searchfox.org/mozilla-central/rev/9975889f5c0d5c59bd22121a454beba774cbae71/dom/base/Navigator.cpp#927-961 . None of those protocols should fail the release assert.

Comment 2

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

I am guessing that this is not an issue for Firefox, but rather for other applications like SeaMonkey or Thunderbird which may have internal handlers for schemes on the safe list. For example, we mark "irc" and "ircs" as safe protocols, and it appears there is an internal protocol registered in chatzilla for them here: https://searchfox.org/comm-central/rev/383d1860fd17e55f2c673fb388f07391a7b380b3/suite/chatzilla/js/lib/protocol-handlers.jsm#21-24. I imagine that we might hit this assert if a website loaded by seamonkey tried to register an irc protocol handler (not sure)?

Comment 3

[[e-mail]]

I was actually trying to investigate this Thunderbird crash: https://crash-stats.mozilla.org/report/index/35a6a845-5f20-4ac5-9533-74aa50210622

Comment 4

[:Gijs (out for now; he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=362538df47e4c749c35844f936df63a058a1b4c6

Comment 5

[:Gijs (out for now; he/him)]

Attachment: Bug 1717833 - reorder asserting for non-external protocols and checking external prefs disallowing registering some schemes, r?nika

Comment 6

[:Gijs (out for now; he/him)]

The other assumption here when this got written is that the navigator.registerProtocolHandler call happens in the child - so if we crash, we just crash the child, which is probably OK. But I guess that doesn't hold for Thunderbird/SM either... though tbf, I am kind of confused about why any code is calling into registerProtocolHandler directly in the parent process, in Thunderbird...

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/976b92bb11db
reorder asserting for non-external protocols and checking external prefs disallowing registering some schemes, r=nika

Comment 8

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/976b92bb11db

Comment 9

[[e-mail]]

(In reply to Gijs from comment #6)

I am kind of confused about why any code is calling into registerProtocolHandler directly in the parent process, in Thunderbird

So am I, but I don't think there's any (well, public at least) information in that crash report that can identify the caller. Thanks for fixing it though anyway.

Comment 10

[Ben Bucksch (:BenB)]

@RyanVM: This is becoming a top crash for Thunderbird 78, see bug 1717042. I don't know of any other way to fix this crash than backporting this change here. Could you please re-consider?

Comment 11

[Ben Bucksch (:BenB)]

Comment on attachment 9228769 [details]
Bug 1717833 - reorder asserting for non-external protocols and checking external prefs disallowing registering some schemes, r?nika

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: DoS: Webpage (e.g. login page) can trivially crash Thunderbird at will, or accidentally, by registering handlers for "mailto" or similar schemes. Apparently, some mail login pages do that. Causes a top crash in TB 78 right now.
• User impact if declined: Thunderbird crashes as soon as the user tries to login to their email address.
• Fix Landed on Version: 91
• Risk to taking this patch: Very low
• Why is the change risky/not risky? (and alternatives if risky): This crash is caused by a misplaced assertion, due to hardcoded assumptions. This is simply moving an assertion a few code lines.
• String or UUID changes made by this patch: none

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9228769 [details]
Bug 1717833 - reorder asserting for non-external protocols and checking external prefs disallowing registering some schemes, r?nika

Sure, approved for 78.14esr.

Comment 13

[Ben Bucksch (:BenB)]

Thank you!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/5d50d1be643b