Ruby: add content edges to API graph

Fixes
2026-04-30 19:26:02 +02:00 · 2022-08-30 14:58:36 +02:00
parent d5e2b93554
commit a51a540582
6 changed files with 94 additions and 5 deletions
--- a/ruby/ql/lib/codeql/ruby/ApiGraphs.qll
+++ b/ruby/ql/lib/codeql/ruby/ApiGraphs.qll
@@ -9,6 +9,7 @@
 private import codeql.ruby.AST
 private import codeql.ruby.DataFlow
 private import codeql.ruby.typetracking.TypeTracker
+private import codeql.ruby.typetracking.TypeTrackerSpecific as TypeTrackerSpecific
 private import codeql.ruby.ast.internal.Module
 private import codeql.ruby.controlflow.CfgNodes
 private import codeql.ruby.dataflow.internal.DataFlowPrivate as DataFlowPrivate
@@ -257,6 +258,32 @@ module API {
     */
    Node getAnImmediateSubclass() { result = this.getASuccessor(Label::subclass()) }

+    /**
+     * Gets a node representing the `content` stored on the base object.
+     */
+    Node getContent(DataFlow::Content content) {
+      result = this.getASuccessor(Label::content(content))
+    }
+
+    /**
+     * Gets a node representing the `contents` stored on the base object.
+     */
+    Node getContents(DataFlow::ContentSet contents) {
+      this instanceof API::Use and
+      result = this.getContent(contents.getAStoreContent()) // The library has stored a value of interest in `contents`
+      or
+      this instanceof API::Def and
+      result = this.getContent(contents.getAReadContent()) // The library going to read a value stored in `contents`
+    }
+
+    /** Gets a node representing the instance field of the given `name`, which must include the `@` character. */
+    Node getField(string name) { result = this.getContent(DataFlowPrivate::TFieldContent(name)) }
+
+    /** Gets a node representing an element of this collection. */
+    Node getAnElement() {
+      result = this.getContents(any(DataFlow::ContentSet set | set.isAnyElement()))
+    }
+
    /**
     * Gets a string representation of the lexicographically least among all shortest access paths
     * from the root to this node.
@@ -495,9 +522,25 @@ module API {
        ref.asExpr() = c and
        read = c.getExpr()
      )
+      or
+      exists(TypeTrackerSpecific::TypeTrackerContent c |
+        TypeTrackerSpecific::basicLoadStep(node, ref, c) and
+        lbl = Label::content(c.asContent())
+      )
      // note: method calls are not handled here as there is no DataFlow::Node for the intermediate MkMethodAccessNode API node
    }

+    /**
+     * Holds if `rhs` is a definition of a node that should have an incoming edge labeled `lbl`,
+     * from a def node that is reachable from `node`.
+     */
+    private predicate defStep(Label::ApiLabel lbl, DataFlow::Node node, DataFlow::Node rhs) {
+      exists(TypeTrackerSpecific::TypeTrackerContent c |
+        TypeTrackerSpecific::basicStoreStep(rhs, node, c) and
+        lbl = Label::content(c.asContent())
+      )
+    }
+
    pragma[nomagic]
    private predicate isUse(DataFlow::Node nd) {
      useRoot(_, nd)
@@ -560,6 +603,8 @@ module API {
      // If a call node is relevant as a use-node, treat its arguments as def-nodes
      argumentStep(_, useCandFwd(), rhs)
      or
+      defStep(_, trackDefNode(_), rhs)
+      or
      rhs = any(EntryPoint entry).getASink()
    }

@@ -682,6 +727,12 @@ module API {
        )
      )
      or
+      exists(DataFlow::Node predNode, DataFlow::Node succNode |
+        def(pred, predNode) and
+        def(succ, succNode) and
+        defStep(lbl, trackDefNode(predNode), succNode)
+      )
+      or
      // `pred` is a use of class A
      // `succ` is a use of class B
      // there exists a class declaration B < A
--- a/ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModelsSpecific.qll
+++ b/ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModelsSpecific.qll
@@ -31,6 +31,7 @@ import codeql.ruby.dataflow.internal.AccessPathSyntax as AccessPathSyntax
 import codeql.ruby.DataFlow::DataFlow as DataFlow
 private import AccessPathSyntax
 private import codeql.ruby.dataflow.internal.FlowSummaryImplSpecific as FlowSummaryImplSpecific
+private import codeql.ruby.dataflow.internal.FlowSummaryImpl::Public
 private import codeql.ruby.dataflow.internal.DataFlowDispatch as DataFlowDispatch

 /**
@@ -118,9 +119,11 @@ API::Node getExtraSuccessorFromNode(API::Node node, AccessPathToken token) {
  result =
    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.
-  // Type-tracking cannot summarize function calls on its own, so it doesn't benefit from synthesized callables.
+  or
+  exists(DataFlow::ContentSet contents |
+    SummaryComponent::content(contents) = FlowSummaryImplSpecific::interpretComponentSpecific(token) and
+    result = node.getContents(contents)
+  )
 }

 /**
@@ -160,7 +163,7 @@ InvokeNode getAnInvocationOf(API::Node node) { result = node }
 */
 bindingset[name]
 predicate isExtraValidTokenNameInIdentifyingAccessPath(string name) {
-  name = ["Member", "Method", "Instance", "WithBlock", "WithoutBlock"]
+  name = ["Member", "Method", "Instance", "WithBlock", "WithoutBlock", "Element", "Field"]
 }

 /**
@@ -177,7 +180,7 @@ predicate isExtraValidNoArgumentTokenInIdentifyingAccessPath(string name) {
 */
 bindingset[name, argument]
 predicate isExtraValidTokenArgumentInIdentifyingAccessPath(string name, string argument) {
-  name = ["Member", "Method"] and
+  name = ["Member", "Method", "Element", "Field"] and
  exists(argument)
  or
  name = ["Argument", "Parameter"] and
--- a/ruby/ql/test/library-tests/dataflow/api-graphs/test1.rb
+++ b/ruby/ql/test/library-tests/dataflow/api-graphs/test1.rb
@@ -71,3 +71,5 @@ end

 array = [A::B::C] #$ use=getMember("Array").getMethod("[]").getReturn()
 array[0].m #$ use=getMember("A").getMember("B").getMember("C").getMethod("m").getReturn()
+
+A::B::C[0] #$ use=getMember("A").getMember("B").getMember("C").getContent(element)
--- a/ruby/ql/test/library-tests/dataflow/summaries/Summaries.expected
+++ b/ruby/ql/test/library-tests/dataflow/summaries/Summaries.expected
@@ -34,6 +34,12 @@ edges
 | summaries.rb:1:11:1:36 | call to identity :  | summaries.rb:117:26:117:32 | tainted |
 | summaries.rb:1:11:1:36 | call to identity :  | summaries.rb:119:23:119:29 | tainted |
 | summaries.rb:1:11:1:36 | call to identity :  | summaries.rb:119:23:119:29 | tainted |
+| summaries.rb:1:11:1:36 | call to identity :  | summaries.rb:122:26:122:32 | tainted |
+| summaries.rb:1:11:1:36 | call to identity :  | summaries.rb:122:26:122:32 | tainted |
+| summaries.rb:1:11:1:36 | call to identity :  | summaries.rb:124:16:124:22 | tainted |
+| summaries.rb:1:11:1:36 | call to identity :  | summaries.rb:124:16:124:22 | tainted |
+| summaries.rb:1:11:1:36 | call to identity :  | summaries.rb:127:39:127:45 | tainted |
+| summaries.rb:1:11:1:36 | call to identity :  | summaries.rb:127:39:127:45 | 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 |
@@ -112,6 +118,9 @@ edges
 | summaries.rb:104:16:104:22 | [post] tainted :  | summaries.rb:114:21:114:27 | tainted |
 | summaries.rb:104:16:104:22 | [post] tainted :  | summaries.rb:117:26:117:32 | tainted |
 | summaries.rb:104:16:104:22 | [post] tainted :  | summaries.rb:119:23:119:29 | tainted |
+| summaries.rb:104:16:104:22 | [post] tainted :  | summaries.rb:122:26:122:32 | tainted |
+| summaries.rb:104:16:104:22 | [post] tainted :  | summaries.rb:124:16:124:22 | tainted |
+| summaries.rb:104:16:104:22 | [post] tainted :  | summaries.rb:127:39:127:45 | tainted |
 | summaries.rb:104:16:104:22 | tainted :  | summaries.rb:104:16:104:22 | [post] tainted :  |
 | summaries.rb:104:16:104:22 | tainted :  | summaries.rb:104:25:104:25 | [post] y :  |
 | summaries.rb:104:16:104:22 | tainted :  | summaries.rb:104:33:104:33 | [post] z :  |
@@ -247,6 +256,12 @@ nodes
 | summaries.rb:117:26:117:32 | tainted | semmle.label | tainted |
 | summaries.rb:119:23:119:29 | tainted | semmle.label | tainted |
 | summaries.rb:119:23:119:29 | tainted | semmle.label | tainted |
+| summaries.rb:122:26:122:32 | tainted | semmle.label | tainted |
+| summaries.rb:122:26:122:32 | tainted | semmle.label | tainted |
+| summaries.rb:124:16:124:22 | tainted | semmle.label | tainted |
+| summaries.rb:124:16:124:22 | tainted | semmle.label | tainted |
+| summaries.rb:127:39:127:45 | tainted | semmle.label | tainted |
+| summaries.rb:127:39:127:45 | tainted | semmle.label | tainted |
 subpaths
 invalidSpecComponent
 #select
@@ -306,6 +321,12 @@ invalidSpecComponent
 | summaries.rb:117:26:117:32 | tainted | summaries.rb:1:20:1:36 | call to source :  | summaries.rb:117:26:117:32 | tainted | $@ | summaries.rb:1:20:1:36 | call to source :  | call to source :  |
 | summaries.rb:119:23:119:29 | tainted | summaries.rb:1:20:1:36 | call to source :  | summaries.rb:119:23:119:29 | tainted | $@ | summaries.rb:1:20:1:36 | call to source :  | call to source :  |
 | summaries.rb:119:23:119:29 | tainted | summaries.rb:1:20:1:36 | call to source :  | summaries.rb:119:23:119:29 | tainted | $@ | summaries.rb:1:20:1:36 | call to source :  | call to source :  |
+| summaries.rb:122:26:122:32 | tainted | summaries.rb:1:20:1:36 | call to source :  | summaries.rb:122:26:122:32 | tainted | $@ | summaries.rb:1:20:1:36 | call to source :  | call to source :  |
+| summaries.rb:122:26:122:32 | tainted | summaries.rb:1:20:1:36 | call to source :  | summaries.rb:122:26:122:32 | tainted | $@ | summaries.rb:1:20:1:36 | call to source :  | call to source :  |
+| summaries.rb:124:16:124:22 | tainted | summaries.rb:1:20:1:36 | call to source :  | summaries.rb:124:16:124:22 | tainted | $@ | summaries.rb:1:20:1:36 | call to source :  | call to source :  |
+| summaries.rb:124:16:124:22 | tainted | summaries.rb:1:20:1:36 | call to source :  | summaries.rb:124:16:124:22 | tainted | $@ | summaries.rb:1:20:1:36 | call to source :  | call to source :  |
+| summaries.rb:127:39:127:45 | tainted | summaries.rb:1:20:1:36 | call to source :  | summaries.rb:127:39:127:45 | tainted | $@ | summaries.rb:1:20:1:36 | call to source :  | call to source :  |
+| summaries.rb:127:39:127:45 | tainted | summaries.rb:1:20:1:36 | call to source :  | summaries.rb:127:39:127:45 | 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 |
--- a/ruby/ql/test/library-tests/dataflow/summaries/Summaries.ql
+++ b/ruby/ql/test/library-tests/dataflow/summaries/Summaries.ql
@@ -132,6 +132,9 @@ private class SinkFromModel extends ModelInput::SinkModelCsv {
        "test;FooOrBar;Method[method].Argument[0];test-sink", //
        ";;Member[Foo].Method[sinkAnyArg].Argument[any];test-sink", //
        ";;Member[Foo].Method[sinkAnyNamedArg].Argument[any-named];test-sink", //
+        ";;Member[Foo].Method[getSinks].ReturnValue.Element[any].Method[mySink].Argument[0];test-sink", //
+        ";;Member[Foo].Method[arraySink].Argument[0].Element[any];test-sink", //
+        ";;Member[Foo].Method[secondArrayElementIsSink].Argument[0].Element[1];test-sink", //
      ]
  }
 }
--- a/ruby/ql/test/library-tests/dataflow/summaries/summaries.rb
+++ b/ruby/ql/test/library-tests/dataflow/summaries/summaries.rb
@@ -118,3 +118,12 @@ Foo.sinkAnyNamedArg(key: tainted) # $ hasValueFlow=tainted

 "magic_string".method(tainted) # $ hasValueFlow=tainted
 "magic_string2".method(tainted)
+
+Foo.getSinks()[0].mySink(tainted) # $ hasValueFlow=tainted
+Foo.arraySink(tainted)
+Foo.arraySink([tainted]) # $ hasValueFlow=tainted
+
+Foo.secondArrayElementIsSink([tainted, "safe", "safe"])
+Foo.secondArrayElementIsSink(["safe", tainted, "safe"]) # $ hasValueFlow=tainted
+Foo.secondArrayElementIsSink(["safe", "safe", tainted])
+Foo.secondArrayElementIsSink([tainted] * 10) # $ MISSING: hasValueFlow=tainted