From 6589813ec7264734a88135d639439af58e253f6b Mon Sep 17 00:00:00 2001 From: Jason Reed Date: Thu, 14 Mar 2019 13:36:53 -0400 Subject: [PATCH 1/5] JavaScript: Add tar-stream extraction to ZipSlip query. --- .../javascript/security/dataflow/ZipSlip.qll | 14 ++++++++------ .../Security/CWE-022/ZipSlip/TarSlipBad.js | 18 ++++++++++++++++++ .../Security/CWE-022/ZipSlip/ZipSlip.expected | 2 ++ 3 files changed, 28 insertions(+), 6 deletions(-) create mode 100644 javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/TarSlipBad.js diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll index 39ae5ef40fd0..90293cde0aa6 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll @@ -46,6 +46,8 @@ module ZipSlip { private DataFlow::SourceNode parsedArchive() { result = DataFlow::moduleImport("unzip").getAMemberCall("Parse") or + result = DataFlow::moduleImport("tar-stream").getAMemberCall("extract") + or // `streamProducer.pipe(unzip.Parse())` is a typical (but not // universal) pattern when using nodejs streams, whose return // value is the parsed stream. @@ -56,6 +58,9 @@ module ZipSlip { ) } + /** Gets a property that is used to get the filename part of an archive entry. */ + private string getAFilenameProperty() { result = "path" or result = "name" } + /** A zip archive entry path access, as a source for unsafe zip extraction. */ class UnzipEntrySource extends Source { // For example, in @@ -74,9 +79,8 @@ module ZipSlip { exists(DataFlow::CallNode cn | cn = parsedArchive().getAMemberCall("on") and cn.getArgument(0).mayHaveStringValue("entry") and - this = cn.getCallback(1) - .getParameter(0) - .getAPropertyRead("path")) + this = cn.getCallback(1).getParameter(0).getAPropertyRead(getAFilenameProperty()) + ) } } @@ -99,9 +103,7 @@ module ZipSlip { /** An expression that sanitizes by calling path.basename */ class BasenameSanitizer extends Sanitizer { - BasenameSanitizer() { - this = DataFlow::moduleImport("path").getAMemberCall("basename") - } + BasenameSanitizer() { this = DataFlow::moduleImport("path").getAMemberCall("basename") } } /** diff --git a/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/TarSlipBad.js b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/TarSlipBad.js new file mode 100644 index 000000000000..a96b89a5c17e --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/TarSlipBad.js @@ -0,0 +1,18 @@ +const fs = require('fs'); +const tar = require('tar-stream'); +const extract = tar.extract(); + +extract.on('entry', (header, stream, next) => { + const out = fs.createWriteStream(header.name); + stream.pipe(out); + stream.on('end', () => { + next(); + }) + stream.resume(); +}) + +extract.on('finish', () => { + console.log('finished'); +}); + +fs.createReadStream('./bad.tar').pipe(extract); 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 5dae853958e9..34df80c61af6 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,4 +1,5 @@ nodes +| TarSlipBad.js:6:36:6:46 | header.name | | ZipSlipBad2.js:5:9:5:46 | fileName | | ZipSlipBad2.js:5:20:5:46 | 'output ... ry.path | | ZipSlipBad2.js:5:37:5:46 | entry.path | @@ -13,5 +14,6 @@ edges | 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 +| 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 | | 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 | From a674dbb5cd1f227991006c7766f75e4b89f821aa Mon Sep 17 00:00:00 2001 From: Jason Reed Date: Fri, 15 Mar 2019 08:16:50 -0400 Subject: [PATCH 2/5] JavaScript: Update docstrings to reflect generalization. --- .../javascript/security/dataflow/ZipSlip.qll | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll index 90293cde0aa6..c77a6630077b 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll @@ -1,31 +1,32 @@ /** - * Provides a taint tracking configuration for reasoning about unsafe zip extraction. + * Provides a taint tracking configuration for reasoning about unsafe + * zip and tar archive extraction. */ import javascript module ZipSlip { /** - * A data flow source for unsafe zip extraction. + * A data flow source for unsafe archive extraction. */ abstract class Source extends DataFlow::Node { } /** - * A data flow sink for unsafe zip extraction. + * A data flow sink for unsafe archive extraction. */ abstract class Sink extends DataFlow::Node { } /** - * A sanitizer for unsafe zip extraction. + * A sanitizer for unsafe archive extraction. */ abstract class Sanitizer extends DataFlow::Node { } /** - * A sanitizer guard for unsafe zip extraction. + * A sanitizer guard for unsafe archive extraction. */ abstract class SanitizerGuard extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode { } - /** A taint tracking configuration for unsafe zip extraction. */ + /** A taint tracking configuration for unsafe archive extraction. */ class Configuration extends TaintTracking::Configuration { Configuration() { this = "ZipSlip" } @@ -41,7 +42,7 @@ module ZipSlip { } /** - * Gets a node that can be a parsed zip archive. + * Gets a node that can be a parsed archive. */ private DataFlow::SourceNode parsedArchive() { result = DataFlow::moduleImport("unzip").getAMemberCall("Parse") @@ -61,7 +62,7 @@ module ZipSlip { /** Gets a property that is used to get the filename part of an archive entry. */ private string getAFilenameProperty() { result = "path" or result = "name" } - /** A zip archive entry path access, as a source for unsafe zip extraction. */ + /** An archive entry path access, as a source for unsafe archive extraction. */ class UnzipEntrySource extends Source { // For example, in // ```javascript @@ -84,7 +85,7 @@ module ZipSlip { } } - /** A call to `fs.createWriteStream`, as a sink for unsafe zip extraction. */ + /** A call to `fs.createWriteStream`, as a sink for unsafe archive extraction. */ class CreateWriteStreamSink extends Sink { CreateWriteStreamSink() { // This is not covered by `FileSystemWriteSink`, because it is @@ -96,7 +97,7 @@ module ZipSlip { } } - /** A file path of a file write, as a sink for unsafe zip extraction. */ + /** A file path of a file write, as a sink for unsafe archive extraction. */ class FileSystemWriteSink extends Sink { FileSystemWriteSink() { exists(FileSystemWriteAccess fsw | fsw.getAPathArgument() = this) } } From 8124980f58594215567acfd44598398bad122170 Mon Sep 17 00:00:00 2001 From: Jason Reed Date: Fri, 15 Mar 2019 08:47:31 -0400 Subject: [PATCH 3/5] JavaScript: Add change note and comment. --- change-notes/1.21/analysis-javascript.md | 1 + .../ql/src/semmle/javascript/security/dataflow/ZipSlip.qll | 6 +++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/change-notes/1.21/analysis-javascript.md b/change-notes/1.21/analysis-javascript.md index d42e2d080b87..c4a8d55ee441 100644 --- a/change-notes/1.21/analysis-javascript.md +++ b/change-notes/1.21/analysis-javascript.md @@ -19,5 +19,6 @@ |--------------------------------|------------------------------|---------------------------------------------------------------------------| | Expression has no effect | Fewer false-positive results | This rule now treats uses of `Object.defineProperty` more conservatively. | | Useless assignment to property | Fewer false-positive results | This rule now ignore reads of additional getters. | +| ZipSlip | More results | This rule now considers more libraries, including tar as well as zip. | ## Changes to QL libraries diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll index c77a6630077b..99d39776558e 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll @@ -60,7 +60,11 @@ module ZipSlip { } /** Gets a property that is used to get the filename part of an archive entry. */ - private string getAFilenameProperty() { result = "path" or result = "name" } + private string getAFilenameProperty() { + result = "path" // Used by library 'unzip'. + or + result = "name" // Used by library 'tar-stream'. + } /** An archive entry path access, as a source for unsafe archive extraction. */ class UnzipEntrySource extends Source { From aa9ba9557cbfa9f1daeff81403a27fdbe57c875a Mon Sep 17 00:00:00 2001 From: Jason Reed Date: Fri, 15 Mar 2019 09:26:08 -0400 Subject: [PATCH 4/5] JavaScript: Include 'unzipper' library in ZipSlip. --- .../ql/src/semmle/javascript/security/dataflow/ZipSlip.qll | 2 ++ 1 file changed, 2 insertions(+) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll index 99d39776558e..57af27cdf55f 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll @@ -45,6 +45,8 @@ module ZipSlip { * Gets a node that can be a parsed archive. */ private DataFlow::SourceNode parsedArchive() { + result = DataFlow::moduleImport("unzipper").getAMemberCall("Parse") + or result = DataFlow::moduleImport("unzip").getAMemberCall("Parse") or result = DataFlow::moduleImport("tar-stream").getAMemberCall("extract") From 4475dd4b9fdf06eea28043fe21a36f3b9662f221 Mon Sep 17 00:00:00 2001 From: Jason Reed Date: Fri, 15 Mar 2019 14:40:48 -0400 Subject: [PATCH 5/5] JavaScript: Add test and fix change note. --- change-notes/1.21/analysis-javascript.md | 2 +- .../Security/CWE-022/ZipSlip/ZipSlip.expected | 6 ++++++ .../Security/CWE-022/ZipSlip/ZipSlipBadUnzipper.js | 9 +++++++++ 3 files changed, 16 insertions(+), 1 deletion(-) create mode 100644 javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBadUnzipper.js diff --git a/change-notes/1.21/analysis-javascript.md b/change-notes/1.21/analysis-javascript.md index c4a8d55ee441..6da4049f494d 100644 --- a/change-notes/1.21/analysis-javascript.md +++ b/change-notes/1.21/analysis-javascript.md @@ -19,6 +19,6 @@ |--------------------------------|------------------------------|---------------------------------------------------------------------------| | Expression has no effect | Fewer false-positive results | This rule now treats uses of `Object.defineProperty` more conservatively. | | Useless assignment to property | Fewer false-positive results | This rule now ignore reads of additional getters. | -| ZipSlip | More results | This rule now considers more libraries, including tar as well as zip. | +| Arbitrary file write during zip extraction ("Zip Slip") | More results | This rule now considers more libraries, including tar as well as zip. | ## Changes to QL libraries 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 34df80c61af6..1e4ae3e38513 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 @@ -7,13 +7,19 @@ nodes | ZipSlipBad.js:7:11:7:31 | fileName | | ZipSlipBad.js:7:22:7:31 | entry.path | | ZipSlipBad.js:8:37:8:44 | fileName | +| ZipSlipBadUnzipper.js:7:9:7:29 | fileName | +| ZipSlipBadUnzipper.js:7:20:7:29 | entry.path | +| ZipSlipBadUnzipper.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 | +| 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 | #select | 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 | | 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 | +| ZipSlipBadUnzipper.js:8:37:8:44 | fileName | ZipSlipBadUnzipper.js:7:20:7:29 | entry.path | ZipSlipBadUnzipper.js:8:37:8:44 | fileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBadUnzipper.js:7:20:7:29 | entry.path | item path | diff --git a/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBadUnzipper.js b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBadUnzipper.js new file mode 100644 index 000000000000..239b3df895d5 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBadUnzipper.js @@ -0,0 +1,9 @@ +const fs = require('fs'); +const unzipper = require('unzipper'); + +fs.createReadStream('path/to/archive.zip') + .pipe(unzipper.Parse()) + .on('entry', function (entry) { + var fileName = entry.path; + entry.pipe(fs.createWriteStream(fileName)); + });