Connections upgraded with STARTTLS are not reused #422

Closed
ehlertjd opened this Issue Sep 10, 2015 · 9 comments

Projects

None yet

3 participants

@ehlertjd
Contributor

As stated in title, upgraded connections are not reused.
Issue appears to be in ConnectionExists of url.c. The check (needle->handler->protocol & check->handler->protocol) resolves to false because the handler was switched to Curl_handler_imaps in imap_to_imaps called from imap_perform_upgrade_tls.

Code to reproduce the issue: https://gist.github.com/ehlertjd/d10e4f050e40f77b448d
(you'll need credentials against a mail server running IMAP with STARTTLS)

Tested against 7.42.1, but appears to still be present in 7.44.0.

@bagder bagder added the SSL/TLS label Sep 12, 2015
@bagder
Member
bagder commented Sep 12, 2015

Without checking the code closer, I could also guess that this might also apply to other STARTTLS protocols...

@bagder bagder added the IMAP label Nov 5, 2015
@captain-caveman2k
Member

I've finally had some time to look at the problem and it does affect the other protocols that support TLS upgrade - currently testing SMTP here at the moment.

I believe the problem has existed since commit 710f14e when the protocol flag became a single bit rather than being the non-SSL and SSL bits or'd together which then means the check in url.c:3260 fails :(

I am still searching for an alternative fix to introducing a new protocol handler structure specifically for TLS even though I do quite like that solution ;-)

@captain-caveman2k captain-caveman2k changed the title from IMAP Connections upgraded with STARTTLS are not reused to Connections upgraded with STARTTLS are not reused Dec 1, 2015
@captain-caveman2k captain-caveman2k added a commit to captain-caveman2k/curl that referenced this issue Mar 5, 2016
@captain-caveman2k captain-caveman2k imap/pop3/smtp: Fixed connections upgraded with TLS are not reused
Bug: curl#422
Reported-by: Justin Ehlert
f97194a
@captain-caveman2k
Member

Hi @ehlertjd,

Sorry it has taken me a while to get an "alternative" fix for this but I am started looking at this issue a few times and each time I backed out my changes. However, I think I finally cracked it earlier this evening after reading your comments from 14 Oct in #484 ;-)

I have pushed commit f97194a to my own fork if you and @bagder would like to take a look.

I haven't pushed it to badger/curl as I would like feedback from you guys as well as giving myself the chance to test it on my Linux VMs against the test harness - so far I have only tested this against an Exchange server using SMTP via curl command line and the --next option.

Note: The fix currently doesn't include FTP either - as I couldn't find whether the handler is changed to the SSL handler in ftp.c - any pointers would be very welcome ;-)

Kind Regards

Steve

@captain-caveman2k
Member

Just a quick update... I've ran 'make test' and all but Test 46 (which is currently broken) run okay - so 1000 out of 1001.

@bagder
Member
bagder commented Mar 6, 2016

The fix currently doesn't include FTP either - as I couldn't find whether the handler is changed to the SSL handler in ftp.c

The handler is not modified within ftp.c, as it isn't changed dynamically. It only uses the ftps handler struct if used with an explicit ftps:// URL.

@captain-caveman2k
Member

The handler is not modified within ftp.c, as it isn't changed dynamically. It only uses the ftps handler struct if used with an explicit ftps:// URL.

Do you happen to know why we dynamically change it for the email protocols? I'm now wondering if we need to do this or whether we should do the same as FTP and only use the SSL handler for explicit TLS/SSL connections.

@bagder
Member
bagder commented Mar 6, 2016

I can't recall the exact reasoning for it. As they're much newer implementations I think it was simply made like that because it seemed simpler or more appropriate, while the FTP code was moved into the handler "architecture" from an older style.

@captain-caveman2k captain-caveman2k removed the FTP label Mar 7, 2016
@captain-caveman2k captain-caveman2k added a commit that referenced this issue Mar 8, 2016
@captain-caveman2k captain-caveman2k imap/pop3/smtp: Fixed connections upgraded with TLS are not reused
Regression since commit 710f14e.

Bug: #422
Reported-by: Justin Ehlert
a5aec58
@captain-caveman2k
Member

My fix has been pushed.

@ehlertjd
Contributor
ehlertjd commented Mar 8, 2016

Sorry I didn't reply sooner, the changes look nice and simple to me, I
don't see any issues looking at the code.

Thanks for fixing this!

On Tue, Mar 8, 2016 at 1:46 PM, Steve Holme notifications@github.com
wrote:

Closed #422 #422.


Reply to this email directly or view it on GitHub
#422 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment