diff --git a/java/ql/src/experimental/Security/CWE/CWE-089/MyBatisAnnotationSqlInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-089/MyBatisAnnotationSqlInjection.ql index c46888fbd64..2d1e605c426 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-089/MyBatisAnnotationSqlInjection.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-089/MyBatisAnnotationSqlInjection.ql @@ -53,7 +53,7 @@ where unsafeExpression = getAMybatisAnnotationSqlValue(isoa) and ( isMybatisXmlOrAnnotationSqlInjection(sink.getNode(), ma, unsafeExpression) or - isMybatisAnnotationCollectionTypeSqlInjection(sink.getNode(), ma, unsafeExpression) + isMybatisCollectionTypeSqlInjection(sink.getNode(), ma, unsafeExpression) ) select sink.getNode(), source, sink, "MyBatis annotation SQL injection might include code from $@ to $@.", source.getNode(), diff --git a/java/ql/src/experimental/Security/CWE/CWE-089/MyBatisCommonLib.qll b/java/ql/src/experimental/Security/CWE/CWE-089/MyBatisCommonLib.qll index 2039ffb1276..e7956821a40 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-089/MyBatisCommonLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-089/MyBatisCommonLib.qll @@ -76,7 +76,7 @@ string getAMybatisAnnotationSqlValue(IbatisSqlOperationAnnotation isoa) { /** Holds if `node` is an argument to `ma` that is vulnerable to SQL injection attacks if `unsafeExpression` occurs in a MyBatis SQL expression. */ bindingset[unsafeExpression] -predicate isMybatisAnnotationCollectionTypeSqlInjection( +predicate isMybatisCollectionTypeSqlInjection( DataFlow::Node node, MethodAccess ma, string unsafeExpression ) { not unsafeExpression.regexpMatch("\\$\\{" + getAMybatisConfigurationVariableKey() + "\\}") and @@ -100,38 +100,6 @@ predicate isMybatisAnnotationCollectionTypeSqlInjection( ) } -/** Holds if `node` is an argument to `ma` that is vulnerable to SQL injection attacks if `unsafeExpression` occurs in a MyBatis SQL expression. */ -bindingset[unsafeExpression] -predicate isMybatisXmlCollectionTypeSqlInjection( - DataFlow::Node node, MethodAccess ma, string unsafeExpression, MyBatisMapperXMLElement mmxe -) { - not unsafeExpression.regexpMatch("\\$\\{" + getAMybatisConfigurationVariableKey() + "\\}") and - // The parameter type of the MyBatis method parameter is Map or List or Array. - // SQL injection vulnerability caused by improper use of this parameter. - // e.g. - // - // ```java - // Test test(Map map); - // - // ``` - exists(int i, MyBatisMapperForeach mbmf | - mbmf = mmxe and - not ma.getMethod().getParameter(i).getAnAnnotation().getType() instanceof TypeParam and - ( - ma.getMethod().getParameterType(i) instanceof MapType or - ma.getMethod().getParameterType(i) instanceof ListType or - ma.getMethod().getParameterType(i) instanceof Array - ) and - unsafeExpression.matches("${%}") and - ma.getArgument(i) = node.asExpr() - ) -} - /** Holds if `node` is an argument to `ma` that is vulnerable to SQL injection attacks if `unsafeExpression` occurs in a MyBatis SQL expression. */ bindingset[unsafeExpression] predicate isMybatisXmlOrAnnotationSqlInjection( diff --git a/java/ql/src/experimental/Security/CWE/CWE-089/MyBatisMapperXmlSqlInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-089/MyBatisMapperXmlSqlInjection.ql index cc25b6ad8b8..908234fa3f3 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-089/MyBatisMapperXmlSqlInjection.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-089/MyBatisMapperXmlSqlInjection.ql @@ -52,8 +52,10 @@ where myBatisMapperXMLElementFromMethod(ma.getMethod(), mmxe) and unsafeExpression = getAMybatisXmlSetValue(mmxe) and ( - isMybatisXmlOrAnnotationSqlInjection(sink.getNode(), ma, unsafeExpression) or - isMybatisXmlCollectionTypeSqlInjection(sink.getNode(), ma, unsafeExpression, mmxe) + isMybatisXmlOrAnnotationSqlInjection(sink.getNode(), ma, unsafeExpression) + or + mmxe instanceof MyBatisMapperForeach and + isMybatisCollectionTypeSqlInjection(sink.getNode(), ma, unsafeExpression) ) select sink.getNode(), source, sink, "MyBatis Mapper XML SQL injection might include code from $@ to $@.", source.getNode(),