Skip to content

Commit c3973c0

Browse files
author
Esben Sparre Andreasen
committed
JS: split ZipSlip.qll
1 parent 29e69b3 commit c3973c0

File tree

2 files changed

+145
-130
lines changed

2 files changed

+145
-130
lines changed

‎javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll‎

Lines changed: 5 additions & 130 deletions
Original file line numberDiff line numberDiff line change
@@ -1,30 +1,16 @@
11
/**
22
* Provides a taint tracking configuration for reasoning about unsafe
33
* zip and tar archive extraction.
4+
*
5+
* Note, for performance reasons: only import this file if
6+
* `ZipSlip::Configuration` is needed, otherwise
7+
* `ZipSlipCustomizations` should be imported instead.
48
*/
59

610
import javascript
711

812
module ZipSlip {
9-
/**
10-
* A data flow source for unsafe archive extraction.
11-
*/
12-
abstract class Source extends DataFlow::Node { }
13-
14-
/**
15-
* A data flow sink for unsafe archive extraction.
16-
*/
17-
abstract class Sink extends DataFlow::Node { }
18-
19-
/**
20-
* A sanitizer for unsafe archive extraction.
21-
*/
22-
abstract class Sanitizer extends DataFlow::Node { }
23-
24-
/**
25-
* A sanitizer guard for unsafe archive extraction.
26-
*/
27-
abstract class SanitizerGuard extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode { }
13+
import ZipSlipCustomizations::ZipSlip
2814

2915
/** A taint tracking configuration for unsafe archive extraction. */
3016
class Configuration extends TaintTracking::Configuration {
@@ -40,115 +26,4 @@ module ZipSlip {
4026
nd instanceof SanitizerGuard
4127
}
4228
}
43-
44-
/**
45-
* Gets a node that can be a parsed archive.
46-
*/
47-
private DataFlow::SourceNode parsedArchive() {
48-
result = DataFlow::moduleImport("unzipper").getAMemberCall("Parse")
49-
or
50-
result = DataFlow::moduleImport("unzip").getAMemberCall("Parse")
51-
or
52-
result = DataFlow::moduleImport("tar-stream").getAMemberCall("extract")
53-
or
54-
// `streamProducer.pipe(unzip.Parse())` is a typical (but not
55-
// universal) pattern when using nodejs streams, whose return
56-
// value is the parsed stream.
57-
exists(DataFlow::MethodCallNode pipe |
58-
pipe = result and
59-
pipe.getMethodName() = "pipe" and
60-
parsedArchive().flowsTo(pipe.getArgument(0))
61-
)
62-
}
63-
64-
/** Gets a property that is used to get the filename part of an archive entry. */
65-
private string getAFilenameProperty() {
66-
result = "path" // Used by library 'unzip'.
67-
or
68-
result = "name" // Used by library 'tar-stream'.
69-
}
70-
71-
/** An archive entry path access, as a source for unsafe archive extraction. */
72-
class UnzipEntrySource extends Source {
73-
// For example, in
74-
// ```javascript
75-
// const unzip = require('unzip');
76-
//
77-
// fs.createReadStream('archive.zip')
78-
// .pipe(unzip.Parse())
79-
// .on('entry', entry => {
80-
// const path = entry.path;
81-
// });
82-
// ```
83-
// there is an `UnzipEntrySource` node corresponding to
84-
// the expression `entry.path`.
85-
UnzipEntrySource() {
86-
exists(DataFlow::CallNode cn |
87-
cn = parsedArchive().getAMemberCall("on") and
88-
cn.getArgument(0).mayHaveStringValue("entry") and
89-
this = cn.getCallback(1).getParameter(0).getAPropertyRead(getAFilenameProperty())
90-
)
91-
}
92-
}
93-
94-
/** An archive entry path access using the `adm-zip` package. */
95-
class AdmZipEntrySource extends Source {
96-
AdmZipEntrySource() {
97-
exists(DataFlow::SourceNode admZip, DataFlow::SourceNode entry |
98-
admZip = DataFlow::moduleImport("adm-zip").getAnInstantiation() and
99-
this = entry.getAPropertyRead("entryName")
100-
|
101-
entry = admZip.getAMethodCall("getEntry")
102-
or
103-
exists(DataFlow::SourceNode entries | entries = admZip.getAMethodCall("getEntries") |
104-
entry = entries.getAPropertyRead()
105-
or
106-
exists(string map | map = "map" or map = "forEach" |
107-
entry = entries.getAMethodCall(map).getCallback(0).getParameter(0)
108-
)
109-
)
110-
)
111-
}
112-
}
113-
114-
/** A call to `fs.createWriteStream`, as a sink for unsafe archive extraction. */
115-
class CreateWriteStreamSink extends Sink {
116-
CreateWriteStreamSink() {
117-
// This is not covered by `FileSystemWriteSink`, because it is
118-
// required that a write actually takes place to the stream.
119-
// However, we want to consider even the bare `createWriteStream`
120-
// to be a zipslip vulnerability since it may truncate an
121-
// existing file.
122-
this = DataFlow::moduleImport("fs").getAMemberCall("createWriteStream").getArgument(0)
123-
}
124-
}
125-
126-
/** A file path of a file write, as a sink for unsafe archive extraction. */
127-
class FileSystemWriteSink extends Sink {
128-
FileSystemWriteSink() { exists(FileSystemWriteAccess fsw | fsw.getAPathArgument() = this) }
129-
}
130-
131-
/** An expression that sanitizes by calling path.basename */
132-
class BasenameSanitizer extends Sanitizer {
133-
BasenameSanitizer() { this = DataFlow::moduleImport("path").getAMemberCall("basename") }
134-
}
135-
136-
/**
137-
* Gets a string which is sufficient to exclude to make
138-
* a filepath definitely not refer to parent directories.
139-
*/
140-
private string getAParentDirName() { result = ".." or result = "../" }
141-
142-
/** A check that a path string does not include '..' */
143-
class NoParentDirSanitizerGuard extends SanitizerGuard {
144-
StringOps::Includes incl;
145-
146-
NoParentDirSanitizerGuard() { this = incl }
147-
148-
override predicate sanitizes(boolean outcome, Expr e) {
149-
incl.getPolarity().booleanNot() = outcome and
150-
incl.getBaseString().asExpr() = e and
151-
incl.getSubstring().mayHaveStringValue(getAParentDirName())
152-
}
153-
}
15429
}

‎javascript/ql/src/semmle/javascript/security/dataflow/ZipSlipCustomizations.qll‎

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
/**
2+
* Provides default sources, sinks and sanitisers for reasoning about
3+
* unsafe zip and tar archive extraction, as well as extension points
4+
* for adding your own.
5+
*/
6+
7+
import javascript
8+
9+
module ZipSlip {
10+
/**
11+
* A data flow source for unsafe archive extraction.
12+
*/
13+
abstract class Source extends DataFlow::Node { }
14+
15+
/**
16+
* A data flow sink for unsafe archive extraction.
17+
*/
18+
abstract class Sink extends DataFlow::Node { }
19+
20+
/**
21+
* A sanitizer for unsafe archive extraction.
22+
*/
23+
abstract class Sanitizer extends DataFlow::Node { }
24+
25+
/**
26+
* A sanitizer guard for unsafe archive extraction.
27+
*/
28+
abstract class SanitizerGuard extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode { }
29+
30+
/**
31+
* Gets a node that can be a parsed archive.
32+
*/
33+
private DataFlow::SourceNode parsedArchive() {
34+
result = DataFlow::moduleImport("unzipper").getAMemberCall("Parse")
35+
or
36+
result = DataFlow::moduleImport("unzip").getAMemberCall("Parse")
37+
or
38+
result = DataFlow::moduleImport("tar-stream").getAMemberCall("extract")
39+
or
40+
// `streamProducer.pipe(unzip.Parse())` is a typical (but not
41+
// universal) pattern when using nodejs streams, whose return
42+
// value is the parsed stream.
43+
exists(DataFlow::MethodCallNode pipe |
44+
pipe = result and
45+
pipe.getMethodName() = "pipe" and
46+
parsedArchive().flowsTo(pipe.getArgument(0))
47+
)
48+
}
49+
50+
/** Gets a property that is used to get the filename part of an archive entry. */
51+
private string getAFilenameProperty() {
52+
result = "path" // Used by library 'unzip'.
53+
or
54+
result = "name" // Used by library 'tar-stream'.
55+
}
56+
57+
/** An archive entry path access, as a source for unsafe archive extraction. */
58+
class UnzipEntrySource extends Source {
59+
// For example, in
60+
// ```javascript
61+
// const unzip = require('unzip');
62+
//
63+
// fs.createReadStream('archive.zip')
64+
// .pipe(unzip.Parse())
65+
// .on('entry', entry => {
66+
// const path = entry.path;
67+
// });
68+
// ```
69+
// there is an `UnzipEntrySource` node corresponding to
70+
// the expression `entry.path`.
71+
UnzipEntrySource() {
72+
exists(DataFlow::CallNode cn |
73+
cn = parsedArchive().getAMemberCall("on") and
74+
cn.getArgument(0).mayHaveStringValue("entry") and
75+
this = cn.getCallback(1).getParameter(0).getAPropertyRead(getAFilenameProperty())
76+
)
77+
}
78+
}
79+
80+
/** An archive entry path access using the `adm-zip` package. */
81+
class AdmZipEntrySource extends Source {
82+
AdmZipEntrySource() {
83+
exists(DataFlow::SourceNode admZip, DataFlow::SourceNode entry |
84+
admZip = DataFlow::moduleImport("adm-zip").getAnInstantiation() and
85+
this = entry.getAPropertyRead("entryName")
86+
|
87+
entry = admZip.getAMethodCall("getEntry")
88+
or
89+
exists(DataFlow::SourceNode entries | entries = admZip.getAMethodCall("getEntries") |
90+
entry = entries.getAPropertyRead()
91+
or
92+
exists(string map | map = "map" or map = "forEach" |
93+
entry = entries.getAMethodCall(map).getCallback(0).getParameter(0)
94+
)
95+
)
96+
)
97+
}
98+
}
99+
100+
/** A call to `fs.createWriteStream`, as a sink for unsafe archive extraction. */
101+
class CreateWriteStreamSink extends Sink {
102+
CreateWriteStreamSink() {
103+
// This is not covered by `FileSystemWriteSink`, because it is
104+
// required that a write actually takes place to the stream.
105+
// However, we want to consider even the bare `createWriteStream`
106+
// to be a zipslip vulnerability since it may truncate an
107+
// existing file.
108+
this = DataFlow::moduleImport("fs").getAMemberCall("createWriteStream").getArgument(0)
109+
}
110+
}
111+
112+
/** A file path of a file write, as a sink for unsafe archive extraction. */
113+
class FileSystemWriteSink extends Sink {
114+
FileSystemWriteSink() { exists(FileSystemWriteAccess fsw | fsw.getAPathArgument() = this) }
115+
}
116+
117+
/** An expression that sanitizes by calling path.basename */
118+
class BasenameSanitizer extends Sanitizer {
119+
BasenameSanitizer() { this = DataFlow::moduleImport("path").getAMemberCall("basename") }
120+
}
121+
122+
/**
123+
* Gets a string which is sufficient to exclude to make
124+
* a filepath definitely not refer to parent directories.
125+
*/
126+
private string getAParentDirName() { result = ".." or result = "../" }
127+
128+
/** A check that a path string does not include '..' */
129+
class NoParentDirSanitizerGuard extends SanitizerGuard {
130+
StringOps::Includes incl;
131+
132+
NoParentDirSanitizerGuard() { this = incl }
133+
134+
override predicate sanitizes(boolean outcome, Expr e) {
135+
incl.getPolarity().booleanNot() = outcome and
136+
incl.getBaseString().asExpr() = e and
137+
incl.getSubstring().mayHaveStringValue(getAParentDirName())
138+
}
139+
}
140+
}

0 commit comments

Comments
 (0)