From 905d9045435f90ebf92d29bc44679e05475ff1b9 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 21 Jan 2025 09:40:24 +0100 Subject: [PATCH 1/5] add a few failing tests --- .../IncorrectSuffixCheck.expected | 2 ++ .../Security/CWE-020/IncorrectSuffixCheck/tst.js | 12 ++++++++++++ 2 files changed, 14 insertions(+) diff --git a/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck/IncorrectSuffixCheck.expected b/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck/IncorrectSuffixCheck.expected index 92e2f5968b87..b6b0b2b1b367 100644 --- a/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck/IncorrectSuffixCheck.expected +++ b/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck/IncorrectSuffixCheck.expected @@ -9,3 +9,5 @@ | 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. | | tst.js:80:10:80:57 | x.index ... th + 1) | This suffix check is missing a length comparison to correctly handle indexOf returning -1. | +| tst.js:105:23:105:80 | ind === ... gth - 1 | This suffix check is missing a length comparison to correctly handle indexOf returning -1. | +| tst.js:110:65:110:164 | trusted ... 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/IncorrectSuffixCheck/tst.js b/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck/tst.js index 9ef9fa87ee60..55fd5869b10d 100644 --- a/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck/tst.js @@ -97,3 +97,15 @@ function lastIndexNeqMinusOne(x) { function lastIndexEqMinusOne(x) { return x.lastIndexOf("example.com") === -1 || x.lastIndexOf("example.com") === x.length - "example.com".length; // OK } + +function sameCheck(allowedOrigin) { + const trustedAuthority = "example.com"; + + const ind = trustedAuthority.indexOf("." + allowedOrigin); + return ind > 0 && ind === trustedAuthority.length - allowedOrigin.length - 1; // OK - but currently failing +} + +function sameConcatenation(allowedOrigin) { + const trustedAuthority = "example.com"; + return trustedAuthority.indexOf("." + allowedOrigin) > 0 && trustedAuthority.indexOf("." + allowedOrigin) === trustedAuthority.length - allowedOrigin.length - 1; // OK - but currently failing +} \ No newline at end of file From d5529e3a7ec24f6565bf26f9e03e54af1da6a05c Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 21 Jan 2025 09:42:30 +0100 Subject: [PATCH 2/5] ensure an `indexOf` call is equivalent with itself. (`getAUse()` is used later to find matching `indexOf` calls) --- javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.ql | 2 ++ .../CWE-020/IncorrectSuffixCheck/IncorrectSuffixCheck.expected | 1 - .../query-tests/Security/CWE-020/IncorrectSuffixCheck/tst.js | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.ql b/javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.ql index 650b71dd62f7..d54fa398d99f 100644 --- a/javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.ql +++ b/javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.ql @@ -44,6 +44,8 @@ class IndexOfCall extends DataFlow::MethodCallNode { * Gets an `indexOf` call with the same receiver, argument, and method name, including this call itself. */ IndexOfCall getAnEquivalentIndexOfCall() { + result = this + or exists(DataFlow::Node recv, string m | this.receiverAndMethodName(recv, m) and result.receiverAndMethodName(recv, m) | diff --git a/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck/IncorrectSuffixCheck.expected b/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck/IncorrectSuffixCheck.expected index b6b0b2b1b367..d5ffdd6788b2 100644 --- a/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck/IncorrectSuffixCheck.expected +++ b/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck/IncorrectSuffixCheck.expected @@ -9,5 +9,4 @@ | 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. | | tst.js:80:10:80:57 | x.index ... th + 1) | This suffix check is missing a length comparison to correctly handle indexOf returning -1. | -| tst.js:105:23:105:80 | ind === ... gth - 1 | This suffix check is missing a length comparison to correctly handle indexOf returning -1. | | tst.js:110:65:110:164 | trusted ... 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/IncorrectSuffixCheck/tst.js b/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck/tst.js index 55fd5869b10d..e6cd668a67c8 100644 --- a/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck/tst.js @@ -102,7 +102,7 @@ function sameCheck(allowedOrigin) { const trustedAuthority = "example.com"; const ind = trustedAuthority.indexOf("." + allowedOrigin); - return ind > 0 && ind === trustedAuthority.length - allowedOrigin.length - 1; // OK - but currently failing + return ind > 0 && ind === trustedAuthority.length - allowedOrigin.length - 1; // OK } function sameConcatenation(allowedOrigin) { From 17afab7d0f0e101e4b8a1a2fcf8e26019f50d1ae Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 21 Jan 2025 09:43:57 +0100 Subject: [PATCH 3/5] support that two `indexOf()` calls use the same string-concatenation in `getAnEquivalentIndexOfCall()` --- .../ql/src/Security/CWE-020/IncorrectSuffixCheck.ql | 11 +++++++++++ .../IncorrectSuffixCheck.expected | 1 - .../Security/CWE-020/IncorrectSuffixCheck/tst.js | 2 +- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.ql b/javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.ql index d54fa398d99f..b69a79101e20 100644 --- a/javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.ql +++ b/javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.ql @@ -49,9 +49,20 @@ class IndexOfCall extends DataFlow::MethodCallNode { exists(DataFlow::Node recv, string m | this.receiverAndMethodName(recv, m) and result.receiverAndMethodName(recv, m) | + // both directly reference the same value result.getArgument(0).getALocalSource() = this.getArgument(0).getALocalSource() or + // both use the same string literal result.getArgument(0).getStringValue() = this.getArgument(0).getStringValue() + or + // both use the same concatenation of a string and a value + exists(Expr origin, StringLiteral str, AddExpr otherAdd | + this.getArgument(0).asExpr().(AddExpr).hasOperands(origin, str) and + otherAdd = result.getArgument(0).asExpr().(AddExpr) + | + otherAdd.getAnOperand().(StringLiteral).getStringValue() = str.getStringValue() and + otherAdd.getAnOperand().flow().getALocalSource() = origin.flow().getALocalSource() + ) ) } diff --git a/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck/IncorrectSuffixCheck.expected b/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck/IncorrectSuffixCheck.expected index d5ffdd6788b2..92e2f5968b87 100644 --- a/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck/IncorrectSuffixCheck.expected +++ b/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck/IncorrectSuffixCheck.expected @@ -9,4 +9,3 @@ | 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. | | tst.js:80:10:80:57 | x.index ... th + 1) | This suffix check is missing a length comparison to correctly handle indexOf returning -1. | -| tst.js:110:65:110:164 | trusted ... 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/IncorrectSuffixCheck/tst.js b/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck/tst.js index e6cd668a67c8..f50c014b1853 100644 --- a/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck/tst.js @@ -107,5 +107,5 @@ function sameCheck(allowedOrigin) { function sameConcatenation(allowedOrigin) { const trustedAuthority = "example.com"; - return trustedAuthority.indexOf("." + allowedOrigin) > 0 && trustedAuthority.indexOf("." + allowedOrigin) === trustedAuthority.length - allowedOrigin.length - 1; // OK - but currently failing + return trustedAuthority.indexOf("." + allowedOrigin) > 0 && trustedAuthority.indexOf("." + allowedOrigin) === trustedAuthority.length - allowedOrigin.length - 1; // OK } \ No newline at end of file From 2f1bd75ee9230a382f24405c4f1e9d02b97e70a9 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 21 Jan 2025 09:51:14 +0100 Subject: [PATCH 4/5] remove redundant cast --- javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.ql b/javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.ql index b69a79101e20..23c080f1535a 100644 --- a/javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.ql +++ b/javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.ql @@ -58,7 +58,7 @@ class IndexOfCall extends DataFlow::MethodCallNode { // both use the same concatenation of a string and a value exists(Expr origin, StringLiteral str, AddExpr otherAdd | this.getArgument(0).asExpr().(AddExpr).hasOperands(origin, str) and - otherAdd = result.getArgument(0).asExpr().(AddExpr) + otherAdd = result.getArgument(0).asExpr() | otherAdd.getAnOperand().(StringLiteral).getStringValue() = str.getStringValue() and otherAdd.getAnOperand().flow().getALocalSource() = origin.flow().getALocalSource() From 04bbd5919aae45ccc8cd9939e94a21c42edd16dd Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Wed, 22 Jan 2025 10:16:11 +0100 Subject: [PATCH 5/5] add change-note --- .../ql/src/change-notes/2025-01-22-indexof-suffix-check.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 javascript/ql/src/change-notes/2025-01-22-indexof-suffix-check.md diff --git a/javascript/ql/src/change-notes/2025-01-22-indexof-suffix-check.md b/javascript/ql/src/change-notes/2025-01-22-indexof-suffix-check.md new file mode 100644 index 000000000000..b8aa44faff54 --- /dev/null +++ b/javascript/ql/src/change-notes/2025-01-22-indexof-suffix-check.md @@ -0,0 +1,4 @@ +--- +category: majorAnalysis +--- +* The `js/incorrect-suffix-check` query now recognises some good patterns of the form `origin.indexOf("." + allowedOrigin)` that were previously falsely flagged. \ No newline at end of file