Skip to content

JavaScript: Add tar-stream extraction to ZipSlip query. #1118

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
semmle-qlci merged 5 commits into from
Mar 18, 2019
Merged

JavaScript: Add tar-stream extraction to ZipSlip query. #1118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6589813
JavaScript: Add tar-stream extraction to ZipSlip query.
jcreedcmu Mar 14, 2019
a674dbb
JavaScript: Update docstrings to reflect generalization.
jcreedcmu Mar 15, 2019
8124980
JavaScript: Add change note and comment.
jcreedcmu Mar 15, 2019
aa9ba95
JavaScript: Include 'unzipper' library in ZipSlip.
jcreedcmu Mar 15, 2019
4475dd4
JavaScript: Add test and fix change note.
jcreedcmu Mar 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions change-notes/1.21/analysis-javascript.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
41 changes: 25 additions & 16 deletions javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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" }

Expand All @@ -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")

@xiemaisi xiemaisi Mar 15, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test case for this one as well?

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.
Expand All @@ -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. */

@xiemaisi xiemaisi Mar 15, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume it's "path" for unzip and "name" for tar-stream? This may be worth clarifying in the comment (but of course it's fine to allow both for both packages as you do here.)

@jcreedcmu jcreedcmu Mar 15, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, done.

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
Expand All @@ -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
Expand All @@ -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") }
}

/**
Expand Down
18 changes: 18 additions & 0 deletions javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/TarSlipBad.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
8 changes: 8 additions & 0 deletions javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlip.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,25 @@
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 |
| 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 |
| 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 |
9 changes: 9 additions & 0 deletions javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBadUnzipper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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));
});