From 0e149f0523f8639b6b6c1c527e98ab9e111ac5ad Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Fri, 18 Jun 2021 10:27:03 +0200 Subject: [PATCH 01/11] Move from experimental --- .../CWE/CWE-297/InsecureJavaMail.qhelp | 27 ++++++++++++++ .../Security/CWE/CWE-297/InsecureJavaMail.ql | 0 .../Security/CWE/CWE-297/JavaMail.java | 0 .../Security/CWE/CWE-297/SimpleMail.java | 0 .../CWE/CWE-297/InsecureJavaMail.qhelp | 36 ------------------- .../security/CWE-297/InsecureJavaMail.qlref | 1 - .../query-tests/security/CWE-297/options | 1 - .../CWE-297/InsecureJavaMail.expected | 0 .../security/CWE-297/InsecureJavaMail.java | 0 .../security/CWE-297/InsecureJavaMail.qlref | 1 + .../test/query-tests/security/CWE-297/options | 1 + 11 files changed, 29 insertions(+), 38 deletions(-) create mode 100644 java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.qhelp rename java/ql/src/{experimental => }/Security/CWE/CWE-297/InsecureJavaMail.ql (100%) rename java/ql/src/{experimental => }/Security/CWE/CWE-297/JavaMail.java (100%) rename java/ql/src/{experimental => }/Security/CWE/CWE-297/SimpleMail.java (100%) delete mode 100644 java/ql/src/experimental/Security/CWE/CWE-297/InsecureJavaMail.qhelp delete mode 100644 java/ql/test/experimental/query-tests/security/CWE-297/InsecureJavaMail.qlref delete mode 100644 java/ql/test/experimental/query-tests/security/CWE-297/options rename java/ql/test/{experimental => }/query-tests/security/CWE-297/InsecureJavaMail.expected (100%) rename java/ql/test/{experimental => }/query-tests/security/CWE-297/InsecureJavaMail.java (100%) create mode 100644 java/ql/test/query-tests/security/CWE-297/InsecureJavaMail.qlref create mode 100644 java/ql/test/query-tests/security/CWE-297/options diff --git a/java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.qhelp b/java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.qhelp new file mode 100644 index 00000000000..9945227ff0f --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.qhelp @@ -0,0 +1,27 @@ + + + + +

JavaMail is commonly used in Java applications to send emails. There are popular third-party libraries like Apache Commons Email which are built on JavaMail and facilitate integration. Authenticated mail sessions require user credentials and mail sessions can require SSL/TLS authentication. It is a common security vulnerability that host-specific certificate data is not validated or is incorrectly validated. Failing to validate the certificate makes the SSL session susceptible to a man-in-the-middle attack.

+

This query checks whether SSL certificate is validated when username/password is sent in authenticator and when SSL is enabled.

+

The query has code for both plain JavaMail invocation and mailing through Apache SimpleMail to make it more comprehensive.

+ + + +

Validate SSL certificate when sensitive information is sent in email communications.

+ + + +

The following two examples show two ways of configuring secure emails through JavaMail or Apache SimpleMail. In the 'BAD' case, +credentials are sent in an SSL session without certificate validation. In the 'GOOD' case, the certificate is validated.

