From 374f7ab65dc5cfaf7545fc22bc076fee3508e409 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 30 Nov 2018 11:29:05 +0000 Subject: [PATCH] JS: address comments --- .../CWE-020/IncorrectSuffixCheck.qhelp | 6 +++--- .../Security/CWE-020/IncorrectSuffixCheck.ql | 21 ++++++++++++------- .../CWE-020/IncorrectSuffixCheck.expected | 1 + .../test/query-tests/Security/CWE-020/tst.js | 5 +++++ 4 files changed, 22 insertions(+), 11 deletions(-) diff --git a/javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.qhelp b/javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.qhelp index 871db659a5d..2c2818d2d5f 100644 --- a/javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.qhelp +++ b/javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.qhelp @@ -8,7 +8,7 @@ The indexOf and lastIndexOf methods are sometimes used to check if a substring occurs at a certain position in a string. However, if the returned index - is compared to an expression that might evaluate to -1, the check can may pass in some + is compared to an expression that might evaluate to -1, the check may pass in some cases where the substring was not found at all.

@@ -26,7 +26,7 @@ Use String.prototype.endsWith if it is available. Otherwise, explicitly handle the -1 case, either by checking the relative - lengths of the strings, or check if the returned index is -1. + lengths of the strings, or by checking if the returned index is -1.

@@ -47,7 +47,7 @@ However, if y is one character longer than x, the right-hand side x.length - y.length becomes -1, which then equals the return value - of lastIndexOf. This will make the test pass, evne though x does not + of lastIndexOf. This will make the test pass, even though x does not end with y.

diff --git a/javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.ql b/javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.ql index 4e350928455..fbbff53c3c9 100644 --- a/javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.ql +++ b/javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.ql @@ -36,10 +36,17 @@ class IndexOfCall extends DataFlow::MethodCallNode { result.getArgument(0).getALocalSource() = this.getArgument(0).getALocalSource() and result.getMethodName() = this.getMethodName() } + + /** + * Gets an expression that refers to the return value of this call. + */ + Expr getAUse() { + this.flowsToExpr(result) + } } /** - * Gets a source of the given string value. + * Gets a source of the given string value, or one of its operands if it is a concatenation. */ DataFlow::SourceNode getStringSource(DataFlow::Node node) { result = node.getALocalSource() @@ -65,7 +72,7 @@ class LiteralLengthExpr extends DotExpr { } /** - * Holds if `node` is derived from the length of the given `indexOf`-operand. + * Holds if `length` is derived from the length of the given `indexOf`-operand. */ predicate isDerivedFromLength(DataFlow::Node length, DataFlow::Node operand) { exists (IndexOfCall call | operand = call.getAnOperand() | @@ -84,9 +91,7 @@ predicate isDerivedFromLength(DataFlow::Node length, DataFlow::Node operand) { length = lengthExpr.flow()) ) or - exists (DataFlow::Node mid | - isDerivedFromLength(mid, operand) and - length = mid.getASuccessor()) + isDerivedFromLength(length.getAPredecessor(), operand) or exists (SubExpr sub | isDerivedFromLength(sub.getAnOperand().flow(), operand) and @@ -101,7 +106,7 @@ class UnsafeIndexOfComparison extends EqualityTest { DataFlow::Node testedValue; UnsafeIndexOfComparison() { - hasOperands(indexOf.asExpr(), testedValue.asExpr()) and + hasOperands(indexOf.getAUse(), testedValue.asExpr()) and isDerivedFromLength(testedValue, indexOf.getReceiver()) and isDerivedFromLength(testedValue, indexOf.getArgument(0)) and @@ -118,13 +123,13 @@ class UnsafeIndexOfComparison extends EqualityTest { // Check for indexOf being -1 not exists (EqualityTest test, Expr minusOne | - test.hasOperands(indexOf.getAnEquivalentIndexOfCall().asExpr(), minusOne) and + test.hasOperands(indexOf.getAnEquivalentIndexOfCall().getAUse(), minusOne) and minusOne.getIntValue() = -1 ) and // Check for indexOf being >1, or >=0, etc not exists (RelationalComparison test | - test.getGreaterOperand() = indexOf.getAnEquivalentIndexOfCall().asExpr() and + test.getGreaterOperand() = indexOf.getAnEquivalentIndexOfCall().getAUse() and exists (int value | value = test.getLesserOperand().getIntValue() | value >= 0 or diff --git a/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck.expected b/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck.expected index cdf16835876..3c44bc28637 100644 --- a/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck.expected +++ b/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck.expected @@ -7,3 +7,4 @@ | tst.js:39:10:39:49 | x.lastI ... .length | This suffix check is missing a length comparison to correctly handle lastIndexOf returning -1. | | tst.js:55:32:55:71 | x.index ... gth - 1 | This suffix check is missing a length comparison to correctly handle indexOf returning -1. | | tst.js:67:32:67:71 | x.index ... gth - 1 | This suffix check is missing a length comparison to correctly handle indexOf returning -1. | +| tst.js:76:25:76:57 | index = ... gth - 1 | This suffix check is missing a length comparison to correctly handle indexOf returning -1. | diff --git a/javascript/ql/test/query-tests/Security/CWE-020/tst.js b/javascript/ql/test/query-tests/Security/CWE-020/tst.js index 4b3cd811895..6115cf16740 100644 --- a/javascript/ql/test/query-tests/Security/CWE-020/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-020/tst.js @@ -70,3 +70,8 @@ function indexOfCheckBad(x, y) { function endsWithSlash(x) { return x.indexOf("/") === x.length - 1; // OK - even though it also matches the empty string } + +function withIndexOfCheckBad(x, y) { + let index = x.indexOf(y); + return index !== 0 && index === x.length - y.length - 1; // NOT OK +}