Partial modification 2

Author: haby0

java/ql/lib/semmle/code/java/frameworks/MyBatis.qll

@@ -70,7 +70,7 @@ private class IbatisUpdateAnnotationType extends AnnotationType {
 }
 
 /**
- * Ibatis sql operation annotation.
+ * An Ibatis SQL operation annotation.
  */
 class IbatisSqlOperationAnnotation extends Annotation {
   IbatisSqlOperationAnnotation() {
@@ -84,9 +84,7 @@ class IbatisSqlOperationAnnotation extends Annotation {
    * Get the SQL statement string.
    */
   string getSqlValue() {
-    result = this.getValue("value").(CompileTimeConstantExpr).getStringValue() or
-    result =
-      this.getValue("value").(ArrayInit).getInit(_).(CompileTimeConstantExpr).getStringValue()
+    result = this.getAValue("value").(CompileTimeConstantExpr).getStringValue()
   }
 }
 

java/ql/src/experimental/Security/CWE/CWE-089/MyBatisAnnotationSqlInjection.qhelp

@@ -4,21 +4,21 @@
   "qhelp.dtd">
 <qhelp>
 <overview>
-<p>MyBatis allows operating the database by annotating a method with the annotations <code>@Select</code>, <code>@Insert</code>, etc. to construct dynamic SQL statements.
+<p>MyBatis uses methods with the annotations <code>@Select</code>, <code>@Insert</code>, etc. to construct dynamic SQL statements.
 If the syntax <code>${param}</code> is used in those statements, and <code>param</code> is a parameter of the annotated method, attackers can exploit this to tamper with the SQL statements or execute arbitrary SQL commands.</p>
 </overview>
 
 <<recommendation>
 <p>
-When writing MyBatis mapping statements, try to use the syntax <code>#{xxx}</code>. If the syntax <code>${xxx}</code> must be used, any parameters included in it should be sanitized to prevent SQL injection attacks.
+When writing MyBatis mapping statements, use the syntax <code>#{xxx}</code> whenever possible. If the syntax <code>${xxx}</code> must be used, any parameters included in it should be sanitized to prevent SQL injection attacks.
 </p>
 </recommendation>
 
 <example>
 <p>
 The following sample shows a bad and a good example of MyBatis annotations usage. The <code>bad1</code> method uses <code>$(name)</code> 
 in the <code>@Select</code> annotation to dynamically build a SQL statement, which causes a SQL injection vulnerability. 
-The <code>good1</code> method uses <code>#{name}</code> in the <code>@Select</code> annotation to to dynamically include the parameter in a SQL statement, which allows the MyBatis framework to handle the sanitization, preventing the vulnerability.
+The <code>good1</code> method uses <code>#{name}</code> in the <code>@Select</code> annotation to dynamically include the parameter in a SQL statement, which causes the MyBatis framework to sanitize the input provided, preventing the vulnerability.
 </p>
 <sample src="MyBatisAnnotationSqlInjection.java" />
 </example>

java/ql/src/experimental/Security/CWE/CWE-089/MyBatisAnnotationSqlInjection.ql

@@ -23,7 +23,7 @@ private class MyBatisAnnotationSqlInjectionConfiguration extends TaintTracking::
   override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
 
   override predicate isSink(DataFlow::Node sink) {
-    sink instanceof MyBatisAnnotationMethodCallAnArgument
+    sink instanceof MyBatisAnnotatedMethodCallArgument
   }
 
   override predicate isSanitizer(DataFlow::Node node) {

java/ql/src/experimental/Security/CWE/CWE-089/MyBatisAnnotationSqlInjectionLib.qll

@@ -1,15 +1,15 @@
 /**
- * Provides classes for SQL injection detection in MyBatis annotation.
+ * Provides classes for SQL injection detection regarding MyBatis annotated methods.
  */
 
 import java
 import MyBatisCommonLib
 import semmle.code.java.dataflow.FlowSources
 import semmle.code.java.frameworks.Properties
 
-/** A sink for MyBatis annotation method call an argument. */
-class MyBatisAnnotationMethodCallAnArgument extends DataFlow::Node {
-  MyBatisAnnotationMethodCallAnArgument() {
+/** An argument of a MyBatis annotated method. */
+class MyBatisAnnotatedMethodCallArgument extends DataFlow::Node {
+  MyBatisAnnotatedMethodCallArgument() {
     exists(MyBatisSqlOperationAnnotationMethod msoam, MethodAccess ma | ma.getMethod() = msoam |
       ma.getAnArgument() = this.asExpr()
     )

java/ql/src/experimental/Security/CWE/CWE-089/MyBatisCommonLib.qll

@@ -68,12 +68,12 @@ MethodAccess getMyBatisSqlOperationAnnotationMethodAccess(IbatisSqlOperationAnno
 }
 
 /** Get the #{...} or ${...} parameters in the Mybatis mapper xml file. */
-private string getAnMybatiXmlSetValue(XMLElement xmle) {
+private string getAnMybatisXmlSetValue(XMLElement xmle) {
   result = xmle.getTextValue().trim().regexpFind("(#|\\$)\\{[^\\}]*\\}", _, _)
 }
 
 /** Get the #{...} or ${...} parameters in the Mybatis sql operation annotation value. */
-private string getAnMybatiAnnotationSetValue(IbatisSqlOperationAnnotation isoa) {
+private string getAMybatisAnnotationSqlValue(IbatisSqlOperationAnnotation isoa) {
   result = isoa.getSqlValue().trim().regexpFind("(#|\\$)\\{[^\\}]*\\}", _, _)
 }
 
@@ -84,11 +84,12 @@ predicate isMybatisXmlOrAnnotationSqlInjection(
   exists(MethodAccess ma, string setValue |
     (
       ma = getMyBatisMapperXmlMethodAccess(xmle) and
-      setValue = getAnMybatiXmlSetValue(xmle)
+      setValue = getAnMybatisXmlSetValue(xmle)
       or
       ma = getMyBatisSqlOperationAnnotationMethodAccess(isoa) and
-      setValue = getAnMybatiAnnotationSetValue(isoa)
+      setValue = getAMybatisAnnotationSqlValue(isoa)
     ) and
+    not setValue.regexpMatch("\\$\\{" + getAnMybatisConfigurationVariableKey() + "\\}") and
     (
       // The method parameters use `@Param` annotation. Due to improper use of this parameter, SQL injection vulnerabilities are caused.
       // e.g.
@@ -154,7 +155,6 @@ predicate isMybatisXmlOrAnnotationSqlInjection(
           ma.getMethod().getParameterType(i) instanceof Array
         ) and
         setValue.matches("%${%}") and
-        not setValue = "${" + getAnMybatisConfigurationVariableKey() + "}" and
         ma.getArgument(i) = node.asExpr()
       )
       or
@@ -170,7 +170,6 @@ predicate isMybatisXmlOrAnnotationSqlInjection(
         ma.getMethod().getNumberOfParameters() = i and
         not ma.getMethod().getAParameter().getAnAnnotation().getType() instanceof TypeParam and
         setValue.matches("%${%}") and
-        not setValue = "${" + getAnMybatisConfigurationVariableKey() + "}" and
         ma.getAnArgument() = node.asExpr()
       )
     )