From baa4f0825991a1b22a64af3b71fc3b8eb0013fa5 Mon Sep 17 00:00:00 2001 From: Jason Reed Date: Mon, 11 Feb 2019 10:02:36 -0500 Subject: [PATCH 01/12] JS: Add new query for ZipSlip (CWE-022) --- .../ql/src/Security/CWE-022/ZipSlip.qhelp | 38 +++++++++ javascript/ql/src/Security/CWE-022/ZipSlip.ql | 22 +++++ .../ql/src/Security/CWE-022/ZipSlipBad.js | 9 ++ .../javascript/security/dataflow/ZipSlip.qll | 84 +++++++++++++++++++ .../Security/CWE-022/ZipSlip/ZipSlip.expected | 5 ++ .../Security/CWE-022/ZipSlip/ZipSlip.qlref | 1 + .../Security/CWE-022/ZipSlip/ZipSlipBad.js | 9 ++ .../Security/CWE-022/ZipSlip/ZipSlipGood.js | 14 ++++ 8 files changed, 182 insertions(+) create mode 100644 javascript/ql/src/Security/CWE-022/ZipSlip.qhelp create mode 100644 javascript/ql/src/Security/CWE-022/ZipSlip.ql create mode 100644 javascript/ql/src/Security/CWE-022/ZipSlipBad.js create mode 100644 javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll create mode 100644 javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlip.expected create mode 100644 javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlip.qlref create mode 100644 javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBad.js create mode 100644 javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipGood.js diff --git a/javascript/ql/src/Security/CWE-022/ZipSlip.qhelp b/javascript/ql/src/Security/CWE-022/ZipSlip.qhelp new file mode 100644 index 000000000000..c595ceb7739a --- /dev/null +++ b/javascript/ql/src/Security/CWE-022/ZipSlip.qhelp @@ -0,0 +1,38 @@ + + + + +

Extracting files from a malicious zip archive without validating that the destination file path +is within the destination directory can cause files outside the destination directory to be +overwritten, due to the possible presence of directory traversal elements (..) in +archive paths.

+ +

Zip archives contain archive entries representing each file in the archive. These entries +include a file path for the entry, but these file paths are not restricted and may contain +unexpected special elements such as the directory traversal element (..). If these +file paths are used to determine an output file to write the contents of the archive item to, then +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.

+ + + + +

Ensure that output paths constructed from zip archive entries are validated +to prevent writing files to unexpected locations.

+ + + + +

+Here is an example of extracting an archive without validating +filenames. If archive.zip contained relative paths (for +instance, if it were created by something like zip archive.zip +../file.txt) then executing this code would write to those paths. +

+ + + + diff --git a/javascript/ql/src/Security/CWE-022/ZipSlip.ql b/javascript/ql/src/Security/CWE-022/ZipSlip.ql new file mode 100644 index 000000000000..6c6466b64ab9 --- /dev/null +++ b/javascript/ql/src/Security/CWE-022/ZipSlip.ql @@ -0,0 +1,22 @@ +/** + * @name Arbitrary file write during zip extraction ("Zip Slip") + * @description Extracting files from a malicious zip archive without validating that the + * destination file path is within the destination directory can cause files outside + * the destination directory to be overwritten. + * @kind path-problem + * @id cs/zipslip + * @problem.severity error + * @precision high + * @tags security + * external/cwe/cwe-022 + */ + +import javascript +import semmle.javascript.security.dataflow.ZipSlip::ZipSlip +import DataFlow::PathGraph + +from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink +where cfg.hasFlowPath(source, sink) +select sink.getNode(), source, sink, + "Unsanitized zip archive $@, which may contain '..', is used in a file system operation.", + source.getNode(), "item path" \ No newline at end of file diff --git a/javascript/ql/src/Security/CWE-022/ZipSlipBad.js b/javascript/ql/src/Security/CWE-022/ZipSlipBad.js new file mode 100644 index 000000000000..fc6ac2514485 --- /dev/null +++ b/javascript/ql/src/Security/CWE-022/ZipSlipBad.js @@ -0,0 +1,9 @@ +const fs = require('fs'); +const unzip = require('unzip'); + +fs.createReadStream('archive.zip') + .pipe(unzip.Parse()) + .on('entry', entry => { + const fileName = entry.path; + entry.pipe(fs.createWriteStream(entry.path)); + }); diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll new file mode 100644 index 000000000000..43c00711a8b9 --- /dev/null +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll @@ -0,0 +1,84 @@ +/** + * Provides a taint tracking configuration for reasoning about unsafe zip extraction. + */ + +import javascript + +module ZipSlip { + /** + * A data flow source for unsafe zip extraction. + */ + abstract class Source extends DataFlow::Node { } + + /** + * A data flow sink for unsafe zip extraction. + */ + abstract class Sink extends DataFlow::Node { } + + /** + * A sanitizer guard for unsafe zip extraction. + */ + abstract class SanitizerGuard extends + TaintTracking::SanitizerGuardNode, + DataFlow::ValueNode { } + + /** A taint tracking configuration for Zip Slip */ + class Configuration extends TaintTracking::Configuration { + Configuration() { this = "ZipSlip" } + + override predicate isSource(DataFlow::Node source) { source instanceof Source } + + override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } + + override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode nd) { + nd instanceof SanitizerGuard + } + } + + /** + * An access to the filepath of an entry of a zipfile being extracted by + * npm module `unzip`. + */ + class UnzipEntrySource extends Source { + UnzipEntrySource() { + exists(DataFlow::MethodCallNode pipe, DataFlow::MethodCallNode on | + pipe.getMethodName() = "pipe" + and pipe.getArgument(0) = DataFlow::moduleImport("unzip").getAMemberCall("Parse") + and on = pipe.getAMemberCall("on") + and this = 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. + */ + class CreateWriteStreamSink extends Sink { + CreateWriteStreamSink() { + this = DataFlow::moduleImport("fs").getAMemberCall("createWriteStream").getArgument(0) + } + } + + /** A sink that is a file path that gets written to. */ + class FileSystemWriteSink extends Sink { + FileSystemWriteSink() { + exists(FileSystemWriteAccess fsw | fsw.getAPathArgument() = this) + } + } + + /** 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("..") + } + } +} diff --git a/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlip.expected b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlip.expected new file mode 100644 index 000000000000..88b9ef8722e9 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlip.expected @@ -0,0 +1,5 @@ +nodes +| ZipSlipBad.js:8:37:8:46 | entry.path | +edges +#select +| ZipSlipBad.js:8:37:8:46 | entry.path | ZipSlipBad.js:8:37:8:46 | entry.path | ZipSlipBad.js:8:37:8:46 | entry.path | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.js:8:37:8:46 | entry.path | item path | diff --git a/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlip.qlref b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlip.qlref new file mode 100644 index 000000000000..0ac6382f48ab --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlip.qlref @@ -0,0 +1 @@ +Security/CWE-022/ZipSlip.ql diff --git a/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBad.js b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBad.js new file mode 100644 index 000000000000..fc6ac2514485 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBad.js @@ -0,0 +1,9 @@ +const fs = require('fs'); +const unzip = require('unzip'); + +fs.createReadStream('archive.zip') + .pipe(unzip.Parse()) + .on('entry', entry => { + const fileName = entry.path; + entry.pipe(fs.createWriteStream(entry.path)); + }); diff --git a/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipGood.js b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipGood.js new file mode 100644 index 000000000000..325e9586223f --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/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); + } + }); From abd2644af7b9b26f09b1af236decf65fdfb12276 Mon Sep 17 00:00:00 2001 From: Jason Reed Date: Wed, 13 Feb 2019 13:53:23 -0500 Subject: [PATCH 02/12] JS: Address review comments --- javascript/ql/src/Security/CWE-022/ZipSlip.ql | 2 +- .../javascript/security/dataflow/ZipSlip.qll | 27 ++++++++++++++++--- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/javascript/ql/src/Security/CWE-022/ZipSlip.ql b/javascript/ql/src/Security/CWE-022/ZipSlip.ql index 6c6466b64ab9..44842ab70199 100644 --- a/javascript/ql/src/Security/CWE-022/ZipSlip.ql +++ b/javascript/ql/src/Security/CWE-022/ZipSlip.ql @@ -4,7 +4,7 @@ * destination file path is within the destination directory can cause files outside * the destination directory to be overwritten. * @kind path-problem - * @id cs/zipslip + * @id js/zipslip * @problem.severity error * @precision high * @tags security diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll index 43c00711a8b9..a7d42649d7bc 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll @@ -36,14 +36,25 @@ module ZipSlip { } /** - * An access to the filepath of an entry of a zipfile being extracted by - * npm module `unzip`. + * 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`. */ class UnzipEntrySource extends Source { UnzipEntrySource() { exists(DataFlow::MethodCallNode pipe, DataFlow::MethodCallNode on | pipe.getMethodName() = "pipe" - and pipe.getArgument(0) = DataFlow::moduleImport("unzip").getAMemberCall("Parse") + and pipe.getArgument(0).getALocalSource() = DataFlow::moduleImport("unzip").getAMemberCall("Parse") and on = pipe.getAMemberCall("on") and this = on.getCallback(1).getParameter(0).getAPropertyRead("path")) } @@ -69,6 +80,14 @@ module ZipSlip { } } + /** + * Gets a string which suffices to search for to ensure that a + * filepath will not refer to parent directories. + */ + string getAParentDirName() { + result = any(string s | s = ".." or s = "../") + } + /** A check that a path string does not include '..' */ class NoParentDirSanitizerGuard extends SanitizerGuard { StringOps::Includes incl; @@ -78,7 +97,7 @@ module ZipSlip { override predicate sanitizes(boolean outcome, Expr e) { incl.getPolarity().booleanNot() = outcome and incl.getBaseString().asExpr() = e - and incl.getSubstring().mayHaveStringValue("..") + and incl.getSubstring().mayHaveStringValue(getAParentDirName()) } } } From 32d48ba98b13899335042664683acb79a1c5000d Mon Sep 17 00:00:00 2001 From: Jason Reed Date: Wed, 13 Feb 2019 14:01:26 -0500 Subject: [PATCH 03/12] JS: Run auto-formatter --- javascript/ql/src/Security/CWE-022/ZipSlip.ql | 2 +- .../javascript/security/dataflow/ZipSlip.qll | 30 ++++++++----------- 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/javascript/ql/src/Security/CWE-022/ZipSlip.ql b/javascript/ql/src/Security/CWE-022/ZipSlip.ql index 44842ab70199..e9c92163d055 100644 --- a/javascript/ql/src/Security/CWE-022/ZipSlip.ql +++ b/javascript/ql/src/Security/CWE-022/ZipSlip.ql @@ -19,4 +19,4 @@ from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink where cfg.hasFlowPath(source, sink) select sink.getNode(), source, sink, "Unsanitized zip archive $@, which may contain '..', is used in a file system operation.", - source.getNode(), "item path" \ No newline at end of file + source.getNode(), "item path" diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll index a7d42649d7bc..1ab50d459bb8 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll @@ -18,9 +18,7 @@ module ZipSlip { /** * A sanitizer guard for unsafe zip extraction. */ - abstract class SanitizerGuard extends - TaintTracking::SanitizerGuardNode, - DataFlow::ValueNode { } + abstract class SanitizerGuard extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode { } /** A taint tracking configuration for Zip Slip */ class Configuration extends TaintTracking::Configuration { @@ -53,10 +51,12 @@ module ZipSlip { class UnzipEntrySource extends Source { UnzipEntrySource() { exists(DataFlow::MethodCallNode pipe, DataFlow::MethodCallNode on | - pipe.getMethodName() = "pipe" - and pipe.getArgument(0).getALocalSource() = DataFlow::moduleImport("unzip").getAMemberCall("Parse") - and on = pipe.getAMemberCall("on") - and this = on.getCallback(1).getParameter(0).getAPropertyRead("path")) + pipe.getMethodName() = "pipe" and + pipe.getArgument(0).getALocalSource() = DataFlow::moduleImport("unzip") + .getAMemberCall("Parse") and + on = pipe.getAMemberCall("on") and + this = on.getCallback(1).getParameter(0).getAPropertyRead("path") + ) } } @@ -75,29 +75,25 @@ module ZipSlip { /** A sink that is a file path that gets written to. */ class FileSystemWriteSink extends Sink { - FileSystemWriteSink() { - exists(FileSystemWriteAccess fsw | fsw.getAPathArgument() = this) - } + FileSystemWriteSink() { exists(FileSystemWriteAccess fsw | fsw.getAPathArgument() = this) } } /** * Gets a string which suffices to search for to ensure that a * filepath will not refer to parent directories. */ - string getAParentDirName() { - result = any(string s | s = ".." or s = "../") - } + string getAParentDirName() { result = any(string s | s = ".." or s = "../") } /** A check that a path string does not include '..' */ class NoParentDirSanitizerGuard extends SanitizerGuard { StringOps::Includes incl; - NoParentDirSanitizerGuard() { this = 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()) + incl.getPolarity().booleanNot() = outcome and + incl.getBaseString().asExpr() = e and + incl.getSubstring().mayHaveStringValue(getAParentDirName()) } } } From 23d37c716713e0ec93acc6f80892a1824ef49d2e Mon Sep 17 00:00:00 2001 From: Jason Reed Date: Thu, 14 Feb 2019 13:25:09 -0500 Subject: [PATCH 04/12] JS: Unbreak TaintedPath --- .../Security/CWE-022/{ => TaintedPath}/TaintedPath-es6.js | 0 .../Security/CWE-022/{ => TaintedPath}/TaintedPath.expected | 0 .../query-tests/Security/CWE-022/{ => TaintedPath}/TaintedPath.js | 0 .../Security/CWE-022/{ => TaintedPath}/TaintedPath.qlref | 0 .../ql/test/query-tests/Security/CWE-022/{ => TaintedPath}/fs.js | 0 .../Security/CWE-022/{ => TaintedPath}/tainted-array-steps.js | 0 .../Security/CWE-022/{ => TaintedPath}/tainted-require.js | 0 .../Security/CWE-022/{ => TaintedPath}/tainted-sendFile.js | 0 .../test/query-tests/Security/CWE-022/{ => TaintedPath}/views.js | 0 9 files changed, 0 insertions(+), 0 deletions(-) rename javascript/ql/test/query-tests/Security/CWE-022/{ => TaintedPath}/TaintedPath-es6.js (100%) rename javascript/ql/test/query-tests/Security/CWE-022/{ => TaintedPath}/TaintedPath.expected (100%) rename javascript/ql/test/query-tests/Security/CWE-022/{ => TaintedPath}/TaintedPath.js (100%) rename javascript/ql/test/query-tests/Security/CWE-022/{ => TaintedPath}/TaintedPath.qlref (100%) rename javascript/ql/test/query-tests/Security/CWE-022/{ => TaintedPath}/fs.js (100%) rename javascript/ql/test/query-tests/Security/CWE-022/{ => TaintedPath}/tainted-array-steps.js (100%) rename javascript/ql/test/query-tests/Security/CWE-022/{ => TaintedPath}/tainted-require.js (100%) rename javascript/ql/test/query-tests/Security/CWE-022/{ => TaintedPath}/tainted-sendFile.js (100%) rename javascript/ql/test/query-tests/Security/CWE-022/{ => TaintedPath}/views.js (100%) diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath-es6.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath-es6.js similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-022/TaintedPath-es6.js rename to javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath-es6.js diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath.expected b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-022/TaintedPath.expected rename to javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.js similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-022/TaintedPath.js rename to javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.js diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath.qlref b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.qlref similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-022/TaintedPath.qlref rename to javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.qlref diff --git a/javascript/ql/test/query-tests/Security/CWE-022/fs.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/fs.js similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-022/fs.js rename to javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/fs.js diff --git a/javascript/ql/test/query-tests/Security/CWE-022/tainted-array-steps.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/tainted-array-steps.js similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-022/tainted-array-steps.js rename to javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/tainted-array-steps.js diff --git a/javascript/ql/test/query-tests/Security/CWE-022/tainted-require.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/tainted-require.js similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-022/tainted-require.js rename to javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/tainted-require.js diff --git a/javascript/ql/test/query-tests/Security/CWE-022/tainted-sendFile.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/tainted-sendFile.js similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-022/tainted-sendFile.js rename to javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/tainted-sendFile.js diff --git a/javascript/ql/test/query-tests/Security/CWE-022/views.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/views.js similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-022/views.js rename to javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/views.js From b0636dd41067f330cf93991f872072bc78f8909e Mon Sep 17 00:00:00 2001 From: Jason Reed Date: Fri, 22 Feb 2019 09:56:44 -0500 Subject: [PATCH 05/12] JS: Better local flow through `.pipe` chaining --- .../javascript/security/dataflow/ZipSlip.qll | 37 ++++++++++++++++--- .../Security/CWE-022/ZipSlip/ZipSlip.expected | 8 ++++ .../Security/CWE-022/ZipSlip/ZipSlipBad2.js | 8 ++++ .../Security/CWE-022/ZipSlip/externs.js | 11 ++++++ 4 files changed, 58 insertions(+), 6 deletions(-) create mode 100644 javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBad2.js create mode 100644 javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/externs.js diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll index 1ab50d459bb8..bcf388ffaa02 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll @@ -33,6 +33,33 @@ module ZipSlip { } } + /** + * Holds if `node1` flows to `node2` in one step by virtue of + * `node2` being of the form `.pipe(node1)`. The reason this flow + * exists is that `.pipe` returns its argument to make chained + * stream operations work. + */ + predicate pipeStep(DataFlow::Node node1, DataFlow::MethodCallNode node2) { + node2.getMethodName() = "pipe" and + node1 = node2.getArgument(0) + } + + /** + * Holds if `node1` flows to `node2` in one step including the assumption that + * `x` flows to `.pipe(x)` + */ + predicate stepsThroughPipe(DataFlow::Node node1, DataFlow::Node node2) { + DataFlow::localFlowStep(node1, node2) or pipeStep(node1, node2) + } + + /** + * Holds if `node1` flows to `node2` including the assumption that + * `x` flows to `.pipe(x)` + */ + predicate flowsThroughPipe(DataFlow::Node node1, DataFlow::Node node2) { + stepsThroughPipe*(node1, node2) + } + /** * An access to the filepath of an entry of a zipfile being extracted * by npm module `unzip`. For example, in @@ -50,12 +77,10 @@ module ZipSlip { */ class UnzipEntrySource extends Source { UnzipEntrySource() { - exists(DataFlow::MethodCallNode pipe, DataFlow::MethodCallNode on | - pipe.getMethodName() = "pipe" and - pipe.getArgument(0).getALocalSource() = DataFlow::moduleImport("unzip") - .getAMemberCall("Parse") and - on = pipe.getAMemberCall("on") and - this = on.getCallback(1).getParameter(0).getAPropertyRead("path") + exists(DataFlow::SourceNode parsed | + flowsThroughPipe(DataFlow::moduleImport("unzip").getAMemberCall("Parse"), parsed) + and + this = parsed.getAMemberCall("on").getCallback(1).getParameter(0).getAPropertyRead("path") ) } } diff --git a/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlip.expected b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlip.expected index 88b9ef8722e9..a9e316d7d469 100644 --- a/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlip.expected +++ b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlip.expected @@ -1,5 +1,13 @@ nodes +| ZipSlipBad2.js:5:9:5:46 | fileName | +| ZipSlipBad2.js:5:20:5:46 | 'output ... ry.path | +| ZipSlipBad2.js:5:37:5:46 | entry.path | +| ZipSlipBad2.js:6:22:6:29 | fileName | | ZipSlipBad.js:8:37:8:46 | entry.path | edges +| 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:37:5:46 | entry.path | ZipSlipBad2.js:5:20:5:46 | 'output ... ry.path | #select +| 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:46 | entry.path | ZipSlipBad.js:8:37:8:46 | entry.path | ZipSlipBad.js:8:37:8:46 | entry.path | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.js:8:37:8:46 | entry.path | item path | diff --git a/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBad2.js b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBad2.js new file mode 100644 index 000000000000..d582c680ef8e --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBad2.js @@ -0,0 +1,8 @@ +var fs = require('fs'); +var unzip = require('unzip'); +fs.readFile('path/to/archive.zip', function (err, zipContents) { + unzip.Parse(zipContents).on('entry', function (entry) { + var fileName = 'output/path/' + entry.path; + fs.writeFileSync(fileName, entry.contents); + }); +}); diff --git a/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/externs.js b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/externs.js new file mode 100644 index 000000000000..1a27aa787d74 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/externs.js @@ -0,0 +1,11 @@ +/** + * @externs + */ +var fs = {}; + +/** + * @param {string} filename + * @param {*} data + * @return {void} + */ +fs.writeFileSync = function(filename, data) {}; From 09b9a577837ea99c91a974eedc5ade77cd15a1f5 Mon Sep 17 00:00:00 2001 From: Jason Reed Date: Mon, 25 Feb 2019 08:59:30 -0500 Subject: [PATCH 06/12] JS: More efficient reasoning through pipe --- .../javascript/security/dataflow/ZipSlip.qll | 40 +++++-------------- 1 file changed, 11 insertions(+), 29 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll index bcf388ffaa02..f21c02e04c5e 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll @@ -34,30 +34,16 @@ module ZipSlip { } /** - * Holds if `node1` flows to `node2` in one step by virtue of - * `node2` being of the form `.pipe(node1)`. The reason this flow - * exists is that `.pipe` returns its argument to make chained - * stream operations work. + * Gets a node that can be a parsed zip archive. */ - predicate pipeStep(DataFlow::Node node1, DataFlow::MethodCallNode node2) { - node2.getMethodName() = "pipe" and - node1 = node2.getArgument(0) - } - - /** - * Holds if `node1` flows to `node2` in one step including the assumption that - * `x` flows to `.pipe(x)` - */ - predicate stepsThroughPipe(DataFlow::Node node1, DataFlow::Node node2) { - DataFlow::localFlowStep(node1, node2) or pipeStep(node1, node2) - } - - /** - * Holds if `node1` flows to `node2` including the assumption that - * `x` flows to `.pipe(x)` - */ - predicate flowsThroughPipe(DataFlow::Node node1, DataFlow::Node node2) { - stepsThroughPipe*(node1, node2) + 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))) } /** @@ -77,11 +63,7 @@ module ZipSlip { */ class UnzipEntrySource extends Source { UnzipEntrySource() { - exists(DataFlow::SourceNode parsed | - flowsThroughPipe(DataFlow::moduleImport("unzip").getAMemberCall("Parse"), parsed) - and - this = parsed.getAMemberCall("on").getCallback(1).getParameter(0).getAPropertyRead("path") - ) + this = parsedArchive().getAMemberCall("on").getCallback(1).getParameter(0).getAPropertyRead("path") } } @@ -107,7 +89,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 = any(string s | s = ".." or s = "../") } + string getAParentDirName() { result = ".." or result = "../" } /** A check that a path string does not include '..' */ class NoParentDirSanitizerGuard extends SanitizerGuard { From 2fc2a393b7052c60fab9bf4650f80b3ad2b6f09b Mon Sep 17 00:00:00 2001 From: Jason Reed Date: Tue, 26 Feb 2019 08:08:52 -0500 Subject: [PATCH 07/12] JS: Address review comments --- .../ql/src/Security/CWE-022/ZipSlip.qhelp | 18 +++++- javascript/ql/src/Security/CWE-022/ZipSlip.ql | 2 +- .../ql/src/Security/CWE-022/ZipSlipGood.js | 14 +++++ .../javascript/security/dataflow/ZipSlip.qll | 61 ++++++++++--------- 4 files changed, 64 insertions(+), 31 deletions(-) create mode 100644 javascript/ql/src/Security/CWE-022/ZipSlipGood.js diff --git a/javascript/ql/src/Security/CWE-022/ZipSlip.qhelp b/javascript/ql/src/Security/CWE-022/ZipSlip.qhelp index c595ceb7739a..07c4fb0c5790 100644 --- a/javascript/ql/src/Security/CWE-022/ZipSlip.qhelp +++ b/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.

- +

For example, if a zip file contains a file entry ..\sneaky-file, and the zip file +is extracted to the directory c:\output, then naively combining the paths would result +in an output file path of c:\output\..\sneaky-file, which would cause the file to be +written to c:\sneaky-file.

+ +

Ensure that output paths constructed from zip archive entries are validated to prevent writing files to unexpected locations.

+

The recommended way of writing an output file from a zip archive entry is to check that +".." does not occur in the path. +

+ @@ -34,5 +43,12 @@ instance, if it were created by something like zip archive.zip

+ +

To fix this vulnerability, we can to check that the path does not +contain any ".." in it. +

+ + + diff --git a/javascript/ql/src/Security/CWE-022/ZipSlip.ql b/javascript/ql/src/Security/CWE-022/ZipSlip.ql index e9c92163d055..46b0269147cb 100644 --- a/javascript/ql/src/Security/CWE-022/ZipSlip.ql +++ b/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 */ diff --git a/javascript/ql/src/Security/CWE-022/ZipSlipGood.js b/javascript/ql/src/Security/CWE-022/ZipSlipGood.js new file mode 100644 index 000000000000..325e9586223f --- /dev/null +++ b/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); + } + }); diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll index f21c02e04c5e..0d000b132609 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll +++ b/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 { From caebdd2f688b270f0104487d7baf7dd5d02e0f93 Mon Sep 17 00:00:00 2001 From: Jason Reed Date: Wed, 27 Feb 2019 09:16:45 -0500 Subject: [PATCH 08/12] JS: Fix incorrect sample link --- javascript/ql/src/Security/CWE-022/ZipSlip.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/Security/CWE-022/ZipSlip.qhelp b/javascript/ql/src/Security/CWE-022/ZipSlip.qhelp index 07c4fb0c5790..536347701133 100644 --- a/javascript/ql/src/Security/CWE-022/ZipSlip.qhelp +++ b/javascript/ql/src/Security/CWE-022/ZipSlip.qhelp @@ -48,7 +48,7 @@ instance, if it were created by something like zip archive.zip contain any ".." in it.

- + From 674d2790b4c0077b7c30f508a23919c9bc1c715c Mon Sep 17 00:00:00 2001 From: Jason Reed Date: Wed, 27 Feb 2019 10:22:06 -0500 Subject: [PATCH 09/12] JS: Address review comments --- javascript/ql/src/Security/CWE-022/ZipSlip.qhelp | 2 +- .../semmle/javascript/security/dataflow/ZipSlip.qll | 12 +++++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/javascript/ql/src/Security/CWE-022/ZipSlip.qhelp b/javascript/ql/src/Security/CWE-022/ZipSlip.qhelp index 536347701133..d708965a8818 100644 --- a/javascript/ql/src/Security/CWE-022/ZipSlip.qhelp +++ b/javascript/ql/src/Security/CWE-022/ZipSlip.qhelp @@ -45,7 +45,7 @@ instance, if it were created by something like zip archive.zip

To fix this vulnerability, we can to check that the path does not -contain any ".." in it. +contain any ".." elements in it.

diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll index 0d000b132609..a752b25d59c0 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll @@ -43,6 +43,7 @@ module ZipSlip { // universal) pattern when using nodejs streams, whose return // value is the parsed stream. exists(DataFlow::MethodCallNode pipe | + pipe = result and pipe.getMethodName() = "pipe" and parsedArchive().flowsTo(pipe.getArgument(0)) ) @@ -63,11 +64,12 @@ module ZipSlip { // there is an `UnzipEntrySource` node corresponding to // the expression `entry.path`. UnzipEntrySource() { - this = parsedArchive() - .getAMemberCall("on") - .getCallback(1) - .getParameter(0) - .getAPropertyRead("path") + exists(DataFlow::CallNode cn | + cn = parsedArchive().getAMemberCall("on") and + cn.getArgument(0).mayHaveStringValue("entry") and + this = cn.getCallback(1) + .getParameter(0) + .getAPropertyRead("path")) } } From c5e57dacf88ff31175ad334cf77742a035c0801d Mon Sep 17 00:00:00 2001 From: Jason Reed Date: Wed, 27 Feb 2019 10:23:32 -0500 Subject: [PATCH 10/12] JS: Actually use fileName in examples --- javascript/ql/src/Security/CWE-022/ZipSlipBad.js | 2 +- javascript/ql/src/Security/CWE-022/ZipSlipGood.js | 6 +++--- .../query-tests/Security/CWE-022/ZipSlip/ZipSlip.expected | 8 ++++++-- .../query-tests/Security/CWE-022/ZipSlip/ZipSlipBad.js | 2 +- .../query-tests/Security/CWE-022/ZipSlip/ZipSlipGood.js | 6 +++--- 5 files changed, 14 insertions(+), 10 deletions(-) diff --git a/javascript/ql/src/Security/CWE-022/ZipSlipBad.js b/javascript/ql/src/Security/CWE-022/ZipSlipBad.js index fc6ac2514485..e4fdbe7d1f38 100644 --- a/javascript/ql/src/Security/CWE-022/ZipSlipBad.js +++ b/javascript/ql/src/Security/CWE-022/ZipSlipBad.js @@ -5,5 +5,5 @@ fs.createReadStream('archive.zip') .pipe(unzip.Parse()) .on('entry', entry => { const fileName = entry.path; - entry.pipe(fs.createWriteStream(entry.path)); + entry.pipe(fs.createWriteStream(fileName)); }); diff --git a/javascript/ql/src/Security/CWE-022/ZipSlipGood.js b/javascript/ql/src/Security/CWE-022/ZipSlipGood.js index 325e9586223f..6a55fc81715d 100644 --- a/javascript/ql/src/Security/CWE-022/ZipSlipGood.js +++ b/javascript/ql/src/Security/CWE-022/ZipSlipGood.js @@ -5,10 +5,10 @@ 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)); + if (fileName.indexOf('..') == -1) { + entry.pipe(fs.createWriteStream(fileName)); } else { - console.log('skipping bad path', entry.path); + console.log('skipping bad path', fileName); } }); diff --git a/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlip.expected b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlip.expected index a9e316d7d469..5dae853958e9 100644 --- a/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlip.expected +++ b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlip.expected @@ -3,11 +3,15 @@ nodes | ZipSlipBad2.js:5:20:5:46 | 'output ... ry.path | | ZipSlipBad2.js:5:37:5:46 | entry.path | | ZipSlipBad2.js:6:22:6:29 | fileName | -| ZipSlipBad.js:8:37:8:46 | entry.path | +| ZipSlipBad.js:7:11:7:31 | fileName | +| ZipSlipBad.js:7:22:7:31 | entry.path | +| ZipSlipBad.js:8:37:8:44 | fileName | edges | 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: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:22:7:31 | entry.path | ZipSlipBad.js:7:11:7:31 | fileName | #select | 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:46 | entry.path | ZipSlipBad.js:8:37:8:46 | entry.path | ZipSlipBad.js:8:37:8:46 | entry.path | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.js:8:37:8: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 | diff --git a/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBad.js b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBad.js index fc6ac2514485..e4fdbe7d1f38 100644 --- a/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBad.js +++ b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBad.js @@ -5,5 +5,5 @@ fs.createReadStream('archive.zip') .pipe(unzip.Parse()) .on('entry', entry => { const fileName = entry.path; - entry.pipe(fs.createWriteStream(entry.path)); + entry.pipe(fs.createWriteStream(fileName)); }); diff --git a/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipGood.js b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipGood.js index 325e9586223f..6a55fc81715d 100644 --- a/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipGood.js +++ b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipGood.js @@ -5,10 +5,10 @@ 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)); + if (fileName.indexOf('..') == -1) { + entry.pipe(fs.createWriteStream(fileName)); } else { - console.log('skipping bad path', entry.path); + console.log('skipping bad path', fileName); } }); From c1b218a5ffe4721e64f283a6d96b09ed45393d20 Mon Sep 17 00:00:00 2001 From: Jason Reed Date: Thu, 28 Feb 2019 06:25:25 -0500 Subject: [PATCH 11/12] JS: Documentation fixes --- .../ql/src/Security/CWE-022/ZipSlip.qhelp | 21 +++++++++++++++---- .../ql/src/Security/CWE-022/ZipSlipBad.js | 1 + .../ql/src/Security/CWE-022/ZipSlipGood.js | 1 + .../javascript/security/dataflow/ZipSlip.qll | 6 +++--- 4 files changed, 22 insertions(+), 7 deletions(-) diff --git a/javascript/ql/src/Security/CWE-022/ZipSlip.qhelp b/javascript/ql/src/Security/CWE-022/ZipSlip.qhelp index d708965a8818..7a3619472166 100644 --- a/javascript/ql/src/Security/CWE-022/ZipSlip.qhelp +++ b/javascript/ql/src/Security/CWE-022/ZipSlip.qhelp @@ -36,19 +36,32 @@ to prevent writing files to unexpected locations.

-Here is an example of extracting an archive without validating -filenames. If archive.zip contained relative paths (for +In this example an archive is extracted without validating file paths. +If archive.zip contained relative paths (for instance, if it were created by something like zip archive.zip -../file.txt) then executing this code would write to those paths. +../file.txt) then executing this code could write to locations +outside the destination directory.

-

To fix this vulnerability, we can to check that the path does not +

To fix this vulnerability, we need to check that the path does not contain any ".." elements in it.

+ + +
  • +Snyk: +Zip Slip Vulnerability. +
    • +
  • +OWASP: +Path Traversal. +
    • + +     diff --git a/javascript/ql/src/Security/CWE-022/ZipSlipBad.js b/javascript/ql/src/Security/CWE-022/ZipSlipBad.js index e4fdbe7d1f38..0b993cbfec35 100644 --- a/javascript/ql/src/Security/CWE-022/ZipSlipBad.js +++ b/javascript/ql/src/Security/CWE-022/ZipSlipBad.js @@ -5,5 +5,6 @@ fs.createReadStream('archive.zip') .pipe(unzip.Parse()) .on('entry', entry => { const fileName = entry.path; + // BAD: This could write any file on the filesystem. entry.pipe(fs.createWriteStream(fileName)); }); diff --git a/javascript/ql/src/Security/CWE-022/ZipSlipGood.js b/javascript/ql/src/Security/CWE-022/ZipSlipGood.js index 6a55fc81715d..0d72a5537af0 100644 --- a/javascript/ql/src/Security/CWE-022/ZipSlipGood.js +++ b/javascript/ql/src/Security/CWE-022/ZipSlipGood.js @@ -5,6 +5,7 @@ fs.createReadStream('archive.zip') .pipe(unzip.Parse()) .on('entry', entry => { const fileName = entry.path; + // GOOD: ensures the path is safe to write to. if (fileName.indexOf('..') == -1) { entry.pipe(fs.createWriteStream(fileName)); } diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll index a752b25d59c0..2182fc038878 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll @@ -78,7 +78,7 @@ module ZipSlip { 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 + // 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) @@ -91,8 +91,8 @@ module ZipSlip { } /** - * Gets a string which suffices to search for to ensure that a - * filepath will not refer to parent directories. + * Gets a string which is sufficient to exclude to make + * a filepath definitely not refer to parent directories. */ private string getAParentDirName() { result = ".." or result = "../" } From 86bbb5fb18f3b59d2af19848c0c924ee67f2d0cc Mon Sep 17 00:00:00 2001 From: Jason Reed Date: Thu, 28 Feb 2019 09:34:21 -0500 Subject: [PATCH 12/12] JS: Add ZipSlip query to security suite --- javascript/config/suites/javascript/security | 1 + 1 file changed, 1 insertion(+) diff --git a/javascript/config/suites/javascript/security b/javascript/config/suites/javascript/security index f572def7e8eb..c1934ae1d5f6 100644 --- a/javascript/config/suites/javascript/security +++ b/javascript/config/suites/javascript/security @@ -4,6 +4,7 @@ + semmlecode-javascript-queries/Security/CWE-020/IncompleteUrlSubstringSanitization.ql: /Security/CWE/CWE-020 + semmlecode-javascript-queries/Security/CWE-020/IncorrectSuffixCheck.ql: /Security/CWE/CWE-020 + semmlecode-javascript-queries/Security/CWE-022/TaintedPath.ql: /Security/CWE/CWE-022 ++ semmlecode-javascript-queries/Security/CWE-022/ZipSlip.ql: /Security/CWE/CWE-022 + semmlecode-javascript-queries/Security/CWE-078/CommandInjection.ql: /Security/CWE/CWE-078 + semmlecode-javascript-queries/Security/CWE-079/ReflectedXss.ql: /Security/CWE/CWE-079 + semmlecode-javascript-queries/Security/CWE-079/StoredXss.ql: /Security/CWE/CWE-079