Skip to content

Commit b3e68ef

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #3806 from erik-krogh/moreDownloads
Approved by asgerf
2 parents e00a8f7 + 27b2c02 commit b3e68ef

File tree

5 files changed

+142
-10
lines changed

5 files changed

+142
-10
lines changed

‎javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,11 @@ class ClientRequest extends DataFlow::InvokeNode {
6868
* wrapped in a promise object.
6969
*/
7070
DataFlow::Node getAResponseDataNode() { result = getAResponseDataNode(_, _) }
71+
72+
/**
73+
* Gets a data-flow node that determines where in the file-system the result of the request should be saved.
74+
*/
75+
DataFlow::Node getASavePath() { result = self.getASavePath() }
7176
}
7277

7378
deprecated class CustomClientRequest = ClientRequest::Range;
@@ -103,6 +108,11 @@ module ClientRequest {
103108
* See the decription of `responseType` in `ClientRequest::getAResponseDataNode`.
104109
*/
105110
DataFlow::Node getAResponseDataNode(string responseType, boolean promise) { none() }
111+
112+
/**
113+
* Gets a data-flow node that determines where in the file-system the result of the request should be saved.
114+
*/
115+
DataFlow::Node getASavePath() { none() }
106116
}
107117

108118
/**
@@ -180,6 +190,14 @@ module ClientRequest {
180190
}
181191

182192
override DataFlow::Node getADataNode() { result = getArgument(1) }
193+
194+
override DataFlow::Node getASavePath() {
195+
exists(DataFlow::CallNode write |
196+
write = DataFlow::moduleMember("fs", "createWriteStream").getACall() and
197+
write = this.getAMemberCall("pipe").getArgument(0).getALocalSource() and
198+
result = write.getArgument(0)
199+
)
200+
}
183201
}
184202

185203
/** Gets the string `url` or `uri`. */
@@ -632,6 +650,10 @@ module ClientRequest {
632650
override DataFlow::Node getHost() { none() }
633651

634652
override DataFlow::Node getADataNode() { none() }
653+
654+
override DataFlow::Node getASavePath() {
655+
result = this.getArgument(1).getALocalSource().getAPropertyWrite("target").getRhs()
656+
}
635657
}
636658

637659
/**

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,12 @@ module InsecureDownload {
2020
class Configuration extends DataFlow::Configuration {
2121
Configuration() { this = "InsecureDownload" }
2222

23-
override predicate isSource(DataFlow::Node source) { source instanceof Source }
23+
override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
24+
source.(Source).getALabel() = label
25+
}
2426

25-
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
27+
override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
28+
sink.(Sink).getALabel() = label
29+
}
2630
}
2731
}

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

Lines changed: 87 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,12 @@ module InsecureDownload {
1313
/**
1414
* A data flow source for download of sensitive file through insecure connection.
1515
*/
16-
abstract class Source extends DataFlow::Node { }
16+
abstract class Source extends DataFlow::Node {
17+
/**
18+
* Gets a flow-label for this source.
19+
*/
20+
abstract DataFlow::FlowLabel getALabel();
21+
}
1722

1823
/**
1924
* A data flow sink for download of sensitive file through insecure connection.
@@ -23,28 +28,67 @@ module InsecureDownload {
2328
* Gets the call that downloads the sensitive file.
2429
*/
2530
abstract DataFlow::Node getDownloadCall();
31+
32+
/**
33+
* Gets a flow-label where this sink is vulnerable.
34+
*/
35+
abstract DataFlow::FlowLabel getALabel();
2636
}
2737

2838
/**
2939
* A sanitizer for download of sensitive file through insecure connection.
3040
*/
3141
abstract class Sanitizer extends DataFlow::Node { }
3242

