Apply suggestions from code review

Co-authored-by: Anders Schack-Mulligen <[email protected]>

Author: atorralba

java/ql/src/semmle/code/java/frameworks/Mail.qll renamed to java/ql/lib/semmle/code/java/frameworks/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,19 +39,20 @@ 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
   )
 }
 
 /**
- * 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() {

java/ql/src/semmle/code/java/security/Mail.qll renamed to java/ql/lib/semmle/code/java/security/Mail.qll

@@ -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"

java/ql/src/Security/CWE/CWE-297/InsecureJavaMail.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())
     )
   }
 }