Skip to content

Commit

Permalink
Security fixes in SMTP_SSL and SMTP_TLS strategies
Browse files Browse the repository at this point in the history
  - The SMTP_SSL and SMTP_TLS transport strategies now validate certificates
    by setting JavaMail's `mail.<protocol>.ssl.checkserveridentity` property
    to true.

    Previously, no identity validation was performed, leaving SMTPS and
    STARTTLS connections vulnerable to man-in-the-middle attacks. Without
    identity validation, JavaMail accepts _any_ certificate issued by a
    JVM-trusted CA, regardless of the identity encoded in the certificate.

  - The SMTP_TLS transport strategy now requires STARTTLS support by setting
    JavaMail's `mail.smtp.starttls.required` property to true.

    Previously, STARTTLS support was not required, enabling a man-in-the-middle
    attack whereby an attacker could strip the STARTTLS request from an SMTP
    connection, causing JavaMail to fall back to plaintext SMTP for
    authentication and email transport.
  • Loading branch information
cbarcenas committed Oct 30, 2017
1 parent c8f122e commit 4916171
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ public String propertyNameAuthenticate() {
},
/**
* SMTPS / SSL transport strategy, that returns the ".smtps." variation of the SMTP_PLAIN version. Additionally the transport protocol is
* explicitly set to smtps. Finally, he property "mail.smtps.quitwait" is set to false, to get rid of a strange SSL exception:<br>
* explicitly set to smtps, and strict certificate validation is enabled by setting "mail.smtps.ssl.checkserveridentity" to true. Finally,
* the property "mail.smtps.quitwait" is set to false, to get rid of a strange SSL exception:<br>
* <p>
* <pre>
* javax.mail.MessagingException: Exception reading response;
Expand All @@ -79,14 +80,16 @@ public String propertyNameAuthenticate() {
*/
SMTP_SSL {
/**
* Here protocol "mail.transport.protocol" is set to "smtps" and the quitwait property "mail.smtps.quitwait" is set to "false".
* Here protocol "mail.transport.protocol" is set to "smtps", certificate validation "mail.smtps.ssl.checkserveridentity" is set to "true", and
* the quitwait property "mail.smtps.quitwait" is set to "false".
*
* @see TransportStrategy#SMTP_SSL
*/
@Override
public Properties generateProperties() {
final Properties properties = super.generateProperties();
properties.put("mail.transport.protocol", "smtps");
properties.put("mail.smtps.ssl.checkserveridentity", "true");
properties.put("mail.smtps.quitwait", "false");
return properties;
}
Expand Down Expand Up @@ -127,11 +130,13 @@ public String propertyNameAuthenticate() {
* <strong>NOTE: this code is in untested beta state</strong>
* <p>
* Uses standard ".smtp." property names (like {@link TransportStrategy#SMTP_PLAIN}). Additionally the transport protocol is explicitly set to
* smtp. Finally, the property "mail.smtp.starttls.enable" is being set to true.
* smtp. STARTTLS is enabled by setting "mail.smtp.starttls.enable" and "mail.smtp.starttls.required" to true, and strict certificate validation
* is enabled by setting "mail.smtp.ssl.checkserveridentity" to true.
*/
SMTP_TLS {
/**
* Here protocol "mail.transport.protocol" is set to "smtp" and the tls property "mail.smtp.starttls.enable" is set to "true".
* Here protocol "mail.transport.protocol" is set to "smtp", STARTTLS properties "mail.smtp.starttls.enable" and "mail.smtp.starttls.required"
* are set to "true", and certificate validation "mail.smtp.ssl.checkserveridentity" is set to "true".
*
* @see TransportStrategy#SMTP_TLS
*/
Expand All @@ -140,6 +145,8 @@ public Properties generateProperties() {
final Properties props = super.generateProperties();
props.put("mail.transport.protocol", "smtp");
props.put("mail.smtp.starttls.enable", "true");
props.put("mail.smtp.starttls.required", "true");
props.put("mail.smtp.ssl.checkserveridentity", "true");
return props;
}

Expand Down
8 changes: 8 additions & 0 deletions src/test/java/org/simplejavamail/mailer/MailerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ public void createMailSession_AnonymousProxyConstructor_WithoutConfig()
assertThat(session.getProperty("mail.smtp.port")).isEqualTo("25");
assertThat(session.getProperty("mail.transport.protocol")).isEqualTo("smtp");
assertThat(session.getProperty("mail.smtp.starttls.enable")).isEqualTo("true");
assertThat(session.getProperty("mail.smtp.starttls.required")).isEqualTo("true");
assertThat(session.getProperty("mail.smtp.ssl.checkserveridentity")).isEqualTo("true");
assertThat(session.getProperty("mail.smtp.username")).isEqualTo("username smtp");
assertThat(session.getProperty("mail.smtp.auth")).isEqualTo("true");
assertThat(session.getProperty("mail.smtp.socks.host")).isEqualTo("proxy host");
Expand All @@ -112,6 +114,8 @@ public void createMailSession_MaximumConstructor_WithoutConfig()
assertThat(session.getProperty("mail.smtp.port")).isEqualTo("25");
assertThat(session.getProperty("mail.transport.protocol")).isEqualTo("smtp");
assertThat(session.getProperty("mail.smtp.starttls.enable")).isEqualTo("true");
assertThat(session.getProperty("mail.smtp.starttls.required")).isEqualTo("true");
assertThat(session.getProperty("mail.smtp.ssl.checkserveridentity")).isEqualTo("true");
assertThat(session.getProperty("mail.smtp.username")).isEqualTo("username smtp");
assertThat(session.getProperty("mail.smtp.auth")).isEqualTo("true");
// the following two are because authentication is needed, otherwise proxy would be straightworward
Expand All @@ -131,6 +135,8 @@ public void createMailSession_MinimalConstructor_WithConfig() {
assertThat(session.getProperty("mail.smtp.port")).isEqualTo("25");
assertThat(session.getProperty("mail.transport.protocol")).isEqualTo("smtp");
assertThat(session.getProperty("mail.smtp.starttls.enable")).isEqualTo("true");
assertThat(session.getProperty("mail.smtp.starttls.required")).isEqualTo("true");
assertThat(session.getProperty("mail.smtp.ssl.checkserveridentity")).isEqualTo("true");
assertThat(session.getProperty("mail.smtp.username")).isEqualTo("username smtp");
assertThat(session.getProperty("mail.smtp.auth")).isEqualTo("true");
// the following two are because authentication is needed, otherwise proxy would be straightworward
Expand All @@ -150,6 +156,8 @@ public void createMailSession_MaximumConstructor_WithConfig()
assertThat(session.getProperty("mail.smtp.port")).isEqualTo("25");
assertThat(session.getProperty("mail.transport.protocol")).isEqualTo("smtp");
assertThat(session.getProperty("mail.smtp.starttls.enable")).isEqualTo("true");
assertThat(session.getProperty("mail.smtp.starttls.required")).isEqualTo("true");
assertThat(session.getProperty("mail.smtp.ssl.checkserveridentity")).isEqualTo("true");
assertThat(session.getProperty("mail.smtp.username")).isEqualTo("overridden username smtp");
assertThat(session.getProperty("mail.smtp.auth")).isEqualTo("true");
// the following two are because authentication is needed, otherwise proxy would be straightworward
Expand Down

0 comments on commit 4916171

Please sign in to comment.