Ruby: Fix Argument[any,any-named] handling for path component in MaD

Author: RasmusWL

ruby/ql/lib/codeql/ruby/ApiGraphs.qll

@@ -780,6 +780,20 @@ module API {
       or
       pos.isBlock() and
       result = Label::blockParameter()
+      or
+      pos.isAny() and
+      (
+        result = Label::parameter(_)
+        or
+        result = Label::keywordParameter(_)
+        or
+        result = Label::blockParameter()
+        // NOTE: `self` should NOT be included, as described in the QLDoc for `isAny()`
+      )
+      // TODO: needs handling of `self` ArgumentPosition
+      // or
+      // pos.isSelf() and
+      // ...
     }
 
     /** Gets the API graph label corresponding to the given parameter position. */

ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll

@@ -259,7 +259,8 @@ private module Cached {
       exists(any(Call c).getKeywordArgument(name))
       or
       FlowSummaryImplSpecific::ParsePositions::isParsedKeywordParameterPosition(_, name)
-    }
+    } or
+    TAnyArgumentPosition()
 
   cached
   newtype TParameterPosition =
@@ -512,6 +513,12 @@ class ArgumentPosition extends TArgumentPosition {
   /** Holds if this position represents a keyword argument named `name`. */
   predicate isKeyword(string name) { this = TKeywordArgumentPosition(name) }
 
+  /**
+   * Holds if this position represents any argument, except `self` arguments. This
+   * includes both positional, named, and block arguments.
+   */
+  predicate isAny() { this = TAnyArgumentPosition() }
+
   /** Gets a textual representation of this position. */
   string toString() {
     this.isSelf() and result = "self"
@@ -521,6 +528,8 @@ class ArgumentPosition extends TArgumentPosition {
     exists(int pos | this.isPositional(pos) and result = "position " + pos)
     or
     exists(string name | this.isKeyword(name) and result = "keyword " + name)
+    or
+    this.isAny() and result = "any"
   }
 }
 
@@ -540,4 +549,6 @@ predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) {
   exists(string name | ppos.isKeyword(name) and apos.isKeyword(name))
   or
   ppos.isAny() and not apos.isSelf()
+  or
+  apos.isAny() and not ppos.isSelf()
 }

ruby/ql/lib/codeql/ruby/dataflow/internal/FlowSummaryImplSpecific.qll

@@ -257,6 +257,12 @@ ArgumentPosition parseParamBody(string s) {
   or
   s = "block" and
   result.isBlock()
+  or
+  s = "any" and
+  result.isAny()
+  or
+  s = "any-named" and
+  result.isKeyword(_)
 }
 
 /** Gets the parameter position obtained by parsing `X` in `Argument[X]`. */

ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModelsSpecific.qll