+ + + + + +
  • + Log4j2: + Add support for specifying an SSL configuration for SmtpAppender (CVE-2020-9488) +
    • +     +     diff --git a/java/ql/src/experimental/Security/CWE/CWE-297/InsecureJavaMail.ql b/java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.ql similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-297/InsecureJavaMail.ql rename to java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.ql diff --git a/java/ql/src/experimental/Security/CWE/CWE-297/JavaMail.java b/java/ql/src/Security/CWE/CWE-297/JavaMail.java similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-297/JavaMail.java rename to java/ql/src/Security/CWE/CWE-297/JavaMail.java diff --git a/java/ql/src/experimental/Security/CWE/CWE-297/SimpleMail.java b/java/ql/src/Security/CWE/CWE-297/SimpleMail.java similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-297/SimpleMail.java rename to java/ql/src/Security/CWE/CWE-297/SimpleMail.java diff --git a/java/ql/src/experimental/Security/CWE/CWE-297/InsecureJavaMail.qhelp b/java/ql/src/experimental/Security/CWE/CWE-297/InsecureJavaMail.qhelp deleted file mode 100644 index ea1405aa4ee..00000000000 --- a/java/ql/src/experimental/Security/CWE/CWE-297/InsecureJavaMail.qhelp +++ /dev/null @@ -1,36 +0,0 @@ - - - - -

    JavaMail is commonly used in Java applications to send emails. There are popular third-party libraries like Apache Commons Email which are built on JavaMail and facilitate integration. Authenticated mail sessions require user credentials and mail sessions can require SSL/TLS authentication. It is a common security vulnerability that host-specific certificate data is not validated or is incorrectly validated. Failing to validate the certificate makes the SSL session susceptible to a man-in-the-middle attack.

    -

    This query checks whether SSL certificate is validated when username/password is sent in authenticator and when SSL is enabled.

    -

    The query has code for both plain JavaMail invocation and mailing through Apache SimpleMail to make it more comprehensive.

    -     - - -

    Validate SSL certificate when sensitive information is sent in email communications.

    -     - - -

    The following two examples show two ways of configuring secure emails through JavaMail or Apache SimpleMail. In the 'BAD' case, -credentials are sent in an SSL session without certificate validation. In the 'GOOD' case, the certificate is validated.

    - - -     - - -
  • - CWE-297 -
    • -
  • - Log4j2: - Add support for specifying an SSL configuration for SmtpAppender (CVE-2020-9488) -
    • -
  • - SonarSource rule: - SMTP SSL connection should check server identity -
    • -     -     diff --git a/java/ql/test/experimental/query-tests/security/CWE-297/InsecureJavaMail.qlref b/java/ql/test/experimental/query-tests/security/CWE-297/InsecureJavaMail.qlref deleted file mode 100644 index 565779521f3..00000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-297/InsecureJavaMail.qlref +++ /dev/null @@ -1 +0,0 @@ -experimental/Security/CWE/CWE-297/InsecureJavaMail.ql diff --git a/java/ql/test/experimental/query-tests/security/CWE-297/options b/java/ql/test/experimental/query-tests/security/CWE-297/options deleted file mode 100644 index 51c4feeca1b..00000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-297/options +++ /dev/null @@ -1 +0,0 @@ -// semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/apache-commons-email-1.6.0:${testdir}/../../../../stubs/javamail-api-1.6.2 diff --git a/java/ql/test/experimental/query-tests/security/CWE-297/InsecureJavaMail.expected b/java/ql/test/query-tests/security/CWE-297/InsecureJavaMail.expected similarity index 100% rename from java/ql/test/experimental/query-tests/security/CWE-297/InsecureJavaMail.expected rename to java/ql/test/query-tests/security/CWE-297/InsecureJavaMail.expected diff --git a/java/ql/test/experimental/query-tests/security/CWE-297/InsecureJavaMail.java b/java/ql/test/query-tests/security/CWE-297/InsecureJavaMail.java similarity index 100% rename from java/ql/test/experimental/query-tests/security/CWE-297/InsecureJavaMail.java rename to java/ql/test/query-tests/security/CWE-297/InsecureJavaMail.java diff --git a/java/ql/test/query-tests/security/CWE-297/InsecureJavaMail.qlref b/java/ql/test/query-tests/security/CWE-297/InsecureJavaMail.qlref new file mode 100644 index 00000000000..78dddc0f868 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-297/InsecureJavaMail.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-297/InsecureJavaMail.ql diff --git a/java/ql/test/query-tests/security/CWE-297/options b/java/ql/test/query-tests/security/CWE-297/options new file mode 100644 index 00000000000..563a4fdcfc8 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-297/options @@ -0,0 +1 @@ +// semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/apache-commons-email-1.6.0:${testdir}/../../../stubs/javamail-api-1.6.2 From 8c6d58e6d827046f668d26b18bbb2323fa616745 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Fri, 18 Jun 2021 11:11:52 +0200 Subject: [PATCH 02/11] Refactored into libraries --- .../Security/CWE/CWE-297/InsecureJavaMail.ql | 90 +------------------ .../src/semmle/code/java/frameworks/Mail.qll | 27 ++++++ .../ql/src/semmle/code/java/security/Mail.qll | 74 +++++++++++++++ 3 files changed, 104 insertions(+), 87 deletions(-) create mode 100644 java/ql/src/semmle/code/java/frameworks/Mail.qll create mode 100644 java/ql/src/semmle/code/java/security/Mail.qll diff --git a/java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.ql b/java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.ql index d0e0dc8b81a..9697b2ae588 100644 --- a/java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.ql +++ b/java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.ql @@ -12,96 +12,12 @@ */ import java - -/** - * The method to set Java properties - */ -class SetPropertyMethod extends Method { - SetPropertyMethod() { - this.hasName("setProperty") and - this.getDeclaringType().hasQualifiedName("java.util", "Properties") - or - this.hasName("put") and - this.getDeclaringType().getASourceSupertype*().hasQualifiedName("java.util", "Dictionary") - } -} - -/** - * The insecure way to set Java properties in mail sessions. - * 1. Set the mail.smtp.auth property to provide the SMTP Transport with a username and password when connecting to the SMTP server or - * set the mail.smtp.ssl.socketFactory/mail.smtp.ssl.socketFactory.class property to create an SMTP SSL socket. - * 2. No mail.smtp.ssl.checkserveridentity property is enabled. - */ -predicate isInsecureMailPropertyConfig(VarAccess propertiesVarAccess) { - exists(MethodAccess ma | - ma.getMethod() instanceof SetPropertyMethod and - ma.getQualifier() = propertiesVarAccess.getVariable().getAnAccess() and - ( - getStringValue(ma.getArgument(0)).matches("%.auth%") and //mail.smtp.auth - getStringValue(ma.getArgument(1)) = "true" - or - getStringValue(ma.getArgument(0)).matches("%.socketFactory%") //mail.smtp.socketFactory or mail.smtp.socketFactory.class - ) - ) and - not exists(MethodAccess ma | - ma.getMethod() instanceof SetPropertyMethod and - ma.getQualifier() = propertiesVarAccess.getVariable().getAnAccess() and - ( - getStringValue(ma.getArgument(0)).matches("%.ssl.checkserveridentity%") and //mail.smtp.ssl.checkserveridentity - getStringValue(ma.getArgument(1)) = "true" - ) - ) -} - -/** - * Helper method to get string value of an argument - */ -string getStringValue(Expr expr) { - result = expr.(CompileTimeConstantExpr).getStringValue() - or - result = getStringValue(expr.(AddExpr).getLeftOperand()) - or - result = getStringValue(expr.(AddExpr).getRightOperand()) -} - -/** - * The JavaMail session class `javax.mail.Session` - */ -class MailSession extends RefType { - MailSession() { this.hasQualifiedName("javax.mail", "Session") } -} - -/** - * The class of Apache SimpleMail - */ -class SimpleMail extends RefType { - SimpleMail() { this.hasQualifiedName("org.apache.commons.mail", "SimpleEmail") } -} - -/** - * Has TLS/SSL enabled with SimpleMail - */ -predicate enableTLSWithSimpleMail(MethodAccess ma) { - ma.getMethod().hasName("setSSLOnConnect") and - ma.getArgument(0).(BooleanLiteral).getBooleanValue() = true -} - -/** - * Has no certificate check - */ -predicate hasNoCertCheckWithSimpleMail(VarAccess va) { - not exists(MethodAccess ma | - ma.getQualifier() = va.getVariable().getAnAccess() and - ma.getMethod().hasName("setSSLCheckServerIdentity") and - ma.getArgument(0).(BooleanLiteral).getBooleanValue() = true - ) -} +import semmle.code.java.security.Mail from MethodAccess ma where - ma.getMethod().getDeclaringType() instanceof MailSession and - ma.getMethod().getName() = "getInstance" and + ma.getMethod() instanceof MailSessionGetInstanceMethod and isInsecureMailPropertyConfig(ma.getArgument(0)) or - enableTLSWithSimpleMail(ma) and hasNoCertCheckWithSimpleMail(ma.getQualifier()) + enablesEmailSsl(ma) and not hasSslCertificateCheck(ma.getQualifier()) select ma, "Java mailing has insecure SSL configuration" diff --git a/java/ql/src/semmle/code/java/frameworks/Mail.qll b/java/ql/src/semmle/code/java/frameworks/Mail.qll new file mode 100644 index 00000000000..f26cf0c5982 --- /dev/null +++ b/java/ql/src/semmle/code/java/frameworks/Mail.qll @@ -0,0 +1,27 @@ +/** Provides classes and predicates to work with email */ + +import java + +/** + * The class `javax.mail.Session` + */ +class MailSession extends Class { + MailSession() { this.hasQualifiedName("javax.mail", "Session") } +} + +/** + * The method `getInstance` of the class `javax.mail.Session` + */ +class MailSessionGetInstanceMethod extends Method { + MailSessionGetInstanceMethod() { + this.getDeclaringType() instanceof MailSession and + this.getName() = "getInstance" + } +} + +/** + * A subtype of the class `org.apache.commons.mail.Mail` + */ +class ApacheEmail extends Class { + ApacheEmail() { this.getASupertype*().hasQualifiedName("org.apache.commons.mail", "Email") } +} diff --git a/java/ql/src/semmle/code/java/security/Mail.qll b/java/ql/src/semmle/code/java/security/Mail.qll new file mode 100644 index 00000000000..e5fbb5c65a0 --- /dev/null +++ b/java/ql/src/semmle/code/java/security/Mail.qll @@ -0,0 +1,74 @@ +/** Provides classes and predicates to reason about email vulnerabilities. */ + +import java +import semmle.code.java.frameworks.Mail +private import semmle.code.java.frameworks.Properties + +/** + * The insecure way to set Java properties in mail sessions. + * 1. Set the `mail.smtp.auth` property to provide the SMTP Transport with a username and password when connecting to the SMTP server or + * set the `mail.smtp.ssl.socketFactory`/`mail.smtp.ssl.socketFactory.class` property to create an SMTP SSL socket. + * 2. No `mail.smtp.ssl.checkserveridentity` property is enabled. + */ +predicate isInsecureMailPropertyConfig(VarAccess propertiesVarAccess) { + exists(MethodAccess ma | + ma.getMethod() instanceof SetPropertyMethod and + ma.getQualifier() = propertiesVarAccess.getVariable().getAnAccess() + | + getStringValue(ma.getArgument(0)).matches("%.auth%") and //mail.smtp.auth + getStringValue(ma.getArgument(1)) = "true" + or + getStringValue(ma.getArgument(0)).matches("%.socketFactory%") //mail.smtp.socketFactory or mail.smtp.socketFactory.class + ) and + not exists(MethodAccess ma | + ma.getMethod() instanceof SetPropertyMethod and + ma.getQualifier() = propertiesVarAccess.getVariable().getAnAccess() + | + getStringValue(ma.getArgument(0)).matches("%.ssl.checkserveridentity%") and //mail.smtp.ssl.checkserveridentity + getStringValue(ma.getArgument(1)) = "true" + ) +} + +/** + * Holds if `ma` enables TLS/SSL with Apache Email. + */ +predicate enablesEmailSsl(MethodAccess ma) { + ma.getMethod().hasName("setSSLOnConnect") and + ma.getMethod().getDeclaringType() instanceof ApacheEmail and + ma.getArgument(0).(BooleanLiteral).getBooleanValue() = true +} + +/** + * Holds if a SSL certificate check is enabled on `va` with Apache Email + */ +predicate hasSslCertificateCheck(VarAccess va) { + exists(MethodAccess ma | + ma.getQualifier() = va.getVariable().getAnAccess() and + ma.getMethod().hasName("setSSLCheckServerIdentity") and + ma.getMethod().getDeclaringType() instanceof ApacheEmail and + ma.getArgument(0).(BooleanLiteral).getBooleanValue() = true + ) +} + +/** + * Helper method to get string value of an argument + */ +private string getStringValue(Expr expr) { + result = expr.(CompileTimeConstantExpr).getStringValue() + or + result = getStringValue(expr.(AddExpr).getLeftOperand()) + or + result = getStringValue(expr.(AddExpr).getRightOperand()) +} + +/** + * A method to set Java properties + */ +private class SetPropertyMethod extends Method { + SetPropertyMethod() { + this instanceof PropertiesSetPropertyMethod + or + this.hasName("put") and + this.getDeclaringType().getASourceSupertype*().hasQualifiedName("java.util", "Dictionary") + } +} From 73653f77aa8d5aad7a96420dce3b6fa1285924a4 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Fri, 18 Jun 2021 11:34:56 +0200 Subject: [PATCH 03/11] Use InlineExpectationsTest --- .../CWE-297/InsecureJavaMail.expected | 2 - .../security/CWE-297/InsecureJavaMail.qlref | 1 - .../CWE-297/InsecureJavaMailTest.expected | 0 ...avaMail.java => InsecureJavaMailTest.java} | 40 ++++++++++++++++--- .../security/CWE-297/InsecureJavaMailTest.ql | 23 +++++++++++ 5 files changed, 58 insertions(+), 8 deletions(-) delete mode 100644 java/ql/test/query-tests/security/CWE-297/InsecureJavaMail.expected delete mode 100644 java/ql/test/query-tests/security/CWE-297/InsecureJavaMail.qlref create mode 100644 java/ql/test/query-tests/security/CWE-297/InsecureJavaMailTest.expected rename java/ql/test/query-tests/security/CWE-297/{InsecureJavaMail.java => InsecureJavaMailTest.java} (50%) create mode 100644 java/ql/test/query-tests/security/CWE-297/InsecureJavaMailTest.ql diff --git a/java/ql/test/query-tests/security/CWE-297/InsecureJavaMail.expected b/java/ql/test/query-tests/security/CWE-297/InsecureJavaMail.expected deleted file mode 100644 index 0e3c219ace3..00000000000 --- a/java/ql/test/query-tests/security/CWE-297/InsecureJavaMail.expected +++ /dev/null @@ -1,2 +0,0 @@ -| InsecureJavaMail.java:29:27:29:72 | getInstance(...) | Java mailing has insecure SSL configuration | -| InsecureJavaMail.java:37:3:37:29 | setSSLOnConnect(...) | Java mailing has insecure SSL configuration | diff --git a/java/ql/test/query-tests/security/CWE-297/InsecureJavaMail.qlref b/java/ql/test/query-tests/security/CWE-297/InsecureJavaMail.qlref deleted file mode 100644 index 78dddc0f868..00000000000 --- a/java/ql/test/query-tests/security/CWE-297/InsecureJavaMail.qlref +++ /dev/null @@ -1 +0,0 @@ -Security/CWE/CWE-297/InsecureJavaMail.ql diff --git a/java/ql/test/query-tests/security/CWE-297/InsecureJavaMailTest.expected b/java/ql/test/query-tests/security/CWE-297/InsecureJavaMailTest.expected new file mode 100644 index 00000000000..e69de29bb2d diff --git a/java/ql/test/query-tests/security/CWE-297/InsecureJavaMail.java b/java/ql/test/query-tests/security/CWE-297/InsecureJavaMailTest.java similarity index 50% rename from java/ql/test/query-tests/security/CWE-297/InsecureJavaMail.java rename to java/ql/test/query-tests/security/CWE-297/InsecureJavaMailTest.java index 62c64027695..fdfbd1f9f03 100644 --- a/java/ql/test/query-tests/security/CWE-297/InsecureJavaMail.java +++ b/java/ql/test/query-tests/security/CWE-297/InsecureJavaMailTest.java @@ -10,7 +10,7 @@ import org.apache.commons.mail.SimpleEmail; import java.util.Properties; -class InsecureJavaMail { +class InsecureJavaMailTest { public void testJavaMail() { final Properties properties = new Properties(); properties.put("mail.transport.protocol", "protocol"); @@ -24,9 +24,26 @@ class InsecureJavaMail { }; if (null != authenticator) { properties.put("mail.smtp.auth", "true"); - // properties.put("mail.smtp.ssl.checkserveridentity", "true"); } - final Session session = Session.getInstance(properties, authenticator); + final Session session = Session.getInstance(properties, authenticator); // $hasInsecureJavaMail + } + + public void testSecureJavaMail() { + final Properties properties = new Properties(); + properties.put("mail.transport.protocol", "protocol"); + properties.put("mail.smtp.host", "hostname"); + properties.put("mail.smtp.socketFactory.class", "classname"); + + final javax.mail.Authenticator authenticator = new javax.mail.Authenticator() { + protected PasswordAuthentication getPasswordAuthentication() { + return new PasswordAuthentication("username", "password"); + } + }; + if (null != authenticator) { + properties.put("mail.smtp.auth", "true"); + properties.put("mail.smtp.ssl.checkserveridentity", "true"); + } + final Session session = Session.getInstance(properties, authenticator); // Safe } public void testSimpleMail() throws Exception { @@ -34,8 +51,21 @@ class InsecureJavaMail { email.setHostName("config.hostName"); email.setSmtpPort(25); email.setAuthenticator(new DefaultAuthenticator("config.username", "config.password")); - email.setSSLOnConnect(true); - // email.setSSLCheckServerIdentity(true); + 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"); diff --git a/java/ql/test/query-tests/security/CWE-297/InsecureJavaMailTest.ql b/java/ql/test/query-tests/security/CWE-297/InsecureJavaMailTest.ql new file mode 100644 index 00000000000..061aa6711f6 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-297/InsecureJavaMailTest.ql @@ -0,0 +1,23 @@ +import java +import semmle.code.java.security.Mail +import TestUtilities.InlineExpectationsTest + +class InsecureJavaMailTest extends InlineExpectationsTest { + InsecureJavaMailTest() { this = "HasInsecureJavaMailTest" } + + override string getARelevantTag() { result = "hasInsecureJavaMail" } + + override predicate hasActualResult(Location location, string element, string tag, string value) { + tag = "hasInsecureJavaMail" and + exists(MethodAccess ma | + ma.getLocation() = location and + element = ma.toString() and + value = "" + | + ma.getMethod() instanceof MailSessionGetInstanceMethod and + isInsecureMailPropertyConfig(ma.getArgument(0)) + or + enablesEmailSsl(ma) and not hasSslCertificateCheck(ma.getQualifier()) + ) + } +} From c13bf2a2a135ea9976d682dfe8b7849a98853429 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Fri, 18 Jun 2021 11:35:05 +0200 Subject: [PATCH 04/11] Add change note --- java/change-notes/2021-06-18-insecure-java-mail-query.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 java/change-notes/2021-06-18-insecure-java-mail-query.md diff --git a/java/change-notes/2021-06-18-insecure-java-mail-query.md b/java/change-notes/2021-06-18-insecure-java-mail-query.md new file mode 100644 index 00000000000..495a7019f9a --- /dev/null +++ b/java/change-notes/2021-06-18-insecure-java-mail-query.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* The query "Insecure JavaMail SSL Configuration" (`java/insecure-smtp-ssl`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @luchua-bc](https://github.com/github/codeql/pull/3491) \ No newline at end of file From a2e9c2f4abc3ed53a34f0c18a404543e817f050e Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 30 Jun 2021 11:49:31 +0200 Subject: [PATCH 05/11] Apply suggestions from code review Co-authored-by: Marcono1234 --- java/ql/src/semmle/code/java/frameworks/Mail.qll | 2 +- java/ql/src/semmle/code/java/security/Mail.qll | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/java/ql/src/semmle/code/java/frameworks/Mail.qll b/java/ql/src/semmle/code/java/frameworks/Mail.qll index f26cf0c5982..0f8ff1e0c55 100644 --- a/java/ql/src/semmle/code/java/frameworks/Mail.qll +++ b/java/ql/src/semmle/code/java/frameworks/Mail.qll @@ -20,7 +20,7 @@ class MailSessionGetInstanceMethod extends Method { } /** - * A subtype of the class `org.apache.commons.mail.Mail` + * A subtype of the class `org.apache.commons.mail.Email` */ class ApacheEmail extends Class { ApacheEmail() { this.getASupertype*().hasQualifiedName("org.apache.commons.mail", "Email") } diff --git a/java/ql/src/semmle/code/java/security/Mail.qll b/java/ql/src/semmle/code/java/security/Mail.qll index e5fbb5c65a0..4f4c9c87426 100644 --- a/java/ql/src/semmle/code/java/security/Mail.qll +++ b/java/ql/src/semmle/code/java/security/Mail.qll @@ -56,9 +56,7 @@ predicate hasSslCertificateCheck(VarAccess va) { private string getStringValue(Expr expr) { result = expr.(CompileTimeConstantExpr).getStringValue() or - result = getStringValue(expr.(AddExpr).getLeftOperand()) - or - result = getStringValue(expr.(AddExpr).getRightOperand()) + result = getStringValue(expr.(AddExpr).getAnOperand()) } /** From baffb0ed891f1e10cb19fd9900ab966bdd14fc1f Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 30 Jun 2021 12:14:55 +0200 Subject: [PATCH 06/11] Consider Jakarta Mail --- .../src/semmle/code/java/frameworks/Mail.qll | 8 +- .../CWE-297/InsecureJakartaMailTest.java | 42 ++++++++++ .../test/query-tests/security/CWE-297/options | 2 +- .../jakarta/mail/Authenticator.java | 20 +++++ .../jakarta/mail/PasswordAuthentication.java | 28 +++++++ .../jakarta/mail/Session.java | 80 +++++++++++++++++++ 6 files changed, 175 insertions(+), 5 deletions(-) create mode 100644 java/ql/test/query-tests/security/CWE-297/InsecureJakartaMailTest.java create mode 100644 java/ql/test/stubs/jakarta-mail-2.0.1/jakarta/mail/Authenticator.java create mode 100644 java/ql/test/stubs/jakarta-mail-2.0.1/jakarta/mail/PasswordAuthentication.java create mode 100644 java/ql/test/stubs/jakarta-mail-2.0.1/jakarta/mail/Session.java diff --git a/java/ql/src/semmle/code/java/frameworks/Mail.qll b/java/ql/src/semmle/code/java/frameworks/Mail.qll index 0f8ff1e0c55..93116009862 100644 --- a/java/ql/src/semmle/code/java/frameworks/Mail.qll +++ b/java/ql/src/semmle/code/java/frameworks/Mail.qll @@ -3,14 +3,14 @@ import java /** - * The class `javax.mail.Session` + * The class `javax.mail.Session` or `jakarta.mail.Session`. */ class MailSession extends Class { - MailSession() { this.hasQualifiedName("javax.mail", "Session") } + MailSession() { this.hasQualifiedName(["javax.mail", "jakarta.mail"], "Session") } } /** - * The method `getInstance` of the class `javax.mail.Session` + * The method `getInstance` of the classes `javax.mail.Session` or `jakarta.mail.Session`. */ class MailSessionGetInstanceMethod extends Method { MailSessionGetInstanceMethod() { @@ -20,7 +20,7 @@ class MailSessionGetInstanceMethod extends Method { } /** - * A subtype of the class `org.apache.commons.mail.Email` + * A subtype of the class `org.apache.commons.mail.Email`. */ class ApacheEmail extends Class { ApacheEmail() { this.getASupertype*().hasQualifiedName("org.apache.commons.mail", "Email") } diff --git a/java/ql/test/query-tests/security/CWE-297/InsecureJakartaMailTest.java b/java/ql/test/query-tests/security/CWE-297/InsecureJakartaMailTest.java new file mode 100644 index 00000000000..2dd5cab08ec --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-297/InsecureJakartaMailTest.java @@ -0,0 +1,42 @@ +import java.util.Properties; + +import jakarta.mail.Authenticator; +import jakarta.mail.PasswordAuthentication; +import jakarta.mail.Session; + +class InsecureJakartaMailTest { + public void testJavaMail() { + final Properties properties = new Properties(); + properties.put("mail.transport.protocol", "protocol"); + properties.put("mail.smtp.host", "hostname"); + properties.put("mail.smtp.socketFactory.class", "classname"); + + final jakarta.mail.Authenticator authenticator = new jakarta.mail.Authenticator() { + protected PasswordAuthentication getPasswordAuthentication() { + return new PasswordAuthentication("username", "password"); + } + }; + if (null != authenticator) { + properties.put("mail.smtp.auth", "true"); + } + final Session session = Session.getInstance(properties, authenticator); // $hasInsecureJavaMail + } + + public void testSecureJavaMail() { + final Properties properties = new Properties(); + properties.put("mail.transport.protocol", "protocol"); + properties.put("mail.smtp.host", "hostname"); + properties.put("mail.smtp.socketFactory.class", "classname"); + + final jakarta.mail.Authenticator authenticator = new jakarta.mail.Authenticator() { + protected PasswordAuthentication getPasswordAuthentication() { + return new PasswordAuthentication("username", "password"); + } + }; + if (null != authenticator) { + properties.put("mail.smtp.auth", "true"); + properties.put("mail.smtp.ssl.checkserveridentity", "true"); + } + final Session session = Session.getInstance(properties, authenticator); // Safe + } +} diff --git a/java/ql/test/query-tests/security/CWE-297/options b/java/ql/test/query-tests/security/CWE-297/options index 563a4fdcfc8..6fcd80260e3 100644 --- a/java/ql/test/query-tests/security/CWE-297/options +++ b/java/ql/test/query-tests/security/CWE-297/options @@ -1 +1 @@ -// semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/apache-commons-email-1.6.0:${testdir}/../../../stubs/javamail-api-1.6.2 +// semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/apache-commons-email-1.6.0:${testdir}/../../../stubs/javamail-api-1.6.2:${testdir}/../../../stubs/jakarta-mail-2.0.1 diff --git a/java/ql/test/stubs/jakarta-mail-2.0.1/jakarta/mail/Authenticator.java b/java/ql/test/stubs/jakarta-mail-2.0.1/jakarta/mail/Authenticator.java new file mode 100644 index 00000000000..594f3876f19 --- /dev/null +++ b/java/ql/test/stubs/jakarta-mail-2.0.1/jakarta/mail/Authenticator.java @@ -0,0 +1,20 @@ +/* + * Copyright (c) 1997, 2020 Oracle and/or its affiliates. All rights reserved. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v. 2.0, which is available at + * http://www.eclipse.org/legal/epl-2.0. + * + * This Source Code may also be made available under the following Secondary + * Licenses when the conditions for such availability set forth in the + * Eclipse Public License v. 2.0 are satisfied: GNU General Public License, + * version 2 with the GNU Classpath Exception, which is available at + * https://www.gnu.org/software/classpath/license.html. + * + * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 + */ + +package jakarta.mail; + +public abstract class Authenticator { +} diff --git a/java/ql/test/stubs/jakarta-mail-2.0.1/jakarta/mail/PasswordAuthentication.java b/java/ql/test/stubs/jakarta-mail-2.0.1/jakarta/mail/PasswordAuthentication.java new file mode 100644 index 00000000000..20751b7f7d9 --- /dev/null +++ b/java/ql/test/stubs/jakarta-mail-2.0.1/jakarta/mail/PasswordAuthentication.java @@ -0,0 +1,28 @@ +/* + * Copyright (c) 1997, 2020 Oracle and/or its affiliates. All rights reserved. + * + * This program and the accompanying materials are made available under the terms of the Eclipse + * Public License v. 2.0, which is available at http://www.eclipse.org/legal/epl-2.0. + * + * This Source Code may also be made available under the following Secondary Licenses when the + * conditions for such availability set forth in the Eclipse Public License v. 2.0 are satisfied: + * GNU General Public License, version 2 with the GNU Classpath Exception, which is available at + * https://www.gnu.org/software/classpath/license.html. + * + * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 + */ + +package jakarta.mail; + +public final class PasswordAuthentication { + public PasswordAuthentication(String userName, String password) {} + + public String getUserName() { + return null; + } + + public String getPassword() { + return null; + } + +} diff --git a/java/ql/test/stubs/jakarta-mail-2.0.1/jakarta/mail/Session.java b/java/ql/test/stubs/jakarta-mail-2.0.1/jakarta/mail/Session.java new file mode 100644 index 00000000000..6f525babdae --- /dev/null +++ b/java/ql/test/stubs/jakarta-mail-2.0.1/jakarta/mail/Session.java @@ -0,0 +1,80 @@ +/* + * Copyright (c) 1997, 2020 Oracle and/or its affiliates. All rights reserved. + * + * This program and the accompanying materials are made available under the terms of the Eclipse + * Public License v. 2.0, which is available at http://www.eclipse.org/legal/epl-2.0. + * + * This Source Code may also be made available under the following Secondary Licenses when the + * conditions for such availability set forth in the Eclipse Public License v. 2.0 are satisfied: + * GNU General Public License, version 2 with the GNU Classpath Exception, which is available at + * https://www.gnu.org/software/classpath/license.html. + * + * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 + */ + +package jakarta.mail; + +import java.lang.reflect.*; +import java.io.*; +import java.net.*; +import java.security.*; +import java.util.Properties; + +public final class Session { + public static Session getInstance(Properties props, Authenticator authenticator) { + return null; + } + + public static Session getInstance(Properties props) { + return null; + } + + public static synchronized Session getDefaultInstance(Properties props, + Authenticator authenticator) { + return null; + } + + public static Session getDefaultInstance(Properties props) { + return null; + } + + public synchronized void setDebug(boolean debug) {} + + public synchronized boolean getDebug() { + return false; + } + + public synchronized void setDebugOut(PrintStream out) {} + + public synchronized PrintStream getDebugOut() { + return null; + } + + public synchronized Provider[] getProviders() { + return null; + } + + public synchronized Provider getProvider(String protocol) throws NoSuchProviderException { + return null; + } + + public synchronized void setProvider(Provider provider) throws NoSuchProviderException {} + + public PasswordAuthentication requestPasswordAuthentication(InetAddress addr, int port, + String protocol, String prompt, String defaultUserName) { + return null; + } + + public Properties getProperties() { + return null; + } + + public String getProperty(String name) { + return null; + } + + public synchronized void addProvider(Provider provider) {} + + public synchronized void setProtocolForAddress(String addresstype, String protocol) {} + +} From 2d1278ece50a45c8d21ebb452ca1e52f72976e12 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 30 Jun 2021 12:22:48 +0200 Subject: [PATCH 07/11] 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(); + } + } +} From 9c1021134a86257f55aaccb419b0649665625359 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 30 Jun 2021 13:37:20 +0200 Subject: [PATCH 08/11] Add some links to qhelp --- .../src/Security/CWE/CWE-297/InsecureJavaMail.qhelp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.qhelp b/java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.qhelp index 9945227ff0f..06a3df57b18 100644 --- a/java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.qhelp +++ b/java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.qhelp @@ -19,9 +19,17 @@ credentials are sent in an SSL session without certificate validation. In the 'G +
  • + Jakarta Mail: + SSL Notes. +
    • +
  • + Apache Commons: + Email security. +
  • Log4j2: - Add support for specifying an SSL configuration for SmtpAppender (CVE-2020-9488) + Add support for specifying an SSL configuration for SmtpAppender (CVE-2020-9488).
    • - + \ No newline at end of file From 9f54b1065a407b4d6baa10d76979d5251e540f67 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 29 Jul 2021 17:30:01 +0200 Subject: [PATCH 09/11] Apply suggestions from code review Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com> --- java/change-notes/2021-06-18-insecure-java-mail-query.md | 2 +- java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.ql | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/java/change-notes/2021-06-18-insecure-java-mail-query.md b/java/change-notes/2021-06-18-insecure-java-mail-query.md index 495a7019f9a..e2778ec1b02 100644 --- a/java/change-notes/2021-06-18-insecure-java-mail-query.md +++ b/java/change-notes/2021-06-18-insecure-java-mail-query.md @@ -1,2 +1,2 @@ lgtm,codescanning -* The query "Insecure JavaMail SSL Configuration" (`java/insecure-smtp-ssl`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @luchua-bc](https://github.com/github/codeql/pull/3491) \ No newline at end of file +* The query "Insecure JavaMail SSL Configuration" (`java/insecure-smtp-ssl`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @luchua-bc](https://github.com/github/codeql/pull/3491). diff --git a/java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.ql b/java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.ql index 9697b2ae588..3c6fff0678d 100644 --- a/java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.ql +++ b/java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.ql @@ -1,8 +1,8 @@ /** * @name Insecure JavaMail SSL Configuration - * @description Java application configured to use authenticated mail session - * over SSL does not validate the SSL certificate to properly - * ensure that it is actually associated with that host. + * @description Configuring a Java application to use authenticated mail session + * over SSL without certificate validation + * makes the session susceptible to a man-in-the-middle attack. * @kind problem * @problem.severity warning * @precision medium From 3323f7ab1afeba4ef470b9f0b4575306fcb40f14 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 29 Jul 2021 17:34:08 +0200 Subject: [PATCH 10/11] Fix qhelp --- java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.qhelp b/java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.qhelp index 06a3df57b18..406d9666ce6 100644 --- a/java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.qhelp +++ b/java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.qhelp @@ -3,7 +3,7 @@

    JavaMail is commonly used in Java applications to send emails. There are popular third-party libraries like Apache Commons Email which are built on JavaMail and facilitate integration. Authenticated mail sessions require user credentials and mail sessions can require SSL/TLS authentication. It is a common security vulnerability that host-specific certificate data is not validated or is incorrectly validated. Failing to validate the certificate makes the SSL session susceptible to a man-in-the-middle attack.

    -

    This query checks whether SSL certificate is validated when username/password is sent in authenticator and when SSL is enabled.

    +

    This query checks whether the SSL certificate is validated when credentials are used and SSL is enabled in email communications.

    The query has code for both plain JavaMail invocation and mailing through Apache SimpleMail to make it more comprehensive.

     From a86cbd884edf9a2a92593481c359ba51ce888ade Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Tue, 5 Oct 2021 09:40:22 +0200 Subject: [PATCH 11/11] Apply suggestions from code review Co-authored-by: Anders Schack-Mulligen --- .../semmle/code/java/frameworks/Mail.qll | 0 .../semmle/code/java/security/Mail.qll | 18 ++++++++++-------- .../Security/CWE/CWE-297/InsecureJavaMail.ql | 5 +++-- .../security/CWE-297/InsecureJavaMailTest.ql | 5 +++-- 4 files changed, 16 insertions(+), 12 deletions(-) rename java/ql/{src => lib}/semmle/code/java/frameworks/Mail.qll (100%) rename java/ql/{src => lib}/semmle/code/java/security/Mail.qll (78%) diff --git a/java/ql/src/semmle/code/java/frameworks/Mail.qll b/java/ql/lib/semmle/code/java/frameworks/Mail.qll similarity index 100% rename from java/ql/src/semmle/code/java/frameworks/Mail.qll rename to java/ql/lib/semmle/code/java/frameworks/Mail.qll diff --git a/java/ql/src/semmle/code/java/security/Mail.qll b/java/ql/lib/semmle/code/java/security/Mail.qll similarity index 78% rename from java/ql/src/semmle/code/java/security/Mail.qll rename to java/ql/lib/semmle/code/java/security/Mail.qll index c100e5c49dc..e126ca57dff 100644 --- a/java/ql/src/semmle/code/java/security/Mail.qll +++ b/java/ql/lib/semmle/code/java/security/Mail.qll @@ -10,10 +10,10 @@ private import semmle.code.java.frameworks.Properties * set the `mail.smtp.ssl.socketFactory`/`mail.smtp.ssl.socketFactory.class` property to create an SMTP SSL socket. * 2. No `mail.smtp.ssl.checkserveridentity` property is enabled. */ -predicate isInsecureMailPropertyConfig(VarAccess propertiesVarAccess) { +predicate isInsecureMailPropertyConfig(Variable properties) { exists(MethodAccess ma | ma.getMethod() instanceof SetPropertyMethod and - ma.getQualifier() = propertiesVarAccess.getVariable().getAnAccess() + ma.getQualifier() = properties.getAnAccess() | getStringValue(ma.getArgument(0)).matches("%.auth%") and //mail.smtp.auth getStringValue(ma.getArgument(1)) = "true" @@ -22,7 +22,7 @@ predicate isInsecureMailPropertyConfig(VarAccess propertiesVarAccess) { ) and not exists(MethodAccess ma | ma.getMethod() instanceof SetPropertyMethod and - ma.getQualifier() = propertiesVarAccess.getVariable().getAnAccess() + ma.getQualifier() = properties.getAnAccess() | getStringValue(ma.getArgument(0)).matches("%.ssl.checkserveridentity%") and //mail.smtp.ssl.checkserveridentity getStringValue(ma.getArgument(1)) = "true" @@ -39,11 +39,11 @@ predicate enablesEmailSsl(MethodAccess ma) { } /** - * Holds if a SSL certificate check is enabled on `va` with Apache Email + * Holds if a SSL certificate check is enabled on an access of `apacheEmail` with Apache Email. */ -predicate hasSslCertificateCheck(VarAccess va) { +predicate hasSslCertificateCheck(Variable apacheEmail) { exists(MethodAccess ma | - ma.getQualifier() = va.getVariable().getAnAccess() and + ma.getQualifier() = apacheEmail.getAnAccess() and ma.getMethod().hasName("setSSLCheckServerIdentity") and ma.getMethod().getDeclaringType() instanceof ApacheEmail and ma.getArgument(0).(BooleanLiteral).getBooleanValue() = true @@ -51,7 +51,8 @@ predicate hasSslCertificateCheck(VarAccess va) { } /** - * Helper method to get string value of an argument + * Returns the string value of `expr` if it is a `CompileTimeConstantExpr`, + * or the string value of its operands if it is an `AddExpr`. */ private string getStringValue(Expr expr) { result = expr.(CompileTimeConstantExpr).getStringValue() @@ -60,7 +61,8 @@ private string getStringValue(Expr expr) { } /** - * A method to set Java properties + * A method to set Java properties, either using the `Properties` class + * or the `Dictionary` class. */ private class SetPropertyMethod extends Method { SetPropertyMethod() { diff --git a/java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.ql b/java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.ql index 3c6fff0678d..10e122d31c7 100644 --- a/java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.ql +++ b/java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.ql @@ -5,6 +5,7 @@ * makes the session susceptible to a man-in-the-middle attack. * @kind problem * @problem.severity warning + * @security-severity 5.9 * @precision medium * @id java/insecure-smtp-ssl * @tags security @@ -17,7 +18,7 @@ import semmle.code.java.security.Mail from MethodAccess ma where ma.getMethod() instanceof MailSessionGetInstanceMethod and - isInsecureMailPropertyConfig(ma.getArgument(0)) + isInsecureMailPropertyConfig(ma.getArgument(0).(VarAccess).getVariable()) or - enablesEmailSsl(ma) and not hasSslCertificateCheck(ma.getQualifier()) + enablesEmailSsl(ma) and not hasSslCertificateCheck(ma.getQualifier().(VarAccess).getVariable()) select ma, "Java mailing has insecure SSL configuration" diff --git a/java/ql/test/query-tests/security/CWE-297/InsecureJavaMailTest.ql b/java/ql/test/query-tests/security/CWE-297/InsecureJavaMailTest.ql index 061aa6711f6..137dde369f9 100644 --- a/java/ql/test/query-tests/security/CWE-297/InsecureJavaMailTest.ql +++ b/java/ql/test/query-tests/security/CWE-297/InsecureJavaMailTest.ql @@ -15,9 +15,10 @@ class InsecureJavaMailTest extends InlineExpectationsTest { value = "" | ma.getMethod() instanceof MailSessionGetInstanceMethod and - isInsecureMailPropertyConfig(ma.getArgument(0)) + isInsecureMailPropertyConfig(ma.getArgument(0).(VarAccess).getVariable()) or - enablesEmailSsl(ma) and not hasSslCertificateCheck(ma.getQualifier()) + enablesEmailSsl(ma) and + not hasSslCertificateCheck(ma.getQualifier().(VarAccess).getVariable()) ) } }