From 0a91d919b0938efb6d572333641768796b04461e Mon Sep 17 00:00:00 2001 From: Jason Reed Date: Mon, 4 Mar 2019 11:04:45 +0000 Subject: [PATCH 1/3] JS: Allow path.basename sanitization in zipslip. --- .../javascript/security/dataflow/ZipSlip.qll | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll index 2182fc038878..39ae5ef40fd0 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll @@ -15,6 +15,11 @@ module ZipSlip { */ abstract class Sink extends DataFlow::Node { } + /** + * A sanitizer for unsafe zip extraction. + */ + abstract class Sanitizer extends DataFlow::Node { } + /** * A sanitizer guard for unsafe zip extraction. */ @@ -28,6 +33,8 @@ module ZipSlip { override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } + override predicate isSanitizer(DataFlow::Node sanitizer) { sanitizer instanceof Sanitizer } + override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode nd) { nd instanceof SanitizerGuard } @@ -90,6 +97,13 @@ module ZipSlip { 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") + } + } + /** * Gets a string which is sufficient to exclude to make * a filepath definitely not refer to parent directories. From 126e207bd03772100c9ac133d1317ed0eaee2143 Mon Sep 17 00:00:00 2001 From: Jason Reed Date: Mon, 4 Mar 2019 11:18:41 +0000 Subject: [PATCH 2/3] JS: Add change note. --- change-notes/1.20/analysis-javascript.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/change-notes/1.20/analysis-javascript.md b/change-notes/1.20/analysis-javascript.md index 8e6a60ba596b..b61bed33c96d 100644 --- a/change-notes/1.20/analysis-javascript.md +++ b/change-notes/1.20/analysis-javascript.md @@ -24,6 +24,7 @@ | **Query** | **Tags** | **Purpose** | |-----------------------------------------------|------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| Arbitrary file write during archive extraction ("Zip Slip") (`js/zipslip`) | security, external/cwe/cwe-022 | Identifies extraction routines that allow arbitrary file overwrite vulnerabilities. Results are hidden on LGTM by default. | | Arrow method on Vue instance (`js/vue/arrow-method-on-vue-instance`) | reliability, frameworks/vue | Highlights arrow functions that are used as methods on Vue instances. Results are shown on LGTM by default.| | Cross-window communication with unrestricted target origin (`js/cross-window-information-leak`) | security, external/cwe/201, external/cwe/359 | Highlights code that sends potentially sensitive information to another window without restricting the receiver window's origin, indicating a possible violation of [CWE-201](https://cwe.mitre.org/data/definitions/201.html). Results are shown on LGTM by default. | | Double escaping or unescaping (`js/double-escaping`) | correctness, security, external/cwe/cwe-116 | Highlights potential double escaping or unescaping of special characters, indicating a possible violation of [CWE-116](https://cwe.mitre.org/data/definitions/116.html). Results are shown on LGTM by default. | @@ -55,7 +56,7 @@ | Useless assignment to property. | Fewer false-positive results | This rule now treats assignments with complex right-hand sides correctly. | | Unsafe dynamic method access | Fewer false-positive results | This rule no longer flags concatenated strings as unsafe method names. | | Unvalidated dynamic method call | More true-positive results | This rule now flags concatenated strings as unvalidated method names in more cases. | -| Useless conditional | More true-positive results | This rule now flags additional uses of function call values. | +| Useless conditional | More true-positive results | This rule now flags additional uses of function call values. | ## Changes to QL libraries From 8829fde86bd8ff6482d4cccf5df99ddb11208719 Mon Sep 17 00:00:00 2001 From: Jason Reed Date: Mon, 4 Mar 2019 11:20:06 +0000 Subject: [PATCH 3/3] JS: Add test for zipslip basename sanitization. --- .../Security/CWE-022/ZipSlip/ZipSlipGoodBasename.js | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipGoodBasename.js diff --git a/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipGoodBasename.js b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipGoodBasename.js new file mode 100644 index 000000000000..b182aaeb3a5f --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipGoodBasename.js @@ -0,0 +1,10 @@ +const fs = require('fs'); +const unzip = require('unzip'); +const path = require('path'); + +fs.createReadStream('archive.zip') + .pipe(unzip.Parse()) + .on('entry', entry => { + const fileName = entry.path; + entry.pipe(fs.createWriteStream(path.join('my_directory', path.basename(fileName)))); + });