JS: add additional flow steps to js/path-injection

Author: esbena

change-notes/1.24/analysis-javascript.md

@@ -39,6 +39,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 | srclabel = dstlabel |
+        exists(string substringMethodName |
+          substringMethodName = "substr" or
+          substringMethodName = "substring" or
+          substringMethodName = "slice"
+        |
+          mcn.calls(src, substringMethodName) and
+          // to avoid very dynamic transformations, require at least one fixed index
+          exists(mcn.getAnArgument().asExpr().getIntValue()) and
+          dst = mcn
+        ) or
+        exists(string argumentlessMethodName |
+          argumentlessMethodName = "toLocaleLowerCase" or
+          argumentlessMethodName = "toLocaleUpperCase" or
+          argumentlessMethodName = "toLowerCase" or
+          argumentlessMethodName = "toUpperCase" or
+          argumentlessMethodName = "trim" or
+          argumentlessMethodName = "trimLeft" or
+          argumentlessMethodName = "trimRight"
+        |
+          mcn.calls(src, argumentlessMethodName) and
+          dst = mcn
+        )
+        or
+        mcn.calls(src, "split") and
+        dst = mcn 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 |