Merge pull request #2741 from esbena/js/split-and-slice-for-tainted-path

Approved by erik-krogh

Author: semmle-qlci

change-notes/1.24/analysis-javascript.md

@@ -42,6 +42,7 @@
 | Expression has no effect (`js/useless-expression`) | Fewer false positive results | The query now recognizes block-level flow type annotations and ignores the first statement of a try block. |
 | Use of call stack introspection in strict mode (`js/strict-mode-call-stack-introspection`) | Fewer false positive results | The query no longer flags expression statements. |
 | Missing CSRF middleware (`js/missing-token-validation`) | Fewer false positive results | The query reports fewer duplicates and only flags handlers that explicitly access cookie data. |
+| Uncontrolled data used in path expression (`js/path-injection`) | More results | This query now recognizes additional ways dangerous paths can be constructed. |
 
 ## Changes to libraries
 

javascript/ql/src/semmle/javascript/security/dataflow/TaintedPath.qll

@@ -67,6 +67,40 @@ module TaintedPath {
         read.getPropertyName() != "length" and
         srclabel = dstlabel
       )
+      or
+      // string method calls of interest
+      exists(DataFlow::MethodCallNode mcn, string name |
+        srclabel = dstlabel and dst = mcn and mcn.calls(src, name)
+      |
+        exists(string substringMethodName |
+          substringMethodName = "substr" or
+          substringMethodName = "substring" or
+          substringMethodName = "slice"
+        |
+          name = substringMethodName and
+          // to avoid very dynamic transformations, require at least one fixed index
+          exists(mcn.getAnArgument().asExpr().getIntValue())
+        )
+        or
+        exists(string argumentlessMethodName |
+          argumentlessMethodName = "toLocaleLowerCase" or
+          argumentlessMethodName = "toLocaleUpperCase" or
+          argumentlessMethodName = "toLowerCase" or
+          argumentlessMethodName = "toUpperCase" or
+          argumentlessMethodName = "trim" or
+          argumentlessMethodName = "trimLeft" or
+          argumentlessMethodName = "trimRight"
+        |
+          name = argumentlessMethodName
+        )
+        or
+        name = "split" and
+        not exists(DataFlow::Node splitBy | splitBy = mcn.getArgument(0) |
+          splitBy.mayHaveStringValue("/") or
+          any(DataFlow::RegExpLiteralNode reg | reg.getRoot().getAMatchedString() = "/")
+              .flowsTo(splitBy)
+        )
+      )
     }
 
     /**

javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/Consistency.expected

@@ -1 +1,6 @@
 | normalizedPaths.js:208:38:208:63 | // OK - ...  anyway | Spurious alert |
+| tainted-string-steps.js:13:41:13:72 | // NOT  ... flagged | Missing alert |
+| tainted-string-steps.js:14:41:14:72 | // NOT  ... flagged | Missing alert |
+| tainted-string-steps.js:15:50:15:81 | // NOT  ... flagged | Missing alert |
+| tainted-string-steps.js:25:43:25:74 | // NOT  ... flagged | Missing alert |
+| tainted-string-steps.js:26:49:26:74 | // OK - ... flagged | Spurious alert |