Merge pull request github#2810 from erik-krogh/CVE74

Approved by asgerf

Author: semmle-qlci

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

@@ -93,13 +93,68 @@ module TaintedPath {
         |
           name = argumentlessMethodName
         )
-        or
+      )
+      or
+      // array method calls of interest
+      exists(DataFlow::MethodCallNode mcn, string name | dst = mcn and mcn.calls(src, name) |
+        // A `str.split()` call can either split into path elements (`str.split("/")`) or split by some other string.
         name = "split" and
-        not exists(DataFlow::Node splitBy | splitBy = mcn.getArgument(0) |
-          splitBy.mayHaveStringValue("/") or
-          any(DataFlow::RegExpLiteralNode reg | reg.getRoot().getAMatchedString() = "/")
-              .flowsTo(splitBy)
+        (
+          if
+            exists(DataFlow::Node splitBy | splitBy = mcn.getArgument(0) |
+              splitBy.mayHaveStringValue("/") or
+              any(DataFlow::RegExpCreationNode reg | reg.getRoot().getAMatchedString() = "/")
+                  .flowsTo(splitBy)
+            )
+          then
+            srclabel.(Label::PosixPath).canContainDotDotSlash() and
+            dstlabel instanceof Label::SplitPath
+          else srclabel = dstlabel
         )
+        or
+        (
+          name = "pop" or
+          name = "shift"
+        ) and
+        srclabel instanceof Label::SplitPath and
+        dstlabel.(Label::PosixPath).canContainDotDotSlash()
+        or
+        (
+          name = "slice" or
+          name = "splice" or
+          name = "concat"
+        ) and
+        dstlabel instanceof Label::SplitPath and
+        srclabel instanceof Label::SplitPath
+        or
+        name = "join" and
+        mcn.getArgument(0).mayHaveStringValue("/") and
+        srclabel instanceof Label::SplitPath and
+        dstlabel.(Label::PosixPath).canContainDotDotSlash()
+      )
+      or
+      // prefix.concat(path)
+      exists(DataFlow::MethodCallNode mcn |
+        mcn.getMethodName() = "concat" and mcn.getAnArgument() = src
+      |
+        dst = mcn and
+        dstlabel instanceof Label::SplitPath and
+        srclabel instanceof Label::SplitPath
+      )
+      or
+      // reading unknown property of split path
+      exists(DataFlow::PropRead read | read = dst |
+        src = read.getBase() and
+        not read.getPropertyName() = "length" and
+        not exists(read.getPropertyNameExpr().getIntValue()) and
+        // split[split.length - 1]
+        not exists(BinaryExpr binop |
+          read.getPropertyNameExpr() = binop and
+          binop.getAnOperand().getIntValue() = 1 and
+          binop.getAnOperand().(PropAccess).getPropertyName() = "length"
+        ) and
+        srclabel instanceof Label::SplitPath and
+        dstlabel.(Label::PosixPath).canContainDotDotSlash()
       )
     }
 

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

@@ -108,6 +108,15 @@ module TaintedPath {
         not (isNormalized() and isAbsolute())
       }
     }
+
+    /**
+     * A flow label representing an array of path elements that may include "..". 
+     */ 
+    class SplitPath extends DataFlow::FlowLabel {
+      SplitPath() {
+        this = "splitPath"
+      }
+    }
   }
 
   /**

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

@@ -1,3 +1,4 @@
 | normalizedPaths.js:208:38:208:63 | // OK - ...  anyway | Spurious 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 |
+| tainted-string-steps.js:28:39:28:70 | // NOT  ... flagged | Missing alert |