Closed Bug 1717833 Opened 1 year ago Closed 1 year ago

Pref to defend against unwanted protocol registration inoperative

Categories

(Firefox :: General, defect)

defect

Tracking

()

RESOLVED FIXED
91 Branch
Tracking Status
firefox-esr78 --- fixed
firefox89 --- wontfix
firefox90 --- wontfix
firefox91 --- fixed

People

(Reporter: neil, Assigned: Gijs)

References

(Regression)

Details

(Keywords: regression, Whiteboard: regression,sec-low, )

Attachments

(1 file)

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.

Flags: needinfo?(nika)

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.

Flags: needinfo?(neil)
Flags: needinfo?(nika)

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)?

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

Flags: needinfo?(neil)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED

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...

Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/976b92bb11db
reorder asserting for non-external protocols and checking external prefs disallowing registering some schemes, r=nika
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 91 Branch

(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.

@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?

Flags: needinfo?(ryanvm)

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
Attachment #9228769 - Flags: approval-mozilla-esr91?

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.

Flags: needinfo?(ryanvm)
Attachment #9228769 - Flags: approval-mozilla-esr91? → approval-mozilla-esr78+

Thank you!

Whiteboard: regression,sec-low,
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.