Merge pull request #18288 from jcogs33/jcogs33/csrf-unprotected-request-type

Java: add CSRF query

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

@@ -34,3 +34,19 @@ class ResultSetGetStringMethod extends Method {
     this.getReturnType() instanceof TypeString
   }
 }
+
+/** A method with the name `executeUpdate` declared in `java.sql.PreparedStatement`. */
+class PreparedStatementExecuteUpdateMethod extends Method {
+  PreparedStatementExecuteUpdateMethod() {
+    this.getDeclaringType() instanceof TypePreparedStatement and
+    this.hasName("executeUpdate")
+  }
+}
+
+/** A method with the name `executeLargeUpdate` declared in `java.sql.PreparedStatement`. */
+class PreparedStatementExecuteLargeUpdateMethod extends Method {
+  PreparedStatementExecuteLargeUpdateMethod() {
+    this.getDeclaringType() instanceof TypePreparedStatement and
+    this.hasName("executeLargeUpdate")
+  }
+}

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

@@ -128,3 +128,114 @@ private class MyBatisProviderStep extends TaintTracking::AdditionalValueStep {
     )
   }
 }
+
+/**
+ * A MyBatis Mapper XML file.
+ */
+class MyBatisMapperXmlFile extends XmlFile {
+  MyBatisMapperXmlFile() {
+    count(XmlElement e | e = this.getAChild()) = 1 and
+    this.getAChild().getName() = "mapper"
+  }
+}
+
+/**
+ * An XML element in a `MyBatisMapperXMLFile`.
+ */
+class MyBatisMapperXmlElement extends XmlElement {
+  MyBatisMapperXmlElement() { this.getFile() instanceof MyBatisMapperXmlFile }
+
+  /**
+   * Gets the value for this element, with leading and trailing whitespace trimmed.
+   */
+  string getValue() { result = this.allCharactersString().trim() }
+
+  /**
+   * Gets the reference type bound to MyBatis Mapper XML File.
+   */
+  RefType getNamespaceRefType() {
+    result.getQualifiedName() = this.getAttribute("namespace").getValue()
+  }
+}
+
+/**
+ * An MyBatis Mapper sql operation element.
+ */
+abstract class MyBatisMapperSqlOperation extends MyBatisMapperXmlElement {
+  /**
+   * Gets the value of the `id` attribute of MyBatis Mapper sql operation element.
+   */
+  string getId() { result = this.getAttribute("id").getValue() }
+
+  /**
+   * Gets the `<include>` element in a `MyBatisMapperSqlOperation`.
+   */
+  MyBatisMapperInclude getInclude() { result = this.getAChild*() }
+
+  /**
+   * Gets the method bound to MyBatis Mapper XML File.
+   */
+  Method getMapperMethod() {
+    result.getName() = this.getId() and
+    result.getDeclaringType() = this.getParent().(MyBatisMapperXmlElement).getNamespaceRefType()
+  }
+}
+
+/**
+ * A `<insert>` element in a `MyBatisMapperSqlOperation`.
+ */
+class MyBatisMapperInsert extends MyBatisMapperSqlOperation {
+  MyBatisMapperInsert() { this.getName() = "insert" }
+}
+
+/**
+ * A `<update>` element in a `MyBatisMapperSqlOperation`.
+ */
+class MyBatisMapperUpdate extends MyBatisMapperSqlOperation {
+  MyBatisMapperUpdate() { this.getName() = "update" }
+}
+
+/**
+ * A `<delete>` element in a `MyBatisMapperSqlOperation`.
+ */
+class MyBatisMapperDelete extends MyBatisMapperSqlOperation {
+  MyBatisMapperDelete() { this.getName() = "delete" }
+}
+
+/**
+ * A `<select>` element in a `MyBatisMapperSqlOperation`.
+ */
+class MyBatisMapperSelect extends MyBatisMapperSqlOperation {
+  MyBatisMapperSelect() { this.getName() = "select" }
+}
+
+/**
+ * A `<sql>` element in a `MyBatisMapperXMLElement`.
+ */
+class MyBatisMapperSql extends MyBatisMapperXmlElement {
+  MyBatisMapperSql() { this.getName() = "sql" }
+
+  /**
+   * Gets the value of the `id` attribute of this `<sql>`.
+   */
+  string getId() { result = this.getAttribute("id").getValue() }
+}
+
+/**
+ * A `<include>` element in a `MyBatisMapperXMLElement`.
+ */
+class MyBatisMapperInclude extends MyBatisMapperXmlElement {
+  MyBatisMapperInclude() { this.getName() = "include" }
+
+  /**
+   * Gets the value of the `refid` attribute of this `<include>`.
+   */
+  string getRefid() { result = this.getAttribute("refid").getValue() }
+}
+
+/**
+ * A `<foreach>` element in a `MyBatisMapperXMLElement`.
+ */
+class MyBatisMapperForeach extends MyBatisMapperXmlElement {
+  MyBatisMapperForeach() { this.getName() = "foreach" }
+}

java/ql/lib/semmle/code/java/frameworks/spring/SpringController.qll

