JS: add IncorrectSuffixCheck query

change-notes/1.20/analysis-javascript.md

@@ -7,6 +7,7 @@
 | **Query**                                     | **Tags**                                             | **Purpose**                                                                                                                                                                 |
 |-----------------------------------------------|------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
 | Double escaping or unescaping (`js/double-escaping`) | correctness, security, external/cwe/cwe-116 | Highlights potential double escaping or unescaping of special characters, indicating a possible violation of [CWE-116](https://cwe.mitre.org/data/definitions/116.html). Results are shown on LGTM by default. |
+| Incorrect suffix check (`js/incorrect-suffix-check`) | correctness, security, external/cwe/cwe-020 | Highlights error-prone suffix checks based on `indexOf`, indicating a potential violation of [CWE-20](https://cwe.mitre.org/data/definitions/20.html). |
 | Useless comparison test (`js/useless-comparison-test`) | correctness | Highlights code that is unreachable due to a numeric comparison that is always true or always false. |
 
 ## Changes to existing queries

javascript/config/suites/javascript/security

@@ -1,5 +1,6 @@
 + semmlecode-javascript-queries/DOM/TargetBlank.ql: /Security/CWE/CWE-200
 + semmlecode-javascript-queries/Electron/EnablingNodeIntegration.ql: /Security/CWE/CWE-094
++ semmlecode-javascript-queries/Security/CWE-020/IncorrectSuffixCheck.ql: /Security/CWE/CWE-020
 + semmlecode-javascript-queries/Security/CWE-022/TaintedPath.ql: /Security/CWE/CWE-022
 + semmlecode-javascript-queries/Security/CWE-078/CommandInjection.ql: /Security/CWE/CWE-078
 + semmlecode-javascript-queries/Security/CWE-079/ReflectedXss.ql: /Security/CWE/CWE-079

javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.qhelp

@@ -0,0 +1,72 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+
+    <overview>
+        <p>
+
+        The <code>indexOf</code> and <code>lastIndexOf</code> 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
+        cases where the substring was not found at all.
+
+        </p>
+        <p>
+
+        Specifically, this can easily happen when implementing <code>endsWith</code> using
+        <code>indexOf</code>.
+
+        </p>
+    </overview>
+
+    <recommendation>
+
+        <p>
+
+            Use <code>String.prototype.endsWith</code> 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.
+
+        </p>
+
+    </recommendation>
+
+    <example>
+
+        <p>
+
+            The following example uses <code>lastIndexOf</code> to determine if the string <code>x</code>
+            ends with the string <code>y</code>:
+
+        </p>
+
+        <sample src="examples/IncorrectSuffixCheck.js"/>
+        
+        <p>
+
+            However, if <code>y</code> is one character longer than <code>x</code>, the right-hand side
+            <code>x.length - y.length</code> becomes -1, which then equals the return value
+            of <code>lastIndexOf</code>. This will make the test pass, evne though <code>x</code> does not
+            end with <code>y</code>.
+
+        </p>
+        
+        <p>
+
+            To avoid this, explicitly check for the -1 case:
+
+        </p>
+
+        <sample src="examples/IncorrectSuffixCheckGood.js"/>
+
+
+    </example>
+
+    <references>
+
+        <li>MDN: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/endsWith">String.prototype.endsWith</a></li>
+        <li>MDN: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/indexOf">String.prototype.indexOf</a></li>
+
+    </references>
+</qhelp>

javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.ql

@@ -0,0 +1,142 @@
+/**
+ * @name Incorrect suffix check
+ * @description Using indexOf to implement endsWith functionality is error prone if the -1 case is not explicitly handled.
+ * @kind problem
+ * @problem.severity error
+ * @precision high
+ * @id js/incorrect-suffix-check
+ * @tags security
+ *       correctness
+ *       external/cwe/cwe-020
+ */
+import javascript
+
+/**
+ * A call to `indexOf` or `lastIndexOf`.
+ */
+class IndexOfCall extends DataFlow::MethodCallNode {
+  IndexOfCall() {
+    exists (string name | name = getMethodName() |
+      name = "indexOf" or
+      name = "lastIndexOf") and
+    getNumArgument() = 1
+  }
+
+  /** Gets the receiver or argument of this call. */
+  DataFlow::Node getAnOperand() {
+    result = getReceiver() or
+    result = getArgument(0)
+  }
+  
+  /**
+   * Gets an `indexOf` call with the same receiver, argument, and method name, including this call itself.
+   */
+  IndexOfCall getAnEqualiventIndexOfCall() {
+    result.getReceiver().getALocalSource() = this.getReceiver().getALocalSource() and
+    result.getArgument(0).getALocalSource() = this.getArgument(0).getALocalSource() and
+    result.getMethodName() = this.getMethodName()
+  }
+}
+
+/**
+ * Gets a source of the given string value.
+ */
+DataFlow::SourceNode getStringSource(DataFlow::Node node) {
+  result = node.getALocalSource()
+  or
+  result = StringConcatenation::getAnOperand(node).getALocalSource()
+}
+
+/**
+ * An expression that takes the length of a string literal.
+ */
+class LiteralLengthExpr extends DotExpr {
+  LiteralLengthExpr() {
+    getPropertyName() = "length" and
+    getBase() instanceof StringLiteral
+  }
+
+  /**
+   * Gets the value of the string literal whose length is taken.
+   */
+  string getBaseValue() {
+    result = getBase().getStringValue()
+  }
+}
+
+/**
+ * Holds if `node` is derived from the length of the given `indexOf`-operand.
+ */
+predicate isDerivedFromLength(DataFlow::Node length, DataFlow::Node operand) {
+  exists (IndexOfCall call | operand = call.getAnOperand() |
+    length = getStringSource(operand).getAPropertyRead("length")
+    or
+    // Find a literal length with the same string constant
+    exists (LiteralLengthExpr lengthExpr |
+      lengthExpr.getContainer() = call.getContainer() and
+      lengthExpr.getBaseValue() = operand.asExpr().getStringValue() and
+      length = lengthExpr.flow())
+    or
+    // Find an integer constants that equals the length of string constant
+    exists (Expr lengthExpr |
+      lengthExpr.getContainer() = call.getContainer() and
+      lengthExpr.getIntValue() = operand.asExpr().getStringValue().length() and
+      length = lengthExpr.flow())
+  )
+  or
+  exists (DataFlow::Node mid |
+    isDerivedFromLength(mid, operand) and
+    length = mid.getASuccessor())
+  or
+  exists (SubExpr sub |
+    isDerivedFromLength(sub.getAnOperand().flow(), operand) and
+    length = sub.flow())
+}
+
+/**
+ * An equality comparison of form `A.indexOf(B) === A.length - B.length` or similar. 
+ */
+class UnsafeIndexOfComparison extends EqualityTest {
+  IndexOfCall indexOf;
+  DataFlow::Node testedValue;
+
+  UnsafeIndexOfComparison() {
+    hasOperands(indexOf.asExpr(), testedValue.asExpr()) and
+    isDerivedFromLength(testedValue, indexOf.getReceiver()) and
+    isDerivedFromLength(testedValue, indexOf.getArgument(0)) and
+
+    // Ignore cases like `x.indexOf("/") === x.length - 1` that can only be bypassed if `x` is the empty string.
+    // Sometimes strings are just known to be non-empty from the context, and it is unlikely to be a security issue,
+    // since it's obviously not a domain name check.
+    not indexOf.getArgument(0).mayHaveStringValue(any(string s | s.length() = 1)) and
+
+    // Relative string length comparison, such as A.length > B.length, or (A.length - B.length) > 0
+    not exists (RelationalComparison compare |
+      isDerivedFromLength(compare.getAnOperand().flow(), indexOf.getReceiver()) and
+      isDerivedFromLength(compare.getAnOperand().flow(), indexOf.getArgument(0))
+    ) and
+
+    // Check for indexOf being -1
+    not exists (EqualityTest test, Expr minusOne |
+      test.hasOperands(indexOf.getAnEqualiventIndexOfCall().asExpr(), minusOne) and
+      minusOne.getIntValue() = -1
+    ) and
+    
+    // Check for indexOf being >1, or >=0, etc
+    not exists (RelationalComparison test |
+      test.getGreaterOperand() = indexOf.getAnEqualiventIndexOfCall().asExpr() and
+      exists (int value | value = test.getLesserOperand().getIntValue() |
+        value >= 0
+        or
+        not test.isInclusive() and
+        value = -1)
+    )
+  }
+
+  IndexOfCall getIndexOf() {
+    result = indexOf
+  }
+}
+
+from UnsafeIndexOfComparison comparison
+select comparison, "This suffix check is missing a length comparison to correctly handle " + comparison.getIndexOf().getMethodName() + " returning -1."

javascript/ql/src/Security/CWE-020/examples/IncorrectSuffixCheck.js

@@ -0,0 +1,3 @@
+function endsWith(x, y) {
+  return x.lastIndexOf(y) === x.length - y.length;
+}

javascript/ql/src/Security/CWE-020/examples/IncorrectSuffixCheckGood.js

@@ -0,0 +1,4 @@
+function endsWith(x, y) {
+  let index = x.lastIndexOf(y);
+  return index !== -1 && index === x.length - y.length;
+}

javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck.expected

@@ -0,0 +1,9 @@
+| examples/IncorrectSuffixCheck.js:2:10:2:49 | x.lastI ... .length | This suffix check is missing a length comparison to correctly handle lastIndexOf returning -1. |
+| tst.js:2:10:2:45 | x.index ... .length | This suffix check is missing a length comparison to correctly handle indexOf returning -1. |
+| tst.js:9:10:9:55 | x.index ... gth - 1 | This suffix check is missing a length comparison to correctly handle indexOf returning -1. |
+| tst.js:17:10:17:31 | x.index ... = delta | This suffix check is missing a length comparison to correctly handle indexOf returning -1. |
+| tst.js:25:10:25:69 | x.index ... .length | This suffix check is missing a length comparison to correctly handle indexOf returning -1. |
+| tst.js:32:10:32:51 | x.index ... th - 11 | This suffix check is missing a length comparison to correctly handle indexOf returning -1. |
+| 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. |

javascript/ql/test/query-tests/Security/CWE-020/IncorrectSuffixCheck.qlref

@@ -0,0 +1 @@
+Security/CWE-020/IncorrectSuffixCheck.ql

javascript/ql/test/query-tests/Security/CWE-020/examples/IncorrectSuffixCheck.js

@@ -0,0 +1,3 @@
+function endsWith(x, y) {
+  return x.lastIndexOf(y) === x.length - y.length;
+}

javascript/ql/test/query-tests/Security/CWE-020/examples/IncorrectSuffixCheckGood.js

@@ -0,0 +1,4 @@
+function endsWith(x, y) {
+  let index = x.lastIndexOf(y);
+  return index !== -1 && index === x.length - y.length;
+}

javascript/ql/test/query-tests/Security/CWE-020/tst.js

@@ -0,0 +1,72 @@
+function endsWith(x, y) {
+  return x.indexOf(y) === x.length - y.length; // NOT OK
+}
+function endsWithGood(x, y) {
+  return x.length >= y.length && x.indexOf(y) === x.length - y.length; // OK
+}
+
+function withStringConcat(x, y) {
+  return x.indexOf("/" + y) === x.length - y.length - 1; // NOT OK
+}
+function withStringConcatGood(x, y) {
+  return x.length > y.length && x.indexOf("/" + y) === x.length - y.length - 1; // OK
+}
+
+function withDelta(x, y) {
+  let delta = x.length - y.length;
+  return x.indexOf(y) === delta; // NOT OK
+}
+function withDeltaGood(x, y) {
+  let delta = x.length - y.length;
+  return delta >= 0 && x.indexOf(y) === delta; // OK
+}
+
+function literal(x) {
+  return x.indexOf("example.com") === x.length - "example.com".length; // NOT OK
+}
+function literalGood(x) {
+  return x.length >= "example.com".length && x.indexOf("example.com") === x.length - "example.com".length;
+}
+
+function intLiteral(x) {
+  return x.indexOf("example.com") === x.length - 11; // NOT OK
+}
+function intLiteralGood(x) {
+  return x.length >= 11 && x.indexOf("example.com") === x.length - 11;
+}
+
+function lastIndexOf(x, y) {
+  return x.lastIndexOf(y) === x.length - y.length; // NOT OK
+}
+function lastIndexOfGood(x, y) {
+  return x.length >= y.length && x.lastIndexOf(y) === x.length - y.length; // OK
+}
+
+function withIndexOfCheckGood(x, y) {
+  let index = x.indexOf(y);
+  return index !== -1 && index === x.length - y.length - 1; // OK
+}
+
+function indexOfCheckEquality(x, y) {
+  return x.indexOf(y) !== -1 && x.indexOf(y) === x.length - y.length - 1; // OK
+}
+
+function indexOfCheckEqualityBad(x, y) {
+  return x.indexOf(y) !== 0 && x.indexOf(y) === x.length - y.length - 1; // NOT OK
+}
+
+function indexOfCheckGood(x, y) {
+  return x.indexOf(y) >= 0 && x.indexOf(y) === x.length - y.length - 1; // OK
+}
+
+function indexOfCheckGoodSharp(x, y) {
+  return x.indexOf(y) > -1 && x.indexOf(y) === x.length - y.length - 1; // OK
+}
+
+function indexOfCheckBad(x, y) {
+  return x.indexOf(y) >= -1 && x.indexOf(y) === x.length - y.length - 1; // NOT OK
+}
+
+function endsWithSlash(x) {
+  return x.indexOf("/") === x.length - 1; // OK - even though it also matches the empty string
+}