Closed Bug 1590473 Opened 5 years ago Closed 4 years ago

Mail account wizard: Implement new handling for bad server certificates on SSL/TLS connections

Categories

(Thunderbird :: Account Manager, defect, P1)

Product:
Thunderbird
Component:
Account Manager
Platform:
Unspecified
All
Type:
defect

Tracking

(thunderbird_esr68 unaffected, thunderbird_esr78+ fixed, thunderbird72 wontfix, thunderbird73 wontfix, thunderbird74+ wontfix, thunderbird75+ wontfix, thunderbird79 wontfix, thunderbird81 affected)

Status:
RESOLVED FIXED
Milestone:
82 Branch
Tracking Flags:
Tracking Status
thunderbird_esr68 --- unaffected
thunderbird_esr78 + fixed
thunderbird72 --- wontfix
thunderbird73 --- wontfix
thunderbird74 + wontfix
thunderbird75 + wontfix
thunderbird79 --- wontfix
thunderbird81 --- affected

People

(Reporter: KaiE, Assigned: khushil324)

References

(Regression)

Details

(Keywords: regression, Whiteboard: [regression:TB72])

Attachments

(6 files, 10 obsolete files)

1590473-fix-imap-read-error-propagation-1.patch
7.99 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
1590473-skip-imap-login-if-borked-connection-1.patch
907 bytes, patch
mkmelin
: review+
Details | Diff | Splinter Review
1590473-suppress-pop3-nss-alertbox-1.patch
1.48 KB, patch
Details | Diff | Splinter Review
bug1590473_selfsigned_cert.patch
21.85 KB, patch
mkmelin
: review+
wsmwk
: approval-comm-esr78+
Details | Diff | Splinter Review
protonmail-ssl.png
49.14 KB, image/png
Details
win64-cert.png
20.33 KB, image/png
Details

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 the following files:

  • mail/components/accountcreation/content/MyBadCertHandler.js
  • mail/components/accountcreation/content/emailWizard.js
  • mail/components/accountcreation/content/emailWizard.xul
  • mail/components/accountcreation/content/guessConfig.js
  • mail/components/accountcreation/content/verifyConfig.js
  • mail/components/newmailaccount/content/uriListener.js
See Also: → 1590474
OS: Unspecified → All
Summary: Mail account wizardl: Implement new handling for bad server certificates on SSL/TLS connections → Mail account wizard: Implement new handling for bad server certificates on SSL/TLS connections
Version: unspecified → Trunk
Priority: -- → P2

Kaie, This missed 74. Are you taking an action to resolve this for 75?

status-thunderbird73: affected → wontfix
status-thunderbird75: --- → affected
tracking-thunderbird75: --- → ?
Flags: needinfo?(kaie)

Let's let Ben take this.

Assignee: nobody → benc
tracking-thunderbird75: ? → +
Flags: needinfo?(kaie)
Attached patch 1590473-guessconfig-1.patch (obsolete) — — Splinter Review

Just a tentative stab at one of them. Still pretty messy and a few things I've not quite worked out, but I just wanted to make sure I was understanding the issue correctly:

Instead of a dedicated nsIBadCertListener2 interface to handle bad certificates, requests now just fail with appropriate error codes (under ERROR_CLASS_BAD_CERT).

As far as I can tell, nsIRequestObserver.onStopRequest() is the sole place that needs to check for bad cert errors. If I detect one, I should (for now) just try to invoke whatever handling was previously done by the nsIBadCertListener2.notifyCertProblem() implementation - for guessconfig it's just quietly adding a temporary ignore, while other places will inform the user via a dialog box and let them decide what to do...

Am I missing anything important?

I suspect that the bigger task is working out some unit tests to make sure all this stuff is actually working as intended...

Flags: needinfo?(kaie)

Sounds good to me. I don't know the wizard code. While you're working on it, my advice is, make sure to never send the password over a plaintext connection, and never into a connection that uses a bad-cert-override.

Flags: needinfo?(kaie)

(In reply to Kai Engert (:KaiE:) from comment #4)

Sounds good to me. I don't know the wizard code.

That makes two of us :-)

While you're working on it, my advice is, make sure to never send the password over a plaintext connection, and never into a connection that uses a bad-cert-override.

Good point. I'm not planning to change any of the previous behaviour. The guessconfig does add temporary overrides to probe the server, but I'm not sure it goes as far as sending a password. So it's something I'll keep a lookout for.

I'm going to pass this over to Khushil (with bug 1590474)

Assignee: benc → khushil324
status-thunderbird_esr68: --- → unaffected

I tried this with ESR 68. In the account wizard, if we try to sign up with our Fastmail account(TLS 1.3, security.tls.version.min=4), we are only getting the error "Thunderbird failed to find the settings for your email account" but we have https://searchfox.org/comm-central/source/mail/components/accountcreation/content/verifyConfig.js#384 which suggests we should open the exceptionDialog.xhtml dialog. What is the correct behavior?

Ah, yes that will give ERROR_CLASS_SSL_PROTOCOL and not ERROR_CLASS_BAD_CERT which is what we want.
You could generate a self signed certificate which would fall into this class I think.

An SSL/TLS protocol is a hard failure and cannot be fixed.

Only bad cert errors can be potentially overridden.

Attachment #9131794 - Attachment is obsolete: true
Attachment #9141513 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Comment on attachment 9141513 [details] [diff] [review]
Bug-1590473_account-wizard-bad-certificate-handling-0.patch

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

