From 493a31d98d849c3decc2127bb9ea3525ded1bc8b Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Wed, 11 Sep 2019 12:53:59 +0100 Subject: [PATCH] more fixes based on review --- .../src/Security/CWE-834/TaintedLength.qhelp | 13 ++++---- .../CWE-834/examples/TaintedLength.js | 17 +++------- .../CWE-834/examples/TaintedLength_fixed.js | 18 +++-------- .../security/dataflow/TaintedLength.qll | 2 +- .../dataflow/TaintedLengthCustomizations.qll | 32 +++++++++++-------- 5 files changed, 36 insertions(+), 46 deletions(-) diff --git a/javascript/ql/src/Security/CWE-834/TaintedLength.qhelp b/javascript/ql/src/Security/CWE-834/TaintedLength.qhelp index bd1c9e0394c..f69a011b63f 100644 --- a/javascript/ql/src/Security/CWE-834/TaintedLength.qhelp +++ b/javascript/ql/src/Security/CWE-834/TaintedLength.qhelp @@ -7,11 +7,12 @@

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.

@@ -35,7 +36,7 @@ This is not secure since an attacker can control the value of 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 { }