share implementation between TaintedPath and ZipSlip

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

@@ -32,227 +32,14 @@ module TaintedPath {
     }
 
     override predicate isBarrierGuard(DataFlow::BarrierGuardNode guard) {
-      guard instanceof StartsWithDotDotSanitizer or
-      guard instanceof StartsWithDirSanitizer or
-      guard instanceof IsAbsoluteSanitizer or
-      guard instanceof ContainsDotDotSanitizer or
-      guard instanceof RelativePathStartsWithSanitizer or
-      guard instanceof IsInsideCheckSanitizer
+      guard instanceof BarrierGuardNode
     }
 
     override predicate isAdditionalFlowStep(
       DataFlow::Node src, DataFlow::Node dst, DataFlow::FlowLabel srclabel,
       DataFlow::FlowLabel dstlabel
     ) {
-      isTaintedPathStep(src, dst, srclabel, dstlabel)
-      or
-      // Ignore all preliminary sanitization after decoding URI components
-      srclabel instanceof Label::PosixPath and
-      dstlabel instanceof Label::PosixPath and
-      (
-        any(UriLibraryStep step).step(src, dst)
-        or
-        exists(DataFlow::CallNode decode |
-          decode.getCalleeName() = "decodeURIComponent" or decode.getCalleeName() = "decodeURI"
-        |
-          src = decode.getArgument(0) and
-          dst = decode
-        )
-      )
-      or
-      promiseTaintStep(src, dst) and srclabel = dstlabel
-      or
-      any(TaintTracking::PersistentStorageTaintStep st).step(src, dst) and srclabel = dstlabel
-      or
-      exists(DataFlow::PropRead read | read = dst |
-        src = read.getBase() and
-        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
-      // A `str.split()` call can either split into path elements (`str.split("/")`) or split by some other string.
-      exists(StringSplitCall mcn | dst = mcn and mcn.getBaseString() = src |
-        if mcn.getSeparator() = "/"
-        then
-          srclabel.(Label::PosixPath).canContainDotDotSlash() and
-          dstlabel instanceof Label::SplitPath
-        else srclabel = dstlabel
-      )
-      or
-      // array method calls of interest
-      exists(DataFlow::MethodCallNode mcn, string name | dst = mcn and mcn.calls(src, name) |
-        (
-          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()
-      )
-    }
-
-    /**
-     * Holds if we should include a step from `src -> dst` with labels `srclabel -> dstlabel`, and the
-     * standard taint step `src -> dst` should be suppresesd.
-     */
-    predicate isTaintedPathStep(
-      DataFlow::Node src, DataFlow::Node dst, Label::PosixPath srclabel, Label::PosixPath dstlabel
-    ) {
-      // path.normalize() and similar
-      exists(NormalizingPathCall call |
-        src = call.getInput() and
-        dst = call.getOutput() and
-        dstlabel = srclabel.toNormalized()
-      )
-      or
-      // path.resolve() and similar
-      exists(ResolvingPathCall call |
-        src = call.getInput() and
-        dst = call.getOutput() and
-        dstlabel.isAbsolute() and
-        dstlabel.isNormalized()
-      )
-      or
-      // path.relative() and similar
-      exists(NormalizingRelativePathCall call |
-        src = call.getInput() and
-        dst = call.getOutput() and
-        dstlabel.isRelative() and
-        dstlabel.isNormalized()
-      )
-      or
-      // path.dirname() and similar
-      exists(PreservingPathCall call |
-        src = call.getInput() and
-        dst = call.getOutput() and
-        srclabel = dstlabel
-      )
-      or
-      // foo.replace(/\./, "") and similar
-      exists(DotRemovingReplaceCall call |
-        src = call.getInput() and
-        dst = call.getOutput() and
-        srclabel.isAbsolute() and
-        dstlabel.isAbsolute() and
-        dstlabel.isNormalized()
-      )
-      or
-      // foo.replace(/(\.\.\/)*/, "") and similar
-      exists(DotDotSlashPrefixRemovingReplace call |
-        src = call.getInput() and
-        dst = call.getOutput()
-      |
-        // the 4 possible combinations of normalized + relative for `srclabel`, and the possible values for `dstlabel` in each case.
-        srclabel.isNonNormalized() and srclabel.isRelative() // raw + relative -> any()
-        or
-        srclabel.isNormalized() and srclabel.isAbsolute() and srclabel = dstlabel // normalized + absolute -> normalized + absolute
-        or
-        srclabel.isNonNormalized() and srclabel.isAbsolute() and dstlabel.isAbsolute() // raw + absolute -> raw/normalized + absolute
-        // normalized + relative -> none()
-      )
-      or
-      // path.join()
-      exists(DataFlow::CallNode join, int n |
-        join = NodeJSLib::Path::moduleMember("join").getACall()
-      |
-        src = join.getArgument(n) and
-        dst = join and
-        (
-          // If the initial argument is tainted, just normalize it. It can be relative or absolute.
-          n = 0 and
-          dstlabel = srclabel.toNormalized()
-          or
-          // For later arguments, the flow label depends on whether the first argument is absolute or relative.
-          // If in doubt, we assume it is absolute.
-          n > 0 and
-          srclabel.canContainDotDotSlash() and
-          dstlabel.isNormalized() and
-          if isRelative(join.getArgument(0).getStringValue())
-          then dstlabel.isRelative()
-          else dstlabel.isAbsolute()
-        )
-      )
-      or
-      // String concatenation - behaves like path.join() except without normalization
-      exists(DataFlow::Node operator, int n |
-        StringConcatenation::taintStep(src, dst, operator, n)
-      |
-        // use ordinary taint flow for the first operand
-        n = 0 and
-        srclabel = dstlabel
-        or
-        n > 0 and
-        srclabel.canContainDotDotSlash() and
-        dstlabel.isNonNormalized() and // The ../ is no longer at the beginning of the string.
-        (
-          if isRelative(StringConcatenation::getOperand(operator, 0).getStringValue())
-          then dstlabel.isRelative()
-          else dstlabel.isAbsolute()
-        )
-      )
+      isAdditionalTaintedPathFlowStep(src, dst, srclabel, dstlabel)
     }
   }
 }

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

@@ -28,6 +28,11 @@ module TaintedPath {
    */
   abstract class Sanitizer extends DataFlow::Node { }
 
+  /**
+   * A barrier guard for tainted-path vulnerabilities.
+   */
+  abstract class BarrierGuardNode extends DataFlow::LabeledBarrierGuardNode { }
+
   module Label {
     /**
      * A string indicating if a path is normalized, that is, whether internal `../` components
@@ -343,7 +348,7 @@ module TaintedPath {
    *
    * This is relevant for paths that are known to be normalized.
    */
-  class StartsWithDotDotSanitizer extends DataFlow::LabeledBarrierGuardNode {
+  class StartsWithDotDotSanitizer extends BarrierGuardNode {
     StringOps::StartsWith startsWith;
 
     StartsWithDotDotSanitizer() {
@@ -369,7 +374,7 @@ module TaintedPath {
    * A check of form `x.startsWith(dir)` that sanitizes normalized absolute paths, since it is then
    * known to be in a subdirectory of `dir`.
    */
-  class StartsWithDirSanitizer extends DataFlow::LabeledBarrierGuardNode {
+  class StartsWithDirSanitizer extends BarrierGuardNode {
     StringOps::StartsWith startsWith;
 
     StartsWithDirSanitizer() {
@@ -393,7 +398,7 @@ module TaintedPath {
    * A call to `path.isAbsolute` as a sanitizer for relative paths in true branch,
    * and a sanitizer for absolute paths in the false branch.
    */
-  class IsAbsoluteSanitizer extends DataFlow::LabeledBarrierGuardNode {
+  class IsAbsoluteSanitizer extends BarrierGuardNode {
     DataFlow::Node operand;
     boolean polarity;
     boolean negatable;
@@ -429,7 +434,7 @@ module TaintedPath {
   /**
    * An expression of form `x.includes("..")` or similar.
    */
-  class ContainsDotDotSanitizer extends DataFlow::LabeledBarrierGuardNode {
+  class ContainsDotDotSanitizer extends BarrierGuardNode {
     StringOps::Includes contains;
 
     ContainsDotDotSanitizer() {
@@ -465,7 +470,7 @@ module TaintedPath {
    * }
    * ```
    */
-  class RelativePathStartsWithSanitizer extends DataFlow::BarrierGuardNode {
+  class RelativePathStartsWithSanitizer extends BarrierGuardNode {
     StringOps::StartsWith startsWith;
     DataFlow::CallNode pathCall;
     string member;
@@ -507,7 +512,7 @@ module TaintedPath {
    * An expression of form `isInside(x, y)` or similar, where `isInside` is
    * a library check for the relation between `x` and `y`.
    */
-  class IsInsideCheckSanitizer extends DataFlow::LabeledBarrierGuardNode {
+  class IsInsideCheckSanitizer extends BarrierGuardNode {
     DataFlow::Node checked;
     boolean onlyNormalizedAbsolutePaths;
 
@@ -614,4 +619,221 @@ module TaintedPath {
   class SendPathSink extends Sink, DataFlow::ValueNode {
     SendPathSink() { this = DataFlow::moduleImport("send").getACall().getArgument(1) }
   }
+
+  /**
+   * Holds if there is a flow step from `src` with flow-label `srclabel` to
+   * `dst` with flow-label `dstlabel` for a tainted-path vulnerability.
+   */
+  predicate isAdditionalTaintedPathFlowStep(
+    DataFlow::Node src, DataFlow::Node dst, DataFlow::FlowLabel srclabel,
+    DataFlow::FlowLabel dstlabel
+  ) {
+    isTaintedPathStep(src, dst, srclabel, dstlabel)
+    or
+    // Ignore all preliminary sanitization after decoding URI components
+    srclabel instanceof Label::PosixPath and
+    dstlabel instanceof Label::PosixPath and
+    (
+      any(UriLibraryStep step).step(src, dst)
+      or
+      exists(DataFlow::CallNode decode |
+        decode.getCalleeName() = "decodeURIComponent" or decode.getCalleeName() = "decodeURI"
+      |
+        src = decode.getArgument(0) and
+        dst = decode
+      )
+    )
+    or
+    promiseTaintStep(src, dst) and srclabel = dstlabel
+    or
+    any(TaintTracking::PersistentStorageTaintStep st).step(src, dst) and srclabel = dstlabel
+    or
+    exists(DataFlow::PropRead read | read = dst |
+      src = read.getBase() and
+      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
+    // A `str.split()` call can either split into path elements (`str.split("/")`) or split by some other string.
+    exists(StringSplitCall mcn | dst = mcn and mcn.getBaseString() = src |
+      if mcn.getSeparator() = "/"
+      then
+        srclabel.(Label::PosixPath).canContainDotDotSlash() and
+        dstlabel instanceof Label::SplitPath
+      else srclabel = dstlabel
+    )
+    or
+    // array method calls of interest
+    exists(DataFlow::MethodCallNode mcn, string name | dst = mcn and mcn.calls(src, name) |
+      (
+        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()
+    )
+  }
+
+  /**
+   * Holds if we should include a step from `src -> dst` with labels `srclabel -> dstlabel`, and the
+   * standard taint step `src -> dst` should be suppresesd.
+   */
+  private predicate isTaintedPathStep(
+    DataFlow::Node src, DataFlow::Node dst, Label::PosixPath srclabel, Label::PosixPath dstlabel
+  ) {
+    // path.normalize() and similar
+    exists(NormalizingPathCall call |
+      src = call.getInput() and
+      dst = call.getOutput() and
+      dstlabel = srclabel.toNormalized()
+    )
+    or
+    // path.resolve() and similar
+    exists(ResolvingPathCall call |
+      src = call.getInput() and
+      dst = call.getOutput() and
+      dstlabel.isAbsolute() and
+      dstlabel.isNormalized()
+    )
+    or
+    // path.relative() and similar
+    exists(NormalizingRelativePathCall call |
+      src = call.getInput() and
+      dst = call.getOutput() and
+      dstlabel.isRelative() and
+      dstlabel.isNormalized()
+    )
+    or
+    // path.dirname() and similar
+    exists(PreservingPathCall call |
+      src = call.getInput() and
+      dst = call.getOutput() and
+      srclabel = dstlabel
+    )
+    or
+    // foo.replace(/\./, "") and similar
+    exists(DotRemovingReplaceCall call |
+      src = call.getInput() and
+      dst = call.getOutput() and
+      srclabel.isAbsolute() and
+      dstlabel.isAbsolute() and
+      dstlabel.isNormalized()
+    )
+    or
+    // foo.replace(/(\.\.\/)*/, "") and similar
+    exists(DotDotSlashPrefixRemovingReplace call |
+      src = call.getInput() and
+      dst = call.getOutput()
+    |
+      // the 4 possible combinations of normalized + relative for `srclabel`, and the possible values for `dstlabel` in each case.
+      srclabel.isNonNormalized() and srclabel.isRelative() // raw + relative -> any()
+      or
+      srclabel.isNormalized() and srclabel.isAbsolute() and srclabel = dstlabel // normalized + absolute -> normalized + absolute
+      or
+      srclabel.isNonNormalized() and srclabel.isAbsolute() and dstlabel.isAbsolute() // raw + absolute -> raw/normalized + absolute
+      // normalized + relative -> none()
+    )
+    or
+    // path.join()
+    exists(DataFlow::CallNode join, int n |
+      join = NodeJSLib::Path::moduleMember("join").getACall()
+    |
+      src = join.getArgument(n) and
+      dst = join and
+      (
+        // If the initial argument is tainted, just normalize it. It can be relative or absolute.
+        n = 0 and
+        dstlabel = srclabel.toNormalized()
+        or
+        // For later arguments, the flow label depends on whether the first argument is absolute or relative.
+        // If in doubt, we assume it is absolute.
+        n > 0 and
+        srclabel.canContainDotDotSlash() and
+        dstlabel.isNormalized() and
+        if isRelative(join.getArgument(0).getStringValue())
+        then dstlabel.isRelative()
+        else dstlabel.isAbsolute()
+      )
+    )
+    or
+    // String concatenation - behaves like path.join() except without normalization
+    exists(DataFlow::Node operator, int n | StringConcatenation::taintStep(src, dst, operator, n) |
+      // use ordinary taint flow for the first operand
+      n = 0 and
+      srclabel = dstlabel
+      or
+      n > 0 and
+      srclabel.canContainDotDotSlash() and
+      dstlabel.isNonNormalized() and // The ../ is no longer at the beginning of the string.
+      (
+        if isRelative(StringConcatenation::getOperand(operator, 0).getStringValue())
+        then dstlabel.isRelative()
+        else dstlabel.isAbsolute()
+      )
+    )
+  }
 }

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

@@ -13,17 +13,31 @@ module ZipSlip {
   import ZipSlipCustomizations::ZipSlip
 
   /** A taint tracking configuration for unsafe archive extraction. */
-  class Configuration extends TaintTracking::Configuration {
+  class Configuration extends DataFlow::Configuration {
     Configuration() { this = "ZipSlip" }
 
-    override predicate isSource(DataFlow::Node source) { source instanceof Source }
+    override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
+      label = source.(Source).getAFlowLabel()
+    }
 
-    override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+    override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
+      label = sink.(Sink).getAFlowLabel()
+    }
 
-    override predicate isSanitizer(DataFlow::Node sanitizer) { sanitizer instanceof Sanitizer }
+    override predicate isBarrier(DataFlow::Node node) {
+      super.isBarrier(node) or
+      node instanceof TaintedPath::Sanitizer
+    }
 
-    override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode nd) {
-      nd instanceof SanitizerGuard
+    override predicate isBarrierGuard(DataFlow::BarrierGuardNode guard) {
+      guard instanceof TaintedPath::BarrierGuardNode
+    }
+
+    override predicate isAdditionalFlowStep(
+      DataFlow::Node src, DataFlow::Node dst, DataFlow::FlowLabel srclabel,
+      DataFlow::FlowLabel dstlabel
+    ) {
+      TaintedPath::isAdditionalTaintedPathFlowStep(src, dst, srclabel, dstlabel)
     }
   }
 }

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

@@ -7,25 +7,23 @@
 import javascript
 
 module ZipSlip {
+  import TaintedPathCustomizations::TaintedPath as TaintedPath
+
   /**
    * A data flow source for unsafe archive extraction.
    */
-  abstract class Source extends DataFlow::Node { }
+  abstract class Source extends DataFlow::Node {
+    /** Gets a flow label denoting the type of value for which this is a source. */
+    TaintedPath::Label::PosixPath getAFlowLabel() { result.isRelative() }
+  }
 
   /**
    * A data flow sink for unsafe archive extraction.
    */
-  abstract class Sink extends DataFlow::Node { }
-
-  /**
-   * A sanitizer for unsafe archive extraction.
-   */
-  abstract class Sanitizer extends DataFlow::Node { }
-
-  /**
-   * A sanitizer guard for unsafe archive extraction.
-   */
-  abstract class SanitizerGuard extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode { }
+  abstract class Sink extends DataFlow::Node {
+    /** Gets a flow label denoting the type of value for which this is a sink. */
+    TaintedPath::Label::PosixPath getAFlowLabel() { any() }
+  }
 
   /**
    * Gets a node that can be a parsed archive.
@@ -122,42 +120,4 @@ module ZipSlip {
   class FileSystemWriteSink extends Sink {
     FileSystemWriteSink() { exists(FileSystemWriteAccess fsw | fsw.getAPathArgument() = this) }
   }
-
-  /** An expression that sanitizes by calling path.basename */
-  class BasenameSanitizer extends Sanitizer {
-    BasenameSanitizer() { this = DataFlow::moduleImport("path").getAMemberCall("basename") }
-  }
-
-  /**
-   * An expression that forces the output path to be in the current working folder.
-   * Recognizes the pattern: `path.join(cwd, path.join('/', orgPath))`.
-   */
-  class PathSanitizer extends Sanitizer, DataFlow::CallNode {
-    PathSanitizer() {
-      this = NodeJSLib::Path::moduleMember("join").getACall() and
-      exists(DataFlow::CallNode inner | inner = getArgument(1) |
-        inner = NodeJSLib::Path::moduleMember("join").getACall() and
-        inner.getArgument(0).mayHaveStringValue("/")
-      )
-    }
-  }
-
-  /**
-   * Gets a string which is sufficient to exclude to make
-   * a filepath definitely not refer to parent directories.
-   */
-  private string getAParentDirName() { result = ".." or result = "../" }
-
-  /** A check that a path string does not include '..' */
-  class NoParentDirSanitizerGuard extends SanitizerGuard {
-    StringOps::Includes incl;
-
-    NoParentDirSanitizerGuard() { this = incl }
-
-    override predicate sanitizes(boolean outcome, Expr e) {
-      incl.getPolarity().booleanNot() = outcome and
-      incl.getBaseString().asExpr() = e and
-      incl.getSubstring().mayHaveStringValue(getAParentDirName())
-    }
-  }
 }

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

@@ -2,36 +2,55 @@ nodes
 | AdmZipBad.js:6:24:6:41 | zipEntry.entryName |
 | AdmZipBad.js:6:24:6:41 | zipEntry.entryName |
 | AdmZipBad.js:6:24:6:41 | zipEntry.entryName |
+| AdmZipBad.js:6:24:6:41 | zipEntry.entryName |
 | TarSlipBad.js:6:36:6:46 | header.name |
 | TarSlipBad.js:6:36:6:46 | header.name |
 | TarSlipBad.js:6:36:6:46 | header.name |
+| TarSlipBad.js:6:36:6:46 | header.name |
+| TarSlipBad.js:9:17:9:31 | header.linkname |
 | TarSlipBad.js:9:17:9:31 | header.linkname |
 | TarSlipBad.js:9:17:9:31 | header.linkname |
 | TarSlipBad.js:9:17:9:31 | header.linkname |
 | ZipSlipBad2.js:5:9:5:46 | fileName |
+| ZipSlipBad2.js:5:9:5:46 | fileName |
+| ZipSlipBad2.js:5:20:5:46 | 'output ... ry.path |
 | ZipSlipBad2.js:5:20:5:46 | 'output ... ry.path |
 | ZipSlipBad2.js:5:37:5:46 | entry.path |
 | ZipSlipBad2.js:5:37:5:46 | entry.path |
+| ZipSlipBad2.js:5:37:5:46 | entry.path |
+| ZipSlipBad2.js:6:22:6:29 | fileName |
 | ZipSlipBad2.js:6:22:6:29 | fileName |
 | ZipSlipBad2.js:6:22:6:29 | fileName |
 | ZipSlipBad.js:7:11:7:31 | fileName |
+| ZipSlipBad.js:7:11:7:31 | fileName |
 | ZipSlipBad.js:7:22:7:31 | entry.path |
 | ZipSlipBad.js:7:22:7:31 | entry.path |
+| ZipSlipBad.js:7:22:7:31 | entry.path |
+| ZipSlipBad.js:8:37:8:44 | fileName |
 | ZipSlipBad.js:8:37:8:44 | fileName |
 | ZipSlipBad.js:8:37:8:44 | fileName |
 | ZipSlipBad.js:15:11:15:31 | fileName |
+| ZipSlipBad.js:15:11:15:31 | fileName |
 | ZipSlipBad.js:15:22:15:31 | entry.path |
 | ZipSlipBad.js:15:22:15:31 | entry.path |
+| ZipSlipBad.js:15:22:15:31 | entry.path |
+| ZipSlipBad.js:16:30:16:37 | fileName |
 | ZipSlipBad.js:16:30:16:37 | fileName |
 | ZipSlipBad.js:16:30:16:37 | fileName |
 | ZipSlipBad.js:22:11:22:31 | fileName |
+| ZipSlipBad.js:22:11:22:31 | fileName |
 | ZipSlipBad.js:22:22:22:31 | entry.path |
 | ZipSlipBad.js:22:22:22:31 | entry.path |
+| ZipSlipBad.js:22:22:22:31 | entry.path |
+| ZipSlipBad.js:23:28:23:35 | fileName |
 | ZipSlipBad.js:23:28:23:35 | fileName |
 | ZipSlipBad.js:23:28:23:35 | fileName |
 | ZipSlipBadUnzipper.js:7:9:7:29 | fileName |
+| ZipSlipBadUnzipper.js:7:9:7:29 | fileName |
 | ZipSlipBadUnzipper.js:7:20:7:29 | entry.path |
 | ZipSlipBadUnzipper.js:7:20:7:29 | entry.path |
+| ZipSlipBadUnzipper.js:7:20:7:29 | entry.path |
+| ZipSlipBadUnzipper.js:8:37:8:44 | fileName |
 | ZipSlipBadUnzipper.js:8:37:8:44 | fileName |
 | ZipSlipBadUnzipper.js:8:37:8:44 | fileName |
 edges
@@ -40,23 +59,44 @@ edges
 | TarSlipBad.js:9:17:9:31 | header.linkname | TarSlipBad.js:9:17:9:31 | header.linkname |
 | ZipSlipBad2.js:5:9:5:46 | fileName | ZipSlipBad2.js:6:22:6:29 | fileName |
 | ZipSlipBad2.js:5:9:5:46 | fileName | ZipSlipBad2.js:6:22:6:29 | fileName |
+| ZipSlipBad2.js:5:9:5:46 | fileName | ZipSlipBad2.js:6:22:6:29 | fileName |
+| ZipSlipBad2.js:5:9:5:46 | fileName | ZipSlipBad2.js:6:22:6:29 | fileName |
+| ZipSlipBad2.js:5:20:5:46 | 'output ... ry.path | ZipSlipBad2.js:5:9:5:46 | fileName |
 | ZipSlipBad2.js:5:20:5:46 | 'output ... ry.path | ZipSlipBad2.js:5:9:5:46 | fileName |
 | ZipSlipBad2.js:5:37:5:46 | entry.path | ZipSlipBad2.js:5:20:5:46 | 'output ... ry.path |
 | ZipSlipBad2.js:5:37:5:46 | entry.path | ZipSlipBad2.js:5:20:5:46 | 'output ... ry.path |
+| ZipSlipBad2.js:5:37:5:46 | entry.path | ZipSlipBad2.js:5:20:5:46 | 'output ... ry.path |
+| ZipSlipBad2.js:5:37:5:46 | entry.path | ZipSlipBad2.js:5:20:5:46 | 'output ... ry.path |
+| ZipSlipBad.js:7:11:7:31 | fileName | ZipSlipBad.js:8:37:8:44 | fileName |
+| ZipSlipBad.js:7:11:7:31 | fileName | ZipSlipBad.js:8:37:8:44 | fileName |
 | ZipSlipBad.js:7:11:7:31 | fileName | ZipSlipBad.js:8:37:8:44 | fileName |
 | ZipSlipBad.js:7:11:7:31 | fileName | ZipSlipBad.js:8:37:8:44 | fileName |
 | ZipSlipBad.js:7:22:7:31 | entry.path | ZipSlipBad.js:7:11:7:31 | fileName |
 | ZipSlipBad.js:7:22:7:31 | entry.path | ZipSlipBad.js:7:11:7:31 | fileName |
+| ZipSlipBad.js:7:22:7:31 | entry.path | ZipSlipBad.js:7:11:7:31 | fileName |
+| ZipSlipBad.js:7:22:7:31 | entry.path | ZipSlipBad.js:7:11:7:31 | fileName |
+| ZipSlipBad.js:15:11:15:31 | fileName | ZipSlipBad.js:16:30:16:37 | fileName |
+| ZipSlipBad.js:15:11:15:31 | fileName | ZipSlipBad.js:16:30:16:37 | fileName |
 | ZipSlipBad.js:15:11:15:31 | fileName | ZipSlipBad.js:16:30:16:37 | fileName |
 | ZipSlipBad.js:15:11:15:31 | fileName | ZipSlipBad.js:16:30:16:37 | fileName |
 | ZipSlipBad.js:15:22:15:31 | entry.path | ZipSlipBad.js:15:11:15:31 | fileName |
 | ZipSlipBad.js:15:22:15:31 | entry.path | ZipSlipBad.js:15:11:15:31 | fileName |
+| ZipSlipBad.js:15:22:15:31 | entry.path | ZipSlipBad.js:15:11:15:31 | fileName |
+| ZipSlipBad.js:15:22:15:31 | entry.path | ZipSlipBad.js:15:11:15:31 | fileName |
+| ZipSlipBad.js:22:11:22:31 | fileName | ZipSlipBad.js:23:28:23:35 | fileName |
+| ZipSlipBad.js:22:11:22:31 | fileName | ZipSlipBad.js:23:28:23:35 | fileName |
 | ZipSlipBad.js:22:11:22:31 | fileName | ZipSlipBad.js:23:28:23:35 | fileName |
 | ZipSlipBad.js:22:11:22:31 | fileName | ZipSlipBad.js:23:28:23:35 | fileName |
 | ZipSlipBad.js:22:22:22:31 | entry.path | ZipSlipBad.js:22:11:22:31 | fileName |
 | ZipSlipBad.js:22:22:22:31 | entry.path | ZipSlipBad.js:22:11:22:31 | fileName |
+| ZipSlipBad.js:22:22:22:31 | entry.path | ZipSlipBad.js:22:11:22:31 | fileName |
+| ZipSlipBad.js:22:22:22:31 | entry.path | ZipSlipBad.js:22:11:22:31 | fileName |
 | ZipSlipBadUnzipper.js:7:9:7:29 | fileName | ZipSlipBadUnzipper.js:8:37:8:44 | fileName |
 | ZipSlipBadUnzipper.js:7:9:7:29 | fileName | ZipSlipBadUnzipper.js:8:37:8:44 | fileName |
+| ZipSlipBadUnzipper.js:7:9:7:29 | fileName | ZipSlipBadUnzipper.js:8:37:8:44 | fileName |
+| ZipSlipBadUnzipper.js:7:9:7:29 | fileName | ZipSlipBadUnzipper.js:8:37:8:44 | fileName |
+| ZipSlipBadUnzipper.js:7:20:7:29 | entry.path | ZipSlipBadUnzipper.js:7:9:7:29 | fileName |
+| ZipSlipBadUnzipper.js:7:20:7:29 | entry.path | ZipSlipBadUnzipper.js:7:9:7:29 | fileName |
 | ZipSlipBadUnzipper.js:7:20:7:29 | entry.path | ZipSlipBadUnzipper.js:7:9:7:29 | fileName |
 | ZipSlipBadUnzipper.js:7:20:7:29 | entry.path | ZipSlipBadUnzipper.js:7:9:7:29 | fileName |
 #select

javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipGood.js

@@ -29,3 +29,11 @@ fs.createReadStream('archive.zip')
       entry.pipe(fs.createWriteStream(fileName)); // OK.
     }
   });
+
+fs.createReadStream('archive.zip')
+  .pipe(unzip.Parse())
+  .on('entry', entry => {
+    const fileName = path.normalize(entry.path);
+
+    entry.pipe(fs.createWriteStream(path.basename(fileName))); // OK.
+  });