1520283 - [Regression] Detection of SSL/TLS mail server connections in account creation is broken

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[:aceman]

In the account creation wizard, the patch in bug 721369 disabled checking for possibility to connect to mail server over SSL, when SSL was not specifically asked for. The code is in
https://searchfox.org/comm-central/source/mail/components/accountcreation/content/guessConfig.js, functions getIncomingTryOrder() and getOutgoingTryOrder()

The mail/components/test/unit/test_autoconfigUtils.js test was adapted in bug 1519742 to this change so that it passes.

However, it is not exactly clear why the SSL change was done.
It should be investigate if it is possible to restore automatic checking for SSL connections to mail servers again.

Comment 1

[Ben Bucksch (:BenB)]

I don't know why the normal SSL options are disabled and only the STARTTLS options are tried. I traced it back to https://github.com/mozilla/releases-comm-central/commit/28d1c8be479449eec1e6b012b53ce3975ca9a403 from bug 721369, which is my own patch from 2012, and which fixed a nasty SSL cert override bug. But it shows no hint of why the normal SSL options were disabled. I don't remember, either, I am puzzled, too. All I could find was bug 721369 comment 51.

It looks like it would make sense to re-enable it. But it needs to be carefully tested, esp. with regard to cert errors and how happens with the cert exceptions, as that was the bug when this change happened.

Comment 2

[Ben Bucksch (:BenB)]

This is actually a rather serious security bug, because we're not offering an SSL connection when we could and would in some cases unnecessarily fall back to plaintext protocols.

Comment 3

[Ben Bucksch (:BenB)]

Situation:

• Set up an email address in Thunderbird, e.g. [e-mail]
• The email domain example.com must not be in the ISPDB
• The server must have guessable hostnames like imap.example.com or pop.example.com .
• The server must offer normal SSL on port 993 (IMAP SSL) or port 995 (POP3 SSL), and also plaintext protocols on port 143 (IMAP) or port 110 (POP3), but must not offer STARTTLS. (This is a fairly common configuration.)
• Optional: To make it worse, the server might not offer encrypted passwords. (That is common, too, because MD5 is considered insecure and bcrypt and co are used to store passwords, and admins expect users to use SSL.)

Reproduction:

1. Open account setup dialog
2. Enter [e-mail], password, and your name
3. -> ISPDB fails, guess config starts, finds the hostname

Actual result:

• Guess config offers imap.example.com, port 143, no TLS
• The user is warned that the configuration is insecure

Expected result:

• Guess config offers imap.example.com, port 993, with TLS

I haven't actually tested it at the moment, the reproduction is based on the older comments.

Comment 4

[Ben Bucksch (:BenB)]

Ben Campbell, you've recently touched the SSL code in guess config. Want to take a look at this?

Comment 5

[Ben Bucksch (:BenB)]

This should be fixed in TB 78, too.

Comment 6

[Wayne Mery (:wsmwk)]

