Skip to content

Commit e2c941c

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #1916 from erik-krogh/taintedLength
Approved by asger-semmle, xiemaisi
2 parents 7a57a3c + 814c553 commit e2c941c

19 files changed

+865
-2
lines changed

‎change-notes/1.23/analysis-javascript.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,8 @@
1414

1515
| **Query** | **Tags** | **Purpose** |
1616
|---------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
17-
| Unused index variable (`js/unused-index-variable`) | correctness | Highlights loops that iterate over an array, but do not use the index variable to access array elements, indicating a possible typo or logic error. |
18-
17+
| Unused index variable (`js/unused-index-variable`) | correctness | Highlights loops that iterate over an array, but do not use the index variable to access array elements, indicating a possible typo or logic error. Results are shown on LGTM by default. |
18+
| Loop bound injection (`js/loop-bound-injection`) | security, external/cwe/cwe-834 | Highlights loops where a user-controlled object with an arbitrary .length value can trick the server to loop indefinitely. Results are not shown on LGTM by default. |
1919

2020
## Changes to existing queries
2121

‎javascript/config/suites/javascript/security‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
+ semmlecode-javascript-queries/Security/CWE-798/HardcodedCredentials.ql: /Security/CWE/CWE-798
4343
+ semmlecode-javascript-queries/Security/CWE-807/ConditionalBypass.ql: /Security/CWE/CWE-807
4444
+ semmlecode-javascript-queries/Security/CWE-807/DifferentKindsComparisonBypass.ql: /Security/CWE/CWE-807
45+
+ semmlecode-javascript-queries/Security/CWE-834/LoopBoundInjection.ql: /Security/CWE/CWE-834
4546
+ semmlecode-javascript-queries/Security/CWE-843/TypeConfusionThroughParameterTampering.ql: /Security/CWE/CWE-834
4647
+ semmlecode-javascript-queries/Security/CWE-916/InsufficientPasswordHash.ql: /Security/CWE/CWE-916
4748
+ semmlecode-javascript-queries/Security/CWE-918/RequestForgery.ql: /Security/CWE/CWE-918

‎javascript/ql/src/Security/CWE-834/LoopBoundInjection.qhelp‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
Using the <code>.length</code> property of an untrusted object as a loop bound may
9+
cause indefinite looping since a malicious attacker can set the
10+
<code>.length</code> property to a very large number. For example,
11+
when a program that expects an array is passed a JSON object such as
12+
<code>{length: 1e100}</code>, the loop will be run for 10<sup>100</sup>
13+
iterations. This may cause the program to hang or run out of memory,
14+
which can be used to mount a denial-of-service (DoS) attack.
15+
</p>
16+
</overview>
17+
18+
<recommendation>
19+
<p>
20+
Either check that the object is indeed an array or limit the
21+
size of the <code>.length</code> property.
22+
</p>
23+
</recommendation>
24+
25+
<example>
26+
<p>
27+
In the example below, an HTTP request handler iterates over a
28+
user-controlled object <code>obj</code> using the
29+
<code>obj.length</code> property in order to copy the elements from
30+
<code>obj</code> to an array.
31+
</p>
32+
33+
<sample src="examples/LoopBoundInjection.js" />
34+
35+
<p>
36+
This is not secure since an attacker can control the value of
37+
<code>obj.length</code>, and thereby cause the loop to iterate
38+
indefinitely. Here the potential DoS is fixed by enforcing that
39+
the user-controlled object is an array.
40+
</p>
41+
42+
<sample src="examples/LoopBoundInjection_fixed.js" />
43+
</example>
44+
45+
<references></references>
46+
</qhelp>

‎javascript/ql/src/Security/CWE-834/LoopBoundInjection.ql‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
/**
2+
* @name Loop bound injection
3+
* @description Iterating over an object with a user-controlled .length
4+
* property can cause indefinite looping.
5+
* @kind path-problem
6+
* @problem.severity warning
7+
* @id js/loop-bound-injection
8+
* @tags security
9+
* external/cwe/cwe-834
10+
* @precision medium
11+
*/
12+
13+
import javascript
14+
import semmle.javascript.security.dataflow.LoopBoundInjection::LoopBoundInjection
15+
import DataFlow::PathGraph
16+
17+
from Configuration dataflow, DataFlow::PathNode source, DataFlow::PathNode sink
18+
where dataflow.hasFlowPath(source, sink)
19+
select sink, source, sink,
20+
"Iterating over user-controlled object with a potentially unbounded .length property from $@.",
21+
source, "here"

‎javascript/ql/src/Security/CWE-834/examples/LoopBoundInjection.js‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
var express = require('express');
2+
var app = express();
3+
4+
app.post("/foo", (req, res) => {
5+
var obj = req.body;
6+
7+
var ret = [];
8+
9+
// Potential DoS if obj.length is large.
10+
for (var i = 0; i < obj.length; i++) {
11+
ret.push(obj[i]);
12+
}
13+
});

‎javascript/ql/src/Security/CWE-834/examples/LoopBoundInjection_fixed.js‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
var express = require('express');
2+
var app = express();
3+
4+
app.post("/foo", (req, res) => {
5+
var obj = req.body;
6+
7+
if (!(obj instanceof Array)) { // Prevents DoS.
8+
return [];
9+
}
10+
11+
var ret = [];
12+
13+
for (var i = 0; i < obj.length; i++) {
14+
ret.push(obj[i]);
15+
}
16+
});

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

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
/**
2+
* Provides a taint tracking configuration for reasoning about DoS attacks
3+
* using a user-controlled object with an unbounded .length property.
4+
*
5+
* Note, for performance reasons: only import this file if
6+
* `LoopBoundInjection::Configuration` is needed, otherwise
7+
* `LoopBoundInjectionCustomizations` should be imported instead.
8+
*/
9+
10+
import javascript
11+
import semmle.javascript.security.TaintedObject
12+
13+
module LoopBoundInjection {
14+
import LoopBoundInjectionCustomizations::LoopBoundInjection
15+
16+
/**
17+
* A taint tracking configuration for reasoning about looping on tainted objects with unbounded length.
18+
*/
19+
class Configuration extends TaintTracking::Configuration {
20+
Configuration() { this = "LoopBoundInjection" }
21+
22+
override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
23+
source instanceof Source and label = TaintedObject::label()
24+
}
25+
26+
override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
27+
sink instanceof Sink and label = TaintedObject::label()
28+
}
29+
30+
override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
31+
guard instanceof TaintedObject::SanitizerGuard or
32+
guard instanceof IsArraySanitizerGuard or
33+
guard instanceof InstanceofArraySanitizerGuard or
34+
guard instanceof LengthCheckSanitizerGuard
35+
}
36+
37+
override predicate isAdditionalFlowStep(
38+
DataFlow::Node src, DataFlow::Node trg, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl
39+
) {
40+
TaintedObject::step(src, trg, inlbl, outlbl)
41+
}
42+
}
43+
}

0 commit comments

Comments
 (0)