From 2d1278ece50a45c8d21ebb452ca1e52f72976e12 Mon Sep 17 00:00:00 2001
From: Tony Torralba <atorralba@users.noreply.github.com>
Date: Wed, 30 Jun 2021 12:22:48 +0200
Subject: [PATCH] Consider setStartTLSRequired for Apache SimpleEmail

---
 .../ql/src/semmle/code/java/security/Mail.qll |  2 +-
 .../CWE-297/InsecureJavaMailTest.java         | 31 ----------
 .../CWE-297/InsecureSimpleEmailTest.java      | 62 +++++++++++++++++++
 3 files changed, 63 insertions(+), 32 deletions(-)
 create mode 100644 java/ql/test/query-tests/security/CWE-297/InsecureSimpleEmailTest.java

diff --git a/java/ql/src/semmle/code/java/security/Mail.qll b/java/ql/src/semmle/code/java/security/Mail.qll
index 4f4c9c87426..c100e5c49dc 100644
--- a/java/ql/src/semmle/code/java/security/Mail.qll
+++ b/java/ql/src/semmle/code/java/security/Mail.qll
@@ -33,7 +33,7 @@ predicate isInsecureMailPropertyConfig(VarAccess propertiesVarAccess) {
  * Holds if `ma` enables TLS/SSL with Apache Email.
  */
 predicate enablesEmailSsl(MethodAccess ma) {
-  ma.getMethod().hasName("setSSLOnConnect") and
+  ma.getMethod().hasName(["setSSLOnConnect", "setStartTLSRequired"]) and
   ma.getMethod().getDeclaringType() instanceof ApacheEmail and
   ma.getArgument(0).(BooleanLiteral).getBooleanValue() = true
 }
diff --git a/java/ql/test/query-tests/security/CWE-297/InsecureJavaMailTest.java b/java/ql/test/query-tests/security/CWE-297/InsecureJavaMailTest.java
index fdfbd1f9f03..a9880c20339 100644
--- a/java/ql/test/query-tests/security/CWE-297/InsecureJavaMailTest.java
+++ b/java/ql/test/query-tests/security/CWE-297/InsecureJavaMailTest.java
@@ -4,12 +4,6 @@ import javax.mail.Authenticator;
 import javax.mail.PasswordAuthentication;
 import javax.mail.Session;
 
-import org.apache.commons.mail.DefaultAuthenticator;
-import org.apache.commons.mail.Email;
-import org.apache.commons.mail.SimpleEmail;
-
-import java.util.Properties;
-
 class InsecureJavaMailTest {
 	public void testJavaMail() {
 		final Properties properties = new Properties();
@@ -46,30 +40,5 @@ class InsecureJavaMailTest {
 		final Session session = Session.getInstance(properties, authenticator); // Safe
 	}
 
-	public void testSimpleMail() throws Exception {
-		Email email = new SimpleEmail();
-		email.setHostName("config.hostName");
-		email.setSmtpPort(25);
-		email.setAuthenticator(new DefaultAuthenticator("config.username", "config.password"));
-		email.setSSLOnConnect(true); // $hasInsecureJavaMail
-		email.setFrom("fromAddress");
-		email.setSubject("subject");
-		email.setMsg("body");
-		email.addTo("toAddress");
-		email.send();
-	}
 
-	public void testSecureSimpleMail() throws Exception {
-		Email email = new SimpleEmail();
-		email.setHostName("config.hostName");
-		email.setSmtpPort(25);
-		email.setAuthenticator(new DefaultAuthenticator("config.username", "config.password"));
-		email.setSSLOnConnect(true); // Safe
-		email.setSSLCheckServerIdentity(true);
-		email.setFrom("fromAddress");
-		email.setSubject("subject");
-		email.setMsg("body");
-		email.addTo("toAddress");
-		email.send();
-	}
 }
diff --git a/java/ql/test/query-tests/security/CWE-297/InsecureSimpleEmailTest.java b/java/ql/test/query-tests/security/CWE-297/InsecureSimpleEmailTest.java
new file mode 100644
index 00000000000..5940dbbd457
--- /dev/null
+++ b/java/ql/test/query-tests/security/CWE-297/InsecureSimpleEmailTest.java
@@ -0,0 +1,62 @@
+import org.apache.commons.mail.DefaultAuthenticator;
+import org.apache.commons.mail.Email;
+import org.apache.commons.mail.SimpleEmail;
+
+public class InsecureSimpleEmailTest {
+    public void test() throws Exception {
+        // with setSSLOnConnect
+        {
+            Email email = new SimpleEmail();
+            email.setHostName("config.hostName");
+            email.setSmtpPort(25);
+            email.setAuthenticator(new DefaultAuthenticator("config.username", "config.password"));
+            email.setSSLOnConnect(true); // $hasInsecureJavaMail
+            email.setFrom("fromAddress");
+            email.setSubject("subject");
+            email.setMsg("body");
+            email.addTo("toAddress");
+            email.send();
+        }
+        // with setStartTLSRequired
+        {
+            Email email = new SimpleEmail();
+            email.setHostName("config.hostName");
+            email.setSmtpPort(25);
+            email.setAuthenticator(new DefaultAuthenticator("config.username", "config.password"));
+            email.setStartTLSRequired(true); // $hasInsecureJavaMail
+            email.setFrom("fromAddress");
+            email.setSubject("subject");
+            email.setMsg("body");
+            email.addTo("toAddress");
+            email.send();
+        }
+        // safe with setSSLOnConnect
+        {
+            Email email = new SimpleEmail();
+            email.setHostName("config.hostName");
+            email.setSmtpPort(25);
+            email.setAuthenticator(new DefaultAuthenticator("config.username", "config.password"));
+            email.setSSLOnConnect(true); // Safe
+            email.setSSLCheckServerIdentity(true);
+            email.setFrom("fromAddress");
+            email.setSubject("subject");
+            email.setMsg("body");
+            email.addTo("toAddress");
+            email.send();
+        }
+        // safe with setStartTLSRequired
+        {
+            Email email = new SimpleEmail();
+            email.setHostName("config.hostName");
+            email.setSmtpPort(25);
+            email.setAuthenticator(new DefaultAuthenticator("config.username", "config.password"));
+            email.setStartTLSRequired(true); // Safe
+            email.setSSLCheckServerIdentity(true);
+            email.setFrom("fromAddress");
+            email.setSubject("subject");
+            email.setMsg("body");
+            email.addTo("toAddress");
+            email.send();
+        }
+    }
+}