From 49161719620c06418b2223a4ed405e2f642dce18 Mon Sep 17 00:00:00 2001 From: Christian Barcenas Date: Thu, 19 Oct 2017 10:11:23 -0700 Subject: [PATCH] Security fixes in SMTP_SSL and SMTP_TLS strategies - The SMTP_SSL and SMTP_TLS transport strategies now validate certificates by setting JavaMail's `mail..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. --- .../mailer/config/TransportStrategy.java | 15 +++++++++++---- .../org/simplejavamail/mailer/MailerTest.java | 8 ++++++++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/simplejavamail/mailer/config/TransportStrategy.java b/src/main/java/org/simplejavamail/mailer/config/TransportStrategy.java index 0ce7ab201..453d24c14 100644 --- a/src/main/java/org/simplejavamail/mailer/config/TransportStrategy.java +++ b/src/main/java/org/simplejavamail/mailer/config/TransportStrategy.java @@ -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:
+ * 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:
*

* 

 	 * javax.mail.MessagingException: Exception reading response;
@@ -79,7 +80,8 @@ 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
 		 */
@@ -87,6 +89,7 @@ public String propertyNameAuthenticate() {
 		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;
 		}
@@ -127,11 +130,13 @@ public String propertyNameAuthenticate() {
 	 * NOTE: this code is in untested beta state
 	 * 

 	 * 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
 		 */
@@ -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;
 		}
 
diff --git a/src/test/java/org/simplejavamail/mailer/MailerTest.java b/src/test/java/org/simplejavamail/mailer/MailerTest.java
index 73e56bbd5..0e2d2166e 100644
--- a/src/test/java/org/simplejavamail/mailer/MailerTest.java
+++ b/src/test/java/org/simplejavamail/mailer/MailerTest.java
@@ -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");
@@ -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
@@ -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
@@ -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