JS: Address review comments

javascript/ql/src/Security/CWE-022/ZipSlip.qhelp

@@ -16,13 +16,22 @@ file paths are used to determine an output file to write the contents of the arc
 the file may be written to an unexpected location. This can result in sensitive information being
 revealed or deleted, or an attacker being able to influence behavior by modifying unexpected
 files.</p>
-</overview>
 
+<p>For example, if a zip file contains a file entry <code>..\sneaky-file</code>, and the zip file
+is extracted to the directory <code>c:\output</code>, then naively combining the paths would result
+in an output file path of <code>c:\output\..\sneaky-file</code>, which would cause the file to be
+written to <code>c:\sneaky-file</code>.</p>
+
+</overview>
 <recommendation>
 
 <p>Ensure that output paths constructed from zip archive entries are validated
 to prevent writing files to unexpected locations.</p>
 
+<p>The recommended way of writing an output file from a zip archive entry is to check that
+<code>".."</code> does not occur in the path.
+</p>
+
 </recommendation>
 
 <example>
@@ -34,5 +43,12 @@ instance, if it were created by something like <code>zip archive.zip
 </p>
 
 <sample src="ZipSlipBad.js" />
+
+<p>To fix this vulnerability, we can to check that the path does not
+contain any <code>".."</code> in it.
+</p>
+
+<sample src="ZipSlipGood.java" />
+
 </example>
 </qhelp>

javascript/ql/src/Security/CWE-022/ZipSlip.ql

@@ -6,7 +6,7 @@
  * @kind path-problem
  * @id js/zipslip
  * @problem.severity error
- * @precision high
+ * @precision medium
  * @tags security
  *       external/cwe/cwe-022
  */

javascript/ql/src/Security/CWE-022/ZipSlipGood.js

@@ -0,0 +1,14 @@
+const fs = require('fs');
+const unzip = require('unzip');
+
+fs.createReadStream('archive.zip')
+  .pipe(unzip.Parse())
+  .on('entry', entry => {
+    const fileName = entry.path;
+    if (entry.path.indexOf('..') == -1) {
+      entry.pipe(fs.createWriteStream(entry.path));
+    }
+    else {
+      console.log('skipping bad path', entry.path);
+    }
+  });

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

@@ -20,7 +20,7 @@ module ZipSlip {
    */
   abstract class SanitizerGuard extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode { }
 
-  /** A taint tracking configuration for Zip Slip */
+  /** A taint tracking configuration for unsafe zip extraction. */
   class Configuration extends TaintTracking::Configuration {
     Configuration() { this = "ZipSlip" }
 
@@ -36,51 +36,54 @@ module ZipSlip {
   /**
    * Gets a node that can be a parsed zip archive.
    */
-  DataFlow::SourceNode parsedArchive() {
+  private DataFlow::SourceNode parsedArchive() {
     result = DataFlow::moduleImport("unzip").getAMemberCall("Parse")
     or
     // `streamProducer.pipe(unzip.Parse())` is a typical (but not
     // universal) pattern when using nodejs streams, whose return
     // value is the parsed stream.
-    exists(DataFlow::MethodCallNode pipe | pipe.getMethodName() = "pipe"
-      and parsedArchive().flowsTo(pipe.getArgument(0)))
+    exists(DataFlow::MethodCallNode pipe |
+      pipe.getMethodName() = "pipe" and
+      parsedArchive().flowsTo(pipe.getArgument(0))
+    )
   }
 
-  /**
-   * An access to the filepath of an entry of a zipfile being extracted
-   * by npm module `unzip`. For example, in
-   * ```javascript
-   * const unzip = require('unzip');
-   *
-   * fs.createReadStream('archive.zip')
-   *   .pipe(unzip.Parse())
-   *   .on('entry', entry => {
-   *      const path = entry.path;
-   *   });
-   * ```
-   * there is an `UnzipEntrySource` node corresponding to
-   * the expression `entry.path`.
-   */
+  /** A zip archive entry path access, as a source for unsafe zip extraction. */
   class UnzipEntrySource extends Source {
+    // For example, in
+    // ```javascript
+    // const unzip = require('unzip');
+    //
+    // fs.createReadStream('archive.zip')
+    //   .pipe(unzip.Parse())
+    //   .on('entry', entry => {
+    //      const path = entry.path;
+    //   });
+    // ```
+    // there is an `UnzipEntrySource` node corresponding to
+    // the expression `entry.path`.
     UnzipEntrySource() {
-      this = parsedArchive().getAMemberCall("on").getCallback(1).getParameter(0).getAPropertyRead("path")
+      this = parsedArchive()
+            .getAMemberCall("on")
+            .getCallback(1)
+            .getParameter(0)
+            .getAPropertyRead("path")
     }
   }
 
-  /**
-   * A sink that is the path that a createWriteStream gets created at.
-   * This is not covered by FileSystemWriteSink, because it is
-   * required that a write actually takes place to the stream.
-   * However, we want to consider even the bare createWriteStream to
-   * be a zipslip vulnerability since it may truncate an existing file.
-   */
+  /** A call to `fs.createWriteStream`, as a sink for unsafe zip extraction. */
   class CreateWriteStreamSink extends Sink {
     CreateWriteStreamSink() {
+      // This is not covered by `FileSystemWriteSink`, because it is
+      // required that a write actually takes place to the stream.
+      // However, we want to consider even the bare createWriteStream
+      // to be a zipslip vulnerability since it may truncate an
+      // existing file.
       this = DataFlow::moduleImport("fs").getAMemberCall("createWriteStream").getArgument(0)
     }
   }
 
-  /** A sink that is a file path that gets written to. */
+  /** A file path of a file write, as a sink for unsafe zip extraction. */
   class FileSystemWriteSink extends Sink {
     FileSystemWriteSink() { exists(FileSystemWriteAccess fsw | fsw.getAPathArgument() = this) }
   }
@@ -89,7 +92,7 @@ module ZipSlip {
    * Gets a string which suffices to search for to ensure that a
    * filepath will not refer to parent directories.
    */
-  string getAParentDirName() { result = ".." or result = "../" }
+  private string getAParentDirName() { result = ".." or result = "../" }
 
   /** A check that a path string does not include '..' */
   class NoParentDirSanitizerGuard extends SanitizerGuard {