@@ -115,7 +115,7 @@ API::Node getExtraSuccessorFromNode(API::Node node, AccessPathToken token) {
   or
   token.getName() = "Parameter" and
   result =
-    node.getASuccessor(API::Label::getLabelFromArgumentPosition(FlowSummaryImplSpecific::parseParamBody(token
+    node.getASuccessor(API::Label::getLabelFromParameterPosition(FlowSummaryImplSpecific::parseArgBody(token
               .getAnArgument())))
   // Note: The "Element" token is not implemented yet, as it ultimately requires type-tracking and
   // API graphs to be aware of the steps involving Element contributed by the standard library model.
@@ -129,7 +129,7 @@ bindingset[token]
 API::Node getExtraSuccessorFromInvoke(InvokeNode node, AccessPathToken token) {
   token.getName() = "Argument" and
   result =
-    node.getASuccessor(API::Label::getLabelFromParameterPosition(FlowSummaryImplSpecific::parseArgBody(token
+    node.getASuccessor(API::Label::getLabelFromArgumentPosition(FlowSummaryImplSpecific::parseParamBody(token
               .getAnArgument())))
 }
 

ruby/ql/test/library-tests/dataflow/summaries/Summaries.expected

@@ -30,6 +30,8 @@ edges
 | summaries.rb:1:11:1:36 | call to identity :  | summaries.rb:102:16:102:22 | tainted |
 | summaries.rb:1:11:1:36 | call to identity :  | summaries.rb:103:21:103:27 | tainted |
 | summaries.rb:1:11:1:36 | call to identity :  | summaries.rb:103:21:103:27 | tainted |
+| summaries.rb:1:11:1:36 | call to identity :  | summaries.rb:106:26:106:32 | tainted |
+| summaries.rb:1:11:1:36 | call to identity :  | summaries.rb:106:26:106:32 | 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 |
@@ -97,6 +99,7 @@ edges
 | summaries.rb:93:16:93:22 | [post] tainted :  | summaries.rb:99:14:99:20 | tainted :  |
 | summaries.rb:93:16:93:22 | [post] tainted :  | summaries.rb:102:16:102:22 | tainted |
 | summaries.rb:93:16:93:22 | [post] tainted :  | summaries.rb:103:21:103:27 | tainted |
+| summaries.rb:93:16:93:22 | [post] tainted :  | summaries.rb:106:26:106:32 | tainted |
 | summaries.rb:93:16:93:22 | tainted :  | summaries.rb:93:16:93:22 | [post] tainted :  |
 | summaries.rb:93:16:93:22 | tainted :  | summaries.rb:93:25:93:25 | [post] y :  |
 | summaries.rb:93:16:93:22 | tainted :  | summaries.rb:93:33:93:33 | [post] z :  |
@@ -216,6 +219,8 @@ nodes
 | summaries.rb:102:16:102:22 | tainted | semmle.label | tainted |
 | summaries.rb:103:21:103:27 | tainted | semmle.label | tainted |
 | summaries.rb:103:21:103:27 | tainted | semmle.label | tainted |
+| summaries.rb:106:26:106:32 | tainted | semmle.label | tainted |
+| summaries.rb:106:26:106:32 | tainted | semmle.label | tainted |
 subpaths
 invalidSpecComponent
 #select
@@ -268,6 +273,8 @@ invalidSpecComponent
 | summaries.rb:102:16:102:22 | tainted | summaries.rb:1:20:1:36 | call to source :  | summaries.rb:102:16:102:22 | tainted | $@ | summaries.rb:1:20:1:36 | call to source :  | call to source :  |
 | summaries.rb:103:21:103:27 | tainted | summaries.rb:1:20:1:36 | call to source :  | summaries.rb:103:21:103:27 | tainted | $@ | summaries.rb:1:20:1:36 | call to source :  | call to source :  |
 | summaries.rb:103:21:103:27 | tainted | summaries.rb:1:20:1:36 | call to source :  | summaries.rb:103:21:103:27 | tainted | $@ | summaries.rb:1:20:1:36 | call to source :  | call to source :  |
+| summaries.rb:106:26:106:32 | tainted | summaries.rb:1:20:1:36 | call to source :  | summaries.rb:106:26:106:32 | tainted | $@ | summaries.rb:1:20:1:36 | call to source :  | call to source :  |
+| summaries.rb:106:26:106:32 | tainted | summaries.rb:1:20:1:36 | call to source :  | summaries.rb:106:26:106:32 | tainted | $@ | summaries.rb:1:20:1:36 | 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 |

ruby/ql/test/library-tests/dataflow/summaries/summaries.rb

@@ -103,4 +103,4 @@ def userDefinedFunction(x, y)
 Foo.sinkAnyArg(key: tainted) # $ hasValueFlow=tainted
 
 Foo.sinkAnyNamedArg(tainted)
-Foo.sinkAnyNamedArg(key: tainted) # $ MISSING: hasValueFlow=tainted
+Foo.sinkAnyNamedArg(key: tainted) # $ hasValueFlow=tainted