Merge pull request github#11730 from smowton/smowton/admin/improve-sql-unescaped-docs

Java: improve naming and description of SqlUnescaped.ql

Author: smowton

java/ql/lib/semmle/code/java/security/SqlUnescapedLib.qll renamed to java/ql/lib/semmle/code/java/security/SqlConcatenatedLib.qll

@@ -1,4 +1,4 @@
-/** Definitions used by `SqlUnescaped.ql`. */
+/** Definitions used by `SqlConcatenated.ql`. */
 
 import semmle.code.java.security.ControlledString
 import semmle.code.java.dataflow.TaintTracking

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

@@ -33,7 +33,7 @@ class QueryInjectionFlowConfig extends TaintTracking::Configuration {
 
 /**
  * Implementation of `SqlTainted.ql`. This is extracted to a QLL so that it
- * can be excluded from `SqlUnescaped.ql` to avoid overlapping results.
+ * can be excluded from `SqlConcatenated.ql` to avoid overlapping results.
  */
 predicate queryTaintedBy(
   QueryInjectionSink query, DataFlow::PathNode source, DataFlow::PathNode sink

java/ql/src/Security/CWE/CWE-089/SqlUnescaped.java renamed to java/ql/src/Security/CWE/CWE-089/SqlConcatenated.java

@@ -4,8 +4,8 @@
 <qhelp>
 <overview>
 <p>Even when the components of a SQL query are not fully controlled by
-a user, it is a vulnerability to concatenate those components into a
-SQL query without neutralizing special characters. Perhaps a separate
+a user, it is a vulnerability to build the query by directly
+concatenating those components. Perhaps a separate
 vulnerability will allow the user to gain control of the component. As
 well, a user who cannot gain full control of an input might influence
 it enough to cause the SQL query to fail to run.</p>
@@ -21,18 +21,18 @@ it enough to cause the SQL query to fail to run.</p>
 <p>In the following example, the code runs a simple SQL query in two different ways.</p>
 
 <p>The first way involves building a query, <code>query1</code>, by concatenating the
-result of <code>getCategory</code> with some string literals. The result of 
+result of <code>getCategory</code> with some string literals. The result of
 <code>getCategory</code> can include special characters, or
 it might be refactored later so that it may return something that contains special characters.</p>
 
-<p>The second way, which shows good practice, involves building a query, <code>query2</code>, with 
+<p>The second way, which shows good practice, involves building a query, <code>query2</code>, with
 a single string literal that includes a wildcard (<code>?</code>). The wildcard
 is then given a value by calling <code>setString</code>. This
 version is immune to injection attacks, because any special characters
 in the result of <code>getCategory</code> are not given any special
 treatment.</p>
 
-<sample src="SqlUnescaped.java" />
+<sample src="SqlConcatenated.java" />
 
 </example>
 <references>

java/ql/src/Security/CWE/CWE-089/SqlUnescaped.qhelp renamed to java/ql/src/Security/CWE/CWE-089/SqlConcatenated.qhelp

@@ -1,7 +1,7 @@
 /**
- * @name Query built without neutralizing special characters
- * @description Building a SQL or Java Persistence query without escaping or otherwise neutralizing any special
- *              characters is vulnerable to insertion of malicious code.
+ * @name Query built by concatenation with a possibly-untrusted string
+ * @description Building a SQL or Java Persistence query by concatenating a possibly-untrusted string
+ *              is vulnerable to insertion of malicious code.
  * @kind problem
  * @problem.severity error
  * @security-severity 8.8
@@ -13,7 +13,7 @@
  */
 
 import java
-import semmle.code.java.security.SqlUnescapedLib
+import semmle.code.java.security.SqlConcatenatedLib
 import semmle.code.java.security.SqlInjectionQuery
 
 class UncontrolledStringBuilderSource extends DataFlow::ExprNode {
@@ -27,7 +27,7 @@ class UncontrolledStringBuilderSource extends DataFlow::ExprNode {
 
 class UncontrolledStringBuilderSourceFlowConfig extends TaintTracking::Configuration {
   UncontrolledStringBuilderSourceFlowConfig() {
-    this = "SqlUnescaped::UncontrolledStringBuilderSourceFlowConfig"
+    this = "SqlConcatenated::UncontrolledStringBuilderSourceFlowConfig"
   }
 
   override predicate isSource(DataFlow::Node src) { src instanceof UncontrolledStringBuilderSource }
@@ -50,5 +50,5 @@ where
     )
   ) and
   not queryTaintedBy(query, _, _)
-select query, "Query might not neutralize special characters in $@.", uncontrolled,
+select query, "Query built by concatenation with $@, which may be untrusted.", uncontrolled,
   "this expression"

java/ql/src/Security/CWE/CWE-089/SqlUnescaped.ql renamed to java/ql/src/Security/CWE/CWE-089/SqlConcatenated.ql

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The name, description and alert message for the query `java/concatenated-sql-query` have been altered to emphasise that the query flags the use of string concatenation to construct SQL queries, not the lack of appropriate escaping. The query's files have been renamed from `SqlUnescaped.ql` and `SqlUnescapedLib.qll` to `SqlConcatenated.ql` and `SqlConcatenatedLib.qll` respectively; in the unlikely event your custom configuration or queries refer to either of these files by name, those references will need to be adjusted. The query id remains `java/concatenated-sql-query`, so alerts should not be re-raised as a result of this change.

java/ql/src/change-notes/2023-01-03-renamed-sql-unescaped.md

@@ -0,0 +1,11 @@
+| Test.java:36:47:36:52 | query1 | Query built by concatenation with $@, which may be untrusted. | Test.java:35:8:35:15 | category | this expression |
+| Test.java:42:57:42:62 | query2 | Query built by concatenation with $@, which may be untrusted. | Test.java:41:51:41:52 | id | this expression |
+| Test.java:50:62:50:67 | query3 | Query built by concatenation with $@, which may be untrusted. | Test.java:49:8:49:15 | category | this expression |
+| Test.java:62:47:62:61 | querySbToString | Query built by concatenation with $@, which may be untrusted. | Test.java:58:19:58:26 | category | this expression |
+| Test.java:70:40:70:44 | query | Query built by concatenation with $@, which may be untrusted. | Test.java:69:50:69:54 | price | this expression |
+| Test.java:70:40:70:44 | query | Query built by concatenation with $@, which may be untrusted. | Test.java:69:77:69:80 | item | this expression |
+| Test.java:78:46:78:50 | query | Query built by concatenation with $@, which may be untrusted. | Test.java:77:50:77:54 | price | this expression |
+| Test.java:78:46:78:50 | query | Query built by concatenation with $@, which may be untrusted. | Test.java:77:77:77:80 | item | this expression |
+| Test.java:98:47:98:60 | queryFromField | Query built by concatenation with $@, which may be untrusted. | Test.java:97:8:97:19 | categoryName | this expression |
+| Test.java:108:47:108:61 | querySbToString | Query built by concatenation with $@, which may be untrusted. | Test.java:104:19:104:30 | categoryName | this expression |
+| Test.java:118:47:118:62 | querySb2ToString | Query built by concatenation with $@, which may be untrusted. | Test.java:114:46:114:57 | categoryName | this expression |

java/ql/test/query-tests/security/CWE-089/semmle/examples/SqlConcatenated.expected

@@ -0,0 +1 @@
+Security/CWE/CWE-089/SqlConcatenated.ql

java/ql/test/query-tests/security/CWE-089/semmle/examples/SqlConcatenated.qlref

@@ -1,4 +1,4 @@
-import semmle.code.java.security.SqlUnescapedLib
+import semmle.code.java.security.SqlConcatenatedLib
 
 from StringBuilderVar sbv, Expr uncontrolled, Method method, int methodLine
 where