Merge pull request github#3467 from erik-krogh/tarSlip

Approved by esbena

Author: semmle-qlci

change-notes/1.25/analysis-javascript.md

@@ -26,6 +26,7 @@
 | Expression has no effect (`js/useless-expression`) | Less results | This query no longer flags an expression when that expression is the only content of the containing file. |
 | Unknown directive (`js/unknown-directive`) | Less results | This query no longer flags directives generated by the Babel compiler. |
 | Code injection (`js/code-injection`) | More results | More potential vulnerabilities involving NoSQL code operators are now recognized. |
+| Zip Slip (`js/zipslip`) | More results | This query now recognizes zip-slip vulnerabilities involving links. |
 
 ## Changes to libraries
 

javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll

@@ -479,7 +479,9 @@ module NodeJSLib {
       methodName = "write" or
       methodName = "writeFile" or
       methodName = "writeFileSync" or
-      methodName = "writeSync"
+      methodName = "writeSync" or
+      methodName = "link" or
+      methodName = "linkSync"
     }
 
     override DataFlow::Node getADataNode() {

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

@@ -47,11 +47,13 @@ module ZipSlip {
     )
   }
 
-  /** Gets a property that is used to get the filename part of an archive entry. */
+  /** Gets a property that is used to get a filename part of an archive entry. */
   private string getAFilenameProperty() {
     result = "path" // Used by library 'unzip'.
     or
     result = "name" // Used by library 'tar-stream'.
+    or
+    result = "linkname" // linked file name, used by 'tar-stream'.
   }
 
   /** An archive entry path access, as a source for unsafe archive extraction. */
@@ -119,6 +121,20 @@ module ZipSlip {
     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.

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

@@ -4,6 +4,11 @@ const extract = tar.extract();
 
 extract.on('entry', (header, stream, next) => {
   const out = fs.createWriteStream(header.name);
+
+  if (header.linkname) {
+    fs.linkSync(header.linkname, "foo");
+  }
+
   stream.pipe(out);
   stream.on('end', () => {
     next();

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

@@ -5,6 +5,9 @@ nodes
 | 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 |
 | ZipSlipBad2.js:5:9:5:46 | fileName |
 | ZipSlipBad2.js:5:20:5:46 | 'output ... ry.path |
 | ZipSlipBad2.js:5:37:5:46 | entry.path |
@@ -29,6 +32,7 @@ nodes
 edges
 | 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: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:20:5:46 | 'output ... ry.path | ZipSlipBad2.js:5:9:5:46 | fileName |
@@ -49,6 +53,7 @@ edges
 #select
 | AdmZipBad.js:6:24:6:41 | zipEntry.entryName | AdmZipBad.js:6:24:6:41 | zipEntry.entryName | AdmZipBad.js:6:24:6:41 | zipEntry.entryName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | AdmZipBad.js:6:24:6:41 | zipEntry.entryName | item path |
 | TarSlipBad.js:6:36:6:46 | header.name | TarSlipBad.js:6:36:6:46 | header.name | TarSlipBad.js:6:36:6:46 | header.name | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | TarSlipBad.js:6:36:6:46 | header.name | item path |
+| TarSlipBad.js:9:17:9:31 | header.linkname | TarSlipBad.js:9:17:9:31 | header.linkname | TarSlipBad.js:9:17:9:31 | header.linkname | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | TarSlipBad.js:9:17:9:31 | header.linkname | item path |
 | ZipSlipBad2.js:6:22:6:29 | fileName | ZipSlipBad2.js:5:37:5:46 | entry.path | ZipSlipBad2.js:6:22:6:29 | fileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad2.js:5:37:5:46 | entry.path | item path |
 | ZipSlipBad.js:8:37:8:44 | fileName | ZipSlipBad.js:7:22:7:31 | entry.path | ZipSlipBad.js:8:37:8:44 | fileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.js:7:22:7:31 | entry.path | item path |
 | ZipSlipBad.js:16:30:16:37 | fileName | ZipSlipBad.js:15:22:15:31 | entry.path | ZipSlipBad.js:16:30:16:37 | fileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.js:15:22:15:31 | entry.path | item path |

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

@@ -1,5 +1,6 @@
 const fs = require('fs');
 const unzip = require('unzip');
+const path = require('path');
 
 fs.createReadStream('archive.zip')
   .pipe(unzip.Parse())
@@ -11,4 +12,6 @@ fs.createReadStream('archive.zip')
     else {
       console.log('skipping bad path', fileName);
     }
+
+    fs.createWriteStream(path.join(cwd, path.join('/', fileName)));
   });

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

@@ -9,3 +9,10 @@ var fs = {};
  * @return {void}
  */
 fs.writeFileSync = function(filename, data) {};
+
+/**
+ * @param {(string|Buffer)} srcpath
+ * @param {(string|Buffer)} dstpath
+ * @return {void}
+ */
+fs.linkSync = function(srcpath, dstpath) {};