diff --git a/change-notes/1.20/analysis-javascript.md b/change-notes/1.20/analysis-javascript.md index 65f6b7864d3..ddc519a440f 100644 --- a/change-notes/1.20/analysis-javascript.md +++ b/change-notes/1.20/analysis-javascript.md @@ -18,6 +18,7 @@ | Incomplete regular expression for hostnames (`js/incomplete-hostname-regexp`) | correctness, security, external/cwe/cwe-020 | Highlights hostname sanitizers that are likely to be incomplete, indicating a violation of [CWE-020](https://cwe.mitre.org/data/definitions/20.html). Results are shown on LGTM by default.| | Incomplete URL substring sanitization | correctness, security, external/cwe/cwe-020 | Highlights URL sanitizers that are likely to be incomplete, indicating a violation of [CWE-020](https://cwe.mitre.org/data/definitions/20.html). Results 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). Results are shown on LGTM by default. | +| Loop iteration skipped due to shifting (`js/loop-iteration-skipped-due-to-shifting`) | correctness | Highlights code that removes an element from an array while iterating over it, causing the loop to skip over some elements. Results are shown on LGTM by default. | | 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. Results are shown on LGTM by default. | ## Changes to existing queries diff --git a/javascript/config/suites/javascript/correctness-more b/javascript/config/suites/javascript/correctness-more index 2e57bcd7327..8552456ec1e 100644 --- a/javascript/config/suites/javascript/correctness-more +++ b/javascript/config/suites/javascript/correctness-more @@ -11,6 +11,7 @@ + semmlecode-javascript-queries/LanguageFeatures/IllegalInvocation.ql: /Correctness/Language Features + semmlecode-javascript-queries/LanguageFeatures/InconsistentNew.ql: /Correctness/Language Features + semmlecode-javascript-queries/LanguageFeatures/SpuriousArguments.ql: /Correctness/Language Features ++ semmlecode-javascript-queries/Statements/LoopIterationSkippedDueToShifting.ql: /Correctness/Statements + semmlecode-javascript-queries/Statements/MisleadingIndentationAfterControlStmt.ql: /Correctness/Statements + semmlecode-javascript-queries/Statements/ReturnOutsideFunction.ql: /Correctness/Statements + semmlecode-javascript-queries/Statements/SuspiciousUnusedLoopIterationVariable.ql: /Correctness/Statements diff --git a/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.qhelp b/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.qhelp new file mode 100644 index 00000000000..ef24e63fa5a --- /dev/null +++ b/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.qhelp @@ -0,0 +1,69 @@ + + + +

+ Items can be removed from an array using the splice method, but when doing so, + all subsequent items will be shifted to a lower index. If this is done while iterating over + the array, the shifting may cause the loop to skip over the element immediately after the + removed element. +

+ + + + +

+Determine what the loop is supposed to do: +

+ + + + + + +

+In this example, a function is intended to remove ".." parts from a path: +

+ + + +

+However, whenever the input contain two ".." parts right after one another, only the first will be removed. +For example, the string "../../secret.txt" will be mapped to "../secret.txt". After removing +the element at index 0, the loop counter is incremented to 1, but the second ".." string has now been shifted down to +index 0 and will therefore be skipped. +

+ +

+One way to avoid this is to decrement the loop counter after removing an element from the array: +

+ + + +

+Alternatively, use the filter method: +

+ + + + + + +
  • MDN: Array.prototype.splice().
    • +
  • MDN: Array.prototype.filter().
    • + +     +     diff --git a/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql b/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql new file mode 100644 index 00000000000..bbaca72234e --- /dev/null +++ b/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql @@ -0,0 +1,163 @@ +/** + * @name Loop iteration skipped due to shifting + * @description Removing elements from an array while iterating over it can cause the loop to skip over some elements, + * unless the loop index is decremented accordingly. + * @kind problem + * @problem.severity warning + * @id js/loop-iteration-skipped-due-to-shifting + * @tags correctness + * @precision high + */ +import javascript + +/** + * An operation that inserts or removes elements from an array while shifting all elements + * occuring after the insertion/removal point. + * + * Does not include `push` and `pop` since these never shift any elements. + */ +class ArrayShiftingCall extends DataFlow::MethodCallNode { + string name; + + ArrayShiftingCall() { + name = getMethodName() and + (name = "splice" or name = "shift" or name = "unshift") + } + + DataFlow::SourceNode getArray() { + result = getReceiver().getALocalSource() + } +} + +/** + * A call to `splice` on an array. + */ +class SpliceCall extends ArrayShiftingCall { + SpliceCall() { + name = "splice" + } + + /** + * Gets the index from which elements are removed and possibly new elemenst are inserted. + */ + DataFlow::Node getIndex() { + result = getArgument(0) + } + + /** + * Gets the number of removed elements. + */ + int getNumRemovedElements() { + result = getArgument(1).asExpr().getIntValue() and + result >= 0 + } + + /** + * Gets the number of inserted elements. + */ + int getNumInsertedElements() { + result = getNumArgument() - 2 and + result >= 0 + } +} + +/** + * A `for` loop iterating over the indices of an array, in increasing order. + */ +class ArrayIterationLoop extends ForStmt { + DataFlow::SourceNode array; + LocalVariable indexVariable; + + ArrayIterationLoop() { + exists (RelationalComparison compare | compare = getTest() | + compare.getLesserOperand() = indexVariable.getAnAccess() and + compare.getGreaterOperand() = array.getAPropertyRead("length").asExpr()) and + getUpdate().(IncExpr).getOperand() = indexVariable.getAnAccess() + } + + /** + * Gets the variable holding the loop variable and current array index. + */ + LocalVariable getIndexVariable() { + result = indexVariable + } + + /** + * Gets the loop entry point. + */ + ReachableBasicBlock getLoopEntry() { + result = getTest().getFirstControlFlowNode().getBasicBlock() + } + + /** + * Gets a call that potentially shifts the elements of the given array. + */ + ArrayShiftingCall getAnArrayShiftingCall() { + result.getArray() = array + } + + /** + * Gets a call to `splice` that removes elements from the looped-over array at the current index + * + * The `splice` call is not guaranteed to be inside the loop body. + */ + SpliceCall getACandidateSpliceCall() { + result = getAnArrayShiftingCall() and + result.getIndex().asExpr() = getIndexVariable().getAnAccess() and + result.getNumRemovedElements() > result.getNumInsertedElements() + } + + /** + * Holds if `cfg` modifies the index variable or shifts array elements, disturbing the + * relationship between the array and the index variable. + */ + predicate hasIndexingManipulation(ControlFlowNode cfg) { + cfg.(VarDef).getAVariable() = getIndexVariable() or + cfg = getAnArrayShiftingCall().asExpr() + } + + /** + * Holds if there is a `loop entry -> cfg` path that does not involve index manipulation or a successful index equality check. + */ + predicate hasPathTo(ControlFlowNode cfg) { + exists(getACandidateSpliceCall()) and // restrict size of predicate + cfg = getLoopEntry().getFirstNode() + or + hasPathTo(cfg.getAPredecessor()) and + getLoopEntry().dominates(cfg.getBasicBlock()) and + not hasIndexingManipulation(cfg) and + + // Ignore splice calls guarded by an index equality check. + // This indicates that the index of an element is the basis for removal, not its value, + // which means it may be okay to skip over elements. + not exists (ConditionGuardNode guard, EqualityTest test | cfg = guard | + test = guard.getTest() and + test.getAnOperand() = getIndexVariable().getAnAccess() and + guard.getOutcome() = test.getPolarity()) and + + // Block flow after inspecting an array element other than that at the current index. + // For example, if the splice happens after inspecting `array[i + 1]`, then the next + // element has already been "looked at" and so it doesn't matter if we skip it. + not exists (IndexExpr index | cfg = index | + array.flowsToExpr(index.getBase()) and + not index.getIndex() = getIndexVariable().getAnAccess()) + } + + /** + * Holds if there is a `loop entry -> splice -> cfg` path that does not involve index manipulation, + * other than the `splice` call. + */ + predicate hasPathThrough(SpliceCall splice, ControlFlowNode cfg) { + splice = getACandidateSpliceCall() and + cfg = splice.asExpr() and + hasPathTo(cfg.getAPredecessor()) + or + hasPathThrough(splice, cfg.getAPredecessor()) and + getLoopEntry().dominates(cfg.getBasicBlock()) and + not hasIndexingManipulation(cfg) + } +} + +from ArrayIterationLoop loop, SpliceCall splice +where loop.hasPathThrough(splice, loop.getUpdate().getFirstControlFlowNode()) +select splice, "Removing an array item without adjusting the loop index '" + loop.getIndexVariable().getName() + "' causes the subsequent array item to be skipped." diff --git a/javascript/ql/src/Statements/examples/LoopIterationSkippedDueToShifting.js b/javascript/ql/src/Statements/examples/LoopIterationSkippedDueToShifting.js new file mode 100644 index 00000000000..ccff5c5d208 --- /dev/null +++ b/javascript/ql/src/Statements/examples/LoopIterationSkippedDueToShifting.js @@ -0,0 +1,9 @@ +function removePathTraversal(path) { + let parts = path.split('/'); + for (let i = 0; i < parts.length; ++i) { + if (parts[i] === '..') { + parts.splice(i, 1); + } + } + return path.join('/'); +} diff --git a/javascript/ql/src/Statements/examples/LoopIterationSkippedDueToShiftingGood.js b/javascript/ql/src/Statements/examples/LoopIterationSkippedDueToShiftingGood.js new file mode 100644 index 00000000000..3ebedc7ced9 --- /dev/null +++ b/javascript/ql/src/Statements/examples/LoopIterationSkippedDueToShiftingGood.js @@ -0,0 +1,10 @@ +function removePathTraversal(path) { + let parts = path.split('/'); + for (let i = 0; i < parts.length; ++i) { + if (parts[i] === '..') { + parts.splice(i, 1); + --i; // adjust for array shift + } + } + return path.join('/'); +} diff --git a/javascript/ql/src/Statements/examples/LoopIterationSkippedDueToShiftingGoodFilter.js b/javascript/ql/src/Statements/examples/LoopIterationSkippedDueToShiftingGoodFilter.js new file mode 100644 index 00000000000..eba08d045d2 --- /dev/null +++ b/javascript/ql/src/Statements/examples/LoopIterationSkippedDueToShiftingGoodFilter.js @@ -0,0 +1,3 @@ +function removePathTraversal(path) { + return path.split('/').filter(part => part !== '..').join('/'); +} diff --git a/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/LoopIterationSkippedDueToShifting.expected b/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/LoopIterationSkippedDueToShifting.expected new file mode 100644 index 00000000000..4b7becd8e16 --- /dev/null +++ b/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/LoopIterationSkippedDueToShifting.expected @@ -0,0 +1,3 @@ +| tst.js:4:27:4:44 | parts.splice(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent array item to be skipped. | +| tst.js:13:29:13:46 | parts.splice(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent array item to be skipped. | +| tst.js:24:9:24:26 | parts.splice(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent array item to be skipped. | diff --git a/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/LoopIterationSkippedDueToShifting.qlref b/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/LoopIterationSkippedDueToShifting.qlref new file mode 100644 index 00000000000..796046d180a --- /dev/null +++ b/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/LoopIterationSkippedDueToShifting.qlref @@ -0,0 +1 @@ +Statements/LoopIterationSkippedDueToShifting.ql \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/tst.js b/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/tst.js new file mode 100644 index 00000000000..cebe8569c08 --- /dev/null +++ b/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/tst.js @@ -0,0 +1,123 @@ +function removeX(string) { + let parts = string.split('/'); + for (let i = 0; i < parts.length; ++i) { + if (parts[i] === 'X') parts.splice(i, 1); // NOT OK + } + return parts.join('/'); +} + +function removeXInnerLoop(string, n) { + let parts = string.split('/'); + for (let j = 0; j < n; ++j) { + for (let i = 0; i < parts.length; ++i) { + if (parts[i] === 'X') parts.splice(i, 1); // NOT OK + } + } + return parts.join('/'); +} + +function removeXOuterLoop(string, n) { + let parts = string.split('/'); + for (let i = 0; i < parts.length; ++i) { + for (let j = 0; j < n; ++j) { + if (parts[i] === 'X') { + parts.splice(i, 1); // NOT OK + break; + } + } + } + return parts.join('/'); +} + +function decrementAfter(string) { + let parts = string.split('/'); + for (let i = 0; i < parts.length; ++i) { + if (parts[i] === 'X') { + parts.splice(i, 1); // OK + --i; + } + } + return parts.join('/'); +} + +function postDecrementArgument(string) { + let parts = string.split('/'); + for (let i = 0; i < parts.length; ++i) { + if (parts[i] === 'X') { + parts.splice(i--, 1); // OK + } + } + return parts.join('/'); +} + + +function breakAfter(string) { + let parts = string.split('/'); + for (let i = 0; i < parts.length; ++i) { + if (parts[i] === 'X') { + parts.splice(i, 1); // OK - only removes first occurrence + break; + } + } + return parts.join('/'); +} + +function insertNewElements(string) { + let parts = string.split('/'); + for (let i = 0; i < parts.length; ++i) { + if (parts[i] === 'X') { + parts.splice(i, 1, '.'); // OK - no shifting due to insert + } + } + return parts.join('/'); +} + +function spliceAfterLoop(string) { + let parts = string.split('/'); + let i = 0; + for (; i < parts.length; ++i) { + if (parts[i] === 'X') break; + } + if (parts[i] === 'X') { + parts.splice(i, 1); // OK - not inside loop + } + return parts.join('/'); +} + +function spliceAfterLoopNested(string) { + let parts = string.split('/'); + for (let j = 0; j < parts.length; ++j) { + let i = j; + for (; i < parts.length; ++i) { + if (parts[i] === 'X') break; + } + parts.splice(i, 1); // OK - not inside 'i' loop + } + return parts.join('/'); +} + +function removeAtSpecificPlace(string, k) { + let parts = string.split('/'); + for (let i = 0; i < parts.length; ++i) { + if (i === k && parts[i] === 'X') parts.splice(i, 1); // OK - more complex logic + } + return parts.join('/'); +} + +function removeFirstAndLast(string) { + let parts = string.split('/'); + for (let i = 0; i < parts.length; ++i) { + if (i === 0 || i === parts.length - 1) parts.splice(i, 1); // OK - out of scope of this query + } + return parts.join('/'); +} + +function inspectNextElement(string) { + let parts = string.split('/'); + for (let i = 0; i < parts.length; ++i) { + if (i < parts.length - 1 && parts[i] === parts[i + 1]) { + parts.splice(i, 1); // OK - next element has been looked at + } + } + return parts.join('/'); +}