1665577 - self signed SMTPS certificate exceptions can't be added

Status: RESOLVED FIXED

(MailNews Core :: Networking: SMTP, defect)

Description

[Ralf Lehmann]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.102 Safari/537.36

Steps to reproduce:

My SMTPS provider uses a wildcard certificate *.hostedmail.com:465
Trying to send mails ends in "Domain does not match" but, in contrast to the 68 Version, Thunderbird does not ask to add an exception.
Managing certificates from the preference menue has no way to add THAT kind of URL.
I tried the following URL's, but non was successfull:
https://mail.hostedmail.com -> worked only for port 443, of course, but wrong here
https://mail.hostedmail.com:465
smpts://mail.hostedmail.com
smtps://mail.hostedmail.com:465
mail.hostedmail.com:465

Actual results:

Unable to send mail to SMTPS because can't add exception

Expected results:

Thunderbird should ask to accept exception or add a way to add exceptions manually.
By the way, updating from 68 Version keeps the already available exception and respects that.
When you upgrade from 68 to 78 you will not have that problem.

Comment 1

[Magnus Melin [:mkmelin]]

I think we don't have a bug for SMTP yet, so let's use this one.

Comment 5

[Ben Campbell]

Attachment: stunt-smtp-server.py

A dummy smtp server in python which handles TLS connections with a self-signed cert (note: doesn't support StartTLS, but probably not hard to change).

Comment 6

[Ben Campbell]

Attachment: 1665577-add-failedSecInfo-to-smtp-1.patch

This patch is the first part of what's needed.

When a socket error occurs, it copies the secInfo over to the url .failedSecInfo attr so the GUI can get at the bad certificate (if it's an NSS error).

The message-sending code already handles displaying an error upon failed sends - including a specific message for bad-cert errors.
However, it just shows an alert message, not the exceptionDialog.
It's all done in C++, so the next step is to figure out how to invoke the equivalent of window.openDialog("chrome://pippki/content/exceptionDialog.xhtml",...) in C++...

I know there's ongoing work to redo a whole bunch of the message sending in JS, but even if this were all ready to go, we'd need a C++ solution for backporting, right?

Comment 7

[Ben Campbell]

Attachment: 1665577-HACK-dump-failedSecInfo-smtp-1.patch

Just a quick hack to ensure the previous patch works as expected, and that we get a valid .failedSecInfo at the point where we need it.
And indeed, it seems to work fine - I see a non-null .failedSecInfo (on my machine, anyway ;- )

For reference, when a send failure occurs, the callstack up to where the GUI alert is displayed looks like this:

#0 nsMsgSendReport::DisplayReport(nsIPrompt*, bool, bool, nsresult*)
#1  nsMsgComposeAndSend::Fail(nsresult, char16_t const*, nsresult*)
#2  nsMsgComposeAndSend::DoDeliveryExitProcessing(nsIURI*, nsresult, bool)
#3  nsMsgComposeAndSend::DeliverAsMailExit(nsIURI*, nsresult)
#4  nsMsgComposeAndSend::SendDeliveryCallback(nsIURI*, bool, nsresult)
#5  MsgDeliveryListener::OnStopRunningUrl(nsIURI*, nsresult)
...

Comment 8

[Ben Campbell]

Attachment: 1665577-NONWORKING-show-cert-exception-dialog-1.patch

Mostly there now. This patch displays exceptionDialog.xhtml when a bad cert error occurs.
The problem is that I can't figure out a way to pass in the required parameters from C++.
The dialog expects arg[0] to be a javascript object holding all the params, like this:

    var params = {
      exceptionAdded: false,
      securityInfo: secInfo,
      prefetchCert: true,
      location,
    };
    window.openDialog(
      "chrome://pippki/content/exceptionDialog.xhtml",
      "",
      "chrome,centerscreen,modal",
      params
    );

However I can't find any way of doing this in C++. All the examples of dialogs I've found which are called from C++ expect explicitly XPCOM objects to be passed in (eg nsIMsgComposeParams, for the compose window, some others use nsIPropertyBag or nsIDialogParamBlock or nsIWriteableVariants).

So unless there's a solution I'm missing, I think we need a new version of exceptionDialog.xhtml which can be called from C++. This isn't too big a deal - I can fork the existing one and I think we need a custom dialog anyway, with better flow and wording to avoid confusing users.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9178140 [details] [diff] [review]
1665577-NONWORKING-show-cert-exception-dialog-1.patch

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

::: mailnews/compose/src/nsMsgSend.cpp
@@ +3143,3 @@
>      }
>      Fail(aExitCode, eMsg.get(), &aExitCode);
>      NotifyListenerOnStopSending(nullptr, aExitCode, nullptr, nullptr);

I would say the problem *is* you're trying to show a dialog from the sending code. If the sending code get's into trouble, it should bail out, notify the UI and come back later to try again. 

Maybe just send the NotifyListenerOnStopSending (or maybe something similar you'd add) the needed cert data, and make sure it propagates up to the UI for handling.  I guess, e.g.  passing it to nsIMsgWindow.notificationCallbacks (which nsMsgCompose::SendMsg can access). Of course notificationCallbacks needs to be hooked up again for this to do anything. but that's needed for the other protocols as well

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9177959 [details] [diff] [review]
1665577-add-failedSecInfo-to-smtp-1.patch

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

Looks reasonable. r=mkmelin

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e9cd81e7d4cc
Add .failedSecInfo support to SMTP URLs for bad-cert handling. r=mkmelin

Comment 12

[Ben Campbell]

Attachment: 1665577-add-nsIMsgSendListener.onBadCert-1.patch

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

Maybe just send the NotifyListenerOnStopSending (or maybe something similar
you'd add) the needed cert data, and make sure it propagates up to the UI
for handling.

I went with adding an extra callback (NotifyListenerOnBadCert()) rather than adding lots of extra params to OnStopSending(). I think this makes it clearer for places that don't want/need to handle it, and gives us the option to support a behind-the-scenes retry at some point (ie onBadCert could return true to indicate the user has added an exception and a retry should be attempted before onStopSending() is invoked).

Comment 13

[Ben Campbell]

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

Maybe just send the NotifyListenerOnStopSending (or maybe something similar
you'd add) the needed cert data, and make sure it propagates up to the UI
for handling. I guess, e.g. passing it to
nsIMsgWindow.notificationCallbacks (which nsMsgCompose::SendMsg can access).
Of course notificationCallbacks needs to be hooked up again for this to do
anything. but that's needed for the other protocols as well

Been thinking about this and I'm not so sure.

1. The bad cert errors are caught in JS via nsIUrlListener (for imap/pop/nntp etc) and nsIMsgSendListener (in this patch, for smtp), and potentially via other methods (ldap etc). Given that we're already in the GUI code by then, what's the benefit of indirecting further via notificationCallbacks?
2. nsIBadCertListener2 has been removed so we'd have to re-add something equivalent.
3. I hate nsIInterfaceRequestor :-) It's fantastic for obfuscating what things are implemented where. Even if we went for using notificationCallback, I think it should be defined as a real interface rather than nsIInterfaceRequestor. (but given point 1, why bother?) See also Bug 1247024.

What I think might be good is to factor out a new TB-centric exception-adding dialog box with UI to handle guiding the user through what went wrong (e.g. it's a "self signed cert", "it belongs to a different site"), why it might be valid to add an exception (e.g. "viruschecker MITM shenanigans"), and asking them if that's what they'd like to do.
And none of the confusing FF-specific stuff (the "Get Certificate" button, the message saying "Legitimate banks, stores and other public sites will not ask you to do this."...).

Comment 14

[Ben Campbell]

(In reply to Ben Campbell from comment #12)

Created attachment 9178634 [details] [diff] [review]
1665577-add-nsIMsgSendListener.onBadCert-1.patch

Forgot to add - try run running:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3336f646d0c7175eb5366b4d57bfc94352437272

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 9178634 [details] [diff] [review]
1665577-add-nsIMsgSendListener.onBadCert-1.patch

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

Please make additions (stubs) for MessageSend.jsm as well.

I'm not sure this all should be on the sendlistener though. The idea behind using something like notificationCallback was that we could hook the same function for all the protocols when handling the problem (where we have notificationCallback, you could have onBadCert instead). I don't insist if you think it's not worth it.

::: mailnews/compose/public/nsIMsgSendListener.idl
@@ +60,5 @@
> +     * This callback is here to provide an opportunity to pop up the
> +     * "Add a certificate exception" dialog box.
> +     * After this callback returns, onStopSending will still be invoked.
> +     * (In future, maybe this function could return a value to indicate that
> +     * the operation should be retried first).

Probably not automatically, as that would complicate the mechanism. I mean, it already failed so we should let higher logic take care as needed.

@@ +66,5 @@
> +     * Bad certificate situations are more common than we'd like. For example,
> +     * a lot of antivirus software man-in-the-middles STMP traffic using a
> +     * locally-running server with a self-signed certificate.
> +     *
> +     * @param {string} msgID      - The message ID.

Is there a reason to pass in message id? It seems you pass null all the time anyway.

@@ +74,5 @@
> +     *                            - Contains the failed certificate.
> +     * @param {ACString} location - The location of the failed connection
> +     *                              ("host:port")
> +     */
> +    void onBadCert(in string msgID, in nsresult status, in nsITransportSecurityInfo secInfo, in ACString location);

There is another use case too for this error handling: the SSL version failures. Those are only overridable through a pref, but it would be good to add UI feedback instead of silent failures for them. For another bug, but let's use a name that can cover that too, e.g. onTransportSecurityError

::: mailnews/compose/src/nsMsgSend.cpp
@@ +3309,5 @@
>  NS_IMETHODIMP
> +nsMsgComposeAndSend::NotifyListenerOnBadCert(const char* msgID, nsresult status,
> +                                             nsITransportSecurityInfo* secInfo,
> +                                             nsACString const& location) {
> +  if (mListener != nullptr)

just check truthy, if (mListener)

Comment 17

[Ben Campbell]

Attachment: 1665577-add-nsIMsgSendListener.onTransportSecurityError-1.patch

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

Please make additions (stubs) for MessageSend.jsm as well.

Done.

I'm not sure this all should be on the sendlistener though. The idea behind
using something like notificationCallback was that we could hook the same
function for all the protocols when handling the problem (where we have
notificationCallback, you could have onBadCert instead). I don't insist if
you think it's not worth it.

I understand (and agree) with the motivation, I just don't think that's the right approach.
Firstly, remember that NSS errors are now handled just like any other kind of error. There are no behind-the-scenes special-favour handling for bad cert errors any more.

For the non-SMTP protocols, all errors are passed back out to the GUI side via nsIUrlListener.onStopRunningUrl(url, status). At that point you've got everything you need to handle the error. You're already in GUI land. You have the error code. For NSS errors, you can get the failed cert via url.failedSecurityInfo if you need to display the exception dialog.

(In fact a lot of protocols use other methods to display error messages before onStopRunningUrl() is called, but I think that should be phased out. But that'd be another bug.)

So by the time we're in the onStopRunningUrl() handler we already know what error it is, how we want to handle it, and we have everything we need to do so.
The onStopRunningUrl() handling could then grab a notificationCallback from somewhere and indirect through there for NSS errors... but it just seems like an unnecessary extra thing that needs to be set up in advance. (Previously we needed to pass notificationCallback all the way down to the sockettransport, so it could find a nsIBadCertListener2 handler).

For the SMTP case, the nsIMsgSend implementation handles nsIUrlListener.onStopRunningUrl(url, status) further down, and the GUI sees nsIMsgSendListener callbacks instead. But the same argument applies here - we've already got everything we need to handle the error.
(It's a little ugly passing out the securityInfo via a different callback, but I figured it'd be more burdensome to add the extra params to onStopSending() - listeners can ignore onTransportSecurityInfo() if they aren't interested in the secInfo.).

There's probably an argument for saying the nsIMsgSend should handle the message GUI stuff down in it's onStopRunningUrl handling like the other protocols, but then you're arbitrarily saying nsIMsgSend implementations are GUI-side code. Which doesn't quite feel right, even if they are migrating to JS.

Is there a reason to pass in message id? It seems you pass null all the time
anyway.

It was just to keep it in line with all the other nsIMsgListener callbacks. They seem to be mostly null too... but I think there are a few places where they aren't. Maybe they could all be removed.

There is another use case too for this error handling: the SSL version
failures. Those are only overridable through a pref, but it would be good to
add UI feedback instead of silent failures for them. For another bug, but
let's use a name that can cover that too, e.g. onTransportSecurityError

Good point. Done.

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 9178898 [details] [diff] [review]
1665577-add-nsIMsgSendListener.onTransportSecurityError-1.patch

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

Thanks, just two nits. Also please update the commit message to say *why*

::: mail/components/compose/content/MsgComposeCommands.js
@@ +764,5 @@
> +      return;
> +    }
> +
> +    // Give the user the option of adding an exception for the bad cert.
> +    var params = {

nit: could use "let"

::: mailnews/compose/src/MessageSend.jsm
@@ +268,5 @@
>      }
>    },
>  
> +  notifyListenerOnTransportSecurityError(msgId, status, secInfo, location) {
> +    if (this._sendListener) {

nit: please just do an early return instead

Comment 21

[Ben Campbell]

Attachment: 1665577-add-nsIMsgSendListener.onTransportSecurityError-2.patch

In addition to the nits, one tiny code change in there: In the C++ sending code, forgot to broaden it to invoke the callback for all NSS errors, not just bad-cert. This one brings the C++ in line with what the javascript send code does.

Magnus: I've reset the r? just to be thorough - it's minor, but is an actual code change. If it looks OK to you, I'm happy for it to land.
The change is in nsMsgComposeAndSend::DoDeliveryExitProcessing().

Comment 22

[Ben Campbell]

Keeping this bug open as there are a couple of oddities I'm still looking at. The patch works fine for me... although sometimes the first time I try sending after setting up a new account, the NSS error doesn't trigger. Needs a little more investigation.

Comment 23

[Magnus Melin [:mkmelin]]

Comment on attachment 9179375 [details] [diff] [review]
1665577-add-nsIMsgSendListener.onTransportSecurityError-2.patch

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

LGTM, r=mkmelin

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ee2a42556222
Add nsIMsgSendListener.onTransportSecurityError to handle NSS errors (e.g. bad certs) during sending. r=mkmelin

Comment 26

[Magnus Melin [:mkmelin]]

Comment on attachment 9177959 [details] [diff] [review]
1665577-add-failedSecInfo-to-smtp-1.patch

[Approval Request Comment]
Regression caused by (bug #): bug 1547096
User impact if declined: can't send email if the smtp uses a self signed certificate
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): Not risky. These patches work, and while there is potentially something more to tidy up here we need to get this out to users.

Comment 27

[Wayne Mery (:wsmwk)]

Comment on attachment 9177959 [details] [diff] [review]
1665577-add-failedSecInfo-to-smtp-1.patch

[Triage Comment]
Approved for beta

Comment 28

[Wayne Mery (:wsmwk)]

Comment on attachment 9179375 [details] [diff] [review]
1665577-add-nsIMsgSendListener.onTransportSecurityError-2.patch

[Triage Comment]
Approved for beta

Comment 29

[Rob Lemley [:rjl]]

Thunderbird 82.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/f3023bdb92a8
Thunderbird 82.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/5ebe9575f332

Comment 31

[CarlB]

Found a workaround.
I'm using hmailserver on a standalone Server 2019 (not a DC). It's for internal mail only, and that's it. So set up for SSL/TLS on port 465 and STARTTLS on port 143 using a self-signed e-mail cert. There's no problem receiving, but unable to send e-mail with a a warning that the certificate can't be verified. Tried to "solve" the problem and I was unable to after a few hours. Overall, I'm not yet convinced where the problem is - the hmailserver program or the Thunderbird program. So time for a new approach. Instead of trying to figure out why it won't work, what can I do to "make" it work?
So with a new approach I decided to see if I could find a work-around. I did.

Navigate to C:\Users\<pofile_name>\AppData\Roaming\Thunderbird\Profiles\<profile_in_use>\ and open the Cert_Override.txt file using notepad. List there you will see the data for your self-signed certificate, but only for the incoming mail port. (Port 143 in my case). It will look something like this:
my.mail.server:143 OID.2.16.840.1.101.3.4.2.1 CE:D6:4C: (buch of key gibberish after this)

Copy the above to a new line, and the only thing you need to change is the port number. In my case, I changed it from port 143 to port 465 since that's what I use on the hmailserver program for the SMTP port. Then save the file. Now now the file looks like this:
my.mail.server:143 OID.2.16.840.1.101.3.4.2.1 CE:D6:4C: (buch of key gibberish after this)
my.mail.server:465 OID.2.16.840.1.101.3.4.2.1 CE:D6:4C: (buch of key gibberish after this)
You can also add for other ports, such as 587 or 993 if those are the ports used.
Now you can restart Thunderbird and when you check the certificate exceptions you'll see the cert listed twice - once for the incoming port and again for the outgoing port. I now have no problem receiving "or" sending e-mail through my end-to-end encrypted hmailserver program.

Comment 32

[Wayne Mery (:wsmwk)]

Comment on attachment 9179375 [details] [diff] [review]
1665577-add-nsIMsgSendListener.onTransportSecurityError-2.patch

[Triage Comment]
Approved for esr78

Comment 33

[Wayne Mery (:wsmwk)]

Comment on attachment 9177959 [details] [diff] [review]
1665577-add-failedSecInfo-to-smtp-1.patch

[Triage Comment]
Approved for esr78

Comment 34

[Magnus Melin [:mkmelin]]

Let's call this fixed and do followups in another bug. Otherwise potential uplifting will be a nightmare.

Comment 35

[Rob Lemley [:rjl]]

Uplifting is already a nightmare... the second patch modifies MessageSend.jsm, which didn't exist until bug 1211292 created it on 2020-08-12. I left it out of the uplift as I don't see it referenced elsewhere.

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

Let's call this fixed and do followups in another bug. Otherwise potential uplifting will be a nightmare.

Comment 36

[Rob Lemley [:rjl]]

Thunderbird 78.4.0:
https://hg.mozilla.org/releases/comm-esr78/rev/9591a2a9ee3d
https://hg.mozilla.org/releases/comm-esr78/rev/d4a30c3f0764

Comment 37

[Magnus Melin [:mkmelin]]

Correct you can ignore the MessageSend.jsm part

Comment 40

[Herre de Jonge]

Copy the above to a new line, and the only thing you need to change is the port number. In my case, I changed it from port 143 to port 465 since that's what I use on the hmailserver program for the SMTP port. Then save the file. Now now the file looks like this:
my.mail.server:143 OID.2.16.840.1.101.3.4.2.1 CE:D6:4C: (buch of key gibberish after this)
my.mail.server:465 OID.2.16.840.1.101.3.4.2.1 CE:D6:4C: (buch of key gibberish after this)
You can also add for other ports, such as 587 or 993 if those are the ports used.
Now you can restart Thunderbird and when you check the certificate exceptions you'll see the cert listed twice - once for the incoming port and again for the outgoing port. I now have no problem receiving "or" sending e-mail through my end-to-end encrypted hmailserver program.

After switching to 78.4 my IMAP stopped working. With this tip I succeeded. Note that the DNS names (not ip addresses) of the server are even different in my case and smtp (exim) and imap (dovecot) both use their own certificate.

So I copied the smtp line and updated DNS name and port to get:
smtp.mydomain.lan:25 OID.2.16.840.1.101.3.4.2.1 CE:D6:4C: (buch of key gibberish after this)
mail.mydomain.lan:993 OID.2.16.840.1.101.3.4.2.1 CE:D6:4C: (buch of key gibberish after this)

In the TB certificates exception list, both lines show the same certificate, but everything works again. I wonder what happens when they expire...

Comment 41

[Herre de Jonge]

Sorry... Cheered too soon. Doesn't work after all. Some other temporary configuration tinkering on the server side made me believe it worked.

Comment 42

[Herre de Jonge]

For what it's worth: I got it working for IMAP by having my apache webserver use the Dovecot certificate. Then using the standard add-exception functionality of TB to get the certificate data in the cert_override.txt file. And finally editing that file to give it the correct name and port.

(Sorry for the spamming. This was the last. It really works now and I hope this helps other people as well.)

Comment 43

[James Burke]

Is there any hope for us normal T'bird (78.6.0 32-bit) users? Our mail is stuck behind this "Add Security Exception" block, and we're waiting for some kind of solution. I apologize for posting here, but this is where the Discussion Forum points since there's nothing else in the way of a work-around. Thanks!

Comment 44

[Herre de Jonge]

If I read the headers correctly, it is solved in branch 83, so it should reach you when that version is released. Or you could try the beta, which seems to be v85 right now.

Comment 45

[Matt]

[Wrong bug posted in]