mirror of
https://github.com/github/codeql.git
synced 2026-05-03 12:45:27 +02:00
changes based on review from max
This commit is contained in:
Erik Krogh Kristensen
@@ -4,48 +4,43 @@
|
||||
<qhelp>
|
||||
|
||||
<overview>
|
||||
<p>
|
||||
Iterating the elements of an untrusted object using the
|
||||
<code>.length</code> property can lead to a server looping
|
||||
indefinitely. This looping causes a denial-of-service or DoS by
|
||||
causing the server to hang or run out of memory.
|
||||
This happens when the server expects an array but an attacker sends
|
||||
a regular JSON object with a huge number in the
|
||||
<code>.length</code> property, such as <code>{length: 1e100}</code>,
|
||||
that the server then loops through.
|
||||
</p>
|
||||
<p>
|
||||
Using the .length property of an untrusted object as a loop bound may
|
||||
cause indefinite looping since a malicious attacker can set the
|
||||
<code>.length</code> property to a very large number. For example,
|
||||
when a program that expects an array is passed a JSON object such as
|
||||
<code>{length: 1e100}</code>, the loop will be run for 1e100
|
||||
iterations. This may cause the program to hang or run out of memory,
|
||||
which can be used to mount a denial-of-service (DoS) attack.
|
||||
</p>
|
||||
</overview>
|
||||
|
||||
<recommendation>
|
||||
<p>
|
||||
Either force the user controlled object to be an array or limit the
|
||||
size of the <code>.length</code> property.
|
||||
</p>
|
||||
<p>
|
||||
Either check that the object is indeed an array or limit the
|
||||
size of the <code>.length</code> property.
|
||||
</p>
|
||||
</recommendation>
|
||||
|
||||
<example>
|
||||
<p>
|
||||
In the example below, a server iterates over a user controlled object
|
||||
<code>obj</code> using the <code>obj.length</code> property in order
|
||||
to copy the elements from <code>obj</code> to an array.
|
||||
</p>
|
||||
<p>
|
||||
In the example below, an HTTP request handler iterates over a
|
||||
user-controlled object <code>obj</code> using the
|
||||
<code>obj.length</code> property in order to copy the elements from
|
||||
<code>obj</code> to an array.
|
||||
</p>
|
||||
|
||||
<sample src="examples/TaintedLength.js"/>
|
||||
<sample src="examples/TaintedLength.js" />
|
||||
|
||||
<p>
|
||||
This is not secure since an attacker can control the value of
|
||||
<code>obj.length</code>, and thereby cause the loop to loop
|
||||
indefinitely.
|
||||
Here the potential DoS is fixed by enforcing that the user controlled
|
||||
object is an array.
|
||||
</p>
|
||||
<p>
|
||||
This is not secure since an attacker can control the value of
|
||||
<code>obj.length</code>, and thereby cause the loop to iterate
|
||||
indefinitely. Here the potential DoS is fixed by enforcing that
|
||||
the user controlled object is an array.
|
||||
</p>
|
||||
|
||||
<sample src="examples/TaintedLength_fixed.js"/>
|
||||
<sample src="examples/TaintedLength_fixed.js" />
|
||||
</example>
|
||||
|
||||
<references>
|
||||
<li>CWE entry:
|
||||
<a href="https://cwe.mitre.org/data/definitions/834.html">CWE-834</a>
|
||||
</li>
|
||||
</references>
|
||||
<references></references>
|
||||
</qhelp>
|
||||
|
||||
@@ -1,22 +1,20 @@
|
||||
/**
|
||||
* @name Tainted .length in loop condition
|
||||
* @description If server-side code iterates over a user-controlled object with
|
||||
* an arbitrary .length value an attacker can trick the server
|
||||
* to loop infinitely.
|
||||
* @description Iterating over an object with a user-controlled .length
|
||||
* property can cause indefinite looping
|
||||
* @kind path-problem
|
||||
* @problem.severity warning
|
||||
* @id js/loop-bound-injection
|
||||
* @tags security
|
||||
* external/cwe/cwe-834
|
||||
* @precision medium
|
||||
*/
|
||||
|
||||
import javascript
|
||||
|
||||
import semmle.javascript.security.dataflow.TaintedLength::TaintedLength
|
||||
|
||||
from
|
||||
Configuration dataflow, DataFlow::PathNode source, DataFlow::PathNode sink
|
||||
from Configuration dataflow, DataFlow::PathNode source, DataFlow::PathNode sink
|
||||
where dataflow.hasFlowPath(source, sink)
|
||||
select sink, source, sink,
|
||||
"Iterating over user controlled object with an unbounded .length property $@.",
|
||||
"Iterating over user-controlled object with a potentially unbounded .length property from $@.",
|
||||
source, "here"
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
/**
|
||||
* Provides a taint tracking configuration for reasoning about DOS attacks
|
||||
* using a user controlled object with an unbounded .length property.
|
||||
* Provides a taint tracking configuration for reasoning about DoS attacks
|
||||
* using a user controlled object with an unbounded .length property.
|
||||
*
|
||||
* Note, for performance reasons: only import this file if
|
||||
* `TaintedLength::Configuration` is needed, otherwise
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
/**
|
||||
* Provides default sources, sinks and sanitisers for reasoning about
|
||||
* DOS attacks using objects with unbounded length property.
|
||||
* As well as extension points for adding your own.
|
||||
* DoS attacks using objects with unbounded length property,
|
||||
* as well as extension points for adding your own.
|
||||
*/
|
||||
|
||||
import javascript
|
||||
@@ -30,48 +30,45 @@ module TaintedLength {
|
||||
}
|
||||
|
||||
/**
|
||||
* A loop that iterates through some array using the `length` property.
|
||||
* The loop is either of the style `for(..; i < arr.length;...)` or `while(i < arr.length) {..;i++;..}`.
|
||||
* A loop that iterates through some array using the `length` property.
|
||||
* The loop is either of the style `for(..; i < arr.length;...)` or `while(i < arr.length) {..;i++;..}`.
|
||||
*/
|
||||
class ArrayIterationLoop extends Stmt {
|
||||
LocalVariable indexVariable;
|
||||
|
||||
LoopStmt loop;
|
||||
|
||||
DataFlow::PropRead lengthRead;
|
||||
|
||||
ArrayIterationLoop() {
|
||||
this = loop and
|
||||
this = loop and
|
||||
exists(RelationalComparison compare |
|
||||
compare = loop.getTest() and
|
||||
compare.getLesserOperand() = indexVariable.getAnAccess() and
|
||||
lengthRead.getPropertyName() = "length" and
|
||||
lengthRead.flowsToExpr(compare.getGreaterOperand())
|
||||
) and
|
||||
(
|
||||
loop.(ForStmt).getUpdate().(IncExpr).getOperand() = indexVariable.getAnAccess() or
|
||||
loop.getBody().getAChild*().(IncExpr).getOperand() = indexVariable.getAnAccess()
|
||||
exists(IncExpr inc | inc.getOperand() = indexVariable.getAnAccess() |
|
||||
inc = loop.(ForStmt).getUpdate()
|
||||
or
|
||||
inc.getEnclosingStmt().getParentStmt*() = loop.getBody()
|
||||
)
|
||||
}
|
||||
|
||||
|
||||
/**
|
||||
* Gets the length read in the loop test
|
||||
*/
|
||||
DataFlow::PropRead getLengthRead() {
|
||||
result = lengthRead
|
||||
}
|
||||
|
||||
DataFlow::PropRead getLengthRead() { result = lengthRead }
|
||||
|
||||
/**
|
||||
* Gets the loop test of this loop.
|
||||
*/
|
||||
Expr getTest() {
|
||||
result = loop.getTest()
|
||||
}
|
||||
|
||||
Expr getTest() { result = loop.getTest() }
|
||||
|
||||
/**
|
||||
* Gets the body of this loop.
|
||||
*/
|
||||
Stmt getBody() {
|
||||
result = loop.getBody()
|
||||
}
|
||||
Stmt getBody() { result = loop.getBody() }
|
||||
|
||||
/**
|
||||
* Gets the variable holding the loop variable and current array index.
|
||||
@@ -91,13 +88,10 @@ module TaintedLength {
|
||||
LoopSink() {
|
||||
exists(ArrayIterationLoop loop |
|
||||
this = loop.getLengthRead().getBase() and
|
||||
|
||||
// In the DOS we are looking for arrayRead will evaluate to undefined,
|
||||
// this may cause an exception to be thrown, thus bailing out of the loop.
|
||||
// A DoS cannot happen if such an exception is thrown.
|
||||
// In the DoS we are looking for arrayRead will evaluate to undefined,
|
||||
// this may cause an exception to be thrown, thus bailing out of the loop.
|
||||
// A DoS cannot happen if such an exception is thrown.
|
||||
not exists(DataFlow::PropRead arrayRead, Expr throws |
|
||||
loop.getBody().getAChild*() = arrayRead.asExpr() and
|
||||
loop.getBody().getAChild*() = throws and
|
||||
arrayRead.getPropertyNameExpr() = loop.getIndexVariable().getAnAccess() and
|
||||
arrayRead.flowsToExpr(throws) and
|
||||
isCrashingWithNullValues(throws)
|
||||
@@ -105,19 +99,19 @@ module TaintedLength {
|
||||
// The existence of some kind of early-exit usually indicates that the loop will stop early and no DOS happens.
|
||||
not exists(BreakStmt br | br.getTarget() = loop) and
|
||||
not exists(ReturnStmt ret |
|
||||
loop.getBody().getAChild*() = ret and
|
||||
ret.getParentStmt*() = loop.getBody() and
|
||||
ret.getContainer() = loop.getContainer()
|
||||
) and
|
||||
not exists(ThrowStmt throw |
|
||||
loop.getBody().getAChild*() = throw and
|
||||
not loop.getBody().getAChild*() = throw.getTarget()
|
||||
loop.getBody() = throw.getParentStmt*() and
|
||||
not loop.getBody() = throw.getTarget().getParent*()
|
||||
)
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if `name` is a method from lodash vulnerable to a DOS attack if called with a tained object.
|
||||
* Holds if `name` is a method from lodash vulnerable to a DOS attack if called with a tained object.
|
||||
*/
|
||||
predicate loopableLodashMethod(string name) {
|
||||
name = "chunk" or
|
||||
@@ -183,8 +177,8 @@ module TaintedLength {
|
||||
}
|
||||
|
||||
/**
|
||||
* A method call to a lodash method that iterates over an array-like structure,
|
||||
* such as `_.filter(sink, ...)`.
|
||||
* A method call to a lodash method that iterates over an array-like structure,
|
||||
* such as `_.filter(sink, ...)`.
|
||||
*/
|
||||
private class LodashIterationSink extends Sink {
|
||||
DataFlow::CallNode call;
|
||||
@@ -194,7 +188,6 @@ module TaintedLength {
|
||||
loopableLodashMethod(name) and
|
||||
call = LodashUnderscore::member(name).getACall() and
|
||||
call.getArgument(0) = this and
|
||||
|
||||
// Here it is just assumed that the array element is the first parameter in the callback function.
|
||||
not exists(DataFlow::FunctionNode func, DataFlow::ParameterNode e |
|
||||
func.flowsTo(call.getAnArgument()) and
|
||||
@@ -203,14 +196,12 @@ module TaintedLength {
|
||||
// Looking for obvious null-pointers happening on the array elements in the iteration.
|
||||
// Similar to what is done in the loop iteration sink.
|
||||
exists(Expr throws |
|
||||
throws = func.asExpr().(Function).getBody().getAChild*() and
|
||||
e.flowsToExpr(throws) and
|
||||
isCrashingWithNullValues(throws)
|
||||
)
|
||||
or
|
||||
// similar to the loop sink - the existence of an early-exit usually means that no DOS can happen.
|
||||
exists(ThrowStmt throw |
|
||||
throw = func.asExpr().(Function).getBody().getAChild*() and
|
||||
throw.getTarget() = func.asExpr()
|
||||
)
|
||||
)
|
||||
@@ -218,24 +209,23 @@ module TaintedLength {
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
/**
|
||||
* A source of objects that can cause DOS if looped over.
|
||||
* A source of objects that can cause DOS if looped over.
|
||||
*/
|
||||
abstract class Source extends DataFlow::Node { }
|
||||
|
||||
|
||||
/**
|
||||
* A source of remote user input objects.
|
||||
*/
|
||||
class TaintedObjectSource extends Source {
|
||||
TaintedObjectSource() { this instanceof TaintedObject::Source }
|
||||
}
|
||||
|
||||
|
||||
/**
|
||||
* A sanitizer that blocks taint flow if the array is checked to be an array using an `isArray` method.
|
||||
*/
|
||||
class IsArraySanitizerGuard extends TaintTracking::LabeledSanitizerGuardNode,
|
||||
DataFlow::ValueNode {
|
||||
class IsArraySanitizerGuard extends TaintTracking::LabeledSanitizerGuardNode, DataFlow::ValueNode {
|
||||
override CallExpr astNode;
|
||||
|
||||
IsArraySanitizerGuard() { astNode.getCalleeName() = "isArray" }
|
||||
@@ -268,8 +258,8 @@ module TaintedLength {
|
||||
|
||||
/**
|
||||
* A sanitizer that blocks taint flow if the length of an array is limited.
|
||||
*
|
||||
* Also implicitly makes sure that only the first DOS-prone loop is selected by the query. (as the .length test has outcome=false when exiting the loop).
|
||||
*
|
||||
* Also implicitly makes sure that only the first DoS-prone loop is selected by the query. (as the .length test has outcome=false when exiting the loop).
|
||||
*/
|
||||
class LengthCheckSanitizerGuard extends TaintTracking::LabeledSanitizerGuardNode,
|
||||
DataFlow::ValueNode {
|
||||
|
||||
@@ -46,12 +46,12 @@ edges
|
||||
| TaintedLengthLodash.js:9:13:9:20 | req.body | TaintedLengthLodash.js:12:18:12:20 | val |
|
||||
| TaintedLengthLodash.js:12:18:12:20 | val | TaintedLengthLodash.js:13:13:13:15 | val |
|
||||
#select
|
||||
| TaintedLengthBad.js:20:25:20:27 | val | TaintedLengthBad.js:8:13:8:20 | req.body | TaintedLengthBad.js:20:25:20:27 | val | Iterating over user controlled object with an unbounded .length property $@. | TaintedLengthBad.js:8:13:8:20 | req.body | here |
|
||||
| TaintedLengthBad.js:29:16:29:18 | val | TaintedLengthBad.js:10:15:10:22 | req.body | TaintedLengthBad.js:29:16:29:18 | val | Iterating over user controlled object with an unbounded .length property $@. | TaintedLengthBad.js:10:15:10:22 | req.body | here |
|
||||
| TaintedLengthBad.js:38:15:38:17 | val | TaintedLengthBad.js:12:25:12:32 | req.body | TaintedLengthBad.js:38:15:38:17 | val | Iterating over user controlled object with an unbounded .length property $@. | TaintedLengthBad.js:12:25:12:32 | req.body | here |
|
||||
| TaintedLengthBad.js:51:25:51:27 | val | TaintedLengthBad.js:14:19:14:26 | req.body | TaintedLengthBad.js:51:25:51:27 | val | Iterating over user controlled object with an unbounded .length property $@. | TaintedLengthBad.js:14:19:14:26 | req.body | here |
|
||||
| TaintedLengthExitBad.js:20:22:20:24 | val | TaintedLengthExitBad.js:8:9:8:16 | req.body | TaintedLengthExitBad.js:20:22:20:24 | val | Iterating over user controlled object with an unbounded .length property $@. | TaintedLengthExitBad.js:8:9:8:16 | req.body | here |
|
||||
| TaintedLengthExitBad.js:34:22:34:24 | val | TaintedLengthExitBad.js:10:9:10:16 | req.body | TaintedLengthExitBad.js:34:22:34:24 | val | Iterating over user controlled object with an unbounded .length property $@. | TaintedLengthExitBad.js:10:9:10:16 | req.body | here |
|
||||
| TaintedLengthExitBad.js:49:22:49:24 | val | TaintedLengthExitBad.js:12:10:12:17 | req.body | TaintedLengthExitBad.js:49:22:49:24 | val | Iterating over user controlled object with an unbounded .length property $@. | TaintedLengthExitBad.js:12:10:12:17 | req.body | here |
|
||||
| TaintedLengthExitBad.js:60:8:60:10 | val | TaintedLengthExitBad.js:14:14:14:21 | req.body | TaintedLengthExitBad.js:60:8:60:10 | val | Iterating over user controlled object with an unbounded .length property $@. | TaintedLengthExitBad.js:14:14:14:21 | req.body | here |
|
||||
| TaintedLengthLodash.js:13:13:13:15 | val | TaintedLengthLodash.js:9:13:9:20 | req.body | TaintedLengthLodash.js:13:13:13:15 | val | Iterating over user controlled object with an unbounded .length property $@. | TaintedLengthLodash.js:9:13:9:20 | req.body | here |
|
||||
| TaintedLengthBad.js:20:25:20:27 | val | TaintedLengthBad.js:8:13:8:20 | req.body | TaintedLengthBad.js:20:25:20:27 | val | Iterating over user-controlled object with a potentially unbounded .length property from $@. | TaintedLengthBad.js:8:13:8:20 | req.body | here |
|
||||
| TaintedLengthBad.js:29:16:29:18 | val | TaintedLengthBad.js:10:15:10:22 | req.body | TaintedLengthBad.js:29:16:29:18 | val | Iterating over user-controlled object with a potentially unbounded .length property from $@. | TaintedLengthBad.js:10:15:10:22 | req.body | here |
|
||||
| TaintedLengthBad.js:38:15:38:17 | val | TaintedLengthBad.js:12:25:12:32 | req.body | TaintedLengthBad.js:38:15:38:17 | val | Iterating over user-controlled object with a potentially unbounded .length property from $@. | TaintedLengthBad.js:12:25:12:32 | req.body | here |
|
||||
| TaintedLengthBad.js:51:25:51:27 | val | TaintedLengthBad.js:14:19:14:26 | req.body | TaintedLengthBad.js:51:25:51:27 | val | Iterating over user-controlled object with a potentially unbounded .length property from $@. | TaintedLengthBad.js:14:19:14:26 | req.body | here |
|
||||
| TaintedLengthExitBad.js:20:22:20:24 | val | TaintedLengthExitBad.js:8:9:8:16 | req.body | TaintedLengthExitBad.js:20:22:20:24 | val | Iterating over user-controlled object with a potentially unbounded .length property from $@. | TaintedLengthExitBad.js:8:9:8:16 | req.body | here |
|
||||
| TaintedLengthExitBad.js:34:22:34:24 | val | TaintedLengthExitBad.js:10:9:10:16 | req.body | TaintedLengthExitBad.js:34:22:34:24 | val | Iterating over user-controlled object with a potentially unbounded .length property from $@. | TaintedLengthExitBad.js:10:9:10:16 | req.body | here |
|
||||
| TaintedLengthExitBad.js:49:22:49:24 | val | TaintedLengthExitBad.js:12:10:12:17 | req.body | TaintedLengthExitBad.js:49:22:49:24 | val | Iterating over user-controlled object with a potentially unbounded .length property from $@. | TaintedLengthExitBad.js:12:10:12:17 | req.body | here |
|
||||
| TaintedLengthExitBad.js:60:8:60:10 | val | TaintedLengthExitBad.js:14:14:14:21 | req.body | TaintedLengthExitBad.js:60:8:60:10 | val | Iterating over user-controlled object with a potentially unbounded .length property from $@. | TaintedLengthExitBad.js:14:14:14:21 | req.body | here |
|
||||
| TaintedLengthLodash.js:13:13:13:15 | val | TaintedLengthLodash.js:9:13:9:20 | req.body | TaintedLengthLodash.js:13:13:13:15 | val | Iterating over user-controlled object with a potentially unbounded .length property from $@. | TaintedLengthLodash.js:9:13:9:20 | req.body | here |
|
||||
|
||||
Reference in New Issue
Block a user