1717042 - `navigator.registerProtocolHandler()` can crash Thunderbird @ mozilla::dom::Navigator::CheckProtocolHandlerAllowed

Status: RESOLVED FIXED

(Thunderbird :: General, defect, P1)

Description

[[e-mail]]

navigator.registerProtocolHandler() allows a small whitelist of permitted schemes. However some of these schemes are registered by Thunderbird. This means that if someone manages to access this API in Thunderbird but tries to pass one of those schemes then Thunderbird will immediately crash.

Comment 1

[[e-mail]]

Now that bug 1717833 is fixed, Trunk Thunderbird can defend itself by setting the appropriate preferences.

Comment 2

[Ben Bucksch (:BenB)]

@Neil: Can you please attach a patch that sets the preferences accordingly, with all relevant URL schemes, and test that it fixes the bug?

Comment 3

[[e-mail]]

Attachment: Proposed patch

Of course on trunk the problem isn't so bad because web pages run in content processes but we might as well set the preferences anyway.

Comment 4

[Ben Bucksch (:BenB)]

Is that all protocols? Don't we have a lot more, like imap:, plus some internal ones like imap-message: and similar?

Comment 5

[[e-mail]]

I've only added prefs for those that cause a crash.

Comment 6

[Ben Bucksch (:BenB)]

OK, could you please add all protocols?

Comment 7

[Wayne Mery (:wsmwk)]

Does this only affect version 78? https://crash-stats.mozilla.org/signature/?signature=mozilla%3A%3Adom%3A%3ANavigator%3A%3ACheckProtocolHandlerAllowed&date=%3E%3D2021-04-12T06%3A47%3A00.000Z&date=%3C2021-07-12T06%3A47%3A00.000Z#summary

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9229538 [details] [diff] [review]
Proposed patch

Review of attachment 9229538 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/app/profile/all-thunderbird.js
@@ +378,5 @@
>  pref("network.protocol-handler.warn-external.ftp", false);
>  
> +// prevent web pages from registering mailnews protocol handlers
> +pref("network.protocol-handler.external.mailto", false);
> +pref("network.protocol-handler.external.news", false);

snews as well I guess?

Comment 9

[[e-mail]]

(In reply to Wayne Mery from comment #7)

Does this only affect version 78?

On trunk the main process doesn't crash, only the child process for the page trying to register the protocol handler. I don't know how that shows up in the crash statistics, if at all.

Comment 10

[[e-mail]]

Attachment: All Thunderbird protocols

snews isn't currently allowed by navigator.registerProtocolHandler(), which is why the original patch didn't have it. If we're going to add it, then as Ben suggested we should add the other 11 Thunderbird protocols as well, just in case.

Comment 11

[Ben Bucksch (:BenB)]

Comment on attachment 9230746 [details] [diff] [review]
All Thunderbird protocols

Thanks. Looks good to me.

Comment 12

[Ben Bucksch (:BenB)]

@Neil: You forgot imap-message:, news-message: etc., as mentioned in comment 4, see https://searchfox.org/comm-central/source/mailnews/base/src/nsNewMailnewsURI.cpp#49 for some.

Comment 13

[[e-mail]]

Attachment: All registered and some unregistered protocols

The -message versions aren't actually registered as Thunderbird protocols. I don't know what smile is doing in nsNewMailnewsURI.cpp but it's also not actually registered as a protocol.

Comment 14

[Ben Bucksch (:BenB)]

Comment on attachment 9230786 [details] [diff] [review]
All registered and some unregistered protocols

Good. There's no legitimate reason to have webpages register imap-message:, so better to block it, just in case.

Thanks. +1 from me.

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 9230786 [details] [diff] [review]
All registered and some unregistered protocols

Review of attachment 9230786 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/app/profile/all-thunderbird.js
@@ +377,5 @@
>  pref("network.protocol-handler.warn-external.https", false);
>  pref("network.protocol-handler.warn-external.ftp", false);
>  
> +// prevent web pages from registering mailnews protocol handlers
> +pref("network.protocol-handler.external.cid", false);

Also add mid:?

Comment 16

[Ben Bucksch (:BenB)]

FYI, Neil is sick at the moment.

Comment 17

[Ben Bucksch (:BenB)]

Attachment: Fix, v4

I'm updateing the patch for Neil, because he cannot.

Adding mid as requested.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f4e363bebf69
Prevent registerProtocolHandler crashing for mailnews protocols r=mkmelin

Comment 19

[Ben Bucksch (:BenB)]

Comment on attachment 9233102 [details] [diff] [review]
Fix, v4

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined:
Thunderbird crashes
Testing completed (on c-c, etc.):
Landed on CC
Risk to taking this patch (and alternatives if risky):
Only changes prefs to tell Gecko that we implement these protocols

Comment 20

[Wayne Mery (:wsmwk)]

Comment on attachment 9233102 [details] [diff] [review]
Fix, v4

[Triage Comment]
Approved for beta

Comment 21

[Rob Lemley [:rjl]]

Thunderbird 91.0b6:
https://hg.mozilla.org/releases/comm-beta/rev/5fe25d96fd85

Comment 22

[Ben Bucksch (:BenB)]

Comment on attachment 9233102 [details] [diff] [review]
Fix, v4

[Approval Request Comment]
Regression caused by (bug #): 1499092
User impact if declined: Crash
Testing completed (on c-c, etc.): Trunk, beta
Risk to taking this patch (and alternatives if risky):

Comment 23

[[e-mail]]

Note that to fully fix Thunderbird 78 bug 1717833 will also need to be backported to Firefox ESR78.

Comment 24

[Wayne Mery (:wsmwk)]

The crash rate spikes starting August 10 https://crash-stats.mozilla.org/signature/?product=Thunderbird&signature=mozilla%3A%3Adom%3A%3ANavigator%3A%3ACheckProtocolHandlerAllowed&date=%3E%3D2021-07-15T15%3A18%3A00.000Z&date=%3C2021-08-15T15%3A18%3A00.000Z#graphs

(In reply to [e-mail] from comment #23)

Note that to fully fix Thunderbird 78 bug 1717833 will also need to be backported to Firefox ESR78.

So this is only/primarily users of OWL, correct?

Note, bug 1717833 has been set as WONTFIX for Firefox ESR78. So is the most suitable solution for those users to move to version 91?

Comment 25

[Wayne Mery (:wsmwk)]

(In reply to Wayne Mery (:wsmwk) from comment #24)

...
So this is only/primarily users of OWL, correct?

Spot checking half dozen crashes indicates it is so.

And FWIW there are no beta or nightly crashes in the past month, only release crashes.

Comment 26

[Ben Bucksch (:BenB)]

Here's what Neil wrote on the internal bug tracker, about 2 months ago:
~~
It's actually quite easy to crash Thunderbird by calling registerProtocolHandler. The problem is that this API is designed for browsers, which means that it assumes that various protocols such as nntp aren't in use. If a web page tries to call registerProtocolHandler passing in nntp as a parameter then it will immediately crash. The crash dumps don't have enough information to tell me what code is calling registerProtocolHandler but it's certainly not Owl. In particular, none of the protocol handlers that Owl registers are allowed by registerProtocolHandler, so it can't even contribute to the crash that way either.

The crash is not in code provided by Owl or even executed by Owl, since there are over 120 crashes where Owl wasn't even installed.
~~

Most of the crashes are right after startup. I suspect that some of the Office365 or Duo (or other third party MFA providers) or Exchange login pages of the server contains a JavaScript call to registerProtocolHandler() in the HTML login page. Owl just happens to open that login page, which is where the correlation comes from, but we cannot avoid the login page. But aside from that, it seems that Owl is in no way triggering this nor causing this. For the same reason, I have no idea how we could possibly work around this.

The only way out that I see is to change RyanVM's mind and to backport bug 1717833 to 78 ESR.

Other ideas?

(I have yet to confirm the above thoery and to find a minimal test case that triggers the crash at will.)

Comment 27

[Ben Bucksch (:BenB)]

RyanVM approved bug 1717833 for backport for 78 ESR. Yay!

Comment 28

[aliledudiable]

So, what are the chances of a fix being introduced for TB 91.0 ?

Comment 29

[Wayne Mery (:wsmwk)]

Comment on attachment 9233102 [details] [diff] [review]
Fix, v4

[Triage Comment]
Approved for esr78

(In reply to aliledudiable from comment #28)

So, what are the chances of a fix being introduced for TB 91.0 ?

100% - it was already in version 91.0. Comment 21 put it in beta 91.

Comment 30

[Wayne Mery (:wsmwk)]

https://crash-stats.mozilla.org/signature/?signature=mozilla%3A%3Adom%3A%3ANavigator%3A%3ACheckProtocolHandlerAllowed&date=%3E%3D2021-07-29T13%3A19%3A00.000Z&date=%3C2021-08-29T13%3A19%3A00.000Z shows there are no version 91 crashes

Comment 31

[aliledudiable]

Well, my TB 91.0.3 crashes with Owl... unless I quickly close the tab it opens on startup (for authentication I guess).

Maybe it's another issue.

Comment 32

[Wayne Mery (:wsmwk)]

Pretty impossible to act on your issue without a crash report ID. https://support.mozilla.org/en-US/kb/mozilla-crash-reporter-tb#w_viewing-crash-reports

Comment 33

[aliledudiable]

A URL to one of them: https://crash-stats.mozilla.org/report/index/f441485e-d322-4b8f-8c2e-1bd180210828

And codes for a bunch of them:
bp-f441485e-d322-4b8f-8c2e-1bd180210828
bp-a833934a-38e3-46dc-b704-a8d180210828
bp-e363457d-1e44-4cb9-9e72-f97710210828
bp-a29ce279-6115-4ac6-99f1-b87f60210828
bp-aebca477-fcc8-4f3f-a6e0-032d30210828
bp-aef07ac4-defe-474d-8dee-790590210828
bp-530746e3-4506-48e1-95c7-d3a940210828
bp-4e3e578c-e152-4851-8ee9-e0c5d0210828

Comment 34

[Ben Bucksch (:BenB)]

@aliledudiable: That sounds like a different bug, indeed. Can you please file a new bug (and mention the bug number here)? Thanks!

Comment 35

[Rob Lemley [:rjl]]

Thunderbird 78.14.0:
https://hg.mozilla.org/releases/comm-esr78/rev/b4f053e8c19a

Comment 36

[Wayne Mery (:wsmwk)]

Signature for this crash no longer appears for 78.14.0