Last Comment Bug 744676 - Upgrade trySTARTTLS to alwaysSTARTTLS, if we know it worked
: Upgrade trySTARTTLS to alwaysSTARTTLS, if we know it worked
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Networking: IMAP (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: Thunderbird 15.0
Assigned To: David :Bienvenu
:
:
Mentors:
Depends on: RFC6154
Blocks: 664636
  Show dependency treegraph
 
Reported: 2012-04-11 18:49 PDT by Ben Bucksch (:BenB)
Modified: 2012-05-15 07:22 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Upgrade trySTARTTLS to alwaysSTARTTLS - v1 (5.59 KB, patch)
2012-04-11 18:51 PDT, Ben Bucksch (:BenB)
mozilla: review-
Details | Diff | Splinter Review
wip (7.01 KB, patch)
2012-04-20 09:01 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
Upgrade trySTARTTLS to alwaysSTARTTLS - v3 (5.42 KB, patch)
2012-05-11 15:49 PDT, Ben Bucksch (:BenB)
mozilla: review+
Details | Diff | Splinter Review

Description Ben Bucksch (:BenB) 2012-04-11 18:49:50 PDT
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.
Comment 1 Ben Bucksch (:BenB) 2012-04-11 18:51:36 PDT
Created attachment 614249 [details] [diff] [review]
Upgrade trySTARTTLS to alwaysSTARTTLS - v1

Patch depends on Attachment #614242 [details] [diff] - Bug 558659 "Capability bitflags 64bit, v7"
Comment 2 David :Bienvenu 2012-04-11 21:16:09 PDT
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.
Comment 3 David :Bienvenu 2012-04-12 10:42:08 PDT
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.
Comment 4 Ben Bucksch (:BenB) 2012-04-12 16:27:43 PDT
> 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).
Comment 5 David :Bienvenu 2012-04-12 16:31:56 PDT
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.
Comment 6 Ben Bucksch (:BenB) 2012-04-12 16:41:09 PDT
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.
Comment 7 David :Bienvenu 2012-04-12 16:52:40 PDT
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.
Comment 8 David :Bienvenu 2012-04-12 17:16:50 PDT
Oh, and if it's not clear, ProcessCurrentUrl is called on the per-connection imap thread.
Comment 9 Ben Bucksch (:BenB) 2012-04-12 17:32:57 PDT
> 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.
Comment 10 David :Bienvenu 2012-04-13 08:32:48 PDT
(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 :-(
Comment 11 Ben Bucksch (:BenB) 2012-04-13 15:40:39 PDT
> I can try to modify the patch, but it won't be today :-(

Yeah, no worries. Bug 558659 doesn't depend on this one.
Comment 12 Ben Bucksch (:BenB) 2012-04-13 15:41:58 PDT
> https://developer.mozilla.org/en/IMAP

wow, now that's a great doc. This is the kind of documentation we need! Thanks!
Comment 13 David :Bienvenu 2012-04-20 09:01:21 PDT
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.
Comment 14 David :Bienvenu 2012-04-24 11:07:25 PDT
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.
Comment 15 Ben Bucksch (:BenB) 2012-05-11 15:49:34 PDT
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.
Comment 16 Ben Bucksch (:BenB) 2012-05-11 16:35:13 PDT
Comment on attachment 623339 [details] [diff] [review]
Upgrade trySTARTTLS to alwaysSTARTTLS - v3

Commited as http://hg.mozilla.org/comm-central/rev/9d5996744bca
Comment 17 Mark Banner (:standard8, limited time in Dec) 2012-05-14 21:35:25 PDT
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.
Comment 18 Ben Bucksch (:BenB) 2012-05-15 03:30:12 PDT
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.
Comment 19 David :Bienvenu 2012-05-15 07:16:50 PDT
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.
Comment 20 Ben Bucksch (:BenB) 2012-05-15 07:22:41 PDT
Thanks.

/me sends drinks with paper umbrellas.

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