Closed Bug 1520283 Opened 6 years ago Closed 1 year ago

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

Categories

(Thunderbird :: Account Manager, defect)

defect

Tracking

(thunderbird_esr78? affected, thunderbird_esr102 affected, thunderbird111 affected, thunderbird112 affected)

RESOLVED FIXED
112 Branch
Tracking Status
thunderbird_esr78 ? affected
thunderbird_esr102 --- affected
thunderbird111 --- affected
thunderbird112 --- affected

People

(Reporter: aceman, Assigned: KaiE)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression, sec-moderate)

Attachments

(2 files)

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.

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.

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.

Keywords: sec-moderate

Situation:

  • Set up an email address in Thunderbird, e.g. user@example.com
  • 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 user@example.com, 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.

Summary: restore detection of SSL mail server connections in account creation → [Regression] Detection of SSL/TLS mail server connections in account creation is broken

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

Severity: normal → S2
Version: Trunk → 60

This should be fixed in TB 78, too.

(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?

Flags: needinfo?(benc)

Magnus, can you suggest a way forward?

Flags: needinfo?(mkmelin+mozilla)

(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.

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

This is bouncing around a bit long without direction.

Flags: needinfo?(alessandro)
Keywords: regression
Regressed by: 721369
Flags: needinfo?(kaie)
  • rename TLS to STARTTLS
  • rename HostTry.ssl to HostTry.socketType
  • avoid converting nsMsgSocketType back and forth to in-file consts
Assignee: nobody → remotenonsense
Status: NEW → ASSIGNED

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

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

Flags: needinfo?(alessandro)

Marking leave-open based on Comment 11

Keywords: leave-open
Target Milestone: --- → 112 Branch

Pushed by martin@humanoids.be:
https://hg.mozilla.org/comm-central/rev/7f9bd4193c5b
Use nsMsgSocketType to simplify GuessConfig in account creation. r=mkmelin

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

Flags: needinfo?(remotenonsense)

Updated the patch to fix creating account with OAuth.

Flags: needinfo?(remotenonsense)

Pushed by martin@humanoids.be:
https://hg.mozilla.org/comm-central/rev/1990e5a64d4f
Use nsMsgSocketType to simplify GuessConfig in account creation. r=mkmelin

Assignee: remotenonsense → nobody
Status: ASSIGNED → NEW

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.

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.

Flags: needinfo?(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.)

(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.

Assignee: nobody → kaie
Status: NEW → ASSIGNED

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.

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

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.

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.

(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.

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.

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.

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.

(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???

Flags: needinfo?(sancus)

(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).

Pushed by alessandro@thunderbird.net:
https://hg.mozilla.org/comm-central/rev/2c761243339f
Reenable auto config for SSL/TLS protocol. r=mkmelin

(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?

(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.

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Keywords: leave-open
Resolution: --- → FIXED
Blocks: 1825043
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: