Merge pull request #8786 from hvitved/ruby/dataflow/argument-tokens

Ruby: Implement `Argument[any]` and `Argument[n..]`
2025-12-17 01:03:14 +01:00 · 2022-04-21 16:31:24 +02:00
parent 0ec5aa6095 ea229d361c
commit bd09c61504
12 changed files with 103 additions and 46 deletions
--- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/AccessPathSyntax.qll
+++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/AccessPathSyntax.qll
@@ -42,9 +42,7 @@ module AccessPath {
   * Parses a lower-bounded interval `n..` and gets the lower bound.
   */
  bindingset[arg]
-  private int parseLowerBound(string arg) {
-    result = arg.regexpCapture("(-?\\d+)\\.\\.", 1).toInt()
-  }
+  int parseLowerBound(string arg) { result = arg.regexpCapture("(-?\\d+)\\.\\.", 1).toInt() }

  /**
   * Parses an integer constant or interval (bounded or unbounded) that explicitly
--- a/java/ql/lib/semmle/code/java/dataflow/internal/AccessPathSyntax.qll
+++ b/java/ql/lib/semmle/code/java/dataflow/internal/AccessPathSyntax.qll
@@ -42,9 +42,7 @@ module AccessPath {
   * Parses a lower-bounded interval `n..` and gets the lower bound.
   */
  bindingset[arg]
-  private int parseLowerBound(string arg) {
-    result = arg.regexpCapture("(-?\\d+)\\.\\.", 1).toInt()
-  }
+  int parseLowerBound(string arg) { result = arg.regexpCapture("(-?\\d+)\\.\\.", 1).toInt() }

  /**
   * Parses an integer constant or interval (bounded or unbounded) that explicitly
--- a/javascript/ql/lib/semmle/javascript/frameworks/data/internal/AccessPathSyntax.qll
+++ b/javascript/ql/lib/semmle/javascript/frameworks/data/internal/AccessPathSyntax.qll
@@ -42,9 +42,7 @@ module AccessPath {
   * Parses a lower-bounded interval `n..` and gets the lower bound.
   */
  bindingset[arg]
-  private int parseLowerBound(string arg) {
-    result = arg.regexpCapture("(-?\\d+)\\.\\.", 1).toInt()
-  }
+  int parseLowerBound(string arg) { result = arg.regexpCapture("(-?\\d+)\\.\\.", 1).toInt() }

  /**
   * Parses an integer constant or interval (bounded or unbounded) that explicitly
--- a/ruby/ql/lib/codeql/ruby/dataflow/internal/AccessPathSyntax.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/AccessPathSyntax.qll
@@ -42,9 +42,7 @@ module AccessPath {
   * Parses a lower-bounded interval `n..` and gets the lower bound.
   */
  bindingset[arg]
-  private int parseLowerBound(string arg) {
-    result = arg.regexpCapture("(-?\\d+)\\.\\.", 1).toInt()
-  }
+  int parseLowerBound(string arg) { result = arg.regexpCapture("(-?\\d+)\\.\\.", 1).toInt() }

  /**
   * Parses an integer constant or interval (bounded or unbounded) that explicitly
--- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll
@@ -268,15 +268,17 @@ private module Cached {
    TPositionalParameterPosition(int pos) {
      pos = any(Parameter p).getPosition()
      or
-      pos in [0 .. 100] // TODO: remove once `Argument[_]` summaries are replaced with `Argument[i..]`
-      or
      FlowSummaryImplSpecific::ParsePositions::isParsedArgumentPosition(_, pos)
    } or
+    TPositionalParameterLowerBoundPosition(int pos) {
+      FlowSummaryImplSpecific::ParsePositions::isParsedArgumentLowerBoundPosition(_, pos)
+    } or
    TKeywordParameterPosition(string name) {
      name = any(KeywordParameter kp).getName()
      or
      FlowSummaryImplSpecific::ParsePositions::isParsedKeywordArgumentPosition(_, name)
-    }
+    } or
+    TAnyParameterPosition()
 }

 import Cached
@@ -468,9 +470,18 @@ class ParameterPosition extends TParameterPosition {
  /** Holds if this position represents a positional parameter at position `pos`. */
  predicate isPositional(int pos) { this = TPositionalParameterPosition(pos) }

+  /** Holds if this position represents any positional parameter starting from position `pos`. */
+  predicate isPositionalLowerBound(int pos) { this = TPositionalParameterLowerBoundPosition(pos) }
+
  /** Holds if this position represents a keyword parameter named `name`. */
  predicate isKeyword(string name) { this = TKeywordParameterPosition(name) }

+  /**
+   * Holds if this position represents any parameter. This includes both positional
+   * and named parameters.
+   */
+  predicate isAny() { this = TAnyParameterPosition() }
+
  /** Gets a textual representation of this position. */
  string toString() {
    this.isSelf() and result = "self"
@@ -479,7 +490,11 @@ class ParameterPosition extends TParameterPosition {
    or
    exists(int pos | this.isPositional(pos) and result = "position " + pos)
    or
+    exists(int pos | this.isPositionalLowerBound(pos) and result = "position " + pos + "..")
+    or
    exists(string name | this.isKeyword(name) and result = "keyword " + name)
+    or
+    this.isAny() and result = "any"
  }
 }

@@ -518,5 +533,11 @@ predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) {
  or
  exists(int pos | ppos.isPositional(pos) and apos.isPositional(pos))
  or
+  exists(int pos1, int pos2 |
+    ppos.isPositionalLowerBound(pos1) and apos.isPositional(pos2) and pos2 >= pos1
+  )
+  or
  exists(string name | ppos.isKeyword(name) and apos.isKeyword(name))
+  or
+  ppos.isAny() and exists(apos)
 }
--- a/ruby/ql/lib/codeql/ruby/dataflow/internal/FlowSummaryImplSpecific.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/FlowSummaryImplSpecific.qll
@@ -61,21 +61,32 @@ predicate summaryElement(
 *
 * This covers all the Ruby-specific components of a flow summary.
 */
-bindingset[c]
 SummaryComponent interpretComponentSpecific(AccessPathToken c) {
-  c = "Argument[_]" and
-  result = FlowSummary::SummaryComponent::argument(any(ParameterPosition pos | pos.isPositional(_)))
+  c.getName() = "Argument" and
+  exists(string arg, ParameterPosition ppos |
+    arg = c.getAnArgument() and
+    result = FlowSummary::SummaryComponent::argument(ppos)
+  |
+    arg = "any" and
+    ppos.isAny()
+    or
+    ppos.isPositionalLowerBound(AccessPath::parseLowerBound(arg))
+  )
  or
-  c = "ArrayElement" and
-  result = FlowSummary::SummaryComponent::arrayElementAny()
-  or
-  c = "ArrayElement[?]" and
-  result = FlowSummary::SummaryComponent::arrayElementUnknown()
-  or
-  exists(int i |
-    c.getName() = "ArrayElement" and
-    i = AccessPath::parseInt(c.getAnArgument()) and
-    result = FlowSummary::SummaryComponent::arrayElementKnown(i)
+  c.getName() = "ArrayElement" and
+  (
+    c.getNumArgument() = 0 and
+    result = FlowSummary::SummaryComponent::arrayElementAny()
+    or
+    exists(string arg | arg = c.getAnArgument() |
+      arg = "?" and
+      result = FlowSummary::SummaryComponent::arrayElementUnknown()
+      or
+      exists(int i |
+        i = AccessPath::parseInt(c.getAnArgument()) and
+        result = FlowSummary::SummaryComponent::arrayElementKnown(i)
+      )
+    )
  )
 }

@@ -201,6 +212,11 @@ module ParsePositions {
    i = AccessPath::parseInt(c)
  }

+  predicate isParsedArgumentLowerBoundPosition(string c, int i) {
+    isArgBody(c) and
+    i = AccessPath::parseLowerBound(c)
+  }
+
  predicate isParsedKeywordParameterPosition(string c, string paramName) {
    isParamBody(c) and
    c = paramName + ":"
@@ -238,6 +254,11 @@ ParameterPosition parseArgBody(string s) {
    result.isPositional(i)
  )
  or
+  exists(int i |
+    ParsePositions::isParsedArgumentLowerBoundPosition(s, i) and
+    result.isPositionalLowerBound(i)
+  )
+  or
  exists(string name |
    ParsePositions::isParsedKeywordArgumentPosition(s, name) and
    result.isKeyword(name)
--- a/ruby/ql/lib/codeql/ruby/frameworks/Files.qll
+++ b/ruby/ql/lib/codeql/ruby/frameworks/Files.qll
@@ -390,7 +390,7 @@ module File {
    }

    override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
-      input = "Argument[_]" and
+      input = "Argument[0..]" and
      output = "ReturnValue" and
      preservesValue = false
    }
--- a/ruby/ql/lib/codeql/ruby/frameworks/core/Array.qll
+++ b/ruby/ql/lib/codeql/ruby/frameworks/core/Array.qll
@@ -507,7 +507,7 @@ module Array {
    ConcatSummary() { this = "concat" }

    override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
-      input = "Argument[_].ArrayElement" and
+      input = "Argument[0..].ArrayElement" and
      output = "Argument[self].ArrayElement[?]" and
      preservesValue = true
    }
--- a/ruby/ql/lib/codeql/ruby/frameworks/core/String.qll
+++ b/ruby/ql/lib/codeql/ruby/frameworks/core/String.qll
@@ -152,7 +152,7 @@ module String {
    }

    override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
-      input = ["Argument[self]", "Argument[_]"] and
+      input = "Argument[self,0..]" and
      output = ["ReturnValue", "Argument[self]"] and
      preservesValue = false
    }
--- a/ruby/ql/test/library-tests/dataflow/summaries/Summaries.expected
+++ b/ruby/ql/test/library-tests/dataflow/summaries/Summaries.expected
@@ -19,7 +19,10 @@ edges
 | summaries.rb:1:11:1:36 | call to identity :  | summaries.rb:37:36:37:42 | tainted |
 | summaries.rb:1:11:1:36 | call to identity :  | summaries.rb:37:36:37:42 | tainted |
 | summaries.rb:1:11:1:36 | call to identity :  | summaries.rb:51:24:51:30 | tainted :  |
-| summaries.rb:1:11:1:36 | call to identity :  | summaries.rb:54:23:54:29 | tainted :  |
+| summaries.rb:1:11:1:36 | call to identity :  | summaries.rb:54:22:54:28 | tainted :  |
+| summaries.rb:1:11:1:36 | call to identity :  | summaries.rb:55:17:55:23 | tainted :  |
+| summaries.rb:1:11:1:36 | call to identity :  | summaries.rb:58:32:58:38 | tainted :  |
+| summaries.rb:1:11:1:36 | call to identity :  | summaries.rb:60:23:60:29 | tainted :  |
 | summaries.rb:1:20:1:36 | call to source :  | summaries.rb:1:11:1:36 | call to identity :  |
 | summaries.rb:1:20:1:36 | call to source :  | summaries.rb:1:11:1:36 | call to identity :  |
 | summaries.rb:4:12:7:3 | call to apply_block :  | summaries.rb:9:6:9:13 | tainted2 |
@@ -52,10 +55,13 @@ edges
 | summaries.rb:44:8:44:8 | t :  | summaries.rb:44:8:44:27 | call to matchedByNameRcv |
 | summaries.rb:48:24:48:41 | call to source :  | summaries.rb:48:8:48:42 | call to preserveTaint |
 | summaries.rb:51:24:51:30 | tainted :  | summaries.rb:51:6:51:31 | call to namedArg |
-| summaries.rb:54:23:54:29 | tainted :  | summaries.rb:54:40:54:40 | x :  |
-| summaries.rb:54:40:54:40 | x :  | summaries.rb:55:8:55:8 | x |
-| summaries.rb:62:24:62:53 | call to source :  | summaries.rb:62:8:62:54 | call to preserveTaint |
-| summaries.rb:65:26:65:56 | call to source :  | summaries.rb:65:8:65:57 | call to preserveTaint |
+| summaries.rb:54:22:54:28 | tainted :  | summaries.rb:54:6:54:29 | call to anyArg |
+| summaries.rb:55:17:55:23 | tainted :  | summaries.rb:55:6:55:24 | call to anyArg |
+| summaries.rb:58:32:58:38 | tainted :  | summaries.rb:58:6:58:39 | call to anyPositionFromOne |
+| summaries.rb:60:23:60:29 | tainted :  | summaries.rb:60:40:60:40 | x :  |
+| summaries.rb:60:40:60:40 | x :  | summaries.rb:61:8:61:8 | x |
+| summaries.rb:68:24:68:53 | call to source :  | summaries.rb:68:8:68:54 | call to preserveTaint |
+| summaries.rb:71:26:71:56 | call to source :  | summaries.rb:71:8:71:57 | call to preserveTaint |
 nodes
 | summaries.rb:1:11:1:36 | call to identity :  | semmle.label | call to identity :  |
 | summaries.rb:1:11:1:36 | call to identity :  | semmle.label | call to identity :  |
@@ -112,13 +118,19 @@ nodes
 | summaries.rb:48:24:48:41 | call to source :  | semmle.label | call to source :  |
 | summaries.rb:51:6:51:31 | call to namedArg | semmle.label | call to namedArg |
 | summaries.rb:51:24:51:30 | tainted :  | semmle.label | tainted :  |
-| summaries.rb:54:23:54:29 | tainted :  | semmle.label | tainted :  |
-| summaries.rb:54:40:54:40 | x :  | semmle.label | x :  |
-| summaries.rb:55:8:55:8 | x | semmle.label | x |
-| summaries.rb:62:8:62:54 | call to preserveTaint | semmle.label | call to preserveTaint |
-| summaries.rb:62:24:62:53 | call to source :  | semmle.label | call to source :  |
-| summaries.rb:65:8:65:57 | call to preserveTaint | semmle.label | call to preserveTaint |
-| summaries.rb:65:26:65:56 | call to source :  | semmle.label | call to source :  |
+| summaries.rb:54:6:54:29 | call to anyArg | semmle.label | call to anyArg |
+| summaries.rb:54:22:54:28 | tainted :  | semmle.label | tainted :  |
+| summaries.rb:55:6:55:24 | call to anyArg | semmle.label | call to anyArg |
+| summaries.rb:55:17:55:23 | tainted :  | semmle.label | tainted :  |
+| summaries.rb:58:6:58:39 | call to anyPositionFromOne | semmle.label | call to anyPositionFromOne |
+| summaries.rb:58:32:58:38 | tainted :  | semmle.label | tainted :  |
+| summaries.rb:60:23:60:29 | tainted :  | semmle.label | tainted :  |
+| summaries.rb:60:40:60:40 | x :  | semmle.label | x :  |
+| summaries.rb:61:8:61:8 | x | semmle.label | x |
+| summaries.rb:68:8:68:54 | call to preserveTaint | semmle.label | call to preserveTaint |
+| summaries.rb:68:24:68:53 | call to source :  | semmle.label | call to source :  |
+| summaries.rb:71:8:71:57 | call to preserveTaint | semmle.label | call to preserveTaint |
+| summaries.rb:71:26:71:56 | call to source :  | semmle.label | call to source :  |
 subpaths
 invalidSpecComponent
 invalidOutputSpecComponent
@@ -150,9 +162,12 @@ invalidOutputSpecComponent
 | summaries.rb:44:8:44:27 | call to matchedByNameRcv | summaries.rb:40:7:40:17 | call to source :  | summaries.rb:44:8:44:27 | call to matchedByNameRcv | $@ | summaries.rb:40:7:40:17 | call to source :  | call to source :  |
 | summaries.rb:48:8:48:42 | call to preserveTaint | summaries.rb:48:24:48:41 | call to source :  | summaries.rb:48:8:48:42 | call to preserveTaint | $@ | summaries.rb:48:24:48:41 | call to source :  | call to source :  |
 | summaries.rb:51:6:51:31 | call to namedArg | summaries.rb:1:20:1:36 | call to source :  | summaries.rb:51:6:51:31 | call to namedArg | $@ | summaries.rb:1:20:1:36 | call to source :  | call to source :  |
-| summaries.rb:55:8:55:8 | x | summaries.rb:1:20:1:36 | call to source :  | summaries.rb:55:8:55:8 | x | $@ | summaries.rb:1:20:1:36 | call to source :  | call to source :  |
-| summaries.rb:62:8:62:54 | call to preserveTaint | summaries.rb:62:24:62:53 | call to source :  | summaries.rb:62:8:62:54 | call to preserveTaint | $@ | summaries.rb:62:24:62:53 | call to source :  | call to source :  |
-| summaries.rb:65:8:65:57 | call to preserveTaint | summaries.rb:65:26:65:56 | call to source :  | summaries.rb:65:8:65:57 | call to preserveTaint | $@ | summaries.rb:65:26:65:56 | call to source :  | call to source :  |
+| summaries.rb:54:6:54:29 | call to anyArg | summaries.rb:1:20:1:36 | call to source :  | summaries.rb:54:6:54:29 | call to anyArg | $@ | summaries.rb:1:20:1:36 | call to source :  | call to source :  |
+| summaries.rb:55:6:55:24 | call to anyArg | summaries.rb:1:20:1:36 | call to source :  | summaries.rb:55:6:55:24 | call to anyArg | $@ | summaries.rb:1:20:1:36 | call to source :  | call to source :  |
+| summaries.rb:58:6:58:39 | call to anyPositionFromOne | summaries.rb:1:20:1:36 | call to source :  | summaries.rb:58:6:58:39 | call to anyPositionFromOne | $@ | summaries.rb:1:20:1:36 | call to source :  | call to source :  |
+| summaries.rb:61:8:61:8 | x | summaries.rb:1:20:1:36 | call to source :  | summaries.rb:61:8:61:8 | x | $@ | summaries.rb:1:20:1:36 | call to source :  | call to source :  |
+| summaries.rb:68:8:68:54 | call to preserveTaint | summaries.rb:68:24:68:53 | call to source :  | summaries.rb:68:8:68:54 | call to preserveTaint | $@ | summaries.rb:68:24:68:53 | call to source :  | call to source :  |
+| summaries.rb:71:8:71:57 | call to preserveTaint | summaries.rb:71:26:71:56 | call to source :  | summaries.rb:71:8:71:57 | call to preserveTaint | $@ | summaries.rb:71:26:71:56 | call to source :  | call to source :  |
 warning
 | CSV type row should have 5 columns but has 2: test;TooFewColumns |
 | CSV type row should have 5 columns but has 8: test;TooManyColumns;;;Member[Foo].Instance;too;many;columns |
--- a/ruby/ql/test/library-tests/dataflow/summaries/Summaries.ql
+++ b/ruby/ql/test/library-tests/dataflow/summaries/Summaries.ql
@@ -78,6 +78,8 @@ private class StepsFromModel extends ModelInput::SummaryModelCsv {
        ";;Member[Foo].Method[onlyWithBlock].WithBlock;Argument[0];ReturnValue;taint",
        ";;Member[Foo].Method[blockArg].Argument[block].Parameter[0].Method[preserveTaint];Argument[0];ReturnValue;taint",
        ";;Member[Foo].Method[namedArg];Argument[foo:];ReturnValue;taint",
+        ";;Member[Foo].Method[anyArg];Argument[any];ReturnValue;taint",
+        ";;Member[Foo].Method[anyPositionFromOne];Argument[1..];ReturnValue;taint",
        ";;Member[Foo].Method[intoNamedCallback];Argument[0];Argument[foo:].Parameter[0];taint",
        ";;Member[Foo].Method[intoNamedParameter];Argument[0];Argument[0].Parameter[foo:];taint",
        ";;Member[Foo].Method[startInNamedCallback].Argument[foo:].Parameter[0].Method[preserveTaint];Argument[0];ReturnValue;taint",
--- a/ruby/ql/test/library-tests/dataflow/summaries/summaries.rb
+++ b/ruby/ql/test/library-tests/dataflow/summaries/summaries.rb
@@ -51,6 +51,12 @@ end
 sink(Foo.namedArg(foo: tainted)) # $ hasTaintFlow=tainted
 sink(Foo.namedArg(tainted))

+sink(Foo.anyArg(foo: tainted)) # $ hasTaintFlow=tainted
+sink(Foo.anyArg(tainted)) # $ hasTaintFlow=tainted
+
+sink(Foo.anyPositionFromOne(tainted))
+sink(Foo.anyPositionFromOne(0, tainted)) # $ hasTaintFlow=tainted
+
 Foo.intoNamedCallback(tainted, foo: ->(x) {
  sink(x) # $ hasTaintFlow=tainted
 })