diff --git a/change-notes/1.21/analysis-javascript.md b/change-notes/1.21/analysis-javascript.md index d42e2d080b87..6da4049f494d 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. | +| 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/src/semmle/javascript/security/dataflow/ZipSlip.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll index 39ae5ef40fd0..57af27cdf55f 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,11 +42,15 @@ 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("unzipper").getAMemberCall("Parse") + or 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,7 +61,14 @@ module ZipSlip { ) } - /** A zip archive entry path access, as a source for unsafe zip extraction. */ + /** Gets a property that is used to get the filename part of an archive entry. */ + 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 { // For example, in // ```javascript @@ -74,13 +86,12 @@ 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()) + ) } } - /** 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 @@ -92,16 +103,14 @@ 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) } } /** 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..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 @@ -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 | @@ -6,12 +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)); + });