Upgrade trySTARTTLS to alwaysSTARTTLS, if we know it worked

RESOLVED FIXED in Thunderbird 15.0

Status

MailNews Core
Networking: IMAP
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: BenB, Assigned: Bienvenu)

Tracking

(Blocks: 1 bug)

unspecified
Thunderbird 15.0
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Split-off from Bug 558659 comment 68

The pref "STARTTLS, if available" is deprecated and removed from the UI, because it's insecure (an attacker can always actively downgrade you).
However, we may well have profiles that still have this legacy setting set.
Also, the Outlook migrator unfortunately sets this.

We should change the pref permanently to alwaysSTARTTLS is set, if the server actually does support STARTTLS.
(Reporter)

Updated

5 years ago
Assignee: nobody → ben.bucksch
Depends on: 558659
(Reporter)

Comment 1

5 years ago
Created attachment 614249 [details] [diff] [review]
Upgrade trySTARTTLS to alwaysSTARTTLS - v1

Patch depends on Attachment #614242 [details] [diff] - Bug 558659 "Capability bitflags 64bit, v7"
(Reporter)

Updated

5 years ago
Attachment #614249 - Flags: review?(dbienvenu)
(Assignee)

Comment 2

5 years ago
Comment on attachment 614249 [details] [diff] [review]
Upgrade trySTARTTLS to alwaysSTARTTLS - v1

you can't access prefs off of the UI thread - you need to proxy over to the ui thread via the server sink.
Attachment #614249 - Flags: review?(dbienvenu) → review-
(Assignee)

Comment 3

5 years ago
if we stop persisting or propagating the start tls capability in the prefs, I'm not convinced we would ever hit the code you changed, because I don't think we'd set the connection type to starttls.

I'm thinking we might want to do something like the following:

If the socket type is tls if available, have SetupWithUrl read the server capability pref directly (it's on the UI thread, so it can do so). If starttls is not set in the capability flag, change the socket type to plain. If it is set, set a flag that says we're going to adjust the socket type one way or the other based on whether starttls worked that time. Then, we don't have to worry about persisting that pref again. I don't think the idea was ever to support servers that went back and forth between advertising STARTTLS, but rather to help users who didn't know. Now with autoconfig, we figure it out for the user.
(Reporter)

Comment 4

5 years ago
> if we stop persisting or propagating the start tls capability in the prefs,
> I'm not convinced we would ever hit the code you changed,
> because I don't think we'd set the connection type to starttls.

Which code do you refer to with "hit the code you changed"?

The idea is simple: If we successfully establish the first STARTTLS IMAP connection, we check whether the pref was trySTARTTLS (and not alwaysSTARTTLS). If so, we change the pref to alwaysSTARTTLS. Any following SetupWithUrl() should then see that the pref is alwaysSTARTTLS and use STARTTLS. (Note: |m_socketType| is the pref, |connection| is the actually used socket.)

I think that's a fairly safe change code-wise. It is definitely more safe than just looking at the capability, because the server might advertize STARTTLS, but it may not actually work (for various reasons).
(Assignee)

Comment 5

5 years ago
will we try start tls with tls if available if we don't check the capability pref? I think your patch removes the reading of the cached capability pref in the imap code, because I wanted to stop using cached capability prefs wherever possible.
(Reporter)

Comment 6

5 years ago
Correct me, if I'm wrong, but from what I understand, we have 2 types of IMAP connections: The first IMAP connection, where we do CAPABILITY and folder discovery and the whole stuff. And then secondary IMAP connections for each folder. And ProcessCurrentURL() line ~1650 is the first, while SetupWithURL are the secondary connections. If that assumption is wrong, then my patch indeed wouldn't make sense.

Assuming I'm right, my patch entirely removes the need for capabilities in SetupWithURL, because we'll be looking at the prefs (stored in m_socketType) for STARTTLS and not at the caps.
(Assignee)

Comment 7

5 years ago
SetupWithUrl is called for every imap url we run, from the UI thread, and it is called before we ever get to ProcessCurrentUrl. Each folder does have a separate imap connection, generally, but each connection does the whole capability, logon sequence. The only thing the non-first connections don't do is folder discovery. SetupUrl is called both for connections that haven't been established (null m_transport) and cached connections.

Similarly, ProcessCurrentUrl is called for new connections and cached, existing connections. The code you pointed at is called for new connections.

The socket type pref is a different pref from the cached server capability pref. The latter remembers the last capability the server told us about.
(Assignee)

Comment 8

5 years ago
Oh, and if it's not clear, ProcessCurrentUrl is called on the per-connection imap thread.
(Reporter)

Comment 9

5 years ago
> Oh, and if it's not clear, ProcessCurrentUrl is called on the per-connection imap thread

Indeed, I wasn't aware of that, that's surprising and confusing. I was assuming that there's one C++ class per thread type.

> The socket type pref is a different pref from the cached server capability pref.
> The latter remembers the last capability the server told us about.

I know, I deliberately exploit that fact in my patch. The server cap is not easily accessible anymore in SetupWithURL once we remove GetCapabilityForHost(). That's what triggered this patch in the first place.

[SetupWithURL and ProcessCurrentURL]

I fear I still don't understand that. Maybe I should just hand this to you.

All I wanted to do was: On the first IMAP connection, if trySTARTTLS and server capability STARTTLS, try to use STARTTLS. If that succeeds, change the pref and m_socketType to alwaysSTARTTLS. That
1) migrates the pref, securing the user
2) ensures that all secondary connections in the same TB instance use STARTTLS
3) doesn't require any capablity reading in SetupWithURL, which removes the need for
   GetCapabilityForHost().