@@ -156,6 +156,11 @@ class SpringRequestMappingMethod extends SpringControllerMethod {
   /** Gets the "value" @RequestMapping annotation value, if present. */
   string getValue() { result = requestMappingAnnotation.getStringValue("value") }
 
+  /** Gets the "method" @RequestMapping annotation value, if present. */
+  string getMethodValue() {
+    result = requestMappingAnnotation.getAnEnumConstantArrayValue("method").getName()
+  }
+
   /** Holds if this is considered an `@ResponseBody` method. */
   predicate isResponseBody() {
     this.getAnAnnotation().getType() instanceof SpringResponseBodyAnnotationType or

java/ql/lib/semmle/code/java/frameworks/stapler/Stapler.qll

@@ -122,3 +122,40 @@ private class PostConstructDataBoundMethod extends Method {
     this.getAnAnnotation() instanceof PostConstructAnnotation
   }
 }
+
+/**
+ * A method intended for Stapler request routing.
+ *
+ * From: https://www.jenkins.io/doc/developer/handling-requests/actions/
+ *   Web methods need to provide some indication that they are intended for Stapler routing:
+ *    - Any applicable annotation recognized by Stapler, e.g., @RequirePOST.
+ *    - Any inferable parameter type, e.g., StaplerRequest.
+ *    - Any applicable parameter annotation, recognized by Stapler, e.g., @AncestorInPath.
+ *    - Any declared exception type implementing HttpResponse, e.g., HttpResponseException.
+ *    - A return type implementing HttpResponse.
+ */
+class StaplerWebMethod extends Method {
+  StaplerWebMethod() {
+    // Any applicable annotation recognized by Stapler, e.g., @RequirePOST.
+    this.hasAnnotation("org.kohsuke.stapler", "WebMethod")
+    or
+    this.hasAnnotation("org.kohsuke.stapler.interceptor", ["RequirePOST", "RespondSuccess"])
+    or
+    this.hasAnnotation("org.kohsuke.stapler.verb", ["DELETE", "GET", "POST", "PUT"])
+    or
+    // Any inferable parameter type, e.g., StaplerRequest.
+    this.getAParamType()
+        .(RefType)
+        .hasQualifiedName("org.kohsuke.stapler", ["StaplerRequest", "StaplerRequest2"])
+    or
+    // Any applicable parameter annotation, recognized by Stapler, e.g., @AncestorInPath
+    this.getAParameter()
+        .hasAnnotation("org.kohsuke.stapler", ["AncestorInPath", "QueryParameter", "Header"])
+    or
+    // A return type implementing HttpResponse
+    this.getReturnType().(RefType).getASourceSupertype*() instanceof HttpResponse
+    or
+    // Any declared exception type implementing HttpResponse, e.g., HttpResponseException
+    this.getAThrownExceptionType().getASourceSupertype*() instanceof HttpResponse
+  }
+}

java/ql/lib/semmle/code/java/security/CsrfUnprotectedRequestTypeQuery.qll

@@ -0,0 +1,266 @@
+/** Provides classes and predicates to reason about CSRF vulnerabilities due to use of unprotected HTTP request types. */
+
+import java
+private import semmle.code.java.frameworks.spring.SpringController
+private import semmle.code.java.frameworks.stapler.Stapler
+private import semmle.code.java.frameworks.MyBatis
+private import semmle.code.java.frameworks.Jdbc
+private import semmle.code.java.dataflow.ExternalFlow
+private import semmle.code.java.dispatch.VirtualDispatch
+private import semmle.code.java.dataflow.TaintTracking
+import CallGraph
+
+/** A method that is not protected from CSRF by default. */
+abstract class CsrfUnprotectedMethod extends Method { }
+
+/**
+ * A Spring request mapping method that is not protected from CSRF by default.
+ *
+ * https://docs.spring.io/spring-security/reference/features/exploits/csrf.html#csrf-protection-read-only
+ */
+private class SpringCsrfUnprotectedMethod extends CsrfUnprotectedMethod instanceof SpringRequestMappingMethod
+{
+  SpringCsrfUnprotectedMethod() {
+    this.hasAnnotation("org.springframework.web.bind.annotation", "GetMapping")
+    or
+    this.hasAnnotation("org.springframework.web.bind.annotation", "RequestMapping") and
+    (
+      this.getMethodValue() = ["GET", "HEAD"]
+      or
+      // If no request type is specified with `@RequestMapping`, then all request types
+      // are possible, so we treat this as unsafe; example: @RequestMapping(value = "test").
+      not exists(this.getMethodValue())
+    )
+  }
+}
+
+/**
+ * A Stapler web method that is not protected from CSRF by default.
+ *
+ * https://www.jenkins.io/doc/developer/security/form-validation/#protecting-from-csrf
+ */
+private class StaplerCsrfUnprotectedMethod extends CsrfUnprotectedMethod instanceof StaplerWebMethod
+{
+  StaplerCsrfUnprotectedMethod() {
+    not this.hasAnnotation("org.kohsuke.stapler.interceptor", "RequirePOST") and
+    // Jenkins only explicitly protects against CSRF for POST requests, but we
+    // also exclude PUT and DELETE since these request types are only exploitable
+    // if there is a CORS issue.
+    not this.hasAnnotation("org.kohsuke.stapler.verb", ["POST", "PUT", "DELETE"])
+  }
+}
+
+/** Gets a word that is interesting because it may indicate a state change. */
+private string getAnInterestingWord() {
+  result =
+    [
+      "post", "put", "patch", "delete", "remove", "create", "add", "update", "edit", "publish",
+      "unpublish", "fill", "move", "transfer", "logout", "login", "access", "connect", "connection",
+      "register", "submit"
+    ]
+}
+
+/**
+ * Gets the regular expression used for matching strings that look like they
+ * contain an interesting word.
+ */
+private string getInterestingWordRegex() {
+  result = "(^|\\w+(?=[A-Z]))((?i)" + concat(getAnInterestingWord(), "|") + ")($|(?![a-z])\\w+)"
+}
+
+/** Gets a word that is uninteresting because it likely does not indicate a state change. */
+private string getAnUninterestingWord() {
+  result = ["get", "show", "view", "list", "query", "find"]
+}
+
+/**
+ * Gets the regular expression used for matching strings that look like they
+ * contain an uninteresting word.
+ */
+private string getUninterestingWordRegex() {
+  result = "^(" + concat(getAnUninterestingWord(), "|") + ")(?![a-z])\\w*"
+}
+
+/** A method that appears to change application state based on its name. */
+private class NameBasedStateChangeMethod extends Method {
+  NameBasedStateChangeMethod() {
+    this.getName().regexpMatch(getInterestingWordRegex()) and
+    not this.getName().regexpMatch(getUninterestingWordRegex())
+  }
+}
+
+/** A method that updates a database. */
+abstract class DatabaseUpdateMethod extends Method { }
+
+/** A MyBatis method that updates a database. */
+private class MyBatisDatabaseUpdateMethod extends DatabaseUpdateMethod {
+  MyBatisDatabaseUpdateMethod() {
+    exists(MyBatisMapperSqlOperation mapperXml |
+      (
+        mapperXml instanceof MyBatisMapperInsert or
+        mapperXml instanceof MyBatisMapperUpdate or
+        mapperXml instanceof MyBatisMapperDelete
+      ) and
+      this = mapperXml.getMapperMethod()
+    )
+    or
+    exists(MyBatisSqlOperationAnnotationMethod m | this = m |
+      not m.getAnAnnotation().getType().hasQualifiedName("org.apache.ibatis.annotations", "Select")
+    )
+    or
+    exists(Method m | this = m |
+      m.hasAnnotation("org.apache.ibatis.annotations", ["Delete", "Update", "Insert"] + "Provider")
+    )
+  }
+}
+
+/** A method declared in `java.sql.PreparedStatement` that updates a database. */
+private class PreparedStatementDatabaseUpdateMethod extends DatabaseUpdateMethod {
+  PreparedStatementDatabaseUpdateMethod() {
+    this instanceof PreparedStatementExecuteUpdateMethod or
+    this instanceof PreparedStatementExecuteLargeUpdateMethod
+  }
+}
+
+/** A method found via the sql-injection sink models which may update a database. */
+private class SqlInjectionDatabaseUpdateMethod extends DatabaseUpdateMethod {
+  SqlInjectionDatabaseUpdateMethod() {
+    exists(DataFlow::Node n | this = n.asExpr().(Argument).getCall().getCallee() |
+      sinkNode(n, "sql-injection") and
+      // do not include `executeQuery` since it is typically used with a select statement
+      this.hasName([
+          "delete", "insert", "update", "batchUpdate", "executeUpdate", "executeLargeUpdate",
+          "execute"
+        ])
+    )
+  }
+}
+
+/**
+ * A taint-tracking configuration for reasoning about SQL statements that update
+ * a database via a call to an `execute` method.
+ */
+private module SqlExecuteConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    exists(StringLiteral sl | source.asExpr() = sl |
+      sl.getValue().regexpMatch("^(?i)(insert|update|delete).*")
+    )
+  }
+
+  predicate isSink(DataFlow::Node sink) {
+    exists(Method m | m = sink.asExpr().(Argument).getCall().getCallee() |
+      m instanceof SqlInjectionDatabaseUpdateMethod and
+      m.hasName("execute")
+    )
+  }
+}
+
+/**
+ * Tracks flow from SQL statements that update a database to the argument of
+ * an `execute` method call.
+ */
+private module SqlExecuteFlow = TaintTracking::Global<SqlExecuteConfig>;
+
+/** Provides classes and predicates representing call graph paths. */
+module CallGraph {
+  private newtype TCallPathNode =
+    TMethod(Method m) or
+    TCall(Call c)
+
+  /** A node in a call path graph */
+  class CallPathNode extends TCallPathNode {
+    /** Gets the method corresponding to this `CallPathNode`, if any. */
+    Method asMethod() { this = TMethod(result) }
+
+    /** Gets the call corresponding to this `CallPathNode`, if any. */
+    Call asCall() { this = TCall(result) }
+
+    /** Gets the string representation of this `CallPathNode`. */
+    string toString() {
+      result = this.asMethod().toString()
+      or
+      result = this.asCall().toString()
+    }
+
+    private CallPathNode getACallee() {
+      [viableCallable(this.asCall()), this.asCall().getCallee()] = result.asMethod()
+    }
+
+    pragma[nomagic]
+    private predicate canTargetDatabaseUpdateMethod() {
+      exists(CallPathNode p |
+        p = this.getACallee() and
+        p.asMethod() instanceof DatabaseUpdateMethod
+      )
+    }
+
+    /** Gets a successor node of this `CallPathNode`, if any. */
+    CallPathNode getASuccessor() {
+      this.asMethod() = result.asCall().getEnclosingCallable()
+      or
+      result = this.getACallee() and
+      (
+        this.canTargetDatabaseUpdateMethod()
+        implies
+        result.asMethod() instanceof DatabaseUpdateMethod
+      )
+    }
+
+    /** Gets the location of this `CallPathNode`. */
+    Location getLocation() {
+      result = this.asMethod().getLocation()
+      or
+      result = this.asCall().getLocation()
+    }
+  }
+
+  /** Holds if `pred` has a successor node `succ`. */
+  predicate edges(CallPathNode pred, CallPathNode succ) { pred.getASuccessor() = succ }
+}
+
+/** Holds if `sourceMethod` is an unprotected request handler. */
+private predicate source(CallPathNode sourceMethod) {
+  sourceMethod.asMethod() instanceof CsrfUnprotectedMethod
+}
+
+/** Holds if `sinkMethodCall` updates a database. */
+private predicate sink(CallPathNode sinkMethodCall) {
+  exists(CallPathNode sinkMethod |
+    sinkMethod.asMethod() instanceof DatabaseUpdateMethod and
+    sinkMethodCall.getASuccessor() = sinkMethod and
+    // exclude SQL `execute` calls that do not update database
+    if
+      sinkMethod.asMethod() instanceof SqlInjectionDatabaseUpdateMethod and
+      sinkMethod.asMethod().hasName("execute")
+    then SqlExecuteFlow::flowToExpr(sinkMethodCall.asCall().getAnArgument())
+    else any()
+  )
+}
+
+/**
+ * Holds if `sourceMethod` is an unprotected request handler that reaches a
+ * `sinkMethodCall` that updates a database.
+ */
+private predicate unprotectedDatabaseUpdate(CallPathNode sourceMethod, CallPathNode sinkMethodCall) =
+  doublyBoundedFastTC(CallGraph::edges/2, source/1, sink/1)(sourceMethod, sinkMethodCall)
+
+/**
+ * Holds if `sourceMethod` is an unprotected request handler that appears to
+ * change application state based on its name.
+ */
+private predicate unprotectedNameBasedStateChange(CallPathNode sourceMethod, CallPathNode sinkMethod) {
+  sourceMethod.asMethod() instanceof CsrfUnprotectedMethod and
+  sinkMethod.asMethod() instanceof NameBasedStateChangeMethod and
+  sinkMethod = sourceMethod and
+  // exclude any alerts that update a database
+  not unprotectedDatabaseUpdate(sourceMethod, _)
+}
+
+/**
+ * Holds if `source` is an unprotected request handler that may
+ * change an application's state.
+ */
+predicate unprotectedStateChange(CallPathNode source, CallPathNode sink) {
+  unprotectedDatabaseUpdate(source, sink) or
+  unprotectedNameBasedStateChange(source, sink)
+}

java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestType.qhelp

@@ -0,0 +1,72 @@
+<!DOCTYPE qhelp SYSTEM "qhelp.dtd">
+<qhelp>
+
+<overview>
+    <p>
+      Cross-site request forgery (CSRF) is a type of vulnerability in which an
+      attacker is able to force a user to carry out an action that the user did
+      not intend.
+    </p>
+
+    <p>
+      The attacker tricks an authenticated user into submitting a request to the
+      web application. Typically, this request will result in a state change on
+      the server, such as changing the user's password. The request can be
+      initiated when the user visits a site controlled by the attacker. If the
+      web application relies only on cookies for authentication, or on other
+      credentials that are automatically included in the request, then this
+      request will appear as legitimate to the server.
+    </p>
+</overview>
+
+<recommendation>
+<p>Make sure any requests that change application state are protected from CSRF. Some application
+frameworks provide default CSRF protection for unsafe HTTP request methods (such as <code>POST</code>)
+which may change the state of the application. Safe HTTP request methods (such as <code>GET</code>)
+should only perform read-only operations and should not be used for actions that change application
+state.</p>
+
+<p>This query currently supports the Spring and Stapler web frameworks. Spring provides default CSRF protection
+for all unsafe HTTP methods whereas Stapler provides default CSRF protection for the <code>POST</code> method.</p>
+</recommendation>
+
+<example>
+<p> The following examples show Spring request handlers allowing safe HTTP request methods for state-changing actions.
+Since safe HTTP request methods do not have default CSRF protection in Spring, they should not be used when modifying
+application state. Instead, use one of the unsafe HTTP methods which Spring default-protects from CSRF.</p>
+
+<sample src="CsrfUnprotectedRequestTypeBadSpring.java" />
+
+<sample src="CsrfUnprotectedRequestTypeGoodSpring.java" />
+
+<p> The following examples show Stapler web methods allowing safe HTTP request methods for state-changing actions.
+Since safe HTTP request methods do not have default CSRF protection in Stapler, they should not be used when modifying
+application state. Instead, use the <code>POST</code> method which Stapler default-protects from CSRF.</p>
+
+<sample src="CsrfUnprotectedRequestTypeBadStapler.java" />
+
+<sample src="CsrfUnprotectedRequestTypeGoodStapler.java" />
+</example>
+
+<references>
+<li>
+OWASP:
+<a href="https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)">Cross Site Request Forgery (CSRF)</a>.
+</li>
+<li>
+Spring Security Reference:
+<a href="https://docs.spring.io/spring-security/reference/servlet/exploits/csrf.html">
+  Cross Site Request Forgery (CSRF)</a>.
+</li>
+<li>
+Jenkins Developer Documentation:
+<a href="https://www.jenkins.io/doc/developer/security/form-validation/#protecting-from-csrf">
+  Protecting from CSRF</a>.
+</li>
+<li>
+MDN web docs:
+<a href="https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods">
+  HTTP request methods</a>.
+</li>
+</references>
+</qhelp>

java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestType.ql

@@ -0,0 +1,24 @@
+/**
+ * @name HTTP request type unprotected from CSRF
+ * @description Using an HTTP request type that is not default-protected from CSRF for a
+ *              state-changing action makes the application vulnerable to a Cross-Site
+ *              Request Forgery (CSRF) attack.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 8.8
+ * @precision medium
+ * @id java/csrf-unprotected-request-type
+ * @tags security
+ *       external/cwe/cwe-352
+ */
+
+import java
+import semmle.code.java.security.CsrfUnprotectedRequestTypeQuery
+
+query predicate edges(CallPathNode pred, CallPathNode succ) { CallGraph::edges(pred, succ) }
+
+from CallPathNode source, CallPathNode sink
+where unprotectedStateChange(source, sink)
+select source.asMethod(), source, sink,
+  "Potential CSRF vulnerability due to using an HTTP request type which is not default-protected from CSRF for an apparent $@.",
+  sink, "state-changing action"

java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestTypeBadSpring.java

@@ -0,0 +1,14 @@
+import org.springframework.web.bind.annotation.RequestMapping;
+import org.springframework.web.bind.annotation.RequestMethod;
+
+// BAD - a safe HTTP request like GET should not be used for a state-changing action
+@RequestMapping(value="/transfer", method=RequestMethod.GET)
+public boolean doTransfer(HttpServletRequest request, HttpServletResponse response){
+  return transfer(request, response);
+}
+
+// BAD - no HTTP request type is specified, so safe HTTP requests are allowed
+@RequestMapping(value="/delete")
+public boolean doDelete(HttpServletRequest request, HttpServletResponse response){
+  return delete(request, response);
+}

java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestTypeBadStapler.java

@@ -0,0 +1,12 @@
+import org.kohsuke.stapler.verb.GET;
+
+// BAD - a safe HTTP request like GET should not be used for a state-changing action
+@GET
+public HttpRedirect doTransfer() {
+  return transfer();
+}
+
+// BAD - no HTTP request type is specified, so safe HTTP requests are allowed
+public HttpRedirect doPost() {
+  return post();
+}

java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestTypeGoodSpring.java

@@ -0,0 +1,15 @@
+import org.springframework.web.bind.annotation.RequestMapping;
+import org.springframework.web.bind.annotation.RequestMethod;
+import org.springframework.web.bind.annotation.DeleteMapping;
+
+// GOOD - use an unsafe HTTP request like POST
+@RequestMapping(value="/transfer", method=RequestMethod.POST)
+public boolean doTransfer(HttpServletRequest request, HttpServletResponse response){
+  return transfer(request, response);
+}
+
+// GOOD - use an unsafe HTTP request like DELETE
+@DeleteMapping(value="/delete")
+public boolean doDelete(HttpServletRequest request, HttpServletResponse response){
+  return delete(request, response);
+}

java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestTypeGoodStapler.java

@@ -0,0 +1,13 @@
+import org.kohsuke.stapler.verb.POST;
+
+// GOOD - use POST
+@POST
+public HttpRedirect doTransfer() {
+  return transfer();
+}
+
+// GOOD - use POST
+@POST
+public HttpRedirect doPost() {
+  return post();
+}

java/ql/src/change-notes/2024-12-16-csrf-unprotected-request-type.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new query, `java/csrf-unprotected-request-type`, to detect Cross-Site Request Forgery (CSRF) vulnerabilities due to using HTTP request types that are not default-protected from CSRF.

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

@@ -4,7 +4,6 @@
 deprecated module;
 
 import java
-import semmle.code.xml.MyBatisMapperXML
 import semmle.code.java.dataflow.FlowSources
 import semmle.code.java.frameworks.MyBatis
 import semmle.code.java.frameworks.Properties

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

@@ -15,7 +15,6 @@
 import java
 deprecated import MyBatisCommonLib
 deprecated import MyBatisMapperXmlSqlInjectionLib
-deprecated import semmle.code.xml.MyBatisMapperXML
 import semmle.code.java.dataflow.FlowSources
 private import semmle.code.java.security.Sanitizers
 deprecated import MyBatisMapperXmlSqlInjectionFlow::PathGraph

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

@@ -4,7 +4,7 @@
 deprecated module;
 
 import java
-import semmle.code.xml.MyBatisMapperXML
+import semmle.code.java.frameworks.MyBatis
 import semmle.code.java.dataflow.FlowSources
 import semmle.code.java.frameworks.Properties
 

java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.java

@@ -0,0 +1,471 @@
+import org.springframework.stereotype.Controller;
+import org.springframework.web.bind.annotation.RequestMapping;
+import org.springframework.web.bind.annotation.GetMapping;
+import org.springframework.web.bind.annotation.PostMapping;
+import org.springframework.web.bind.annotation.RequestMethod;
+import static org.springframework.web.bind.annotation.RequestMethod.*;
+import org.springframework.web.bind.annotation.RequestParam;
+import org.springframework.beans.factory.annotation.Autowired;
+import java.sql.DriverManager;
+import java.sql.Connection;
+import java.sql.PreparedStatement;
+import java.sql.SQLException;
+import java.sql.Connection;
+import java.sql.ResultSet;
+import java.sql.Statement;
+import org.kohsuke.stapler.WebMethod;
+import org.kohsuke.stapler.interceptor.RequirePOST;
+import org.kohsuke.stapler.verb.POST;
+import org.kohsuke.stapler.verb.GET;
+import org.kohsuke.stapler.verb.PUT;
+import org.kohsuke.stapler.StaplerRequest;
+import org.kohsuke.stapler.QueryParameter;
+import org.kohsuke.stapler.HttpRedirect;
+import org.kohsuke.stapler.HttpResponses;
+import org.apache.ibatis.jdbc.SqlRunner;
+import org.springframework.jdbc.core.JdbcTemplate;
+import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate;
+import java.util.Map;
+
+@Controller
+public class CsrfUnprotectedRequestTypeTest {
+    public static Connection connection;
+
+    // Test Spring sources with `PreparedStatement.executeUpdate()` as a default database update method call
+
+    // BAD: allows request type not default-protected from CSRF when updating a database
+    @RequestMapping("/")
+    public void bad1() { // $ hasCsrfUnprotectedRequestType
+        try {
+            String sql = "DELETE";
+            Connection conn = DriverManager.getConnection("url");
+            PreparedStatement ps = conn.prepareStatement(sql);
+            ps.executeUpdate(); // database update method call
+        } catch (SQLException e) { }
+    }
+
+    // BAD: uses GET request when updating a database
+    @RequestMapping(value = "", method = RequestMethod.GET)
+    public void bad2() { // $ hasCsrfUnprotectedRequestType
+        try {
+            String sql = "DELETE";
+            Connection conn = DriverManager.getConnection("url");
+            PreparedStatement ps = conn.prepareStatement(sql);
+            ps.executeUpdate(); // database update method call
+        } catch (SQLException e) { }
+    }
+
+    // BAD: uses GET request when updating a database
+    @GetMapping(value = "")
+    public void bad3() { // $ hasCsrfUnprotectedRequestType
+        try {
+            String sql = "DELETE";
+            Connection conn = DriverManager.getConnection("url");
+            PreparedStatement ps = conn.prepareStatement(sql);
+            ps.executeUpdate(); // database update method call
+        } catch (SQLException e) { }
+    }
+
+    // BAD: allows GET request when updating a database
+    @RequestMapping(value = "", method = { RequestMethod.GET, RequestMethod.POST })
+    public void bad4() { // $ hasCsrfUnprotectedRequestType
+        try {
+            String sql = "DELETE";
+            Connection conn = DriverManager.getConnection("url");
+            PreparedStatement ps = conn.prepareStatement(sql);
+            ps.executeUpdate(); // database update method call
+        } catch (SQLException e) { }
+    }
+
+    // BAD: uses request type not default-protected from CSRF when updating a database
+    @RequestMapping(value = "", method = { GET, HEAD, OPTIONS, TRACE })
+    public void bad5() { // $ hasCsrfUnprotectedRequestType
+        try {
+            String sql = "DELETE";
+            Connection conn = DriverManager.getConnection("url");
+            PreparedStatement ps = conn.prepareStatement(sql);
+            ps.executeUpdate(); // database update method call
+        } catch (SQLException e) { }
+    }
+
+    // GOOD: uses OPTIONS or TRACE, which are unlikely to be exploitable via CSRF
+    @RequestMapping(value = "", method = { OPTIONS, TRACE })
+    public void good0() {
+        try {
+            String sql = "DELETE";
+            Connection conn = DriverManager.getConnection("url");
+            PreparedStatement ps = conn.prepareStatement(sql);
+            ps.executeUpdate(); // database update method call
+        } catch (SQLException e) { }
+    }
+
+    // GOOD: uses POST request when updating a database
+    @RequestMapping(value = "", method = RequestMethod.POST)
+    public void good1() {
+        try {
+            String sql = "DELETE";
+            Connection conn = DriverManager.getConnection("url");
+            PreparedStatement ps = conn.prepareStatement(sql);
+            ps.executeUpdate(); // database update method call
+        } catch (SQLException e) { }
+    }
+
+    // GOOD: uses POST request when updating a database
+    @RequestMapping(value = "", method = POST)
+    public void good2() {
+        try {
+            String sql = "DELETE";
+            Connection conn = DriverManager.getConnection("url");
+            PreparedStatement ps = conn.prepareStatement(sql);
+            ps.executeUpdate(); // database update method call
+        } catch (SQLException e) { }
+    }
+
+    // GOOD: uses POST request when updating a database
+    @PostMapping(value = "")
+    public void good3() {
+        try {
+            String sql = "DELETE";
+            Connection conn = DriverManager.getConnection("url");
+            PreparedStatement ps = conn.prepareStatement(sql);
+            ps.executeUpdate(); // database update method call
+        } catch (SQLException e) { }
+    }
+
+    // GOOD: uses a request type that is default-protected from CSRF when updating a database
+    @RequestMapping(value = "", method = { POST, PUT, PATCH, DELETE })
+    public void good4() {
+        try {
+            String sql = "DELETE";
+            Connection conn = DriverManager.getConnection("url");
+            PreparedStatement ps = conn.prepareStatement(sql);
+            ps.executeUpdate(); // database update method call
+        } catch (SQLException e) { }
+    }
+
+    // Test database update method calls other than `PreparedStatement.executeUpdate()`
+
+    // BAD: allows request type not default-protected from CSRF when
+    // updating a database using `PreparedStatement.executeLargeUpdate()`
+    @RequestMapping("/")
+    public void bad6() { // $ hasCsrfUnprotectedRequestType
+        try {
+            String sql = "DELETE";
+            Connection conn = DriverManager.getConnection("url");
+            PreparedStatement ps = conn.prepareStatement(sql);
+            ps.executeLargeUpdate(); // database update method call
+        } catch (SQLException e) { }
+    }
+
+    // BAD: allows request type not default-protected from CSRF when
+    // updating a database using `Statement.executeUpdate`
+    @RequestMapping("/")
+    public void badStatementExecuteUpdate() { // $ hasCsrfUnprotectedRequestType
+        try {
+            String item = "item";
+            String price = "price";
+            Statement statement = connection.createStatement();
+            String sql = "UPDATE PRODUCT SET PRICE='" + price + "' WHERE ITEM='" + item + "'";
+            int count = statement.executeUpdate(sql);
+        } catch (SQLException e) { }
+    }
+
+    // BAD: allows request type not default-protected from CSRF when
+    // updating a database using `Statement.executeLargeUpdate`
+    @RequestMapping("/")
+    public void badStatementExecuteLargeUpdate() { // $ hasCsrfUnprotectedRequestType
+        try {
+            String item = "item";
+            String price = "price";
+            Statement statement = connection.createStatement();
+            String sql = "UPDATE PRODUCT SET PRICE='" + price + "' WHERE ITEM='" + item + "'";
+            long count = statement.executeLargeUpdate(sql);
+        } catch (SQLException e) { }
+    }
+
+    // BAD: allows request type not default-protected from CSRF when
+    // updating a database using `Statement.execute` with SQL UPDATE
+    @RequestMapping("/")
+    public void badStatementExecute() { // $ hasCsrfUnprotectedRequestType
+        try {
+            String item = "item";
+            String price = "price";
+            Statement statement = connection.createStatement();
+            String sql = "UPDATE PRODUCT SET PRICE='" + price + "' WHERE ITEM='" + item + "'";
+            boolean bool = statement.execute(sql);
+        } catch (SQLException e) { }
+    }
+
+    // GOOD: does not update a database, queries with SELECT
+    @RequestMapping("/")
+    public void goodStatementExecute() {
+        try {
+            String category = "category";
+            Statement statement = connection.createStatement();
+            String query = "SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY='"
+                    + category + "' ORDER BY PRICE";
+            boolean bool = statement.execute(query);
+        } catch (SQLException e) { }
+    }
+
+    // BAD: allows request type not default-protected from CSRF when
+    // updating a database using `SqlRunner.insert`
+    @RequestMapping("/")
+    public void badSqlRunnerInsert() { // $ hasCsrfUnprotectedRequestType
+        try {
+            String item = "item";
+            String price = "price";
+            String sql = "INSERT PRODUCT SET PRICE='" + price + "' WHERE ITEM='" + item + "'";
+            SqlRunner sqlRunner = new SqlRunner(connection);
+            sqlRunner.insert(sql);
+        } catch (SQLException e) { }
+    }
+
+    // BAD: allows request type not default-protected from CSRF when
+    // updating a database using `SqlRunner.update`
+    @RequestMapping("/")
+    public void badSqlRunnerUpdate() { // $ hasCsrfUnprotectedRequestType
+        try {
+            String item = "item";
+            String price = "price";
+            String sql = "UPDATE PRODUCT SET PRICE='" + price + "' WHERE ITEM='" + item + "'";
+            SqlRunner sqlRunner = new SqlRunner(connection);
+            sqlRunner.update(sql);
+        } catch (SQLException e) { }
+    }
+
+    // BAD: allows request type not default-protected from CSRF when
+    // updating a database using `SqlRunner.delete`
+    @RequestMapping("/")
+    public void badSqlRunnerDelete() { // $ hasCsrfUnprotectedRequestType
+        try {
+            String item = "item";
+            String price = "price";
+            String sql = "DELETE PRODUCT SET PRICE='" + price + "' WHERE ITEM='" + item + "'";
+            SqlRunner sqlRunner = new SqlRunner(connection);
+            sqlRunner.delete(sql);
+        } catch (SQLException e) { }
+    }
+
+    // BAD: allows request type not default-protected from CSRF when
+    // updating a database using `JdbcTemplate.update`
+    @RequestMapping("/")
+    public void badJdbcTemplateUpdate() { // $ hasCsrfUnprotectedRequestType
+        String item = "item";
+        String price = "price";
+        String sql = "UPDATE PRODUCT SET PRICE='" + price + "' WHERE ITEM='" + item + "'";
+        JdbcTemplate jdbcTemplate = new JdbcTemplate();
+        jdbcTemplate.update(sql);
+    }
+
+    // BAD: allows request type not default-protected from CSRF when
+    // updating a database using `JdbcTemplate.batchUpdate`
+    @RequestMapping("/")
+    public void badJdbcTemplateBatchUpdate() { // $ hasCsrfUnprotectedRequestType
+        String item = "item";
+        String price = "price";
+        String sql = "UPDATE PRODUCT SET PRICE='" + price + "' WHERE ITEM='" + item + "'";
+        JdbcTemplate jdbcTemplate = new JdbcTemplate();
+        jdbcTemplate.batchUpdate(sql, null, null);
+    }
+
+    // BAD: allows request type not default-protected from CSRF when
+    // updating a database using `JdbcTemplate.execute`
+    @RequestMapping("/")
+    public void badJdbcTemplateExecute() { // $ hasCsrfUnprotectedRequestType
+        String item = "item";
+        String price = "price";
+        String sql = "UPDATE PRODUCT SET PRICE='" + price + "' WHERE ITEM='" + item + "'";
+        JdbcTemplate jdbcTemplate = new JdbcTemplate();
+        jdbcTemplate.execute(sql);
+    }
+
+    // GOOD: does not update a database, queries with SELECT
+    @RequestMapping("/")
+    public void goodJdbcTemplateExecute() {
+        String category = "category";
+        String query = "SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY='"
+                + category + "' ORDER BY PRICE";
+        JdbcTemplate jdbcTemplate = new JdbcTemplate();
+        jdbcTemplate.execute(query);
+    }
+
+    // BAD: allows request type not default-protected from CSRF when
+    // updating a database using `NamedParameterJdbcTemplate.update`
+    @RequestMapping("/")
+    public void badNamedParameterJdbcTemplateUpdate() { // $ hasCsrfUnprotectedRequestType
+        String item = "item";
+        String price = "price";
+        String sql = "UPDATE PRODUCT SET PRICE='" + price + "' WHERE ITEM='" + item + "'";
+        JdbcTemplate jdbcTemplate = new JdbcTemplate();
+        NamedParameterJdbcTemplate namedParamJdbcTemplate = new NamedParameterJdbcTemplate(jdbcTemplate);
+        namedParamJdbcTemplate.update(sql, null, null);
+    }
+
+    // BAD: allows request type not default-protected from CSRF when
+    // updating a database using `NamedParameterJdbcTemplate.batchUpdate`
+    @RequestMapping("/")
+    public void badNamedParameterJdbcTemplateBatchUpdate() { // $ hasCsrfUnprotectedRequestType
+        String item = "item";
+        String price = "price";
+        String sql = "UPDATE PRODUCT SET PRICE='" + price + "' WHERE ITEM='" + item + "'";
+        JdbcTemplate jdbcTemplate = new JdbcTemplate();
+        NamedParameterJdbcTemplate namedParamJdbcTemplate = new NamedParameterJdbcTemplate(jdbcTemplate);
+        namedParamJdbcTemplate.batchUpdate(sql, (Map<String,?>[]) null);
+    }
+
+    // BAD: allows request type not default-protected from CSRF when
+    // updating a database using `NamedParameterJdbcTemplate.execute`
+    @RequestMapping("/")
+    public void badNamedParameterJdbcTemplateExecute() { // $ hasCsrfUnprotectedRequestType
+        String item = "item";
+        String price = "price";
+        String sql = "UPDATE PRODUCT SET PRICE='" + price + "' WHERE ITEM='" + item + "'";
+        JdbcTemplate jdbcTemplate = new JdbcTemplate();
+        NamedParameterJdbcTemplate namedParamJdbcTemplate = new NamedParameterJdbcTemplate(jdbcTemplate);
+        namedParamJdbcTemplate.execute(sql, null);
+    }
+
+    // GOOD: does not update a database, queries with SELECT
+    @RequestMapping("/")
+    public void goodNamedParameterJdbcTemplateExecute() {
+        String category = "category";
+        String query = "SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY='"
+                + category + "' ORDER BY PRICE";
+        JdbcTemplate jdbcTemplate = new JdbcTemplate();
+        NamedParameterJdbcTemplate namedParamJdbcTemplate = new NamedParameterJdbcTemplate(jdbcTemplate);
+        namedParamJdbcTemplate.execute(query, null);
+    }
+
+    @Autowired
+	private MyBatisService myBatisService;
+
+    // BAD: uses GET request when updating a database with MyBatis XML mapper method
+    @GetMapping(value = "")
+	public void bad7(@RequestParam String name) { // $ hasCsrfUnprotectedRequestType
+		myBatisService.bad7(name);
+	}
+
+    // BAD: uses GET request when updating a database with MyBatis `@DeleteProvider`
+    @GetMapping(value = "badDelete")
+	public void badDelete(@RequestParam String name) { // $ hasCsrfUnprotectedRequestType
+		myBatisService.badDelete(name);
+	}
+
+    // BAD: uses GET request when updating a database with MyBatis `@UpdateProvider`
+	@GetMapping(value = "badUpdate")
+	public void badUpdate(@RequestParam String name) { // $ hasCsrfUnprotectedRequestType
+		myBatisService.badUpdate(name);
+	}
+
+    // BAD: uses GET request when updating a database with MyBatis `@InsertProvider`
+	@GetMapping(value = "badInsert")
+	public void badInsert(@RequestParam String name) { // $ hasCsrfUnprotectedRequestType
+		myBatisService.badInsert(name);
+	}
+
+    // BAD: uses GET request when updating a database with MyBatis `@Delete`
+    @GetMapping(value = "bad8")
+	public void bad8(@RequestParam int id) { // $ hasCsrfUnprotectedRequestType
+		myBatisService.bad8(id);
+	}
+
+    // BAD: uses GET request when updating a database with MyBatis `@Insert`
+    @GetMapping(value = "bad9")
+	public void bad9(@RequestParam String user) { // $ hasCsrfUnprotectedRequestType
+		myBatisService.bad9(user);
+	}
+
+    // BAD: uses GET request when updating a database with MyBatis `@Update`
+    @GetMapping(value = "bad10")
+	public void bad10(@RequestParam String user) { // $ hasCsrfUnprotectedRequestType
+		myBatisService.bad10(user);
+	}
+
+    // Test name-based heuristic for method names that imply a state-change
+    @GetMapping(value = "transfer")
+	public String transfer(@RequestParam String user) { return "transfer"; } // $ hasCsrfUnprotectedRequestType
+
+    @GetMapping(value = "transfer")
+	public String transferData(@RequestParam String user) { return "transfer"; } // $ hasCsrfUnprotectedRequestType
+
+    @GetMapping(value = "transfer")
+	public String doTransfer(@RequestParam String user) { return "transfer"; } // $ hasCsrfUnprotectedRequestType
+
+    @GetMapping(value = "transfer")
+	public String doTransferAllData(@RequestParam String user) { return "transfer"; } // $ hasCsrfUnprotectedRequestType
+
+    @GetMapping(value = "transfer")
+	public String doDataTransfer(@RequestParam String user) { return "transfer"; } // $ hasCsrfUnprotectedRequestType
+
+    @GetMapping(value = "transfer")
+	public String transfered(@RequestParam String user) { return "transfer"; } // OK: we look for 'transfer' only
+
+    @GetMapping(value = "transfer")
+	public String dotransfer(@RequestParam String user) { return "transfer"; } // OK: we look for 'transfer' within camelCase only
+
+    @GetMapping(value = "transfer")
+	public String doTransferdata(@RequestParam String user) { return "transfer"; } // OK: we look for 'transfer' within camelCase only
+
+    @GetMapping(value = "transfer")
+	public String getTransfer(@RequestParam String user) { return "transfer"; } // OK: starts with 'get'
+
+    // Test Stapler web methods with name-based heuristic
+
+    // BAD: Stapler web method annotated with `@WebMethod` and method name that implies a state-change
+    @WebMethod(name = "post")
+	public String doPost(String user) { // $ hasCsrfUnprotectedRequestType
+		return "post";
+	}
+
+    // GOOD: nothing to indicate that this is a Stapler web method
+	public String postNotAWebMethod(String user) {
+		return "post";
+	}
+
+    // GOOD: Stapler web method annotated with `@RequirePOST` and method name that implies a state-change
+    @RequirePOST
+	public String doPost1(String user) {
+		return "post";
+	}
+
+    // GOOD: Stapler web method annotated with `@POST` and method name that implies a state-change
+    @POST
+	public String doPost2(String user) {
+		return "post";
+	}
+
+    // BAD: Stapler web method annotated with `@GET` and method name that implies a state-change
+    @GET
+	public String doPost3(String user) { // $ hasCsrfUnprotectedRequestType
+		return "post";
+	}
+
+    // GOOD: Stapler web method annotated with `@PUT` and method name that implies a state-change
+    // We treat this case as good since PUT is only exploitable if there is a CORS issue.
+    @PUT
+	public String doPut(String user) {
+		return "put";
+	}
+
+    // BAD: Stapler web method parameter of type `StaplerRequest` and method name that implies a state-change
+	public String doPost4(StaplerRequest request) { // $ hasCsrfUnprotectedRequestType
+		return "post";
+	}
+
+    // BAD: Stapler web method parameter annotated with `@QueryParameter` and method name that implies a state-change
+	public String doPost5(@QueryParameter(value="user", fixEmpty=false, required=false) String user) { // $ hasCsrfUnprotectedRequestType
+		return "post";
+	}
+
+    // BAD: Stapler web method with declared exception type implementing HttpResponse and method name that implies a state-change
+	public String doPost6(String user) throws HttpResponses.HttpResponseException { // $ hasCsrfUnprotectedRequestType
+		return "post";
+	}
+
+    // BAD: Stapler web method with return type implementing HttpResponse and method name that implies a state-change
+	public HttpRedirect doPost7(String url) { // $ hasCsrfUnprotectedRequestType
+        HttpRedirect redirect = new HttpRedirect(url);
+		return redirect;
+	}
+}

java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.ql

@@ -0,0 +1,18 @@
+import java
+import semmle.code.java.security.CsrfUnprotectedRequestTypeQuery
+import utils.test.InlineExpectationsTest
+
+module CsrfUnprotectedRequestTypeTest implements TestSig {
+  string getARelevantTag() { result = "hasCsrfUnprotectedRequestType" }
+
+  predicate hasActualResult(Location location, string element, string tag, string value) {
+    tag = "hasCsrfUnprotectedRequestType" and
+    exists(CallPathNode src | unprotectedStateChange(src, _) |
+      src.getLocation() = location and
+      element = src.toString() and
+      value = ""
+    )
+  }
+}
+
+import MakeTest<CsrfUnprotectedRequestTypeTest>

java/ql/test/query-tests/security/CWE-352/MyBatisMapper.java

@@ -0,0 +1,43 @@
+import org.apache.ibatis.annotations.Mapper;
+import org.springframework.stereotype.Repository;
+import org.apache.ibatis.annotations.DeleteProvider;
+import org.apache.ibatis.annotations.UpdateProvider;
+import org.apache.ibatis.annotations.InsertProvider;
+import org.apache.ibatis.annotations.Delete;
+import org.apache.ibatis.annotations.Update;
+import org.apache.ibatis.annotations.Insert;
+
+@Mapper
+@Repository
+public interface MyBatisMapper {
+
+	void bad7(String name);
+
+	//using providers
+	@DeleteProvider(
+			type = MyBatisProvider.class,
+			method = "badDelete"
+	)
+	void badDelete(String input);
+
+	@UpdateProvider(
+			type = MyBatisProvider.class,
+			method = "badUpdate"
+	)
+	void badUpdate(String input);
+
+	@InsertProvider(
+			type = MyBatisProvider.class,
+			method = "badInsert"
+	)
+	void badInsert(String input);
+
+	@Delete("DELETE FROM users WHERE id = #{id}")
+   	boolean bad8(int id);
+
+	@Insert("INSERT INTO users (id, name) VALUES(#{id}, #{name})")
+   	void bad9(String user);
+
+	@Update("UPDATE users SET name = #{name} WHERE id = #{id}")
+   	boolean bad10(String user);
+}

java/ql/test/query-tests/security/CWE-352/MyBatisMapper.xml

@@ -0,0 +1,35 @@
+<?xml version="1.0" encoding="UTF-8" ?>
+<!DOCTYPE mapper PUBLIC "-//mybatis.org//DTD Mapper 3.0//EN"
+  "http://mybatis.org/dtd/mybatis-3-mapper.dtd" >
+<mapper namespace="MyBatisMapper">
+
+  <resultMap id="BaseResultMap" type="Test">
+    <id column="id" jdbcType="INTEGER" property="id"/>
+    <result column="name" jdbcType="VARCHAR" property="name"/>
+    <result column="pass" jdbcType="VARCHAR" property="pass"/>
+  </resultMap>
+
+  <sql id="Update_By_Example_Where_Clause">
+    <where>
+      <if test="test.name != null">
+        and name = ${ test . name , jdbcType = VARCHAR }
+      </if>
+      <if test="test.id != null">
+        and id = #{test.id}
+      </if>
+    </where>
+  </sql>
+
+  <insert id="bad7" parameterType="Test">
+    insert into test (name, pass)
+    <trim prefix="values (" suffix=")" suffixOverrides=",">
+      <if test="name != null">
+        name = ${name,jdbcType=VARCHAR},
+      </if>
+      <if test="pass != null">
+        pass = ${pass},
+      </if>
+    </trim>
+  </insert>
+
+</mapper>

java/ql/test/query-tests/security/CWE-352/MyBatisProvider.java

@@ -0,0 +1,24 @@
+import org.apache.ibatis.annotations.Param;
+import org.apache.ibatis.jdbc.SQL;
+
+public class MyBatisProvider {
+
+    public String badDelete(@Param("input") final String input) {
+        return "DELETE FROM users WHERE username = '" + input + "';";
+    }
+
+    public String badUpdate(@Param("input") final String input) {
+        String s = (new SQL() {
+            {
+                this.UPDATE("users");
+                this.SET("balance = 0");
+                this.WHERE("username = '" + input + "'");
+            }
+        }).toString();
+        return s;
+    }
+
+    public String badInsert(@Param("input") final String input) {
+        return "INSERT INTO users VALUES (1, '" + input + "', 'hunter2');";
+    }
+}

java/ql/test/query-tests/security/CWE-352/MyBatisService.java

@@ -0,0 +1,37 @@
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.stereotype.Service;
+
+@Service
+public class MyBatisService {
+
+	@Autowired
+	private MyBatisMapper myBatisMapper;
+
+	public void bad7(String name) {
+		myBatisMapper.bad7(name);
+	}
+
+	public void badDelete(String input) {
+		myBatisMapper.badDelete(input);
+	}
+
+	public void badUpdate(String input) {
+		myBatisMapper.badUpdate(input);
+	}
+
+	public void badInsert(String input) {
+		myBatisMapper.badInsert(input);
+	}
+
+	public void bad8(int id){
+		myBatisMapper.bad8(id);
+	}
+
+	public void bad9(String user){
+		myBatisMapper.bad9(user);
+	}
+
+	public void bad10(String user){
+		myBatisMapper.bad10(user);
+	}
+}

java/ql/test/query-tests/security/CWE-352/options

@@ -1 +1 @@
-semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/springframework-5.3.8
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/springframework-5.3.8/:${testdir}/../../../stubs/org.mybatis-3.5.4/:${testdir}/../../../stubs/stapler-1.263/:${testdir}/../../../stubs/javax-servlet-2.5:${testdir}/../../../stubs/apache-commons-jelly-1.0.1:${testdir}/../../../stubs/apache-commons-fileupload-1.4:${testdir}/../../../stubs/saxon-xqj-9.x:${testdir}/../../../stubs/apache-commons-beanutils:${testdir}/../../../stubs/dom4j-2.1.1:${testdir}/../../../stubs/apache-commons-lang:${testdir}/../../../stubs/jaxen-1.2.0:${testdir}/../../../stubs/apache-commons-logging-1.2/

java/ql/test/stubs/org.mybatis-3.5.4/org/apache/ibatis/annotations/Delete.java

@@ -0,0 +1,14 @@
+package org.apache.ibatis.annotations;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+@Documented
+@Retention(RetentionPolicy.RUNTIME)
+@Target({ElementType.METHOD})
+public @interface Delete {
+    String[] value();
+}

java/ql/test/stubs/org.mybatis-3.5.4/org/apache/ibatis/annotations/Insert.java

@@ -0,0 +1,14 @@
+package org.apache.ibatis.annotations;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+@Documented
+@Retention(RetentionPolicy.RUNTIME)
+@Target({ElementType.METHOD})
+public @interface Insert {
+    String[] value();
+}

java/ql/test/stubs/org.mybatis-3.5.4/org/apache/ibatis/annotations/Update.java

@@ -0,0 +1,14 @@
+package org.apache.ibatis.annotations;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+@Documented
+@Retention(RetentionPolicy.RUNTIME)
+@Target({ElementType.METHOD})
+public @interface Update {
+    String[] value();
+}

java/ql/test/stubs/org.mybatis-3.5.4/org/apache/ibatis/jdbc/SqlRunner.java

@@ -0,0 +1,37 @@
+package org.apache.ibatis.jdbc;
+
+import java.sql.Connection;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.sql.ResultSetMetaData;
+import java.sql.SQLException;
+import java.sql.Statement;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+
+public class SqlRunner {
+
+  public static final int NO_GENERATED_KEY = Integer.MIN_VALUE + 1001;
+
+  private final Connection connection;
+  private boolean useGeneratedKeySupport;
+
+  public SqlRunner(Connection connection) {
+    this.connection = connection;
+  }
+
+  public void setUseGeneratedKeySupport(boolean useGeneratedKeySupport) { }
+  public Map<String, Object> selectOne(String sql, Object... args) throws SQLException { return null; }
+  public List<Map<String, Object>> selectAll(String sql, Object... args) throws SQLException { return null; }
+  public int insert(String sql, Object... args) throws SQLException { return 0; }
+  public int update(String sql, Object... args) throws SQLException { return 0; }
+  public int delete(String sql, Object... args) throws SQLException { return 0; }
+  public void closeConnection() { }
+  private void setParameters(PreparedStatement ps, Object... args) throws SQLException { }
+  private List<Map<String, Object>> getResults(ResultSet rs) throws SQLException { return null; }
+
+}

java/ql/test/stubs/springframework-5.3.8/org/springframework/jdbc/core/namedparam/NamedParameterJdbcTemplate.java

@@ -0,0 +1,169 @@
+package org.springframework.jdbc.core.namedparam;
+
+import java.sql.PreparedStatement;
+import java.sql.SQLException;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Consumer;
+import java.util.stream.Stream;
+
+import javax.sql.DataSource;
+
+import org.springframework.dao.DataAccessException;
+import org.springframework.jdbc.core.BatchPreparedStatementSetter;
+import org.springframework.jdbc.core.JdbcOperations;
+import org.springframework.jdbc.core.JdbcTemplate;
+import org.springframework.jdbc.core.PreparedStatementCallback;
+import org.springframework.jdbc.core.PreparedStatementCreator;
+import org.springframework.jdbc.core.ResultSetExtractor;
+import org.springframework.jdbc.core.RowCallbackHandler;
+import org.springframework.jdbc.core.RowMapper;
+import org.springframework.jdbc.core.SqlParameter;
+import org.springframework.jdbc.support.KeyHolder;
+import org.springframework.jdbc.support.rowset.SqlRowSet;
+
+public class NamedParameterJdbcTemplate implements NamedParameterJdbcOperations {
+
+	public static final int DEFAULT_CACHE_LIMIT = 256;
+	private final JdbcOperations classicJdbcTemplate;
+	public NamedParameterJdbcTemplate(DataSource dataSource) {
+		this.classicJdbcTemplate = new JdbcTemplate(dataSource);
+	}
+	public NamedParameterJdbcTemplate(JdbcOperations classicJdbcTemplate) {
+		this.classicJdbcTemplate = classicJdbcTemplate;
+	}
+	@Override
+	public JdbcOperations getJdbcOperations() { return null; }
+	public JdbcTemplate getJdbcTemplate() { return null; }
+	public void setCacheLimit(int cacheLimit) { }
+	public int getCacheLimit() { return 0; }
+
+	@Override
+	public <T> T execute(String sql, SqlParameterSource paramSource, PreparedStatementCallback<T> action)
+			throws DataAccessException { return null; }
+
+	@Override
+	public <T> T execute(String sql, Map<String, ?> paramMap, PreparedStatementCallback<T> action)
+			throws DataAccessException { return null; }
+
+	@Override
+	public <T> T execute(String sql, PreparedStatementCallback<T> action) throws DataAccessException { return null; }
+
+	@Override
+	public <T> T query(String sql, SqlParameterSource paramSource, ResultSetExtractor<T> rse)
+			throws DataAccessException { return null; }
+
+	@Override
+	public <T> T query(String sql, Map<String, ?> paramMap, ResultSetExtractor<T> rse)
+			throws DataAccessException { return null; }
+
+	@Override
+	public <T> T query(String sql, ResultSetExtractor<T> rse) throws DataAccessException { return null; }
+
+	@Override
+	public void query(String sql, SqlParameterSource paramSource, RowCallbackHandler rch)
+			throws DataAccessException { }
+
+	@Override
+	public void query(String sql, Map<String, ?> paramMap, RowCallbackHandler rch)
+			throws DataAccessException { }
+
+	@Override
+	public void query(String sql, RowCallbackHandler rch) throws DataAccessException { }
+
+	@Override
+	public <T> List<T> query(String sql, SqlParameterSource paramSource, RowMapper<T> rowMapper)
+			throws DataAccessException { return null; }
+
+	@Override
+	public <T> List<T> query(String sql, Map<String, ?> paramMap, RowMapper<T> rowMapper)
+			throws DataAccessException { return null; }
+
+	@Override
+	public <T> List<T> query(String sql, RowMapper<T> rowMapper) throws DataAccessException { return null; }
+
+	@Override
+	public <T> Stream<T> queryForStream(String sql, SqlParameterSource paramSource, RowMapper<T> rowMapper)
+			throws DataAccessException { return null; }
+
+	@Override
+	public <T> Stream<T> queryForStream(String sql, Map<String, ?> paramMap, RowMapper<T> rowMapper)
+			throws DataAccessException { return null; }
+
+	@Override
+	public <T> T queryForObject(String sql, SqlParameterSource paramSource, RowMapper<T> rowMapper)
+			throws DataAccessException { return null; }
+
+	@Override
+	public <T> T queryForObject(String sql, Map<String, ?> paramMap, RowMapper<T>rowMapper)
+			throws DataAccessException { return null; }
+
+	@Override
+	public <T> T queryForObject(String sql, SqlParameterSource paramSource, Class<T> requiredType)
+			throws DataAccessException { return null; }
+
+	@Override
+	public <T> T queryForObject(String sql, Map<String, ?> paramMap, Class<T> requiredType)
+			throws DataAccessException { return null; }
+
+	@Override
+	public Map<String, Object> queryForMap(String sql, SqlParameterSource paramSource) throws DataAccessException { return null; }
+
+	@Override
+	public Map<String, Object> queryForMap(String sql, Map<String, ?> paramMap) throws DataAccessException { return null; }
+
+	@Override
+	public <T> List<T> queryForList(String sql, SqlParameterSource paramSource, Class<T> elementType)
+			throws DataAccessException { return null; }
+
+	@Override
+	public <T> List<T> queryForList(String sql, Map<String, ?> paramMap, Class<T> elementType)
+			throws DataAccessException { return null; }
+
+	@Override
+	public List<Map<String, Object>> queryForList(String sql, SqlParameterSource paramSource)
+			throws DataAccessException { return null; }
+
+	@Override
+	public List<Map<String, Object>> queryForList(String sql, Map<String, ?> paramMap)
+			throws DataAccessException { return null; }
+
+	@Override
+	public SqlRowSet queryForRowSet(String sql, SqlParameterSource paramSource) throws DataAccessException { return null; }
+
+	@Override
+	public SqlRowSet queryForRowSet(String sql, Map<String, ?> paramMap) throws DataAccessException { return null; }
+
+	@Override
+	public int update(String sql, SqlParameterSource paramSource) throws DataAccessException { return 0; }
+
+	@Override
+	public int update(String sql, Map<String, ?> paramMap) throws DataAccessException { return 0; }
+
+	@Override
+	public int update(String sql, SqlParameterSource paramSource, KeyHolder generatedKeyHolder)
+			throws DataAccessException { return 0; }
+
+	@Override
+	public int update(
+			String sql, SqlParameterSource paramSource, KeyHolder generatedKeyHolder, String[] keyColumnNames)
+			throws DataAccessException { return 0; }
+
+	@Override
+	public int[] batchUpdate(String sql, SqlParameterSource[] batchArgs) { return new int[0]; }
+
+	@Override
+	public int[] batchUpdate(String sql, Map<String, ?>[] batchValues) { return new int[0]; }
+
+	public int[] batchUpdate(String sql, SqlParameterSource[] batchArgs, KeyHolder generatedKeyHolder) { return new int[0]; }
+
+	public int[] batchUpdate(String sql, SqlParameterSource[] batchArgs, KeyHolder generatedKeyHolder,
+			String[] keyColumnNames) { return new int[0]; }
+
+	protected PreparedStatementCreator getPreparedStatementCreator(String sql, SqlParameterSource paramSource) {
+		return null;
+	}
+
+	protected ParsedSql getParsedSql(String sql) { return null; }
+
+}

java/ql/test/stubs/stapler-1.263/org/kohsuke/stapler/AnnotationHandler.java

@@ -0,0 +1,13 @@
+// Generated automatically from org.kohsuke.stapler.AnnotationHandler for testing purposes
+
+package org.kohsuke.stapler;
+
+import java.lang.annotation.Annotation;
+import org.kohsuke.stapler.StaplerRequest;
+
+abstract public class AnnotationHandler<T extends Annotation>
+{
+    protected final Object convert(Class p0, String p1){ return null; }
+    public AnnotationHandler(){}
+    public abstract Object parse(StaplerRequest p0, T p1, Class p2, String p3);
+}

java/ql/test/stubs/stapler-1.263/org/kohsuke/stapler/QueryParameter.java

@@ -0,0 +1,28 @@
+// Generated automatically from org.kohsuke.stapler.QueryParameter for testing purposes
+
+package org.kohsuke.stapler;
+
+import java.lang.annotation.Annotation;
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+import org.kohsuke.stapler.AnnotationHandler;
+import org.kohsuke.stapler.InjectedParameter;
+import org.kohsuke.stapler.StaplerRequest;
+
+@Documented
+@Retention(value=java.lang.annotation.RetentionPolicy.RUNTIME)
+@Target(value={java.lang.annotation.ElementType.PARAMETER})
+public @interface QueryParameter
+{
+    String value();
+    boolean fixEmpty();
+    boolean required();
+    static public class HandlerImpl extends AnnotationHandler<QueryParameter>
+    {
+        public HandlerImpl(){}
+        public Object parse(StaplerRequest p0, QueryParameter p1, Class p2, String p3){ return null; }
+    }
+}

java/ql/test/stubs/stapler-1.263/org/kohsuke/stapler/WebMethod.java

@@ -0,0 +1,18 @@
+// Generated automatically from org.kohsuke.stapler.WebMethod for testing purposes
+
+package org.kohsuke.stapler;
+
+import java.lang.annotation.Annotation;
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+@Documented
+@Retention(value=java.lang.annotation.RetentionPolicy.RUNTIME)
+@Target(value={java.lang.annotation.ElementType.METHOD})
+public @interface WebMethod
+{
+    String[] name();
+}

java/ql/test/stubs/stapler-1.263/org/kohsuke/stapler/interceptor/Interceptor.java

@@ -0,0 +1,15 @@
+// Generated automatically from org.kohsuke.stapler.interceptor.Interceptor for testing purposes
+
+package org.kohsuke.stapler.interceptor;
+
+import org.kohsuke.stapler.Function;
+import org.kohsuke.stapler.StaplerRequest;
+import org.kohsuke.stapler.StaplerResponse;
+
+abstract public class Interceptor
+{
+    protected Function target = null;
+    public Interceptor(){}
+    public abstract Object invoke(StaplerRequest p0, StaplerResponse p1, Object p2, Object[] p3);
+    public void setTarget(Function p0){}
+}

java/ql/test/stubs/stapler-1.263/org/kohsuke/stapler/interceptor/InterceptorAnnotation.java

@@ -0,0 +1,21 @@
+// Generated automatically from org.kohsuke.stapler.interceptor.InterceptorAnnotation for testing purposes
+
+package org.kohsuke.stapler.interceptor;
+
+import java.lang.annotation.Annotation;
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+import org.kohsuke.stapler.interceptor.Interceptor;
+import org.kohsuke.stapler.interceptor.Stage;
+
+@Documented
+@Retention(value=java.lang.annotation.RetentionPolicy.RUNTIME)
+@Target(value={java.lang.annotation.ElementType.ANNOTATION_TYPE})
+public @interface InterceptorAnnotation
+{
+    Class<? extends Interceptor> value();
+    Stage stage();
+}

java/ql/test/stubs/stapler-1.263/org/kohsuke/stapler/interceptor/RequirePOST.java

@@ -0,0 +1,25 @@
+// Generated automatically from org.kohsuke.stapler.interceptor.RequirePOST for testing purposes
+
+package org.kohsuke.stapler.interceptor;
+
+import java.lang.annotation.Annotation;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+import org.kohsuke.stapler.StaplerRequest;
+import org.kohsuke.stapler.StaplerResponse;
+import org.kohsuke.stapler.interceptor.Interceptor;
+import org.kohsuke.stapler.interceptor.InterceptorAnnotation;
+import org.kohsuke.stapler.interceptor.Stage;
+
+@Retention(value=java.lang.annotation.RetentionPolicy.RUNTIME)
+@Target(value={java.lang.annotation.ElementType.METHOD,java.lang.annotation.ElementType.FIELD})
+public @interface RequirePOST
+{
+    static public class Processor extends Interceptor
+    {
+        public Object invoke(StaplerRequest p0, StaplerResponse p1, Object p2, Object[] p3){ return null; }
+        public Processor(){}
+    }
+}

java/ql/test/stubs/stapler-1.263/org/kohsuke/stapler/interceptor/Stage.java

@@ -0,0 +1,10 @@
+// Generated automatically from org.kohsuke.stapler.interceptor.Stage for testing purposes
+
+package org.kohsuke.stapler.interceptor;
+
+
+public enum Stage
+{
+    PREINVOKE, SELECTION;
+    private Stage() {}
+}

java/ql/test/stubs/stapler-1.263/org/kohsuke/stapler/verb/GET.java

@@ -0,0 +1,19 @@
+// Generated automatically from org.kohsuke.stapler.verb.GET for testing purposes
+
+package org.kohsuke.stapler.verb;
+
+import java.lang.annotation.Annotation;
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+import org.kohsuke.stapler.interceptor.InterceptorAnnotation;
+import org.kohsuke.stapler.interceptor.Stage;
+
+@Documented
+@Retention(value=java.lang.annotation.RetentionPolicy.RUNTIME)
+@Target(value={java.lang.annotation.ElementType.METHOD})
+public @interface GET
+{
+}

java/ql/test/stubs/stapler-1.263/org/kohsuke/stapler/verb/POST.java

@@ -0,0 +1,19 @@
+// Generated automatically from org.kohsuke.stapler.verb.POST for testing purposes
+
+package org.kohsuke.stapler.verb;
+
+import java.lang.annotation.Annotation;
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+import org.kohsuke.stapler.interceptor.InterceptorAnnotation;
+import org.kohsuke.stapler.interceptor.Stage;
+
+@Documented
+@Retention(value=java.lang.annotation.RetentionPolicy.RUNTIME)
+@Target(value={java.lang.annotation.ElementType.METHOD})
+public @interface POST
+{
+}

java/ql/test/stubs/stapler-1.263/org/kohsuke/stapler/verb/PUT.java

@@ -0,0 +1,19 @@
+// Generated automatically from org.kohsuke.stapler.verb.PUT for testing purposes
+
+package org.kohsuke.stapler.verb;
+
+import java.lang.annotation.Annotation;
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+import org.kohsuke.stapler.interceptor.InterceptorAnnotation;
+import org.kohsuke.stapler.interceptor.Stage;
+
+@Documented
+@Retention(value=java.lang.annotation.RetentionPolicy.RUNTIME)
+@Target(value={java.lang.annotation.ElementType.METHOD})
+public @interface PUT
+{
+}