Iterating the elements of an untrusted object using the
.length property can lead to a server looping
- indefinitely or crashing. This looping or crashing creates a
- denial-of-service or DOS.
- This happens when the server expects an array but an attacker creates
- a JSON object with an absurdly large number in the .length property
- that the server then loops through.
+ 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 an huge number in the
+ .length property, such as `{length: 1e100}`, that the
+ server then loops through.
obj.length, and thereby cause the loop to loop
indefinitely.
- Here the potential DOS is fixed by enforcing that the user controlled
+ Here the potential DoS is fixed by enforcing that the user controlled
object is an array.
diff --git a/javascript/ql/src/Security/CWE-834/examples/TaintedLength.js b/javascript/ql/src/Security/CWE-834/examples/TaintedLength.js
index 6e621d6b9be..1684d8eddb1 100644
--- a/javascript/ql/src/Security/CWE-834/examples/TaintedLength.js
+++ b/javascript/ql/src/Security/CWE-834/examples/TaintedLength.js
@@ -1,22 +1,13 @@
-'use strict';
-
var express = require('express');
-var router = new express.Router();
-var rootRoute = router.route('foobar');
+var app = express();
-rootRoute.post(function(req, res) {
+app.post("/foo", (req, res) => {
var obj = req.body;
- calcStuff(obj);
-});
-
-function calcStuff(obj) {
var ret = [];
- // potential DOS if obj.length is large.
+ // potential DoS if obj.length is large.
for (var i = 0; i < obj.length; i++) {
ret.push(obj[i]);
}
-
- return ret;
-}
\ No newline at end of file
+});
\ No newline at end of file
diff --git a/javascript/ql/src/Security/CWE-834/examples/TaintedLength_fixed.js b/javascript/ql/src/Security/CWE-834/examples/TaintedLength_fixed.js
index fce7ae9bde2..e710402e82f 100644
--- a/javascript/ql/src/Security/CWE-834/examples/TaintedLength_fixed.js
+++ b/javascript/ql/src/Security/CWE-834/examples/TaintedLength_fixed.js
@@ -1,24 +1,16 @@
-'use strict';
-
var express = require('express');
-var router = new express.Router();
-var rootRoute = router.route('foobar');
+var app = express();
-rootRoute.post(function(req, res) {
+app.post("/foo", (req, res) => {
var obj = req.body;
-
- calcStuff(obj);
-});
-
-function calcStuff(obj) {
+
if (!(obj instanceof Array)) { // prevents DOS
return [];
}
var ret = [];
+
for (var i = 0; i < obj.length; i++) {
ret.push(obj[i]);
}
-
- return ret;
-}
\ No newline at end of file
+});
\ No newline at end of file
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedLength.qll b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedLength.qll
index 657610b9049..b876e724ba0 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedLength.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedLength.qll
@@ -17,7 +17,7 @@ module TaintedLength {
* A taint-tracking configuration for reasoning about looping on tainted objects with unbounded length.
*/
class Configuration extends TaintTracking::Configuration {
- Configuration() { this = "LabeledLoopTaintedObject" }
+ Configuration() { this = "TaintedLength" }
override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
source instanceof Source and label = TaintedObject::label()
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedLengthCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedLengthCustomizations.qll
index fc3aad035da..05f627c43e2 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedLengthCustomizations.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedLengthCustomizations.qll
@@ -36,13 +36,14 @@ module TaintedLength {
class ArrayIterationLoop extends Stmt {
LocalVariable indexVariable;
LoopStmt loop;
+ DataFlow::PropRead lengthRead;
ArrayIterationLoop() {
this = loop and
- exists(RelationalComparison compare, DataFlow::PropRead lengthRead |
+ exists(RelationalComparison compare |
compare = loop.getTest() and
compare.getLesserOperand() = indexVariable.getAnAccess() and
- lengthRead.accesses(_, "length") and
+ lengthRead.getPropertyName() = "length" and
lengthRead.flowsToExpr(compare.getGreaterOperand())
) and
(
@@ -51,6 +52,13 @@ module TaintedLength {
)
}
+ /**
+ * Gets the length read in the loop test
+ */
+ DataFlow::PropRead getLengthRead() {
+ result = lengthRead
+ }
+
/**
* Gets the loop test of this loop.
*/
@@ -77,19 +85,17 @@ module TaintedLength {
abstract class Sink extends DataFlow::Node { }
/**
- * A loop that iterates over an array, such as `for (..; .. sink.length; ...) ...`
+ * An object that that is being iterated in a `for` loop, such as `for (..; .. sink.length; ...) ...`
*/
private class LoopSink extends Sink {
LoopSink() {
- exists(ArrayIterationLoop loop, Expr lengthAccess, DataFlow::PropRead lengthRead |
- loop.getTest().(RelationalComparison).getGreaterOperand() = lengthAccess and
- lengthRead.flowsToExpr(lengthAccess) and
- lengthRead.accesses(this, "length") and
+ exists(ArrayIterationLoop loop |
+ this = loop.getLengthRead().getBase() and
- // In the DOS we are looking for arrayRead will evaluate to undefined.
- // If an obvious nullpointer happens on this undefined, then the DOS cannot happen.
+ // 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 |
- // It doesn't only happen inside the for-loop, outside is also a sink (assuming it happens before).
loop.getBody().getAChild*() = arrayRead.asExpr() and
loop.getBody().getAChild*() = throws and
arrayRead.getPropertyNameExpr() = loop.getIndexVariable().getAnAccess() and
@@ -178,7 +184,7 @@ module TaintedLength {
/**
* A method call to a lodash method that iterates over an array-like structure,
- * such as `_.filter(sink, ...)`
+ * such as `_.filter(sink, ...)`.
*/
private class LodashIterationSink extends Sink {
DataFlow::CallNode call;
@@ -202,7 +208,7 @@ module TaintedLength {
isCrashingWithNullValues(throws)
)
or
- // similar to the loop sink - the existance of an early-exit usually means that no DOS can happen.
+ // 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()
@@ -214,7 +220,7 @@ module TaintedLength {
}
/**
- * A source of objects that can cause DOS is looped over.
+ * A source of objects that can cause DOS if looped over.
*/
abstract class Source extends DataFlow::Node { }