43+
/**
44+
* Flow-labels for reasoning about download of sensitive file through insecure connection.
45+
*/
46+
module Label {
47+
/**
48+
* A flow-label for file URLs that are both sensitive and downloaded over an insecure connection.
49+
*/
50+
class SensitiveInsecureURL extends DataFlow::FlowLabel {
51+
SensitiveInsecureURL() { this = "sensitiveInsecure" }
52+
}
53+
54+
/**
55+
* A flow-label for a URL that is downloaded over an insecure connection.
56+
*/
57+
class InsecureURL extends DataFlow::FlowLabel {
58+
InsecureURL() { this = "insecure" }
59+
}
60+
}
61+
3362
/**
3463
* A HTTP or FTP URL that refers to a file with a sensitive file extension,
3564
* seen as a source for download of sensitive file through insecure connection.
3665
*/
3766
class SensitiveFileUrl extends Source {
67+
string str;
68+
3869
SensitiveFileUrl() {
39-
exists(string str | str = this.getStringValue() |
40-
str.regexpMatch("http://.*|ftp://.*") and
41-
exists(string suffix | suffix = unsafeExtension() |
42-
str.suffix(str.length() - suffix.length() - 1).toLowerCase() = "." + suffix
43-
)
44-
)
70+
str = this.getStringValue() and
71+
str.regexpMatch("http://.*|ftp://.*")
72+
}
73+
74+
override DataFlow::FlowLabel getALabel() {
75+
result instanceof Label::InsecureURL
76+
or
77+
hasUnsafeExtension(str) and
78+
result instanceof Label::SensitiveInsecureURL
4579
}
4680
}
4781

82+
/**
83+
* Holds if `str` is a string that ends with an unsafe file extension.
84+
*/
85+
bindingset[str]
86+
predicate hasUnsafeExtension(string str) {
87+
exists(string suffix | suffix = unsafeExtension() |
88+
str.suffix(str.length() - suffix.length() - 1).toLowerCase() = "." + suffix
89+
)
90+
}
91+
4892
/**
4993
* Gets a file-extension that can potentially be dangerous.
5094
*
@@ -58,13 +102,48 @@ module InsecureDownload {
58102

59103
/**
60104
* A url downloaded by a client-request, seen as a sink for download of
61-
* sensitive file through insecure connection.a
105+
* sensitive file through insecure connection.
62106
*/
63107
class ClientRequestURL extends Sink {
64108
ClientRequest request;
65109

66110
ClientRequestURL() { this = request.getUrl() }
67111

68112
override DataFlow::Node getDownloadCall() { result = request }
113+
114+
override DataFlow::FlowLabel getALabel() {
115+
result instanceof Label::SensitiveInsecureURL
116+
or
117+
hasUnsafeExtension(request.getASavePath().getStringValue()) and
118+
result instanceof Label::InsecureURL
119+
}
120+
}
121+
122+
/**
123+
* Gets a node for the response from `request`, type-tracked using `t`.
124+
*/
125+
DataFlow::SourceNode clientRequestResponse(DataFlow::TypeTracker t, ClientRequest request) {
126+
t.start() and
127+
result = request.getAResponseDataNode()
128+
or
129+
exists(DataFlow::TypeTracker t2 | result = clientRequestResponse(t2, request).track(t2, t))
130+
}
131+
132+
/**
133+
* A url that is downloaded through an insecure connection, where the result ends up being saved to a sensitive location.
134+
*/
135+
class FileWriteSink extends Sink {
136+
ClientRequest request;
137+
FileSystemWriteAccess write;
138+
139+
FileWriteSink() {
140+
this = request.getUrl() and
141+
clientRequestResponse(DataFlow::TypeTracker::end(), request).flowsTo(write.getADataNode()) and
142+
hasUnsafeExtension(write.getAPathArgument().getStringValue())
143+
}
144+
145+
override DataFlow::FlowLabel getALabel() { result instanceof Label::InsecureURL }
146+
147+
override DataFlow::Node getDownloadCall() { result = request }
69148
}
70149
}

‎javascript/ql/test/query-tests/Security/CWE-829/InsecureDownload.expected‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,12 @@ nodes
1717
| insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" |
1818
| insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" |
1919
| insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" |
20+
| insecure-download.js:48:12:48:38 | "http:/ ... unsafe" |
21+
| insecure-download.js:48:12:48:38 | "http:/ ... unsafe" |
22+
| insecure-download.js:48:12:48:38 | "http:/ ... unsafe" |
23+
| insecure-download.js:52:11:52:45 | "http:/ ... nknown" |
24+
| insecure-download.js:52:11:52:45 | "http:/ ... nknown" |
25+
| insecure-download.js:52:11:52:45 | "http:/ ... nknown" |
2026
edges
2127
| insecure-download.js:9:27:9:138 | 'http:/ ... ll.exe' | insecure-download.js:15:18:15:40 | buildTo ... llerUrl |
2228
| insecure-download.js:9:27:9:138 | 'http:/ ... ll.exe' | insecure-download.js:15:18:15:40 | buildTo ... llerUrl |
@@ -30,9 +36,13 @@ edges
3036
| insecure-download.js:36:15:36:45 | "http:/ ... fe.APK" | insecure-download.js:36:9:36:45 | url |
3137
| insecure-download.js:36:15:36:45 | "http:/ ... fe.APK" | insecure-download.js:36:9:36:45 | url |
3238
| insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" | insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" |
39+
| insecure-download.js:48:12:48:38 | "http:/ ... unsafe" | insecure-download.js:48:12:48:38 | "http:/ ... unsafe" |
40+
| insecure-download.js:52:11:52:45 | "http:/ ... nknown" | insecure-download.js:52:11:52:45 | "http:/ ... nknown" |
3341
#select
3442
| insecure-download.js:5:16:5:28 | installer.url | insecure-download.js:9:27:9:138 | 'http:/ ... ll.exe' | insecure-download.js:5:16:5:28 | installer.url | $@ of sensitive file from $@. | insecure-download.js:5:9:5:44 | nugget( ... => { }) | Download | insecure-download.js:9:27:9:138 | 'http:/ ... ll.exe' | HTTP source |
3543
| insecure-download.js:30:12:30:42 | "http:/ ... fe.APK" | insecure-download.js:30:12:30:42 | "http:/ ... fe.APK" | insecure-download.js:30:12:30:42 | "http:/ ... fe.APK" | $@ of sensitive file from $@. | insecure-download.js:30:5:30:43 | nugget( ... e.APK") | Download | insecure-download.js:30:12:30:42 | "http:/ ... fe.APK" | HTTP source |
3644
| insecure-download.js:37:23:37:25 | url | insecure-download.js:36:15:36:45 | "http:/ ... fe.APK" | insecure-download.js:37:23:37:25 | url | $@ of sensitive file from $@. | insecure-download.js:37:5:37:42 | cp.exec ... () {}) | Download | insecure-download.js:36:15:36:45 | "http:/ ... fe.APK" | HTTP source |
3745
| insecure-download.js:39:26:39:28 | url | insecure-download.js:36:15:36:45 | "http:/ ... fe.APK" | insecure-download.js:39:26:39:28 | url | $@ of sensitive file from $@. | insecure-download.js:39:5:39:46 | cp.exec ... () {}) | Download | insecure-download.js:36:15:36:45 | "http:/ ... fe.APK" | HTTP source |
3846
| insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" | insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" | insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" | $@ of sensitive file from $@. | insecure-download.js:41:5:41:42 | nugget( ... e.APK") | Download | insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" | HTTP source |
47+
| insecure-download.js:48:12:48:38 | "http:/ ... unsafe" | insecure-download.js:48:12:48:38 | "http:/ ... unsafe" | insecure-download.js:48:12:48:38 | "http:/ ... unsafe" | $@ of sensitive file from $@. | insecure-download.js:48:5:48:71 | nugget( ... => { }) | Download | insecure-download.js:48:12:48:38 | "http:/ ... unsafe" | HTTP source |
48+
| insecure-download.js:52:11:52:45 | "http:/ ... nknown" | insecure-download.js:52:11:52:45 | "http:/ ... nknown" | insecure-download.js:52:11:52:45 | "http:/ ... nknown" | $@ of sensitive file from $@. | insecure-download.js:52:5:54:6 | $.get(" ... \\n }) | Download | insecure-download.js:52:11:52:45 | "http:/ ... nknown" | HTTP source |

‎javascript/ql/test/query-tests/Security/CWE-829/insecure-download.js‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,3 +40,20 @@ function baz() {
4040

4141
nugget("ftp://example.org/unsafe.APK") // NOT OK
4242
}
43+
44+
const fs = require("fs");
45+
var writeFileAtomic = require("write-file-atomic");
46+
47+
function test() {
48+
nugget("http://example.org/unsafe", {target: "foo.exe"}, () => { }) // NOT OK
49+
50+
nugget("http://example.org/unsafe", {target: "foo.safe"}, () => { }) // OK
51+
52+
$.get("http://example.org/unsafe.unknown", function( data ) {
53+
writeFileAtomic('unsafe.exe', data, {}, function (err) {}); // NOT OK
54+
});
55+
56+
$.get("http://example.org/unsafe.unknown", function( data ) {
57+
writeFileAtomic('foo.safe', data, {}, function (err) {}); // OK
58+
});
59+
}

0 commit comments

Comments
 (0)