Merge pull request #10092 from zbazztian/zbazztian/string.replace-taint

Java: Add additional taint steps for java.lang.String methods
2025-12-24 04:36:35 +01:00 · 2022-08-30 12:24:37 +02:00
parent 55e5fa29cb da79ad854c
commit 1f83c5833b
11 changed files with 112 additions and 26 deletions
--- a/java/ql/lib/change-notes/2022-08-19-string-taint-models.md
+++ b/java/ql/lib/change-notes/2022-08-19-string-taint-models.md
@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+* Added taint flow models for the `java.lang.String.(charAt|getBytes)` methods.
+* Improved taint flow models for the `java.lang.String.(replace|replaceFirst|replaceAll)` methods. Additional results may be found where users do not properly sanitize their inputs.
--- a/java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll
+++ b/java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll
@@ -251,7 +251,7 @@ private predicate qualifierToArgumentStep(Expr tracked, Expr sink) {

 /** Access to a method that passes taint from the qualifier. */
 private predicate qualifierToMethodStep(Expr tracked, MethodAccess sink) {
-  (taintPreservingQualifierToMethod(sink.getMethod()) or unsafeEscape(sink)) and
+  taintPreservingQualifierToMethod(sink.getMethod()) and
  tracked = sink.getQualifier()
 }

@@ -282,28 +282,6 @@ private predicate taintPreservingQualifierToMethod(Method m) {
  )
 }