(In reply to Ben Bucksch (:BenB) from comment #4)

Ben Campbell, you've recently touched the SSL code in guess config. Want to take a look at this?

Comment 7

[Wayne Mery (:wsmwk)]

Magnus, can you suggest a way forward?

Comment 8

[Wayne Mery (:wsmwk)]

(In reply to Ben Bucksch (:BenB) from comment #2)

This is actually a rather serious security bug, because we're not offering an SSL connection when we could and would in some cases unnecessarily fall back to plaintext protocols.

Andrei this is stalled. If it is a serious security issue as BenB suggests we should adjust accordingly.

Comment 9

[Wayne Mery (:wsmwk)]

This is bouncing around a bit long without direction.

Comment 10

[Ping Chen [:rnons] (away)]

Attachment: Bug 1520283 - Use nsMsgSocketType to simplify GuessConfig in account creation. r=#thunderbird-reviewers

• rename TLS to STARTTLS
• rename HostTry.ssl to HostTry.socketType
• avoid converting nsMsgSocketType back and forth to in-file consts

Comment 11

[Ping Chen [:rnons] (away)]

The patch doesn't fix this bug, only a small step to make the code less confusing.

Comment 12

[Alessandro Castellani [:aleca]]

Thanks Ping for taking care of this.
We can discuss this during our next meeting and try to find a path forward

Comment 13

[Alessandro Castellani [:aleca]]

Marking leave-open based on Comment 11

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7f9bd4193c5b
Use nsMsgSocketType to simplify GuessConfig in account creation. r=mkmelin

Comment 15

[Geoff Lankow (:darktrojan)]

This patch is preventing mail accounts with OAuth (e.g. Gmail) from being set up.

Comment 16

[Rob Lemley [:rjl]]

Backout:
https://hg.mozilla.org/comm-central/rev/54fca99f6bd3b08c19ee73c268afebc83c97c40d

Comment 17

[Ping Chen [:rnons] (away)]

Updated the patch to fix creating account with OAuth.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1990e5a64d4f
Use nsMsgSocketType to simplify GuessConfig in account creation. r=mkmelin

Comment 19

[Kai Engert [:KaiE:]]

I need to know a server that uses the configuration from comment 3.
I don't want to enable port 143 on my own server.

Comment 20

[Kai Engert [:KaiE:]]

Ok never mind, looks like I can test using a server I've found. It doesn't seem to strictly require a server with port available and starttls disabled.
Right now, the lookup of the SSL/TLS ports is explicitly disabled.

Comment 21

[Kai Engert [:KaiE:]]

It looks like this is an easy fix.

The testing for the SSL/TLS type had been explicitly disabled.
We should re-enable it.

When enabling it, we run into an exception, because the socket's security info is now accessed differently.
Easy fix.

To allow us to debug the automatic guessing with popular servers which have a known configuration, I introduced a pref that can be used to disable using of known configuration.

I tested an email address at domain gmx.de with known config disabled.

With our current code, we get the STARTTLS config.
With the fix (re-enabling SSL/TLS guessing), we get the SSL/TLS config, as desired.

I'm optimistic this will make it work for servers that don't support STARTTLS.
(Couldn't test yet, because I don't know such a server.)

Comment 22

[Kai Engert [:KaiE:]]

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

I'm optimistic this will make it work for servers that don't support STARTTLS.
(Couldn't test yet, because I don't know such a server.)

I ran an additional test, in which I disabled the code for guessing STARTTLS.
(Keeping only guessing of plain and SSL/TLS enabled.)

This resulted in the desired SSL/TLS decision.

Comment 23

[Kai Engert [:KaiE:]]

Attachment: Bug 1520283 - Reenable auto config for SSL/TLS protocol. r=mkmelin

Comment 24

[Kai Engert [:KaiE:]]

Magnus told me in phab that our existing code is adding automatic certificate overrides for bad server certificates.

I wasn't aware of that, I wasn't involved in developing the auto configuration.

I'm strictly opposed to adding any automatic cert overrides. Not even temporary overrides seem acceptable, we shouldn't risk sending passwords over a connection that uses an invalid cert.

If a site decides to use nonstandard certificates, they MUST use manual configuration.

Comment 25

[Kai Engert [:KaiE:]]

I also don't want us to prompt the user for adding certificate overrides during account discovery.

Comment 26

[Magnus Melin [:mkmelin]]

That is with the patch. For trunk, if self signed and starttls, the user is prompted to add the exception if needed. But with the patch, the overrides for SSL were added without notice, and showed "permanent" in the cert manager.

Comment 27

[Kai Engert [:KaiE:]]

Thanks Magnus for the explanation.

The difference is related to the stages of auto config.

In the first stage, the code checks which combinations of protocols/ports result in a correct protocol conversation.

For STARTTLS, the code doesn't add an exception during this stage. Because those sockets start with a plaintext conversation, the code is happy that the initial plaintext conversation works, and probably finds the STARTTLS keyword being offered by the server (my guess), and concludes STARTTLS is supported on that server.

In other words, detecting a STARTTLS capable server doesn't require starting the SSL/TLS protocol.

To test whether a server/port supports a mail protocol behind SSL/TLS, starting a SSL/TLS connection is mandatory.
That's why the code adds a bad certificate override if necessary during stage 1.

After the code completed testing, it will tell the user, and offer the user to store that configuration.
The user must confirm to proceed to stage 2, which will then test the login.

If the code is working correctly, then the password probably isn't sent at stage 1.
It sounds like it was considered acceptable to add a temporary cert override, under the assumption that the override would be strictly limited to stage 1.

In stage 2 will attempt to verify the login information, and is supposed to execute STARTTLS prior to sending the password. That's what then triggers the exception dialog that Magnus mentioned.

This results in the following question:

Should we allow temporary overrides for the purpose of discovering mail server ports?
My preference is to do not.
I prefer to only allow automatic discovery of mail server ports that have a proper certificate configuration.

I prefer that everyone who operates a server with an improper certificate needs to ask their users to perform a manual account configuration.

Comment 28

[Kai Engert [:KaiE:]]

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

the overrides for SSL were added without notice, and showed "permanent" in the cert manager.

That's because the parameter list of nsICertOverrideService.rememberValidityOverride changed in bug 1781104 and our code wasn't updated.

Comment 29

[Kai Engert [:KaiE:]]

So, as I understand it, during the past 4 years, we never added any temporary overrides during auto config, because we had disabled discovery using the SSL/TLS protocol, and for STARTTLS no overrides were necessary.

Based on that, I see no urgency to offer the ability to allow overrides during discovery.

However, I acknowledge that it was a lot of work to get that code done right, and that it could be useful to some users.

While we might continue to discuss that (in a follow-up bug), I'd like to add a new hidden pref to disable those temporary overrides by default.

Comment 30

[Kai Engert [:KaiE:]]

Well, the code that involves cert overrides is broken in additional ways.

(1) After having added a temporary override, I don't get an automatic retry.
(Only after I quit the auto config, go back to adding an account, and enter the same details again, the override is effective, resulting in SSL/TLS to be detected and offered.)

(2) Also, if overrides are necessary for both SMTP and IMAP, only one override is added.
Only after quitting (see above), then manually trying to add again (with the temporary override still in place),
then SSL/TLS is detected and offered for the second protocol, too.

(3) The code that wants to cleanup and remove temporary overrides (clearValidityOverride) is never reached.

Comment 31

[Kai Engert [:KaiE:]]

Summary:

• we can re-enable guessing of the SSL/TLS protocol
• SSL/TLS detection will work if a server uses a proper configuration with valid certificates
• our code for handling invalid certificates seems very broken
• we shouldn't allow invalid certificates anyway
• let's disable the code that attempts to temporarily allow invalid certificates
  (new pref)

Also, I suggest that we give SSL/TLS a higher priority than STARTTLS.
Because of the nature of STARTTLS, which starts with a plaintext conversation, there are more potential bug vectors.

Comment 32

[Andrei Hajdukewycz [:sancus]]

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

So, as I understand it, during the past 4 years, we never added any temporary overrides during auto config, because we had disabled discovery using the SSL/TLS protocol, and for STARTTLS no overrides were necessary.

• let's disable the code that attempts to temporarily allow invalid certificates
  (new pref)

Seems fine to me. I don't really understand why you would want to allow detection without requiring a manual override first anyway? If the server has an invalid certificate, it's reasonable to expect the user to manual override to connect to it at all even for discovery.

Is there even any real use case for this other than personal servers with self-signed certificates???

Comment 33

[Kai Engert [:KaiE:]]

(In reply to Andrei Hajdukewycz [:sancus] from comment #32)

Seems fine to me. I don't really understand why you would want to allow detection without requiring a manual override first anyway?

The user might not know what overrides will be necessary.

Is there even any real use case for this other than personal servers with self-signed certificates???

This scenario can happen with shared hosting.

If an email provider allows customers to run their own domain on the provider's server, then the certificate on the shared mail server will allow the provider's hostname, only.

If the customer's domain has DNS records for e.g. imap.domain.com etc. then our that guesses a configuration will contact the shared server using this customer hostname, which will result in a cert error because of a hostname mismatch.

The right solution is to have the customer publish configuration information, which can allow Thunderbird to access the shared email host using the correct hostname (provider domain).

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2c761243339f
Reenable auto config for SSL/TLS protocol. r=mkmelin

Comment 35

[Andrei Hajdukewycz [:sancus]]

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

(In reply to Andrei Hajdukewycz [:sancus] from comment #32)

Seems fine to me. I don't really understand why you would want to allow detection without requiring a manual override first anyway?

The user might not know what overrides will be necessary.

The certificate error dialog gives you the option to manual override doesn't it?

Comment 36

[Kai Engert [:KaiE:]]

(In reply to Andrei Hajdukewycz [:sancus] from comment #35)

The certificate error dialog gives you the option to manual override doesn't it?

Not in advance. Your comment sounded like you anticipate that we'd ask for an override, even before we know what server we'll require to use.