If we were to only look at the STARTTLS capability, we would try STARTTLS for servers that advertize STARTTLS, but don't have SSL set up properly and in a working state (quite common, I think). Using the pref instead of the cap, and migrating the pref only when we actually used STARTTLS, avoids that.
(Assignee)

Comment 10

5 years ago
(In reply to Ben Bucksch (:BenB) from comment #9)
> > Oh, and if it's not clear, ProcessCurrentUrl is called on the per-connection imap thread
> 
> Indeed, I wasn't aware of that, that's surprising and confusing. I was
> assuming that there's one C++ class per thread type.

There's only one imap thread type, the imap thread, and for each imap thread, there's an nsImapProtocol instance. And then there's the single UI thread. The cached connections are represented by nsImapProtocol objects (that's what we cache). See https://developer.mozilla.org/en/IMAP
> 
> 
> All I wanted to do was: On the first IMAP connection, 
We have to do this for every imap connection, because we don't have a concept of "first connection". So the code will be there, but will only get triggered the first time, because we'll change the pref one way or the other.

I agree that worrying about STARTTLS failing is the right thing to do, and we should only change the pref if it succeeds.

I can try to modify the patch, but it won't be today :-(
(Reporter)

Comment 11

5 years ago
> I can try to modify the patch, but it won't be today :-(

Yeah, no worries. Bug 558659 doesn't depend on this one.
(Reporter)

Comment 12

5 years ago
> https://developer.mozilla.org/en/IMAP

wow, now that's a great doc. This is the kind of documentation we need! Thanks!
(Assignee)

Comment 13

5 years ago
Created attachment 616986 [details] [diff] [review]
wip

this is turning out to be complicated because unsolicited capability responses after login don't set STARTTLS for some servers, which makes us persist the capability without STARTTLS, which breaks this upgrade path. I'm still working on it, though.
Assignee: ben.bucksch → dbienvenu
Attachment #614249 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
My thinking now is that we should upgrade to alwaysTLS if TLS succeeds, but not downgrade to plain, since there are several code paths for trying TLS and we can't tell right away if starttls is available. I'll try to update the patch soon.
(Reporter)

Updated

5 years ago
Blocks: 664636
(Reporter)

Comment 15

5 years ago
Created attachment 623339 [details] [diff] [review]
Upgrade trySTARTTLS to alwaysSTARTTLS - v3

This is the part of patch v11 (attachment #623329 [details] [diff] [review]) that is for this bug. It's the same as your attachment #619599 [details] [diff] [review] with a few comments changed.
Attachment #616986 - Attachment is obsolete: true
Attachment #623339 - Flags: review?(dbienvenu)
(Reporter)

Comment 16

5 years ago
Comment on attachment 623339 [details] [diff] [review]
Upgrade trySTARTTLS to alwaysSTARTTLS - v3

Commited as http://hg.mozilla.org/comm-central/rev/9d5996744bca
(Reporter)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Target Milestone: --- → Thunderbird 15.0
Can someone do something with the open review flag please? Setting it to r+ with a statement of where it was done would help. Thanks.
(Reporter)

Comment 18

5 years ago
Mark, the patch was jointly developed by bienvenu and me. As comment 15 said, this patch is identical to certain hunks of attachment #619599 [details] [diff] [review], which was attached by bienvenu, with only a few comments changed by me. Given that it was created by bienvenu, and reviewed by me, it should be OK. The r? is only a formality.
(Assignee)

Comment 19

5 years ago
Comment on attachment 623339 [details] [diff] [review]
Upgrade trySTARTTLS to alwaysSTARTTLS - v3

sorry, there are three different versions of various parts of this patch flying around - perhaps BenB got impatient because I was trying to take a vacation :-) - I thought I had r+ all of them.
Attachment #623339 - Flags: review?(dbienvenu) → review+
(Reporter)

Comment 20

5 years ago
Thanks.

/me sends drinks with paper umbrellas.
You need to log in before you can comment on or make changes to this bug.