-private class StringReplaceMethod extends TaintPreservingCallable {
-  StringReplaceMethod() {
-    this.getDeclaringType() instanceof TypeString and
-    (
-      this.hasName("replace") or
-      this.hasName("replaceAll") or
-      this.hasName("replaceFirst")
-    )
-  }
-
-  override predicate returnsTaintFrom(int arg) { arg = 1 }
-}
-
-private predicate unsafeEscape(MethodAccess ma) {
-  // Removing `<script>` tags using a string-replace method is
-  // unsafe if such a tag is embedded inside another one (e.g. `<scr<script>ipt>`).
-  exists(StringReplaceMethod m | ma.getMethod() = m |
-    ma.getArgument(0).(StringLiteral).getValue() = "(<script>)" and
-    ma.getArgument(1).(StringLiteral).getValue() = ""
-  )
-}
-
 /** Access to a method that passes taint from an argument. */
 private predicate argToMethodStep(Expr tracked, MethodAccess sink) {
  exists(Method m, int i |
--- a/java/ql/lib/semmle/code/java/frameworks/Strings.qll
+++ b/java/ql/lib/semmle/code/java/frameworks/Strings.qll
@@ -20,10 +20,17 @@ private class StringSummaryCsv extends SummaryModelCsv {
        "java.lang;String;false;formatted;(Object[]);;Argument[0].ArrayElement;ReturnValue;taint;manual",
        "java.lang;String;false;getChars;;;Argument[-1];Argument[2];taint;manual",
        "java.lang;String;false;getBytes;;;Argument[-1];ReturnValue;taint;manual",
+        "java.lang;String;false;getBytes;;;Argument[-1];Argument[2];taint;manual",
        "java.lang;String;false;indent;;;Argument[-1];ReturnValue;taint;manual",
        "java.lang;String;false;intern;;;Argument[-1];ReturnValue;taint;manual",
        "java.lang;String;false;join;;;Argument[0..1];ReturnValue;taint;manual",
        "java.lang;String;false;repeat;(int);;Argument[-1];ReturnValue;taint;manual",
+        "java.lang;String;false;replace;;;Argument[-1];ReturnValue;taint;manual",
+        "java.lang;String;false;replace;;;Argument[1];ReturnValue;taint;manual",
+        "java.lang;String;false;replaceAll;;;Argument[-1];ReturnValue;taint;manual",
+        "java.lang;String;false;replaceAll;;;Argument[1];ReturnValue;taint;manual",
+        "java.lang;String;false;replaceFirst;;;Argument[-1];ReturnValue;taint;manual",
+        "java.lang;String;false;replaceFirst;;;Argument[1];ReturnValue;taint;manual",
        "java.lang;String;false;split;;;Argument[-1];ReturnValue;taint;manual",
        "java.lang;String;false;String;;;Argument[0];Argument[-1];taint;manual",
        "java.lang;String;false;strip;;;Argument[-1];ReturnValue;taint;manual",
@@ -55,6 +62,7 @@ private class StringSummaryCsv extends SummaryModelCsv {
        "java.lang;StringBuffer;true;StringBuffer;(CharSequence);;Argument[0];Argument[-1];taint;manual",
        "java.lang;StringBuffer;true;StringBuffer;(String);;Argument[0];Argument[-1];taint;manual",
        "java.lang;StringBuilder;true;StringBuilder;;;Argument[0];Argument[-1];taint;manual",
+        "java.lang;CharSequence;true;charAt;;;Argument[-1];ReturnValue;taint;manual",
        "java.lang;CharSequence;true;subSequence;;;Argument[-1];ReturnValue;taint;manual",
        "java.lang;CharSequence;true;toString;;;Argument[-1];ReturnValue;taint;manual"
      ]
--- a/java/ql/src/Security/CWE/CWE-113/ResponseSplitting.ql
+++ b/java/ql/src/Security/CWE/CWE-113/ResponseSplitting.ql
@@ -27,8 +27,21 @@ class ResponseSplittingConfig extends TaintTracking::Configuration {
  override predicate isSink(DataFlow::Node sink) { sink instanceof HeaderSplittingSink }

  override predicate isSanitizer(DataFlow::Node node) {
-    node.getType() instanceof PrimitiveType or
+    node.getType() instanceof PrimitiveType
+    or
    node.getType() instanceof BoxedType
+    or
+    exists(MethodAccess ma, string methodName, CompileTimeConstantExpr target |
+      node.asExpr() = ma and
+      ma.getMethod().hasQualifiedName("java.lang", "String", methodName) and
+      target = ma.getArgument(0) and
+      (
+        methodName = "replace" and target.getIntValue() = [10, 13] // 10 == "\n", 13 == "\r"
+        or
+        methodName = "replaceAll" and
+        target.getStringValue().regexpMatch(".*([\n\r]|\\[\\^[^\\]\r\n]*\\]).*")
+      )
+    )
  }
 }

--- a/java/ql/test/library-tests/dataflow/taint/B.java
+++ b/java/ql/test/library-tests/dataflow/taint/B.java
@@ -41,7 +41,7 @@ public class B {
    String valueOfSubstring = String.valueOf(complex.toCharArray(), 0, 1);
    sink(valueOfSubstring);
    // tainted - unsafe escape
-    String badEscape = constructed.replaceAll("(<script>)", "");
+    String badEscape = constructed.replaceAll("irrelevant", "irrelevant");
    sink(badEscape);
    // tainted - tokenized string
    String token = new StringTokenizer(badEscape).nextToken();
@@ -189,4 +189,32 @@ public class B {
  public static boolean safe() {
    return true;
  }
+
+  public static void extendedTests(){
+    String s = taint()[0];
+
+    String replReceiver = s.replace("irrelevant", "irrelevant");
+    sink(replReceiver);
+
+    String replChar = "a".replace('a', s.charAt(0));
+    sink(replChar);
+
+    String replCharReceiver = s.replace('a', 'b');
+    sink(replCharReceiver);
+
+    String charAt = "";
+    for(int i = 0; i < 10; i++)
+        charAt = charAt + s.charAt(i);
+    sink(charAt);
+
+    byte[] bytes = new byte[10];
+    s.getBytes(0, 1, bytes, 0);
+    sink(bytes);
+
+    String replAll = s.replaceAll("irrelevant", "irrelevant");
+    sink(replAll);
+
+    String replFirst = s.replaceFirst("irrelevant", "irrelevant");
+    sink(replFirst);
+  }
 }
--- a/java/ql/test/library-tests/dataflow/taint/test.expected
+++ b/java/ql/test/library-tests/dataflow/taint/test.expected
@@ -43,6 +43,13 @@
 | B.java:15:21:15:27 | taint(...) | B.java:157:10:157:46 | toFile(...) |
 | B.java:15:21:15:27 | taint(...) | B.java:160:10:160:46 | getAbsoluteFile(...) |
 | B.java:15:21:15:27 | taint(...) | B.java:163:10:163:47 | getCanonicalFile(...) |
+| B.java:194:16:194:22 | taint(...) | B.java:197:10:197:21 | replReceiver |
+| B.java:194:16:194:22 | taint(...) | B.java:200:10:200:17 | replChar |
+| B.java:194:16:194:22 | taint(...) | B.java:203:10:203:25 | replCharReceiver |
+| B.java:194:16:194:22 | taint(...) | B.java:208:10:208:15 | charAt |
+| B.java:194:16:194:22 | taint(...) | B.java:212:10:212:14 | bytes |
+| B.java:194:16:194:22 | taint(...) | B.java:215:10:215:16 | replAll |
+| B.java:194:16:194:22 | taint(...) | B.java:218:10:218:18 | replFirst |
 | CharSeq.java:7:26:7:32 | taint(...) | CharSeq.java:8:12:8:14 | seq |
 | CharSeq.java:7:26:7:32 | taint(...) | CharSeq.java:11:12:11:21 | seqFromSeq |
 | CharSeq.java:7:26:7:32 | taint(...) | CharSeq.java:14:12:14:24 | stringFromSeq |
--- a/java/ql/test/query-tests/security/CWE-089/semmle/examples/taintedString.expected
+++ b/java/ql/test/query-tests/security/CWE-089/semmle/examples/taintedString.expected
@@ -64,5 +64,7 @@
 | Test.java:213:21:213:24 | main | 5 | Test.java:218:14:218:17 | args |
 | Validation.java:6:21:6:35 | checkIdentifier | 1 | Validation.java:7:23:7:24 | id |
 | Validation.java:6:21:6:35 | checkIdentifier | 2 | Validation.java:8:13:8:14 | id |
+| Validation.java:6:21:6:35 | checkIdentifier | 2 | Validation.java:8:13:8:24 | charAt(...) |
+| Validation.java:6:21:6:35 | checkIdentifier | 3 | Validation.java:9:28:9:28 | c |
 | Validation.java:6:21:6:35 | checkIdentifier | 4 | Validation.java:10:32:10:58 | ... + ... |
 | Validation.java:6:21:6:35 | checkIdentifier | 4 | Validation.java:10:57:10:58 | id |
--- a/java/ql/test/query-tests/security/CWE-113/semmle/tests/ResponseSplitting.expected
+++ b/java/ql/test/query-tests/security/CWE-113/semmle/tests/ResponseSplitting.expected
@@ -1,14 +1,20 @@
 edges
 | ResponseSplitting.java:22:20:22:67 | new Cookie(...) : Cookie | ResponseSplitting.java:23:23:23:28 | cookie |
 | ResponseSplitting.java:22:39:22:66 | getParameter(...) : String | ResponseSplitting.java:22:20:22:67 | new Cookie(...) : Cookie |
+| ResponseSplitting.java:53:14:53:48 | getParameter(...) : String | ResponseSplitting.java:59:27:59:27 | t : String |
+| ResponseSplitting.java:59:27:59:27 | t : String | ResponseSplitting.java:59:27:59:57 | replaceFirst(...) |
 nodes
 | ResponseSplitting.java:22:20:22:67 | new Cookie(...) : Cookie | semmle.label | new Cookie(...) : Cookie |
 | ResponseSplitting.java:22:39:22:66 | getParameter(...) : String | semmle.label | getParameter(...) : String |
 | ResponseSplitting.java:23:23:23:28 | cookie | semmle.label | cookie |
 | ResponseSplitting.java:28:38:28:72 | getParameter(...) | semmle.label | getParameter(...) |
 | ResponseSplitting.java:29:38:29:72 | getParameter(...) | semmle.label | getParameter(...) |
+| ResponseSplitting.java:53:14:53:48 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| ResponseSplitting.java:59:27:59:27 | t : String | semmle.label | t : String |
+| ResponseSplitting.java:59:27:59:57 | replaceFirst(...) | semmle.label | replaceFirst(...) |
 subpaths
 #select
 | ResponseSplitting.java:23:23:23:28 | cookie | ResponseSplitting.java:22:39:22:66 | getParameter(...) : String | ResponseSplitting.java:23:23:23:28 | cookie | Response-splitting vulnerability due to this $@. | ResponseSplitting.java:22:39:22:66 | getParameter(...) | user-provided value |
 | ResponseSplitting.java:28:38:28:72 | getParameter(...) | ResponseSplitting.java:28:38:28:72 | getParameter(...) | ResponseSplitting.java:28:38:28:72 | getParameter(...) | Response-splitting vulnerability due to this $@. | ResponseSplitting.java:28:38:28:72 | getParameter(...) | user-provided value |
 | ResponseSplitting.java:29:38:29:72 | getParameter(...) | ResponseSplitting.java:29:38:29:72 | getParameter(...) | ResponseSplitting.java:29:38:29:72 | getParameter(...) | Response-splitting vulnerability due to this $@. | ResponseSplitting.java:29:38:29:72 | getParameter(...) | user-provided value |
+| ResponseSplitting.java:59:27:59:57 | replaceFirst(...) | ResponseSplitting.java:53:14:53:48 | getParameter(...) : String | ResponseSplitting.java:59:27:59:57 | replaceFirst(...) | Response-splitting vulnerability due to this $@. | ResponseSplitting.java:53:14:53:48 | getParameter(...) | user-provided value |
--- a/java/ql/test/query-tests/security/CWE-113/semmle/tests/ResponseSplitting.java
+++ b/java/ql/test/query-tests/security/CWE-113/semmle/tests/ResponseSplitting.java
@@ -48,4 +48,32 @@ public class ResponseSplitting extends HttpServlet {
 		Cookie cookie2 = new Cookie("name", cookie.getName());
 		response.addCookie(cookie2);
 	}
+
+	public void sanitizerTests(HttpServletRequest request, HttpServletResponse response){
+		String t = request.getParameter("contentType");
+
+		// GOOD: whitelist-based sanitization
+		response.setHeader("h", t.replaceAll("[^a-zA-Z]", ""));
+
+		// BAD: not replacing all problematic characters
+		response.setHeader("h", t.replaceFirst("[^a-zA-Z]", ""));
+
+		// GOOD: replace all line breaks
+		response.setHeader("h", t.replace('\n', ' ').replace('\r', ' '));
+
+		// FALSE NEGATIVE: replace only some line breaks
+		response.setHeader("h", t.replace('\n', ' '));
+
+		// FALSE NEGATIVE: replace only some line breaks
+		response.setHeader("h", t.replaceAll("\r", ""));
+
+		// GOOD: replace all linebreaks with a simple regex
+		response.setHeader("h", t.replaceAll("\n", "").replaceAll("\r", ""));
+
+		// GOOD: replace all linebreaks with a complex regex
+		response.setHeader("h", t.replaceAll("[\n\r]", ""));
+
+		// GOOD: replace all linebreaks with a complex regex
+		response.setHeader("h", t.replaceAll("something|[a\nb\rc]+|somethingelse", ""));
+	}
 }
--- a/java/ql/test/query-tests/security/CWE-601/semmle/tests/UrlRedirect.expected
+++ b/java/ql/test/query-tests/security/CWE-601/semmle/tests/UrlRedirect.expected
@@ -1,14 +1,25 @@
 edges
+| UrlRedirect.java:32:37:32:66 | getParameter(...) : String | UrlRedirect.java:32:25:32:67 | weakCleanup(...) |
+| UrlRedirect.java:32:37:32:66 | getParameter(...) : String | UrlRedirect.java:45:28:45:39 | input : String |
 | UrlRedirect.java:36:58:36:89 | getParameter(...) : String | UrlRedirect.java:36:25:36:89 | ... + ... |
+| UrlRedirect.java:45:28:45:39 | input : String | UrlRedirect.java:46:10:46:14 | input : String |
+| UrlRedirect.java:46:10:46:14 | input : String | UrlRedirect.java:46:10:46:40 | replaceAll(...) : String |
 nodes
 | UrlRedirect.java:23:25:23:54 | getParameter(...) | semmle.label | getParameter(...) |
+| UrlRedirect.java:32:25:32:67 | weakCleanup(...) | semmle.label | weakCleanup(...) |
+| UrlRedirect.java:32:37:32:66 | getParameter(...) : String | semmle.label | getParameter(...) : String |
 | UrlRedirect.java:36:25:36:89 | ... + ... | semmle.label | ... + ... |
 | UrlRedirect.java:36:58:36:89 | getParameter(...) : String | semmle.label | getParameter(...) : String |
 | UrlRedirect.java:39:34:39:63 | getParameter(...) | semmle.label | getParameter(...) |
 | UrlRedirect.java:42:43:42:72 | getParameter(...) | semmle.label | getParameter(...) |
+| UrlRedirect.java:45:28:45:39 | input : String | semmle.label | input : String |
+| UrlRedirect.java:46:10:46:14 | input : String | semmle.label | input : String |
+| UrlRedirect.java:46:10:46:40 | replaceAll(...) : String | semmle.label | replaceAll(...) : String |
 subpaths
+| UrlRedirect.java:32:37:32:66 | getParameter(...) : String | UrlRedirect.java:45:28:45:39 | input : String | UrlRedirect.java:46:10:46:40 | replaceAll(...) : String | UrlRedirect.java:32:25:32:67 | weakCleanup(...) |
 #select
 | UrlRedirect.java:23:25:23:54 | getParameter(...) | UrlRedirect.java:23:25:23:54 | getParameter(...) | UrlRedirect.java:23:25:23:54 | getParameter(...) | Potentially untrusted URL redirection due to $@. | UrlRedirect.java:23:25:23:54 | getParameter(...) | user-provided value |
+| UrlRedirect.java:32:25:32:67 | weakCleanup(...) | UrlRedirect.java:32:37:32:66 | getParameter(...) : String | UrlRedirect.java:32:25:32:67 | weakCleanup(...) | Potentially untrusted URL redirection due to $@. | UrlRedirect.java:32:37:32:66 | getParameter(...) | user-provided value |
 | UrlRedirect.java:36:25:36:89 | ... + ... | UrlRedirect.java:36:58:36:89 | getParameter(...) : String | UrlRedirect.java:36:25:36:89 | ... + ... | Potentially untrusted URL redirection due to $@. | UrlRedirect.java:36:58:36:89 | getParameter(...) | user-provided value |
 | UrlRedirect.java:39:34:39:63 | getParameter(...) | UrlRedirect.java:39:34:39:63 | getParameter(...) | UrlRedirect.java:39:34:39:63 | getParameter(...) | Potentially untrusted URL redirection due to $@. | UrlRedirect.java:39:34:39:63 | getParameter(...) | user-provided value |
 | UrlRedirect.java:42:43:42:72 | getParameter(...) | UrlRedirect.java:42:43:42:72 | getParameter(...) | UrlRedirect.java:42:43:42:72 | getParameter(...) | Potentially untrusted URL redirection due to $@. | UrlRedirect.java:42:43:42:72 | getParameter(...) | user-provided value |
--- a/java/ql/test/query-tests/security/CWE-601/semmle/tests/UrlRedirect.java
+++ b/java/ql/test/query-tests/security/CWE-601/semmle/tests/UrlRedirect.java
@@ -27,7 +27,7 @@ public class UrlRedirect extends HttpServlet {
 			response.sendRedirect(VALID_REDIRECT);
 		}
 		
-		// FALSE NEGATIVE: the user attempts to clean the string, but this will fail
+		// BAD: the user attempts to clean the string, but this will fail
 		// if the argument is "hthttp://tp://malicious.com"
 		response.sendRedirect(weakCleanup(request.getParameter("target")));