[SECURITY] Option to enforce STARTTLS

[sfllaw]

mail/smtp.go

Lines 98 to 105 in 1e5036a

	if !d.SSL {
	if ok, _ := c.Extension("STARTTLS"); ok {
	if err := c.StartTLS(d.tlsConfig()); err != nil {
	c.Close()
	return nil, err
	}
	}
	}

This code only tries to upgrade to a TLS connection if the server reports that it supports the STARTTLS extension. If an attacker intercepts the SMTP connection and responds that the extension is unknown, then they can prevent TLS from being turned on.

It would be backwards incompatible to expect STARTTLS by default, but we can make an option that would enforce a STARTTLS connection.

Of course, if the connection is already protected by SSL, then STARTTLS is unnecessary.

[cryptix]

we can make an option that would enforce a STARTTLS connection.

+1 - would like to see this as well.

[ivy]

@sfllaw Thanks for opening this issue. I take security very seriously and would love to see improvements in the way go-mail handles secure communication and authentication. With that in mind, I'm not sure if you've already seen this, but there is a Dialer.SSL option which enforces SSL/TLS. If security is a major concern, this is almost certainly the preferred method over STARTTLS.

I'm curious if you've encountered a case where STARTTLS was your only option? I'm concerned that an additional option will only add confusion and make a misconfiguration more likely. If anything, it may be a good idea for us to improve our documentation on what options are available for SSL/TLS negotiation.

Edit: Clarifying that Dialer.SSL enforces encryption.

[sfllaw]

@ivy Dialer.SSL is only for supporting the deprecated SMTPS protocol, right? STARTTLS is equivalently as secure, as long as the library doesn't fallback to unencrypted SMTP. (Both SMTPS and STARTTLS are vulnerable to a DNS attack.)

I have run into cases where the remote server doesn't support SMTPS, because the legacy 465 port is not open.

I recommend we create a new Dialer.RequireStartTLS enum that defaults to Opportunistic TLS. The reason why we want an enum is that we want to support DANE in the future.

[ivy]

@sfllaw Thanks for clarifying. I wasn't aware that SMTPS was a deprecated protocol, that changes everything. 😅

I'll review your PR shortly.

[ivy]

A fix was merged in b93a540.