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 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..7a3619472166 --- /dev/null +++ b/javascript/ql/src/Security/CWE-022/ZipSlip.qhelp @@ -0,0 +1,67 @@ + + + + +

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.

+ +

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. +

+ + + + +

+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 could write to locations +outside the destination directory. +

+ + + +

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/ZipSlip.ql b/javascript/ql/src/Security/CWE-022/ZipSlip.ql new file mode 100644 index 000000000000..46b0269147cb --- /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 js/zipslip + * @problem.severity error + * @precision medium + * @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" 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..0b993cbfec35 --- /dev/null +++ b/javascript/ql/src/Security/CWE-022/ZipSlipBad.js @@ -0,0 +1,10 @@ +const fs = require('fs'); +const unzip = require('unzip'); + +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 new file mode 100644 index 000000000000..0d72a5537af0 --- /dev/null +++ b/javascript/ql/src/Security/CWE-022/ZipSlipGood.js @@ -0,0 +1,15 @@ +const fs = require('fs'); +const unzip = require('unzip'); + +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)); + } + else { + console.log('skipping bad path', fileName); + } + }); 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..2182fc038878 --- /dev/null +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll @@ -0,0 +1,111 @@ +/** + * 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 unsafe zip extraction. */ + 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 + } + } + + /** + * Gets a node that can be a parsed zip archive. + */ + 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 = result and + pipe.getMethodName() = "pipe" and + parsedArchive().flowsTo(pipe.getArgument(0)) + ) + } + + /** 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() { + exists(DataFlow::CallNode cn | + cn = parsedArchive().getAMemberCall("on") and + cn.getArgument(0).mayHaveStringValue("entry") and + this = cn.getCallback(1) + .getParameter(0) + .getAPropertyRead("path")) + } + } + + /** 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 file path of a file write, as a sink for unsafe zip extraction. */ + class FileSystemWriteSink extends Sink { + FileSystemWriteSink() { exists(FileSystemWriteAccess fsw | fsw.getAPathArgument() = this) } + } + + /** + * 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()) + } + } +} 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 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..5dae853958e9 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlip.expected @@ -0,0 +1,17 @@ +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: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: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/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..e4fdbe7d1f38 --- /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(fileName)); + }); 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/ZipSlipGood.js b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipGood.js new file mode 100644 index 000000000000..6a55fc81715d --- /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 (fileName.indexOf('..') == -1) { + entry.pipe(fs.createWriteStream(fileName)); + } + else { + console.log('skipping bad path', fileName); + } + }); 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) {};