Improve security defaults for SMTP_SSL and SMTP_TLS to prevent man-in-the-middle attacks

[cbarcenas]

I submitted a PR in #104 to fix these issues but I thought I'd also open this ticket for broader discussion about how Simple Java Mail is vulnerable and why this is a bad thing even when taking email's inherent lack of security into account.

TransportStrategy.SMTP_TLS currently gives a false sense of security.

As currently implemented, TransportStrategy.SMTP_TLS is merely opportunistic and can be easily circumvented by an active attacker. If sending emails via TransportStrategy.SMTP_TLS it is reasonable to assume that SMTP credentials and email contents will necessarily be protected via TLS (or otherwise an error should be raised), but that's not the current behavior of this library.

The various ways an active attacker could circumvent STARTTLS are outlined in the spec, RFC 2487 § 7:

A man-in-the-middle attack can be launched by deleting the "250
STARTTLS" response from the server. This would cause the client not
to try to start a TLS session. Another man-in-the-middle attack is
to allow the server to announce its STARTTLS capability, but to alter
the client's request to start TLS and the server's response.

TransportStrategy.SMTP_TLS and TransportStrategy.SMTP_SSL do not perform certificate identity validation.

At present, any certificate signed by a trusted CA and presented by an upstream SMTP server is accepted when negotiating a TLS session. A certificate issued by a public CA to www.totally-not-spying-on-you.com would be accepted by JavaMail during a connection attempt to, for example, smtp.gmail.com.

As noted in the PR, Oracle concedes that this default behavior of JavaMail is bad. As a convenience wrapper, I think Simple Java Mail should make its own default behavior secure. (Since developers should be turning this flag on anyways)

The current behavior is bad even when we consider that SMTP is insecure

SMTP is insecure in that email contents are not guaranteed to be end-to-end encrypted during transit. However, authentication with credentials to an SMTP server should be end-to-end encrypted if SMTPS or STARTTLS is used. The attacks above break this guarantee.

[bbottema]

I can generally agree on all points. I'm only wondering what effect it might have on existing users. I don't want to disturb currently working functionality. If that's the case, instead I might introduce a SMTP_TLS_STRICT for example. Although, the documentation mentioned it's turned off by default due to backwards compatibility reasons (which goes back many years now).

Thoughts?

[cbarcenas]

Hm, I have some reservations with this approach, and I think it would be
better to break library usage for the sake of fixing these security issues,
especially when the changes I suggest below would only break some usages,
and compatibility fixes would basically just be changing the selected
TransportStrategy.

What do you think about the following proposal?

• In SMTP mode (TransportStrategy.SMTP_PLAIN, the default), which has no
  security guarantees, we try to opportunistically use STARTTLS where supported,
  but do not require it and do not perform certificate identity checking. This
  is done by setting mail.smtp.starttls.enable=true.

  This protects against passive eavesdroppers, does not break existing
  consumers of Simple Java Mail, and is fully backwards-compatible with
  plaintext SMTP (because STARTTLS is only negotiated by JavaMail if the SMTP
  EHLO server response indicates STARTTLS support).

  For weird cases where opportunistic STARTTLS upgrades are not wanted at all
  (although I don't know why you'd want this) we could add a Simple Java
  Mail configuration property to disable STARTTLS entirely.

  I further propose we rename the enum simply to TransportStrategy.SMTP.

• In SMTPS mode (TransportStrategy.SMTP_SSL), enforce identity checking
  with mail.smtps.ssl.checkserveridentity=true.

  This is not different from the original proposal above.

  If we're renaming enums, though, I would recommend changing its name to
  TransportStrategy.SMTPS, because the current name
  TransportStrategy.SMTP_SSL is ambiguous with
  TransportStrategy.SMTP_TLS.

• In TLS mode (TransportStrategy.SMTP_TLS), perform certificate
  identity verification with mail.smtp.ssl.checkserveridentity=true,
  and require STARTTLS with mail.smtp.starttls.enable=true and
  mail.smtp.starttls.required=true.

  I firmly stand by my original philosophy that if API consumers go out of
  their way to request TLS transport, they should always get it. At present
  it's super trivial for an intermediate man-in-the-middle attacker to force an
  unencrypted connection, and AFAIK there isn't preexisting documentation that
  discloses this plaintext fallback behavior to Simple Java Mail API consumers.
  In that regard I view the current Simple Java Mail behavior as a serious
  security issue rather than an area where we can simply improve the library.

So, yes, I share your concerns that our changes will break library consumers,
but the breakages will be easy-to-fix and I think it's worth it because
a) I view the current behavior as a fairly serious vulnerability, and b) the
Simple Java Mail API doesn't currently define how it handles the TLS part
of TransportStrategy.SMTPS/TransportStrategy.SMTP_TLS and so we might
as well properly define the TLS behavior "the right way" now for the sake
of a clean API and well-defined behavior.

[bbottema]

Excellent, I can get behind this on all points. Except for the naming scheme, I'm not sure I understand the reasoning. SMTP_SSL and SMTP_TLS describe exactly what they support (and SMTPS is the same as SMTP_SSL in my mind).

Aside from that, I'll have to find some time in the coming weeks to perform the changes (pregnant wife and all :), but if you feel up to it, you're welcome to give it a go for a pull-request.

[cbarcenas]

Except for the naming scheme, I'm not sure I understand the reasoning. SMTP_SSL and SMTP_TLS describe exactly what they support (and SMTPS is the same as SMTP_SSL in my mind).

The confusion between SSL and TLS is very common, and to be honest it's kind of a pet peeve of mine 😄 SSL refers to a very obsolete encryption protocol that has since been superseded by TLS across the entire internet. Yet, for some reason, many people still refer to TLS as SSL!

In a standard JVM, both SMTPS and STARTTLS JavaMail transport modes use the TLS encryption protocol when talking to encrypted mail servers. The suggestion to rename these TransportStrategy enums was motivated by a desire to have their names reflect the actual mail protocols in use (SMTPS and STARTTLS). Since we're already introducing slightly backwards-incompatible behavior, I figured it would be a good time to clean up the naming scheme.

Aside from that, I'll have to find some time in the coming weeks to perform the changes (pregnant wife and all :), but if you feel up to it, you're welcome to give it a go for a pull-request.

I've updated my PR in #104 to include the changes discussed above. It's missing tests, and if you're OK with it I would also like to include the enum naming changes suggested above.

[bbottema]

Alright, I'm on board for these changes. I'll look into merging your changes for review. I'll update the enum names while I'm at it.

[bbottema]

I'm on the fench abount naming TLS mode StransportStrategy.SMTP_TLS or StransportStrategy.TLS. What's your take on this?