From 263de5219ad6287cbdd76a795380dae900faeb6f Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Wed, 24 Oct 2018 13:58:21 +0200 Subject: [PATCH] Java: Add additional SQL injection sinks. --- change-notes/1.19/analysis-java.md | 2 ++ .../Security/CWE/CWE-089/SqlInjectionLib.qll | 18 ++++++++-- .../semmle/code/java/frameworks/Hibernate.qll | 23 ++++++++++++ .../semmle/code/java/frameworks/MyBatis.qll | 27 ++++++++++++++ .../code/java/frameworks/SpringJdbc.qll | 35 +++++++++++++++++++ 5 files changed, 103 insertions(+), 2 deletions(-) create mode 100644 java/ql/src/semmle/code/java/frameworks/Hibernate.qll create mode 100644 java/ql/src/semmle/code/java/frameworks/MyBatis.qll create mode 100644 java/ql/src/semmle/code/java/frameworks/SpringJdbc.qll diff --git a/change-notes/1.19/analysis-java.md b/change-notes/1.19/analysis-java.md index f2d0d5adf97..ee48650e161 100644 --- a/change-notes/1.19/analysis-java.md +++ b/change-notes/1.19/analysis-java.md @@ -12,6 +12,8 @@ | **Query** | **Expected impact** | **Change** | |----------------------------|------------------------|------------------------------------------------------------------| | Array index out of bounds (`java/index-out-of-bounds`) | Fewer false positive results | False positives involving arrays with a length evenly divisible by 3 or some greater number and an index being increased with a similar stride length are no longer reported. | +| Query built from user-controlled sources (`java/sql-injection`) | More results | Sql injection sinks from the Spring JDBC, MyBatis, and Hibernate frameworks are now reported. | +| Query built without neutralizing special characters (`java/concatenated-sql-query`) | More results | Sql injection sinks from the Spring JDBC, MyBatis, and Hibernate frameworks are now reported. | | Unreachable catch clause (`java/unreachable-catch-clause`) | Fewer false positive results | This rule now accounts for calls to generic methods that throw generic exceptions. | | Useless comparison test (`java/constant-comparison`) | Fewer false positive results | Constant comparisons guarding `java.util.ConcurrentModificationException` are no longer reported, as they are intended to always be false in the absence of API misuse. | diff --git a/java/ql/src/Security/CWE/CWE-089/SqlInjectionLib.qll b/java/ql/src/Security/CWE/CWE-089/SqlInjectionLib.qll index b8c80ddf86a..74288aec5e5 100644 --- a/java/ql/src/Security/CWE/CWE-089/SqlInjectionLib.qll +++ b/java/ql/src/Security/CWE/CWE-089/SqlInjectionLib.qll @@ -4,6 +4,9 @@ import semmle.code.java.Expr import semmle.code.java.dataflow.FlowSources import semmle.code.java.frameworks.android.SQLite import semmle.code.java.frameworks.javaee.Persistence +import semmle.code.java.frameworks.SpringJdbc +import semmle.code.java.frameworks.MyBatis +import semmle.code.java.frameworks.Hibernate /** A sink for database query language injection vulnerabilities. */ abstract class QueryInjectionSink extends DataFlow::ExprNode { } @@ -13,8 +16,19 @@ class SqlInjectionSink extends QueryInjectionSink { SqlInjectionSink() { this.getExpr() instanceof SqlExpr or - exists(SQLiteRunner s, MethodAccess m | m.getMethod() = s | - m.getArgument(s.sqlIndex()) = this.getExpr() + exists(MethodAccess ma, Method m, int index | + ma.getMethod() = m and + ma.getArgument(index) = this.getExpr() + | + index = m.(SQLiteRunner).sqlIndex() + or + m instanceof BatchUpdateVarargsMethod + or + index = 0 and jdbcSqlMethod(m) + or + index = 0 and mybatisSqlMethod(m) + or + index = 0 and hibernateSqlMethod(m) ) } } diff --git a/java/ql/src/semmle/code/java/frameworks/Hibernate.qll b/java/ql/src/semmle/code/java/frameworks/Hibernate.qll new file mode 100644 index 00000000000..fdc237aac8a --- /dev/null +++ b/java/ql/src/semmle/code/java/frameworks/Hibernate.qll @@ -0,0 +1,23 @@ +/** + * Provides classes and predicates for working with the Hibernate framework. + */ + +import java + +/** The interface `org.hibernate.Session`. */ +class HibernateSession extends RefType { + HibernateSession() { this.hasQualifiedName("org.hibernate", "Session") } +} + +/** + * Holds if `m` is a method on `HibernateSession` taking an SQL string as its + * first argument. + */ +predicate hibernateSqlMethod(Method m) { + m.getDeclaringType() instanceof HibernateSession and + m.getParameterType(0) instanceof TypeString and + ( + m.hasName("createQuery") or + m.hasName("createSQLQuery") + ) +} diff --git a/java/ql/src/semmle/code/java/frameworks/MyBatis.qll b/java/ql/src/semmle/code/java/frameworks/MyBatis.qll new file mode 100644 index 00000000000..46827d0fb4f --- /dev/null +++ b/java/ql/src/semmle/code/java/frameworks/MyBatis.qll @@ -0,0 +1,27 @@ +/** + * Provides classes and predicates for working with the MyBatis framework. + */ + +import java + +/** The class `org.apache.ibatis.jdbc.SqlRunner`. */ +class MyBatisSqlRunner extends RefType { + MyBatisSqlRunner() { this.hasQualifiedName("org.apache.ibatis.jdbc", "SqlRunner") } +} + +/** + * Holds if `m` is a method on `MyBatisSqlRunner` taking an SQL string as its + * first argument. + */ +predicate mybatisSqlMethod(Method m) { + m.getDeclaringType() instanceof MyBatisSqlRunner and + m.getParameterType(0) instanceof TypeString and + ( + m.hasName("delete") or + m.hasName("insert") or + m.hasName("run") or + m.hasName("selectAll") or + m.hasName("selectOne") or + m.hasName("update") + ) +} diff --git a/java/ql/src/semmle/code/java/frameworks/SpringJdbc.qll b/java/ql/src/semmle/code/java/frameworks/SpringJdbc.qll new file mode 100644 index 00000000000..d563438951f --- /dev/null +++ b/java/ql/src/semmle/code/java/frameworks/SpringJdbc.qll @@ -0,0 +1,35 @@ +/** + * Provides classes and predicates for working with the Spring JDBC framework. + */ + +import java + +/** The class `org.springframework.jdbc.core.JdbcTemplate`. */ +class JdbcTemplate extends RefType { + JdbcTemplate() { this.hasQualifiedName("org.springframework.jdbc.core", "JdbcTemplate") } +} + +/** + * Holds if `m` is a method on `JdbcTemplate` taking an SQL string as its first + * argument. + */ +predicate jdbcSqlMethod(Method m) { + m.getDeclaringType() instanceof JdbcTemplate and + m.getParameterType(0) instanceof TypeString and + ( + m.hasName("batchUpdate") or + m.hasName("execute") or + m.getName().matches("query%") or + m.hasName("update") + ) +} + +/** The method `JdbcTemplate.batchUpdate(String... sql)` */ +class BatchUpdateVarargsMethod extends Method { + BatchUpdateVarargsMethod() { + this.getDeclaringType() instanceof JdbcTemplate and + this.hasName("batchUpdate") and + this.getParameterType(0).(Array).getComponentType() instanceof TypeString and + this.getParameter(0).isVarargs() + } +}