::: mail/components/accountcreation/content/guessConfig.js
@@ +1213,5 @@
> +              isCertError = true;
> +            }
> +          } catch (e) {
> +            // nsINSSErrorsService.getErrorClass throws if given a non-TLS,
> +            // non-cert error, so ignore this.

what certificate would that be?

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

what certificate would that be?

If the error is not from Ci.nsINSSErrorsService.ERROR_CLASS_BAD_CERT or Ci.nsINSSErrorsService.NSS_SSL_ERROR_BASE(i.e any errors from nssErrorsService), nssErrorsService.getErrorClass(status) will throw an error.

Comment on attachment 9141513 [details] [diff] [review]
Bug-1590473_account-wizard-bad-certificate-handling-0.patch

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

This didn't seem to work, but maybe,
Attachment #9141513 - Flags: review?(mkmelin+mozilla)

I have few question here:

Previously, urlListener also had nsIBadCertListener2 interface and notifyCertProblem function in verifyConfig.js. (https://searchfox.org/comm-central/source/mail/components/accountcreation/content/verifyConfig.js#195). This urlListener is a notificationCallback function for account wizard window. (https://searchfox.org/comm-central/source/mail/components/accountcreation/content/verifyConfig.js#170-179)

For the current usage, I thought OnStopRunningUrl(aUrl, aExitCode) from urlListener would work i.e. detecting the error from the aExitCode. I am doing following:


let errorClass = nssErrorsService.getErrorClass(aExitCode);
if (errorClass == Ci.nsINSSErrorsService.ERROR_CLASS_BAD_CERT) {
  this.isCertError = true;
}

If the TLS related issue is there, we are getting errorClass is equal to ERROR_CLASS_SSL_PROTOCOL. But for ERROR_CLASS_BAD_CERT, class checking with aExitCode is not working. Do you know any other things to keep in mind while doing this?

I also tried to add nsIStreamListener interface in the urlListener and try to catch the error in the onStopRequest but it was not getting fired. Any suggestions on that?

What error class do we get for the exit code of ERROR_CLASS_BAD_CERT then?

This seems to be the only similar case in m-c: https://searchfox.org/mozilla-central/rev/8bc4e35c9bb47c1fe3131e6155d9f482e1efef9a/devtools/shared/security/socket.js#368 but that's from onInputStreamReady and I don't know if you have that available anywhere.

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

What error class do we get for the exit code of ERROR_CLASS_BAD_CERT then?

Errorcode is -16370 for ERROR_CLASS_BAD_CERT class.
Currently, we are getting 2147500037, which is the same as the server details are wrong error.
In the feed utils, we are figuring out the error from xhr request: https://searchfox.org/comm-central/source/mailnews/extensions/newsblog/content/FeedUtils.jsm#1582
Can we do something like here? How can we get the request object in verifyConfig.js?

I think it may need to be handled on the protocol level, so that if there's an error due to this we don't set rv NS_ERROR_FAILURE but instead at that point get the nss error and return that.

Flags: needinfo?(benc)

I'm picking my way through the IMAP protocol now - it's a little slow and convoluted.
But am I right in thinking that this shouldn't be an IMAP-specific issue? It'll be all protocols which used to use SSL/TLS and nsIBadCertListener2, right?

Correct.

I've only been looking at IMAP so far, and there does definitely seem to be a problem in the protocol code - I can't see the onStopRequest() being invoked for the listener.
I haven't checked any of the other protocols yet. It could just be an IMAP bug (or maybe they all screw it up ;- )

I'd be interested to see if the javascript-side changes (checking for BAD_CERT in OnStopRequest()) work for protocols other than IMAP.

I think the IMAP one is due to the odd way the nsIMAPProtocol itself is a channel, but then it requires using an nsImapMockChannel (which is also a channel) to actually do stuff. It gets messy, and I think calling onStopRequest() is falling through the cracks somewhere - possibly the listener isn't even attached during the initial connection).
Will keep looking next week.

Version: Trunk → 72

Here are my notes on replicating the bug for IMAP, at least on Debian Linux:

dovecot setup

A default install of dovecot will use a self-signed cert, which is what we need.
Easiest to debug this on a secure connection (ie port 993) rather than faffing about with StartTLS.

The default Debian /etc/dovecot/dovecot.conf uses a verbose split-config-file setup. For this hacking about, I found it easiest to generate a single-file dovecot.conf (doveconf -n will output all the non-default settings, so it's an easy way to generate a dovecot.conf to start from).

I initially had to add "imaps" to my protocols= line:

protocols = "imaps imap"

But a few days later I spotted a log message saying you didn't need "imaps" as it was enabled by default... so I'm not quite sure. Maybe I upgraded to a new version accidentally? Anyway, it still works for the purposes of replicating this bug so I haven't investigated.

TB setup

I just set up an email account with a local username and password, Server: localhost.
Default dovecot config looks for mailboxes in ~/mail and /var/mail/<username> , so if you plonk an mbox in one of those it should show up.
Test it first on an unsecured connection to make sure it's all working (ie you can see the expected messages in TB), then switch it over to port 993 to trigger the bug.
This is testing the IMAP connection in general rather than the config wizard specifically, but the error and cause is the same.

A few false turns, but I found out where the BAD_CERT error is showing up.
It's obscured a little due to how the SocketTransport defers some of the connection and handshaking, but we end up seeing an error when we first attempt to Read from the connection.

A bunch of read errors are caught and handled at this point:
https://searchfox.org/comm-central/source/mailnews/imap/src/nsImapProtocol.cpp#4731

Most of these boil down to invoking nsIMsgMailSession.alertUser(), passing in the named error and the IMAP URL that was being run.

The alertUser() implementation calls onAlert() on any nsIMsgUserFeedbackListener registered to the session, and tries to pop up a prompt dialog for the associated nsIMsgWindow.

As I understand it, previously the low-level SSL/TLS code would just directly invoke a nsIBadCertListener2, and the IMAP code wouldn't have to worry about it.
Now, we'll need to:

  • catch the bad cert error
  • propagate it up to the GUI, which probably means defining a new message type (as for imapConnectionRefusedError, imapUnknownHostError et al)
  • show a dialog box to explain about the bad cert and potentially offer an option to bypass it. (maybe reusing whatever UI nsIBadCertListener2 used).

There's still one disconnect I don't quite grasp: the current (non-bad cert) error handling happens via the onStopRequest() in the nsIRequestObserver/nsIStreamListener for the URL being run. Particularly for the wizard, this seems pretty vital.
But I haven't figured out how the other failures in the read code are being propagated back out to the onStopRequest() handler.

Presumably I now need to figure out how to get the BAD_CERT error code showing up in onStopRequest(), right?

I can't yet see how any of the other read errors are propogated out to onStopRequest(). It's possible they aren't, and that a generic error is generated when the IMAP connection is killed (it's killed as part of the read error handling)...

(and then we likely need to do the same thing for other protocols - SMTP? POP3? what else?)

Just an extra thought:
It seems pretty horrific that the IMAP code directly pokes the GUI, via calling alertUser(). Or that it knows about anything GUI-related at all.
Really, all IMAP errors should be communicated via failure codes passed into onStopRequest()... and then that handler would be responsible for deciding how to inform the user.

This patch should make sure the BAD_CERT error (and other errors that pop up when the socket is read) are propagated back out, rather than just being replaced with a generic NS_ERROR_FAILURE.

Khushil: I think your approach in Comment 14 is the correct one. This patch should mean that the OnStopRunningUrl() in verifyConfig.js is passed the proper error code. From there, I guess the user can be informed, and (in the case of BAD_CERT) given the option to override it.
I haven't looked into how you actually override a bad certificate - let me know if there is anything that needs to happen on the C++ side to support this!

Flags: needinfo?(benc)
Attachment #9164239 - Flags: review?(mkmelin+mozilla)

BTW this bug only affects the verifyConfig.js and the normal IMAP operation. guessConfig.js never hits the BAD_CERT error.

The reason is that guessConfig.js doesn't use the IMAP code at all - it simply tries to open a raw socket connection to a list of likely-looking places.
It does try port 143 and sending an IMAP STARTTLS... but it doesn't read anything further (or call startTLS() on the socket), so the bad cert error never shows up. This is why the guessConfig seems to work but the verifyConfig doesn't.
It doesn't seem to check for an SSL connection on port 993 at all.

There's actually a TODO related to this in the code:

// TODO prefer SSL over STARTTLS,
// either in sortTriesByPreference or in getIncomingTryOrder() (and outgoing)

https://searchfox.org/comm-central/source/mail/components/accountcreation/content/guessConfig.js#853

So two possible improvements to guessConfig.js would be:

  • take the port 143/StartTLS testing further and make sure it actually performs a successful read after STARTTLS (host detection might require some fiddling to call startTLS() on the socket at the appropriate time, which could be fiddly).
  • also test port 993 (which should probably be preferred over 143+StartTLS anyway? i.e. always-on TLS/SSL connections are better than StartTLS ones?)
Attachment #9141513 - Attachment is obsolete: true
Attachment #9164548 - Flags: review?(mkmelin+mozilla)

Just to reiterate, I don't think the error handling in Attachment #9164548 [details] [diff] will trigger in guessConfig.js - the guessing part doesn't go far enough to hit the bad-certificate error. It would need to generate a little traffic after the StartTLS occurs in order to receive the bad-certificate error.
Probably not a big deal - the subsequent verifyConfig.js should trigger the error fine.

Comment on attachment 9164239 [details] [diff] [review]
1590473-fix-imap-read-error-propagation-1.patch

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

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +4719,5 @@
> +// Upon failure, the thread will be flagged for shutdown, and
> +// m_connectionStatus will be set to a failing code.
> +// Remember that some socket errors are deferred until the first read
> +// attempt, so this function could be the first place we hear about
> +// connection issues (eg bad certificates for SSL).

nit: e.g.
Attachment #9164239 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 9164548 [details] [diff] [review]
Bug-1590473_account-wizard-bad-certificate-handling-1.patch

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

Looks good, r=mkmelin
Attachment #9164548 - Flags: review?(mkmelin+mozilla) → review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/d9e554fd5319
Fix upward propagation of IMAP read errors (e.g. SSL bad cert). r=mkmelin
https://hg.mozilla.org/comm-central/rev/13d4a93a840c
Implement handling for bad server certificates on SSL/TLS connections in account wizard. r=mkmelin DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
status-thunderbird79: --- → affected
status-thunderbird_esr78: --- → affected
tracking-thunderbird_esr78: --- → +
Target Milestone: --- → Thunderbird 80.0

Looks like this is causing bc3 failures on all platforms: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310619841&repo=comm-central&lineNumber=2007

Flags: needinfo?(khushil324)

Sure, let me look into it.

Flags: needinfo?(khushil324)
Attachment #9165379 - Flags: review?(mkmelin+mozilla)

Needed to add a try and catch.

So something is still wrong here. Looks like I didn't review closely enough earlier.
With the settings in bug 1590474 comment 6, I can't connect - but it works in 68. (Both things below work in 68).

I get, for StartTLS into the situation the latest patch would try/catch. But then the override dialog doesn't come up.
If I force it to SSL port 993 I do get the override dialog, but the certificate was not fetched so it's actually pretty useless.

Comment on attachment 9165379 [details] [diff] [review]
Bug-1590473_mail-account-wizard-test-fix-0.patch

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

We may want something like this, but atm it's not really helping

::: mail/components/accountcreation/content/verifyConfig.js
@@ +246,5 @@
> +        this.isCertError = true;
> +      }
> +    } catch (e) {
> +      // nsINSSErrorsService.getErrorClass throws if given a non-TLS,
> +      // non-cert error, so ignore this.

well, generic errors yes. but we should probably log something.
Attachment #9165379 - Flags: review?(mkmelin+mozilla)

Backed out the front-end handling to clear up the test failures while we sort this out.
Backout: https://hg.mozilla.org/comm-central/rev/552a05f4d51623859905d6f0784b5ff285617464

I did some debugging and for StartTLS only the generic NS_ERROR_FAILURE happens. Ben, please look into how to propagate that back.
And for direct SSL, the certificate is not filled either, so something more is needed.

Status: RESOLVED → REOPENED
Flags: needinfo?(khushil324)
Flags: needinfo?(benc)
Resolution: FIXED → ---

Gah. The StartTLS handling was in fact setting the bad-cert error correctly, but it would then go ahead and try to log in anyway. And, lo and behold, the login would fail and set a generic NS_ERROR_FAILURE code.
This patch skips the login attempt if the connection is already doomed.

Attachment #9166006 - Flags: review?(mkmelin+mozilla)
Regressions: 1655132
Comment on attachment 9166006 [details] [diff] [review]
1590473-skip-imap-login-if-borked-connection-1.patch

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

With this I get the cert override dialog, but the cert is still not filled
Attachment #9166006 - Flags: review?(mkmelin+mozilla) → review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/1badd9c70808
IMAP fix to avoid login attempt if connection is broken. r=mkmelin DONTBUILD

Status: REOPENED → RESOLVED
Closed: 4 years ago → 4 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---

The POP3 error handling seems OK.
If an SSL or bad-cert error occurs, it'll show up during verifyConfig via the listeners OnStopRunningUrl().

There is an issue in that the POP3 code immediately tells the GUI to display an alert box (sigh).
We don't want that for NSS errors (SSL/bad cert). Luckily the alert box code doesn't know about the NSS errors, and it quietly fails (with an NS_WARNING in debug builds).
However, relying on that seems a bit icky to me, so this patch tries to be a bit more explicit about it.

Flags: needinfo?(khushil324)
Flags: needinfo?(benc)
Attachment #9167582 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9167582 [details] [diff] [review]
1590473-suppress-pop3-nss-alertbox-1.patch

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

Wasn't able to verify this. I'm using the setup from https://bugzilla.mozilla.org/show_bug.cgi?id=1590474#c6 and should be able to use both StartTLS and SSL. (Need to enter your normal smtp as smtp)
I'm not getting the override dialog like with IMAP. (With https://bugzilla.mozilla.org/attachment.cgi?id=9164548 applied).
Attachment #9167582 - Flags: review?(mkmelin+mozilla)

Is there some confusion on which code is passed back, the normal rv or the nss error code?

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

Is there some confusion on which code is passed back, the normal rv or the nss error code?

No, it will still pass the rv code out via the standard listener OnStopRunningUrl().
But this patch checks to see if the error code came from the NSS layer and if so, suppresses the attempt to directly show an alertbox to the user then and there.
Currently, the alertbox only displays anything for a specific small set of rv error codes (none of them NSS errors). It does output an ugly warning in debug builds and it seems that we shouldn't even be attempting to show a direct alert for NSS errors. Hence this patch.

To actually add a exception for a certificate, it looks like you use nsICertOverrideService.rememberValidityOverride.
(we'd want to do it in JS, but there's a C++ example here: https://searchfox.org/mozilla-central/source/dom/base/Document.cpp#1471 )

The offending server certificate can be obtained via the security info on the socket transport after an NSS failure (it's saved there, so we don't need to explicitly go and fetch it again or anything).

Usually this would be done at a higher level, via the nsIRequest object passed in to nsIRequestObserver.OnStopRequest(request, nsresult).... but I don't think that works for us. The MailNews side of things tends to obscure the nsIRequest/nsIChannel system, and we use URLs and nsIUrlListener.OnStopRunningUrl(url, nsresult) instead. Sigh. So there's currently no way to get to the socket transport via the url.

However, all our MailNews URLs are derived from nsIMsgMailNewsUrl, so I should be able to add a method there that can dig into the messy internals to find the socket and get the certificate from it.

Sooooooooo....

On the JS side, error handling would look something like this (very rough off-the-top-of-my-head psuedocode):

function onStopRunningUrl(url,rv) {
  if (!is_a_bad_cert_error(rv) {
    --- normal error handling ---
    return;
  }

  /* GUI magic goes here - Tell the user the cert is bad, and offer the option to add it as an exception, with appropriately loud warnings that it might explode their computer etc etc etc */

 if (!userReallyReallyWantsToOverrideDespiteDireWarnings) {
   --- normal error handling ---
   return;
 }

  nsIMsgMailNewsUrl url2 = QueryInterface(url);
  let port = url2.getPort();
  let hostName = url2.getHost();

  nsITransportSecurityInfo secInfo = url2.getSecurityInfo();
  let cert = secInfo.serverCert;

  let overrrideService = do_getSerivice(cc.nsICertOverrideService)

  overrideService.rememberValidityOverride(hostName, port, cert);

  /* Retry the same failed operation again, this time with the cert exception in place */
}

We'd need this in both the verifyConfig handling and in the mail window (ie Bug 1590474)... maybe some other places too.
It could probably be factored out into some cert exception GUI .jsm or something... (I can imagine there would be some extra GUI there for failsafe checking, eg "tick this checkbox to show you've read and understand all the dire warnings and aren't just clicking 'ok' blindly...")

Kushil/Magnus: does that sound sane from the JS side?

On the C++ side, I need to add mechanisms to nsIMsgMailNewsUrl make sure we can get hold of the hostname, port and certificate, from the failed underlying connection (whatever it is - IMAP, POP, SMTP, whatever).

I'll make a start on the C++ side. I really wouldn't know where to begin on the GUI side of things. I can look into how to do it, but really, nobody wants to see my javascript coding :- ) in my ideal world there'd be something there to handle error on the GUI side, offering the option to override if it's a bad cert and to retry the operation. Then I can just plug in the actual certificate-exception code into it.

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(khushil324)
Comment on attachment 9164548 [details] [diff] [review]
Bug-1590473_account-wizard-bad-certificate-handling-1.patch

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

::: mail/components/accountcreation/content/verifyConfig.js
@@ +248,5 @@
> +    if (this.isCertError) {
> +      this._log.error("cert error");
> +      setTimeout(() => {
> +        try {
> +          this.informUserOfCertError(aExitCode, aUrl.asciiHostPort);

This is wrong, you're now passing the exitCode instead of the secInfo.

Debugging this some more, I'm not seeing the changed code in guessConfig ever getting triggered even if the certificate is self signed. Not sure if it's just obsolete, or if the code around https://searchfox.org/comm-central/rev/ea18e0f4074d618b9bc5917ec26b2e9d8ce5a02a/mail/components/accountcreation/content/guessConfig.js#1087 is making it not show up.

Anyway, I think a fix would be, to at this point when you get to isCertError in verifyConfig, you'd get the socket/transport again and obtain the secInfo like this patch does (should do) in guessConfig. Then if you pass in a proper secInfo to informUserOfCertError the certificate should be overridable in the dialog.

Alternatively, if the code in guessConfig is every exercised the bad cert could kept in some local map, so that you could get to it in case the cert error triggers during verify.

@@ +398,4 @@
>      this.mErrorCallback(ex);
>    },
>  
> +  informUserOfCertError(secInfo, targetSite) {

could rename targetSite to location which is what it is
Attachment #9164548 - Flags: review+ → review-

(In reply to Ben Campbell from comment #50)

To actually add a exception for a certificate, it looks like you use nsICertOverrideService.rememberValidityOverride.
(we'd want to do it in JS, but there's a C++ example here: https://searchfox.org/mozilla-central/source/dom/base/Document.cpp#1471 )

I don't think we don't need to worry about that, that will be handled by the override dialog.

However, all our MailNews URLs are derived from nsIMsgMailNewsUrl, so I should be able to add a method there that can dig into the messy internals to find the socket and get the certificate from it.

Yes one alternative is to make the request (request.channel and request.channel.securityInfo) availabe from the nsIMsgMailNewsUrls.
But might be easier just doing the second socket request through js if there is an error.

Flags: needinfo?(mkmelin+mozilla)

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

Debugging this some more, I'm not seeing the changed code in guessConfig
ever getting triggered even if the certificate is self signed. Not sure if
it's just obsolete, or if the code around
https://searchfox.org/comm-central/rev/
ea18e0f4074d618b9bc5917ec26b2e9d8ce5a02a/mail/components/accountcreation/
content/guessConfig.js#1087 is making it not show up.

it's kind of obsolete, check the comment #28.

Anyway, I think a fix would be, to at this point when you get to isCertError
in verifyConfig, you'd get the socket/transport again and obtain the secInfo
like this patch does (should do) in guessConfig. Then if you pass in a
proper secInfo to informUserOfCertError the certificate should be
overridable in the dialog.

How can we get the socket/transport in the verifyConfig file?

Alternatively, if the code in guessConfig is every exercised the bad cert
could kept in some local map, so that you could get to it in case the cert
error triggers during verify.

Local Maps means?

Flags: needinfo?(khushil324)

I am trying to do this:

let transportService = Cc[
  "@mozilla.org/network/socket-transport-service;1"
].getService(Ci.nsISocketTransportService);
let socketTypeName;
let ssl = aUrl.socketType;
if (ssl == SSL) {
  socketTypeName = ["ssl"];
} else if (ssl == TLS) {
  socketTypeName = ["starttls"];
} else {
  socketTypeName = [];
}
let transport = transportService.createTransport(
  socketTypeName,
  aUrl.hostname,
  aUrl.port,
  null
);
let socket = transport.QueryInterface(Ci.nsISocketTransport);
let secInfo = socket.securityInfo.QueryInterface(
  Ci.nsITransportSecurityInfo
);
this.informUserOfCertError(secInfo, aUrl.asciiHostPort);

Yes but I don't think that ever opens the connection? You probably need to call transport.openOutputStream or something, and then handle the certificate in onInputStreamReady
See some examples on https://searchfox.org/comm-central/search?q=sportService.createTransport&path=

With local map I meant if the obsolete code had been in use, you could then have grabbed the certificate and stored it in a local variable, mapped by location. But since that won't get the certificate that's no use.

Sorry for interrupting this fruitful discussion. I have a more general question:

When eventually a certificate exception can be accepted, what will be the scope of that exception? Just that account, or any SSL connection to any type of server (IMAP, SMTP, NNTP) for any account?

If it's the latter, I see some potential security vulnerability here (but still way better than some mail clients which only provide the option of disabling certificate checking alltogether).

It stores the exception for the specific certificate only, and that is for a given server.

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

It stores the exception for the specific certificate only, and that is for a given server.

Not sure I completely understand.

One example: My mail server (IMAP and SMTP) is mail.tessarakt.de. However, the certificate presented for it is for *.webpack.hosteurope.de.

So is the exception stored for mail.tessarakt.de, or for the certificate and thus *.webpack.hosteurope.de?

Attack scenario:

An attacker steals Hosteurope's catch-all certifcate, and is able to intercept my Internet access. If she would present the *.webpack.hosteurope.de certificate (for which I have accepted an exception) for some other, completely unrelated mail account/server, would Thunderbird accept it?

Yes, I think that would be accepted. If the certificate owners mess up and do not protect it, security is gone.

The certificate stored is the one that would be used, so the webpack one.

I am facing a problem while getting the security information from the request. Do I need to do anything special to get the security information from the request here in the progress listner?

Attachment #9164548 - Attachment is obsolete: true
Attachment #9165379 - Attachment is obsolete: true
Attachment #9169384 - Flags: feedback?(mkmelin+mozilla)
Attachment #9169384 - Flags: feedback?(ben.bucksch)

This patch adds a new read-only .failedSecInfo attribute to nsIMsgMailNewsUrl. The idea is that it is populated when an error occurs, so that the OnStopRunningUrl() handler can pick it up, extract the bad certificate and optionally add an exception.

Took a few (ha!) false starts, but it seems to work now for me - the secInfo of the failed attempt is now available to the JS side.
The JS code in this patch is based on Khushil's code from comment 61. It does indeed display the add-an-exception dialog. Hooray! However, for me, the dialog won't let me actually add the exception. I haven't dug into this, but it might just not like my test server being localhost:143 or something...
Either way, I think this sorts out the C++ side, for IMAP at least. But it'd be nice to know that the add-an-exception dialog was working. Still need to add .failedSecInfo support to POP3, but that code is a lot less brain-melting than for IMAP.

Attachment #9171504 - Flags: feedback?(mkmelin+mozilla)

Revised version, just the C++ side, and this time with POP3 support.
Adds a read-only .failedSecInfo attribute to nsIMsgMailNewsUrl, which is set in the event of an error, and can be accessed during the OnStopRunningUrl() url listener.

Very unhappy about adding extra state to the url object, and it all smacks of yet more special-case hackery... but anyway. It's about as clean as I can get it without massive refactoring :-)

Attachment #9171504 - Attachment is obsolete: true
Attachment #9171504 - Flags: feedback?(mkmelin+mozilla)
Attachment #9171621 - Flags: review?(mkmelin+mozilla)

And this is the JS side. Basically Khushil's earlier patch, with the verifyConfig.js fiddled to use the new .failedSecInfo attr.

It's enough to get the "Add Exception" dialog showing for me on both POP3 and IMAP, although I haven't actually seen it add a cert yet (the buttons are ghosted out for me)...

Comment on attachment 9171621 [details] [diff] [review]
1590473-add-failedSecInfo-attr-2.patch

Scrub that. Seems like the serverCert on the securityInfo disappears by the time the OnStopRunningUrl is invoked. Another approach is needed.

(I suspect it's because the socket has already been shut down by the time we kick off the GUI to deal with it. I had hoped that keeping hold of the secInfo would also keep it's certificate alive, but this doesn't seem to be the case...)

Attachment #9171621 - Flags: review?(mkmelin+mozilla)
Attachment #9171621 - Attachment is obsolete: true
Attachment #9171622 - Attachment is obsolete: true

OK. Fixed. For some reason storing the secInfo as an *nsISupports didn't keep the bad cert in existance, but QIing it to nsITransportSecurityInfo and storing it like that does. Odd. Suspect I'm misunderstanding some aspect of XPCOM here.

Anyway. This now works for me. This patch is the C++ side. The JS side is the same as previously (and still needs some rough edges knocked off, although it's still an improvement).

With this pair of patches installed, bad cert errors during account creation now pop up the "Add Security Exception" dialog and you can add the exception and the secure IMAP or POP3 connection will work. Yay!

(not flagging for review yet - I made a couple of cosmetic changes and it'll be another 30mins recompiling so I can test it again. This is just an end-of-day progress update :- )

Comment on attachment 9171622 [details] [diff] [review]
1590473-use-failSecInfo-in-js-1.patch

With the C++ patch installed, this patch will pass the bad cert on to the "Add Exceptions Dialog" and get everything working.
There are a bunch of little things still to iron out (some for this bug, some could be shunted off to the future), eg:

  • you can't get past the guessConfig phase with an SSL connection yet (because the guessConfig fails with a bad cert).
  • the "Add Security Exception" dialog is a bit browser-centric, I think (eg "Legitimate banks, stores and other public sites will not ask you to do this"). Also We don't need the "download cert" facility. I think ultimately we'd probably better to take a copy of the dialog, strip it down and re-word it to be TB specific.
  • is there a way to revoke security exceptions?
  • bad-cert handling needs to be added to normal IMAP and POP3 operations (not just during account creation).
  • bad cert handling for SMTP and LDAP (and NNTP?). I think there are already separate bugs to cover some of this.
  • probably something I've forgotten.
Attachment #9171622 - Attachment is obsolete: false
Comment on attachment 9171849 [details] [diff] [review]
1590473-add-failedSecInfo-attr-3.patch

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

::: mailnews/local/src/nsPop3Protocol.cpp
@@ +866,4 @@
>    if (m_socketIsOpen) {
>      nsCOMPtr<nsIMsgMailNewsUrl> msgUrl = do_QueryInterface(m_url);
>  
> +    if (NS_FAILED(aStatus)) {

should it also clear the failedSecInfo on success?

You should be able to remove security exceptions by going into the certificate manager, finding what you added and remove it.

I think patch "1590473-use-failSecInfo-in-js-1.patch" is ready to review now. Right?
I will also add a patch for Mail Window Bad Cert Handling and Bad SSL/TLS version handling in Bug 1590474.

This new patch is identical, just has minor clarification as to when .failedSecInfo is valid.
There seems to be some new changes to the accountcreation wizard which means that my old test procedures don't work any more... but this patch is functionally the same as previously and that did work, and is separate from the JS side, so I'm happy for it to go in now.

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

should it also clear the failedSecInfo on success?

It wouldn't hurt, but It's not strictly needed, as the failedSecInfo field is only ever checked if the status code is a fail.
It should be read: "the secInfo that was being used if a failure occurs", not "the secInfo that failed".
(in the IMAP case I can't easily determine if it was a bad cert at the time of setting, as the classifcation code only runs in the main thread anyway. So I just set it for any error).

So for now I've just updated the comment and avoided any code change until I can test again.

Attachment #9171849 - Attachment is obsolete: true
Attachment #9172309 - Flags: review?(mkmelin+mozilla)

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

I think patch "1590473-use-failSecInfo-in-js-1.patch" is ready to review now. Right?

As soon at the C++ side to add the .failedSecInfo attr lands.

I will also add a patch for Mail Window Bad Cert Handling and Bad SSL/TLS version handling in Bug 1590474.

Cool! I'm pretty sure the .failedSecInfo attr patch will be required for Bug 1590474 too.

Attachment #9171622 - Flags: review?(khushil324)
Attachment #9171622 - Flags: review?(khushil324) → review?(mkmelin+mozilla)
Attachment #9169384 - Attachment is obsolete: true
Attachment #9169384 - Flags: feedback?(mkmelin+mozilla)
Attachment #9169384 - Flags: feedback?(ben.bucksch)

Does anyone realize fetch() can be used with a mailnews url? If the purpose is to simply test cert/ssl status of a server, all that's needed is to strip credentials (I believe only imap urls have them) from the url. Testing response from fetch() seems extremely more hardened and future proof than this, which doesn't seem to include nntp/smtp urls.

Attachment #9172309 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 9171622 [details] [diff] [review]
1590473-use-failSecInfo-in-js-1.patch

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

I finally got to checking this out.
For POP it seems to work. 
For IMAP, it kind of works. After going on so far that I get the exception added, it doesn't finish off correctly but gives the error message that verification failed. THEN, with the self signed cert already accepted, I can click verify once more, and the account will be set up.

For both types in the native console, I get 
JavaScript error: chrome://messenger/content/accountcreation/verifyConfig.js, line 277: TypeError: can't access property "authMethod", this.mServer is null
which is https://searchfox.org/comm-central/rev/1b97640106e37ba9a0a208f937b8396de9ac89d9/mail/components/accountcreation/content/verifyConfig.js#257

... and I think that's causing problems later. Seems the account doesn't get a name, so going into the account settings will trap you (since name is required, but can't be edited)
Not sure this happens all the time or not, but I got it a few times at least.

::: mail/components/accountcreation/content/verifyConfig.js
@@ +245,5 @@
> +      if (errorClass == Ci.nsINSSErrorsService.ERROR_CLASS_BAD_CERT) {
> +        this.isCertError = true;
> +      }
> +    } catch (e) {
> +      /* It's not an NSS error */

please use // for comments
Attachment #9171622 - Flags: review?(mkmelin+mozilla) → feedback+

(In reply to alta88 from comment #74)

Does anyone realize fetch() can be used with a mailnews url? If the purpose is to simply test cert/ssl status of a server, all that's needed is to strip credentials (I believe only imap urls have them) from the url.

I didn't recall that actually. However, we'll still need the cert in bug 1590474 too so unfortunately we probably need to add the cert to the mailnewsurl so it's available later when we need it.

Khushil, can you check what more is needed for the js side

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

(In reply to alta88 from comment #74)

Does anyone realize fetch() can be used with a mailnews url? If the purpose is to simply test cert/ssl status of a server, all that's needed is to strip credentials (I believe only imap urls have them) from the url.
I didn't recall that actually. However, we'll still need the cert in bug 1590474 too so unfortunately we probably need to add the cert to the mailnewsurl so it's available later when we need it.

Also, it's not just the account-creation wizard. We need to handle cert failures during normal operations too. Like if someone has manually configured an account, or the server has been reconfigured with another certificate, or whatever.

I specifically said "test cert/ssl status of a server", ie this bug. You have to start somewhere to gain the knowledge of how to do it in a future proof way. Adding stuff to channels and so forth is a hackathon, especially brittle given constant bustage from upstream changes, that should be transitioned away from whenever possible.

The first strategic thing you should consider is removing credentials from imap urls. Then it becomes trivially easy: https://searchfox.org/comm-central/rev/3364959dce12ac17554902d70e7e2bdcef2a27a4/mail/base/content/msgHdrView.js#2160.

(In reply to Ben Campbell from comment #46)

Created attachment 9167582 [details] [diff] [review]
1590473-suppress-pop3-nss-alertbox-1.patch

Ben, do we need this for anything? How can I reproduce. I don't get an alert for bad certificate at least.

Flags: needinfo?(benc)
See Also: → 1665619

alta88: I've filed bug 1665619 for that, but we need to get this in urgently for 78 so now is not a good time to to larger exploration.

Indeed, this is quite a serious support issue, not to mention regression.

Severity: normal → S2
status-thunderbird74: affected → wontfix
status-thunderbird75: affected → wontfix
status-thunderbird79: affected → wontfix
No longer depends on: 1547096
Priority: P2 → P1
Regressed by: 1547096
Whiteboard: [regression:TB72]

I got a partial patch from Khushil, which I've fixed a bit and seems to work now. Cleaning it up now an will fold it into the c++ patch.

Slightly adjusted patch which works for me. I'll land it later.
Please test with tomorrow's build.

Attachment #9171622 - Attachment is obsolete: true
Attachment #9172309 - Attachment is obsolete: true
Attachment #9176294 - Flags: review+
Comment on attachment 9176294 [details] [diff] [review]
bug1590473_selfsigned_cert.patch

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

::: mail/components/accountcreation/content/guessConfig.js
@@ +1225,5 @@
> +          sslErrorHandler.processCertError(secInfo, hostname + ":" + port);
> +          resultCallback(this.data.length ? this.data : null);
> +        } else {
> +          resultCallback(this.data.length ? this.data : null);
> +        }

NIT: Please move the last line after the `if` block. Also, add an empty line before the `resultCallback()` line. This will make the control flow clearer.

It would be nice to move all this "bad cert" code into a helper function, and call it from here and from verifyConfig.js. Not only for code sharing and abstraction, but also to make the code easier to read.
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/9019a3378785
Add failedSecInfo attr to nsIMsgMailNewsUrl for bad-cert handling on IMAP & POP3 and fix account setup for self-signed certificates. r=mkmelin

Yes some helper function somewhere may be in order, but it won't save many lines of code atm.

Status: REOPENED → RESOLVED
Closed: 4 years ago → 4 years ago
status-thunderbird81: --- → affected
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: Thunderbird 80.0 → 82 Branch

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

(In reply to Ben Campbell from comment #46)

Created attachment 9167582 [details] [diff] [review]
1590473-suppress-pop3-nss-alertbox-1.patch

Ben, do we need this for anything? How can I reproduce. I don't get an alert for bad certificate at least.

It's not strictly required. The POP3 code tries to display an alert dialog for the bad cert error, even though its GUI code doesn't handle such an error code. I think it caused a warning in debug builds, but nothing serious.
Leave it for now - when the dust settles I'll check to see if it's still worthwhile.

Flags: needinfo?(benc)

Comment on attachment 9176294 [details] [diff] [review]
bug1590473_selfsigned_cert.patch

[Approval Request Comment]
Regression caused by (bug #): 1547096
User impact if declined: can't set up accounts where self signed certificates are involved
Testing completed (on c-c, etc.): on trunk
Risk to taking this patch (and alternatives if risky): The affected cases are completely broken atm, but normal setup should be tested too (should not change any way).

Asking approval for these:
https://hg.mozilla.org/comm-central/rev/d9e554fd5319
https://hg.mozilla.org/comm-central/rev/1badd9c70808
https://hg.mozilla.org/comm-central/rev/9019a3378785

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

Comment on attachment 9176294 [details] [diff] [review]
bug1590473_selfsigned_cert.patch

[Triage Comment]
Approved for esr78

Magnus "I would take bug 1590473 directly to 78.3. The self-signed cert override stuff is problematic for many" ... including AV users like Avast.

Attachment #9176294 - Flags: approval-comm-esr78? → approval-comm-esr78+

This won't work on esr78 as is. The first patch includes:

--- a/mailnews/imap/src/nsImapProtocol.cpp
+++ b/mailnews/imap/src/nsImapProtocol.cpp
       nsCOMPtr<nsIChannel> channel;
-      rv = NS_NewChannel(getter_AddRefs(channel), aURL, nullPrincipal,
-                         nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_SEC_CONTEXT_IS_NULL,
-                         nsIContentPolicy::TYPE_OTHER);
+      rv =
+          NS_NewChannel(getter_AddRefs(channel), aURL, nullPrincipal,
+                        nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_SEC_CONTEXT_IS_NULL,
+                        nsIContentPolicy::TYPE_OTHER);
       m_mockChannel = do_QueryInterface(channel);
 
       // Certain imap operations (not initiated by the IO Service via AsyncOpen)

That caused a merge conflict. The lines in question on c-esr78L

-      rv = NS_NewChannel(getter_AddRefs(channel), aURL, nullPrincipal,
-                         nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,
-                         nsIContentPolicy::TYPE_OTHER);

SEC_ALLOW_CROSS_ORIGIN_SEC_CONTEXT_IS_NULL does not show up when I grep m-esr78, so that won't compile.

While I'm 90% sure that the answer here is ot keep SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL, I would prefer that someone more knowledgeable with the code backport the three patches to esr78 as there are other conflicts as well.

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(benc)

That caused a merge conflict.

From what I can see, this is a whitespace only change. So, unless I'm mistaken, you can simply ignore that hunk.

This is an excellent example of why such style changes should never be in a functional patch, but separate commits.

Thanks. Sometimes that second set of eyes is needed. :)
(In reply to Ben Bucksch (:BenB) from comment #92)

That caused a merge conflict.

From what I can see, this is a whitespace only change. So, unless I'm mistaken, you can simply ignore that hunk.

This is an excellent example of why such style changes should never be in a functional patch, but separate commits.

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(benc)

Fixed uplift Thunderbird 78.3.0:
https://hg.mozilla.org/releases/comm-esr78/rev/35e647ebd4fa

Regressions: 1667120
Regressed by: 1668599
No longer regressed by: 1668599

still doesn't work with protonmail bridge
Don't know why still but it doesn't work with 78.3.1
I've followed every protocol in the proton documentation. Are we sure this problem is actually resolved?

(In reply to padawan440 from comment #97)

still doesn't work with protonmail bridge
Don't know why still but it doesn't work with 78.3.1
I've followed every protocol in the proton documentation. Are we sure this problem is actually resolved?

this is an example in my protonmail bridge :
{"level":"error","msg":"cannot upgrade connection: remote error: tls: bad certificate\n","pkg":"server-imap","time":"2020-10-01T21:09:14Z"}
{"error":"remote error: tls: bad certificate","level":"error","msg":"IMAP connection couldn't be upgraded to TLS during STARTTLS","time":"2020-10-01T21:09:14Z"}

(In reply to wicked from comment #98)

(In reply to padawan440 from comment #97)

still doesn't work with protonmail bridge
Don't know why still but it doesn't work with 78.3.1
I've followed every protocol in the proton documentation. Are we sure this problem is actually resolved?

this is an example in my protonmail bridge :
{"level":"error","msg":"cannot upgrade connection: remote error: tls: bad certificate\n","pkg":"server-imap","time":"2020-10-01T21:09:14Z"}
{"error":"remote error: tls: bad certificate","level":"error","msg":"IMAP connection couldn't be upgraded to TLS during STARTTLS","time":"2020-10-01T21:09:14Z"}

from the logs of thunderbird
2020-10-01 21:24:47 mail.setup INFO status msg: Checking password…
2
2020-10-01 21:24:47 mail.setup INFO verify config:
Incoming: imap, 127.0.0.1:1143, STARTTLS, auth: plain, username: xxxxxxx@protonmail.com, password: set
Outgoing: smtp, 127.0.0.1:1025, STARTTLS, auth: plain, username: xxxxxxx@protonmail.com, password: set
2
2020-10-01 21:24:47 mail.setup INFO Setting incoming server authMethod to 3
2
2020-10-01 21:24:47 mail.setup INFO verifyLogon for server at 127.0.0.1
2
2020-10-01 21:24:47 mail.setup WARN error Unable to log in at server. Probably wrong configuration, username or password.

As you can see no mentionned of a problem of certificate. The password is a copy paste so it can't be bad. Plus the log of bridge is pretty clear about the certificate.
Ididn't get the certificate error in thunderbird to accept as exception either. And as you can see also the connection is good between bridge and thunderbird because it wouldn't be an error like that then

Thanks for reporting this, I will into the protonmail issue and update this bug with findings and/or open a followup as appropriate.

(In reply to Rob Lemley [:rjl] from comment #100)

Thanks for reporting this, I will into the protonmail issue and update this bug with findings and/or open a followup as appropriate.

no problem . I've reported this an half hour ago through the bug report system of proton.
I'm reporting this here because I was seeing this o ntheir website:

NOTE: Thunderbird 78.3.1 fixed a long persisting bug that causes the account setup with ProtonMail Bridge to fail. Please update to the latest version before troubleshooting.
https://protonmail.com/support/knowledge-base/bridge-ssl-connection-issue/

sorry for the style but I'm not used to markdown and I'm in a rush.

So I would like some people to test on other hosts to see if the problem is the same or if one of my software could interfere with thunderbird maybe. Bitdefender solutions or something else.

Attached image protonmail-ssl.png —

I just set up a ProtonMail account with Thunderbird 78.3.1 using the bridge and it worked as expected. The popup about the SSL certificate appeared and I was able to confirm the exception.
A couple of things to check on: in Thunderbird preferences->Securirty & Privacy open the Certificate Manager. In the Servers tab, is there a certificate listed for 127.0.0.1:1143 and/or 1025? It could be an old version of that certificate causing an issue.

nothing in servers.
Didn't get the popup under windows.
I see you are under macos ... not same system....
I m going to add it manually to see if there is a change

don't know how to attach an image sorry
https://imgur.com/c0B6BjE

it doesn't seem related to antivirus protection or anything alike.
I'm going to try by changing to SSL to see if there is a change

could you setup a windows VM just to try?
it doesn't work.
Changed port number.
protocol.
try to add manually, doesn't work...

it works with the software https://www.ritlabs.com/en/products/thebat/download.php
I didn't want to try with outlook just by principle.
So it means it's not coming from bridge
nor it's coming from the windows networking that would restricted localhost communication somehow.
So the last thing it's that it coming from the windows version of thunderbird specifically. I could reinstall thunderbird completely if you want

(In reply to wicked from comment #107)

it works with the software https://www.ritlabs.com/en/products/thebat/download.php
I didn't want to try with outlook just by principle.
So it means it's not coming from bridge
nor it's coming from the windows networking that would restricted localhost communication somehow.
So the last thing it's that it coming from the windows version of thunderbird specifically. I could reinstall thunderbird completely if you want

the problem with the uninstall is that it doesn't get rid of the local data, just the resources files of the software. so if it's a problem with the local data, it needs to be erased manually.

(In reply to Rob Lemley [:rjl] from comment #102)

Created attachment 9179138 [details]
protonmail-ssl.png

I just set up a ProtonMail account with Thunderbird 78.3.1 using the bridge and it worked as expected. The popup about the SSL certificate appeared and I was able to confirm the exception.
A couple of things to check on: in Thunderbird preferences->Securirty & Privacy open the Certificate Manager. In the Servers tab, is there a certificate listed for 127.0.0.1:1143 and/or 1025? It could be an old version of that certificate causing an issue.

from the devtool box
network section
security
the get 127.0.0.1:1143 gives me -> {"An error occurred:":"SSL_ERROR_RX_RECORD_TOO_LONG"}

stack trace:
18 requests
311.49 KB / 190.24 KB transferred
Finish: 2.08 min
checkCert
chrome://pippki/content/exceptionDialog.js:124:9
addException
chrome://pippki/content/certManager.js:630:42
oncommand
chrome://pippki/content/certManager.xhtml:1:1

and this is from safe mode

If you get SSL_ERROR_RX_RECORD_TOO_LONG that would be something else. I'm not sure what that means but that's not something which would be possible to override. Sounds like a malformed certificate.

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

If you get SSL_ERROR_RX_RECORD_TOO_LONG that would be something else. I'm not sure what that means but that's not something which would be possible to override. Sounds like a malformed certificate.

Well I've just tried an other hosts where the certificate were accepted in february so well before the change to v78, also on windows. And guessed what? same error.
So it doesn't come from the installation.
It doesn't come from any specifics on the hosts because these are installed with different antiviral software from different brands.
etc etc etc
And as I stated before, it works with other programs.
So please, do a windows VM if you are not on windows and try to setup a program. Try to add a certificate manually by trying to connect to bridge and let's see what happens. I did ask some friends to do the same and for the moment they have all the same results.
So there pretty well might be a problem with your code on windows. It's not normal , that your specific software would have that kind of errors, where everyone else's software is not having it. If it was a problem with bridge then we would see a problem with all or majority of existing software.

Attached image win64-cert.png —

Windows 10 x64 78.3.1 connecting through ProtonMail Bridge. I believe this only works through the account setup wizard right now.

Thanks for having taken the time to acutally try that on a windows host.
Well I don't why it doesn't work for my hosts nor for my firends hosts with differents setups and why it does work for other programs.
I gave the maximum information I could give.

and I didn't invent this error message, so someone among you all should know where does this error message come from and which error or function it is referring to, no? So that we can pinpoint how the certificate is malformed and at which moment.

Source: https://searchfox.org/comm-central/search?q=SSL_ERROR_RX_RECORD_TOO_LONG&path=

I wonder if setting security.tls.version.min and security.tls.version.max versions could any effect. 0 is SSL3, 1 is TLS1.0 and so on.
My other idea is you're getting MITM'd by Antivirus software...

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

Source: https://searchfox.org/comm-central/search?q=SSL_ERROR_RX_RECORD_TOO_LONG&path=

I wonder if setting security.tls.version.min and security.tls.version.max versions could any effect. 0 is SSL3, 1 is TLS1.0 and so on.
My other idea is you're getting MITM'd by Antivirus software...

okey indeed it was an all our hosts an MITM of different factors. some were backup solutions that recently had a web filtering option added like acronis, other were some antiviral solutions that were only supposed to be anitvirus and not using web filtering . And others were a combination of those different factors. Thanks for the tip. We had to go to the hassle of actually uninstalling completely those solutions and then reinstall them back in.
We don't know in the long term if there won't be a problem for the send operations because we didn't succeed even with those solutions removed, to get a security warning to accept the exception. We only received one for the IMAP not for the SMTP, even by trying to send a mail.
so :rjl was right when he said that the only place where you can get an exception popup is at the account wizard. We should be able to get them also in the manual operation in the certificate manager no? Or by sending a mail simply.
We still don't know if by reinstalling those solutions we have now a secure transmission between bridge and thunderbird or if the data are still getting intercept by those software.

Anyway thanks for your reactivity and for all your help

and jsut for future references. kaspersky was the easiest to disable. Acronis, bitdefender, trend micro were the most hassle tasks.

This bug was for the setup wiz.
For the mail window there is bug 1590474, which is fixed not yet uplifted to 78.
For SMTP there is bug 1665577 which is now fixed but not yet uplifted to 78.

If beta testing checks out for them, they will likely be in 78.4

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: