Skip to content

Commit e592e2b

Browse files
esbenaesbena
committed
JS: add query js/unclosed-stream
1 parent 1c100bb commit e592e2b

File tree

8 files changed

+283
-0
lines changed

8 files changed

+283
-0
lines changed

Diff for: ‎javascript/config/suites/javascript/security

+1
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
+ semmlecode-javascript-queries/Security/CWE-640/HostHeaderPoisoningInEmailGeneration.ql: /Security/CWE/CWE-640
5050
+ semmlecode-javascript-queries/Security/CWE-643/XpathInjection.ql: /Security/CWE/CWE-643
5151
+ semmlecode-javascript-queries/Security/CWE-730/RegExpInjection.ql: /Security/CWE/CWE-730
52+
+ semmlecode-javascript-queries/Security/CWE-730/UnclosedStream.ql: /Security/CWE/CWE-730
5253
+ semmlecode-javascript-queries/Security/CWE-754/UnvalidatedDynamicMethodCall.ql: /Security/CWE/CWE-754
5354
+ semmlecode-javascript-queries/Security/CWE-770/MissingRateLimiting.ql: /Security/CWE/CWE-770
5455
+ semmlecode-javascript-queries/Security/CWE-776/XmlBomb.ql: /Security/CWE/CWE-776

Diff for: ‎javascript/ql/src/Security/CWE-730/UnclosedStream.qhelp

+69
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
8+
<p>
9+
10+
Callback-based input streams generate calls until they are empty or
11+
closed explicitly, and they will take up system resources until
12+
that point in time.
13+
14+
</p>
15+
16+
<p>
17+
18+
Forgetting to close a stream that no longer is useful can cause
19+
unnecessary exhaustion of system resources, which may cause the
20+
application to be unresponsive or even crash.
21+
22+
</p>
23+
24+
</overview>
25+
26+
<recommendation>
27+
28+
<p>
29+
30+
Close input streams explicitly when there is no interest in their
31+
remaining content.
32+
33+
</p>
34+
35+
</recommendation>
36+
37+
<example>
38+
39+
<p>
40+
41+
In the following example, an array stores the content of a stream
42+
until the array becomes too big. When that happens, an error message
43+
is generated, and the stream processing is supposed to end.
44+
45+
</p>
46+
47+
<sample src="examples/UnclosedStream.js"/>
48+
49+
<p>
50+
51+
The stream will however keep invoking the callback until it becomes
52+
empty, regardless of the programmers intention to not process
53+
additional content. This means that the array will grow indefinitely, if a malicious actor keep sending data.
54+
55+
Close the stream to remedy this:
56+
57+
</p>
58+
59+
<sample src="examples/UnclosedStream_fixed.js"/>
60+
61+
</example>
62+
63+
<references>
64+
65+
<li>Node.js: <a href="https://nodejs.org/api/stream.html">Stream</a></li>
66+
67+
</references>
68+
69+
</qhelp>

Diff for: ‎javascript/ql/src/Security/CWE-730/UnclosedStream.ql

+128
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
/**
2+
* @name Unclosed stream
3+
* @description A stream that is not closed may leak system resources.
4+
* @kind problem
5+
* @problem.severity error
6+
* @precision high
7+
* @id js/unclosed-stream
8+
* @tags security
9+
* external/cwe/cwe-730
10+
* external/cwe/cwe-404
11+
*/
12+
13+
import javascript
14+
15+
/**
16+
* Gets a function that `caller` invokes directly or indirectly as a callback to a library function.
17+
*/
18+
Function getACallee(Function caller) {
19+
exists(DataFlow::InvokeNode invk | invk.getEnclosingFunction() = caller |
20+
result = invk.getACallee()
21+
or
22+
not exists(invk.getACallee()) and
23+
result = invk.getCallback(invk.getNumArgument()).getFunction()
24+
)
25+
}
26+
27+
/**
28+
* A function that can be terminated meaningfully in an asynchronous context.
29+
*/
30+
abstract class AsyncTerminatableFunction extends DataFlow::FunctionNode {
31+
/**
32+
* Gets a node where this function terminates.
33+
*
34+
* It can be expected that no further expressions in this function will be evaluated after the evaluation of this node.
35+
*/
36+
abstract DataFlow::Node getTermination();
37+
38+
/**
39+
* Gets a brief description of this function.
40+
*/
41+
abstract string getKind();
42+
}
43+
44+
/**
45+
* A promise executor as a function that can be terminated in an asynchronous context.
46+
*/
47+
class PromiseExecutor extends AsyncTerminatableFunction {
48+
PromiseDefinition def;
49+
50+
PromiseExecutor() { this = def.getExecutor() }
51+
52+
override DataFlow::CallNode getTermination() {
53+
result = [def.getRejectParameter(), def.getResolveParameter()].getACall()
54+
}
55+
56+
override string getKind() { result = "promise" }
57+
}
58+
59+
/**
60+
* A callback-invoking function heuristic as a function that can be terminated in an asynchronous context.
61+
*/
62+
class FunctionWithCallback extends AsyncTerminatableFunction {
63+
DataFlow::ParameterNode callbackParameter;
64+
65+
FunctionWithCallback() {
66+
// the last parameter is the callback
67+
callbackParameter = this.getLastParameter() and
68+
// simple escape analysis
69+
not exists(DataFlow::Node escape | callbackParameter.flowsTo(escape) |
70+
escape = any(DataFlow::PropWrite w).getRhs() or
71+
escape = any(DataFlow::CallNode c).getAnArgument()
72+
) and
73+
// no return value
74+
(this.getFunction() instanceof ArrowFunctionExpr or not exists(this.getAReturn())) and
75+
// all callback invocations are terminal (note that this permits calls in closures)
76+
forex(DataFlow::CallNode termination | termination = callbackParameter.getACall() |
77+
termination.asExpr() = any(Function f).getExit().getAPredecessor()
78+
) and
79+
// avoid confusion with promises
80+
not this instanceof PromiseExecutor and
81+
not exists(PromiseCandidate c | this.flowsTo(c.getAnArgument()))
82+
}
83+
84+
override DataFlow::CallNode getTermination() { result = callbackParameter.getACall() }
85+
86+
override string getKind() { result = "asynchronous function" }
87+
}
88+
89+
/**
90+
* A data stream.
91+
*/
92+
class Stream extends DataFlow::SourceNode {
93+
DataFlow::FunctionNode processor;
94+
95+
Stream() {
96+
exists(DataFlow::CallNode onData |
97+
this instanceof EventEmitter and
98+
onData = this.getAMethodCall(EventEmitter::on()) and
99+
onData.getArgument(0).mayHaveStringValue("data") and
100+
processor = onData.getCallback(1)
101+
)
102+
}
103+
104+
/**
105+
* Gets a call that closes thes stream.
106+
*/
107+
DataFlow::Node getClose() { result = this.getAMethodCall("destroy") }
108+
109+
/**
110+
* Gets a function that processes the content of the stream.
111+
*/
112+
DataFlow::FunctionNode getProcessor() { result = processor }
113+
}
114+
115+
from
116+
AsyncTerminatableFunction terminatable, DataFlow::CallNode termination, Stream stream,
117+
Function processor
118+
where
119+
stream.getAstNode().getContainer() = getACallee*(terminatable.getFunction()) and
120+
termination = terminatable.getTermination() and
121+
processor = stream.getProcessor().getFunction() and
122+
termination.asExpr().getEnclosingFunction() = getACallee*(processor) and
123+
not exists(Function destroyFunction |
124+
destroyFunction = getACallee*(processor) and
125+
stream.getClose().asExpr().getEnclosingFunction() = destroyFunction
126+
)
127+
select processor, "This stream processor $@ the enclosing $@ without closing the stream.",
128+
termination, "terminates", terminatable, terminatable.getKind()

Diff for: ‎javascript/ql/src/Security/CWE-730/examples/UnclosedStream.js

+16
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
var https = require("https");
2+
3+
new Promise(function dispatchHttpRequest(resolve, reject) {
4+
https.request(options, function(stream) {
5+
var responseBuffer = [];
6+
stream.on("data", function(chunk) {
7+
responseBuffer.push(chunk);
8+
9+
if (responseBuffer.length > 1024) {
10+
reject("Input size of 1024 exceeded."); // BAD
11+
}
12+
});
13+
14+
stream.on("end", () => resolve(responseBuffer));
15+
});
16+
});

Diff for: ‎javascript/ql/src/Security/CWE-730/examples/UnclosedStream_fixed.js

+17
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
var https = require("https");
2+
3+
new Promise(function dispatchHttpRequest(resolve, reject) {
4+
https.request(options, function(stream) {
5+
var responseBuffer = [];
6+
stream.on("data", function(chunk) {
7+
responseBuffer.push(chunk);
8+
9+
if (responseBuffer.length > 1024) {
10+
stream.destroy();
11+
reject("Input size of 1024 exceeded."); // GOOD
12+
}
13+
});
14+
15+
stream.on("end", () => resolve(responseBuffer));
16+
});
17+
});

Diff for: ‎javascript/ql/test/query-tests/Security/CWE-730/UnclosedStream.expected

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
| unclosed-stream.js:9:23:9:50 | d => re ... silly") | This stream processor $@ the enclosing $@ without closing the stream. | unclosed-stream.js:9:28:9:50 | resolve ... silly") | terminates | unclosed-stream.js:3:13:46:1 | functio ... });\\n} | promise |
2+
| unclosed-stream.js:11:23:11:49 | d => re ... silly") | This stream processor $@ the enclosing $@ without closing the stream. | unclosed-stream.js:11:28:11:49 | rejectP ... silly") | terminates | unclosed-stream.js:3:13:46:1 | functio ... });\\n} | promise |
3+
| unclosed-stream.js:13:23:13:50 | d => re ... silly") | This stream processor $@ the enclosing $@ without closing the stream. | unclosed-stream.js:5:5:5:24 | rejectPromise(value) | terminates | unclosed-stream.js:3:13:46:1 | functio ... });\\n} | promise |
4+
| unclosed-stream.js:35:23:35:50 | d => re ... silly") | This stream processor $@ the enclosing $@ without closing the stream. | unclosed-stream.js:35:28:35:50 | resolve ... silly") | terminates | unclosed-stream.js:3:13:46:1 | functio ... });\\n} | promise |
5+
| unclosed-stream.js:43:23:43:50 | d => re ... silly") | This stream processor $@ the enclosing $@ without closing the stream. | unclosed-stream.js:43:28:43:50 | resolve ... silly") | terminates | unclosed-stream.js:3:13:46:1 | functio ... });\\n} | promise |

Diff for: ‎javascript/ql/test/query-tests/Security/CWE-730/UnclosedStream.qlref

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Security/CWE-730/UnclosedStream.ql

Diff for: ‎javascript/ql/test/query-tests/Security/CWE-730/unclosed-stream.js

+46
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
var http = require("http");
2+
3+
new Promise(function(resolvePromise, rejectPromise) {
4+
function rejectIndirect(value) {
5+
rejectPromise(value);
6+
}
7+
8+
http.request({}, function(stream) {
9+
stream.on("data", d => resolvePromise("silly")); // NOT OK
10+
11+
stream.on("data", d => rejectPromise("silly")); // NOT OK
12+
13+
stream.on("data", d => rejectIndirect("silly")); // NOT OK
14+
15+
stream.on(
16+
"error",
17+
e => rejectIndirect(e) // OK
18+
);
19+
20+
stream.on(
21+
"end",
22+
e => resolvePromise(e) // OK
23+
);
24+
});
25+
26+
http.request({}, function(stream) {
27+
stream.on("data", d => {
28+
// OK
29+
stream.destroy();
30+
resolvePromise("silly");
31+
});
32+
});
33+
34+
http.request({}, function(stream) {
35+
stream.on("data", d => resolvePromise("silly")); // OK [INCONSISTENCY]: the other data handler closes the stream
36+
stream.on("data", d => {
37+
// OK
38+
stream.destroy();
39+
resolvePromise("silly");
40+
});
41+
});
42+
http.request({}, function(stream) {
43+
stream.on("data", d => resolvePromise("silly")); // OK [INCONSISTENCY]: the stream is closed automatically
44+
setTimeout(100, () => stream.destroy());
45+
});
46+
});

0 commit comments

Comments
 (0)