postalsys / imapflow

IMAP Client library for EmailEngine Email API (https://emailengine.app)
https://imapflow.com
Other
367 stars 64 forks source link

The secure property is not as secure as the documentation leds to believe #141

Closed CReimer closed 1 year ago

CReimer commented 1 year ago

Describe the bug In the API reference the description for the secure property is "Should the connection be established over TLS. If false then connection is upgraded to TLS using STARTTLS extension before authentication"

Which I interpret as: imapflow tries to connect via SSL directly and if that is not the case, it definitely uses STARTTLS before sending any authentication data.

After quick glance at the source code I'm not so sure, that is the case

In class ImapFlow in method startSession the method upgradeConnection is called. In there is a capabilities check and a missing STARTTLS capability leads to a return false. This return is never used again an there's no Exception thrown

startSession continues on without proper encryption like nothing ever happened

Expected behavior startSession needs to stop if the secure property is set and there's no encryption in place. It must be impossible to accidentally leak login credentials over an unencrypted connection.

andris9 commented 1 year ago

STARTTLS usage is "best effort". If the IMAP server does not support it, then it is not used. If you are connecting to an MITM server that disables STARTTLS extension from the capability listing, then yes, there is a possibility your credentials will be leaked.

So, I'd suggest using only port 993 for IMAP with the secure=true option. If an IMAP server uses any port, then that server is already kind of suspicious. Just in case, I might add an additional option in the future that requires the use of TLS even if the server does not advertise the capability.