Closed Bug 1590471 Opened 5 years ago Closed 3 years ago

Chat: Implement new handling for bad server certificates on SSL/TLS connections

Categories

(Chat Core :: General, defect)

Product:
Chat Core
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird_esr78+ fixed, thunderbird84 affected)

Status:
RESOLVED FIXED
Milestone:
85 Branch
Tracking Flags:
Tracking Status
thunderbird_esr78 + fixed
thunderbird84 --- affected

People

(Reporter: KaiE, Assigned: mkmelin)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

bug1590471_chat_certoverride.patch
21.29 KB, patch
clokep
: review+
wsmwk
: approval-comm-esr78+
Details | Diff | Splinter Review

After bug 1547096, connections to SSL/TLS servers with "bad" certificates will fail in a different way, and might no longer offer an override.

As described in bug 1547096, the Mozilla core engine removes the callback interface nsIBadCertListener2.

A callback notification will no longer be offered. Instead, the code that handles the application protocol over a SSL/TLS must handle error results, and if necessary, query the socket/channel to find out if the error was related to a bad certificate, and act as desired.

This bug suggests to add a new implementation for that, to replace the previous use of nsIBadCertListener2 and notifyCertProblem in file:

  • chat/modules/socket.jsm

Thanks for filing this Kai. Do you know if there's an example of the "new" way to do this?

In 1547096 comment 12 it was mentioned that the fix for bug 497488 might be useful as inspiration.

Assignee: nobody → khushil324
Keywords: regression
Status: NEW → ASSIGNED

Maybe sz2243 (from bug 1673289) could help test/verify if you do a try build.

(In reply to Magnus Melin [:mkmelin] from comment #5)

Maybe sz2243 (from bug 1673289) could help test/verify if you do a try build.

Sure, let me do this.

(In reply to Khushil Mistry [:khushil324] from comment #6)

(In reply to Magnus Melin [:mkmelin] from comment #5)

Maybe sz2243 (from bug 1673289) could help test/verify if you do a try build.

I don't have a public facing server you can test against, but I'd be happy to provide my SSL config, and I'm happy to prepare a docker image mirroring my prosody server setup, if that would help?

The SSL cert was generated with: openssl req -x509 -newkey rsa:4096 -keyout key.pem -out cert.pem -days 365 -nodes

(In reply to sz2243 from comment #7)

I don't have a public facing server you can test against, but I'd be happy to provide my SSL config, and I'm happy to prepare a docker image mirroring my prosody server setup, if that would help?

Which operating system do you use?

For Mac: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/WBm1R78iQb2VXzEhEePg4Q/runs/0/artifacts/public/build/target.dmg
For Linux: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/CiDmaq7nRQ2W1D_SJPPLuQ/runs/0/artifacts/public/build/target.tar.bz2

Can you download the Thunderbird from the above links and test if the problem is solved or not?

(In reply to Khushil Mistry [:khushil324] from comment #4)

Created attachment 9183764 [details] [diff] [review]
Bug-1590471_fix-chat-bad-server-certificates-0.patch

I don't know how to test it so just did what we were doing previously here: https://searchfox.org/comm-central/rev/01f50ef57ff126a8a8146f9091a44d3ff0b31d4e/chat/modules/socket.jsm#492

Yeah it seems pretty reasonable. I used to have an easy way to test this at my old company... I'll try to dig something up about how to do it now.

(In reply to Khushil Mistry [:khushil324] from comment #9)

For Windows: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/YqIE6yiyQ828foACTscxAA/runs/0/artifacts/public/build/target.zip

I just ran the build on Windows - no change, I'm afraid. I'm still seeing a reconnect loop with "Error: The server closed the connection", and I'm still unable to use the certificate manager to add an exception (since the certificate manager won't fetch the cert from the XMPP server).

Here is the debug log from the connection attempt:

(Thunderbird 84.0a1 (20201027132508), Gecko 84.0a1 (20201027132508) on Windows NT 10.0; Win64; x64)
(54 messages omitted)
[10/27/2020, 7:53:50 PM] DEBUG (@ prpl-jabber: connect resource:///modules/socket.jsm:145)
Connecting to: im.pi.lan:5222
[10/27/2020, 7:53:50 PM] DEBUG (@ prpl-jabber: onTransportStatus resource:///modules/socket.jsm:515)
onTransportStatus(STATUS_RESOLVING)
[10/27/2020, 7:53:50 PM] DEBUG (@ prpl-jabber: onTransportStatus resource:///modules/socket.jsm:515)
onTransportStatus(STATUS_RESOLVED)
[10/27/2020, 7:53:50 PM] DEBUG (@ prpl-jabber: onTransportStatus resource:///modules/socket.jsm:515)
onTransportStatus(STATUS_CONNECTING_TO)
[10/27/2020, 7:53:50 PM] DEBUG (@ prpl-jabber: onTransportStatus resource:///modules/socket.jsm:515)
onTransportStatus(STATUS_CONNECTED_TO)
[10/27/2020, 7:53:50 PM] DEBUG (@ prpl-jabber: sendString resource:///modules/socket.jsm:245)
Sending:
<?xml version="1.0"?><stream:stream to="im.pi.lan" xmlns="jabber:client" xmlns:stream="http://etherx.jabber.org/streams" version="1.0">
[10/27/2020, 7:53:50 PM] DEBUG (@ prpl-jabber: onTransportStatus resource:///modules/socket.jsm:515)
onTransportStatus(STATUS_SENDING_TO)
[10/27/2020, 7:53:50 PM] DEBUG (@ prpl-jabber: onTransportStatus resource:///modules/socket.jsm:515)
onTransportStatus(STATUS_RECEIVING_FROM)
[10/27/2020, 7:53:50 PM] DEBUG (@ prpl-jabber: onStartRequest resource:///modules/socket.jsm:437)
onStartRequest
[10/27/2020, 7:53:50 PM] DEBUG (@ prpl-jabber: _logReceivedData resource:///modules/xmpp-xml.jsm:390)
received:
<stream:stream xmlns="http://etherx.jabber.org/streams" version="1.0" from="im.pi.lan" id="42dcf80b-ba40-4aa4-8673-bc1877d48534" xml:lang="en">

[10/27/2020, 7:53:50 PM] DEBUG (@ prpl-jabber: _logReceivedData resource:///modules/xmpp-xml.jsm:390)
received:
<stream:features xmlns="http://etherx.jabber.org/streams">
 <starttls xmlns="urn:ietf:params:xml:ns:xmpp-tls">
  <required xmlns="urn:ietf:params:xml:ns:xmpp-tls"/>
 </starttls>
</stream:features>

[10/27/2020, 7:53:50 PM] DEBUG (@ prpl-jabber: sendString resource:///modules/socket.jsm:245)
Sending:
<starttls xmlns="urn:ietf:params:xml:ns:xmpp-tls" id="dba2de15-932b-4f5f-9f6e-ff43efc41890"/>
[10/27/2020, 7:53:50 PM] DEBUG (@ prpl-jabber: onTransportStatus resource:///modules/socket.jsm:515)
onTransportStatus(STATUS_SENDING_TO)
[10/27/2020, 7:53:50 PM] DEBUG (@ prpl-jabber: onTransportStatus resource:///modules/socket.jsm:515)
onTransportStatus(STATUS_RECEIVING_FROM)
[10/27/2020, 7:53:50 PM] DEBUG (@ prpl-jabber: _logReceivedData resource:///modules/xmpp-xml.jsm:390)
received:
<proceed xmlns="urn:ietf:params:xml:ns:xmpp-tls"/>

[10/27/2020, 7:53:50 PM] DEBUG (@ prpl-jabber: sendString resource:///modules/socket.jsm:245)
Sending:
<?xml version="1.0"?><stream:stream to="im.pi.lan" xmlns="jabber:client" xmlns:stream="http://etherx.jabber.org/streams" version="1.0">
[10/27/2020, 7:53:50 PM] DEBUG (@ prpl-jabber: onStopRequest resource:///modules/socket.jsm:446)
onStopRequest (2153398258)
[10/27/2020, 7:53:50 PM] DEBUG (@ prpl-jabber: disconnect resource:///modules/socket.jsm:190)
Disconnect

This fixes it for me.

Assignee: khushil324 → mkmelin+mozilla
Attachment #9183764 - Attachment is obsolete: true
Attachment #9183764 - Flags: review?(clokep)
Attachment #9191281 - Flags: review?(clokep)
status-thunderbird84: --- → affected
status-thunderbird_esr78: --- → affected
Target Milestone: --- → 85 Branch
Comment on attachment 9191281 [details] [diff] [review]
bug1590471_chat_certoverride.patch

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

Seems good overall, a couple of questions though.

::: chat/content/chat-account-richlistitem.js
@@ -72,5 @@
>                  <label class="disconnected" crop="end" value="&account.disconnected;"></label>
>                  <description class="error error-description"></description>
>                  <description class="error error-reconnect"></description>
> -                <label class="addException text-link" onclick="gAccountManager.addException()"
> -                       data-l10n-id="certmgr-add-exception"></label>

Was this fully unused?

::: chat/modules/socket.jsm
@@ +121,5 @@
>    connectTimeout: 0,
>    readWriteTimeout: 0,
>  
>    // A nsITransportSecurityInfo instance giving details about the certificate error.
> +  // XXX: should we rename to securityInfo, like in nsISocketTransport?

If we're messing around here anyway I think we should go ahead and do it.

::: chat/protocols/xmpp/xmpp-base.jsm
@@ +2684,5 @@
>    onError(aError, aException) {
>      if (aError === null || aError === undefined) {
>        aError = Ci.prplIAccount.ERROR_OTHER_ERROR;
>      }
> +    this._disconnect(aError, aException?.toString());

Is `aException` now undefined sometimes? Or was this an unrelated change?

::: mail/components/im/content/imAccounts.js
@@ +218,5 @@
> +      document.getElementById(account.id).updateConnectionError();
> +      // See NSSErrorsService::ErrorIsOverridable.
> +      if (
> +        [
> +          "MOZILLA_PKIX_ERROR_ADDITIONAL_POLICY_CONSTRAINT_FAILED",

It seems like this check was essentially moved from socket.jsm to here. It seems like an odd fit for the UI to do this check, but don't have strong opinions about it.

(In reply to Patrick Cloke [:clokep] from comment #13)

Comment on attachment 9191281 [details] [diff] [review]

  • this._disconnect(aError, aException?.toString());

Is aException now undefined sometimes? Or was this an unrelated change?

I saw it in an earlier iteration of the patch. Might have been something with wrong changes, so reverted it now.

It seems like this check was essentially moved from socket.jsm to here. It
seems like an odd fit for the UI to do this check.

Yes, if it isn't decided by the UI can't take appropriate action. That is, we should give proper feedback for non-overridable errors too, but not in this patch.

Attachment #9191281 - Attachment is obsolete: true
Attachment #9191281 - Flags: review?(clokep)
Attachment #9191522 - Flags: review?(clokep)
Comment on attachment 9191522 [details] [diff] [review]
bug1590471_chat_certoverride.patch

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

LGTM! Thank you!
Attachment #9191522 - Flags: review?(clokep) → review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/83166415d712
fix ability to add certificate exceptions for XMPP. r=clokep

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED

Comment on attachment 9191522 [details] [diff] [review]
bug1590471_chat_certoverride.patch

[Approval Request Comment]
Regression caused by (bug #): bug 1547096
User impact if declined: can't use XMPP if the server certificate is self signed.
Testing completed (on c-c, etc.): on c-c, now in beta.

Attachment #9191522 - Flags: approval-comm-esr78?

Comment on attachment 9191522 [details] [diff] [review]
bug1590471_chat_certoverride.patch

[Triage Comment]
Approved for esr78

Attachment #9191522 - Flags: approval-comm-esr78? → approval-comm-esr78+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: