From 031a73ff0faaf6ba2ceefe6dc0a044afc0a8299a Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 1 Sep 2021 16:56:09 +0200 Subject: [PATCH 1/3] Add API graph test that exhibits a missing edge --- ql/test/library-tests/dataflow/api-graphs/test1.rb | 7 +++++++ ql/test/library-tests/dataflow/api-graphs/use.expected | 2 ++ 2 files changed, 9 insertions(+) diff --git a/ql/test/library-tests/dataflow/api-graphs/test1.rb b/ql/test/library-tests/dataflow/api-graphs/test1.rb index f45647d64e4..bc9113bb5d0 100644 --- a/ql/test/library-tests/dataflow/api-graphs/test1.rb +++ b/ql/test/library-tests/dataflow/api-graphs/test1.rb @@ -22,3 +22,10 @@ foo::Bar::Baz #$ use=getMember("Foo").getMember("Bar").getMember("Baz") FooAlias = Foo #$ use=getMember("Foo") FooAlias::Bar::Baz #$ use=getMember("Foo").getMember("Bar").getMember("Baz") + +module Outer + module Inner + end +end + +Outer::Inner.foo #$ use=getMember("Outer").getMember("Inner").getReturn("foo") diff --git a/ql/test/library-tests/dataflow/api-graphs/use.expected b/ql/test/library-tests/dataflow/api-graphs/use.expected index e69de29bb2d..a5db3db86de 100644 --- a/ql/test/library-tests/dataflow/api-graphs/use.expected +++ b/ql/test/library-tests/dataflow/api-graphs/use.expected @@ -0,0 +1,2 @@ +| test1.rb:31:1:31:5 | Outer | Unexpected result: use=getMember("Outer") | +| test1.rb:31:18:31:78 | #$ use=getMember("Outer").getMember("Inner").getReturn("foo") | Missing result:use=getMember("Outer").getMember("Inner").getReturn("foo") | From ae70af01cd976b826dcdc17358b72aa19dc9b16a Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 1 Sep 2021 15:30:54 +0200 Subject: [PATCH 2/3] API graphs: Fix bug for resolvable modules --- ql/lib/codeql/ruby/ApiGraphs.qll | 12 ++++++++---- .../library-tests/dataflow/api-graphs/use.expected | 2 -- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/ql/lib/codeql/ruby/ApiGraphs.qll b/ql/lib/codeql/ruby/ApiGraphs.qll index cfefb81504e..e68c7b32294 100644 --- a/ql/lib/codeql/ruby/ApiGraphs.qll +++ b/ql/lib/codeql/ruby/ApiGraphs.qll @@ -253,6 +253,11 @@ module API { /** A use of an API member at the node `nd`. */ MkUse(DataFlow::Node nd) { use(_, _, nd) } + private string resolveTopLevel(ConstantReadAccess read) { + TResolved(result) = resolveScopeExpr(read) and + not result.matches("%::%") + } + /** * Holds if `ref` is a use of a node that should have an incoming edge from `base` labeled * `lbl` in the API graph. @@ -265,11 +270,10 @@ module API { lbl = Label::member(read.getName()) and read = access.getExpr() | - TResolved(name) = resolveScopeExpr(read) and - not name.matches("%::%") + name = resolveTopLevel(read) or name = read.getName() and - not exists(resolveScopeExpr(read)) and + not exists(resolveTopLevel(read)) and not exists(read.getScopeExpr()) ) or @@ -288,7 +292,7 @@ module API { // lbl = `Whatever` // ref = `Rails::Whatever` exists(ExprNodes::ConstantAccessCfgNode c, DataFlow::Node node, ConstantReadAccess read | - not exists(resolveScopeExpr(read)) and + not exists(resolveTopLevel(read)) and pred.flowsTo(node) and node.asExpr() = c.getScopeExpr() and lbl = Label::member(read.getName()) and diff --git a/ql/test/library-tests/dataflow/api-graphs/use.expected b/ql/test/library-tests/dataflow/api-graphs/use.expected index a5db3db86de..e69de29bb2d 100644 --- a/ql/test/library-tests/dataflow/api-graphs/use.expected +++ b/ql/test/library-tests/dataflow/api-graphs/use.expected @@ -1,2 +0,0 @@ -| test1.rb:31:1:31:5 | Outer | Unexpected result: use=getMember("Outer") | -| test1.rb:31:18:31:78 | #$ use=getMember("Outer").getMember("Inner").getReturn("foo") | Missing result:use=getMember("Outer").getMember("Inner").getReturn("foo") | From 03e91a22bcc56522bfaa87b3e809a4ea80d94d53 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 1 Sep 2021 15:32:49 +0200 Subject: [PATCH 3/3] API graphs: Performance fixes --- ql/lib/codeql/ruby/ApiGraphs.qll | 33 ++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/ql/lib/codeql/ruby/ApiGraphs.qll b/ql/lib/codeql/ruby/ApiGraphs.qll index e68c7b32294..5330a4c00ea 100644 --- a/ql/lib/codeql/ruby/ApiGraphs.qll +++ b/ql/lib/codeql/ruby/ApiGraphs.qll @@ -277,7 +277,7 @@ module API { not exists(read.getScopeExpr()) ) or - exists(DataFlow::LocalSourceNode src, DataFlow::LocalSourceNode pred | + exists(ExprCfgNode node | // First, we find a predecessor of the node `ref` that we want to determine. The predecessor // is any node that is a type-tracked use of a data flow node (`src`), which is itself a // reference to the API node `base`. Thus, `pred` and `src` both represent uses of `base`. @@ -285,25 +285,23 @@ module API { // Once we have identified the predecessor, we define its relation to the successor `ref` as // well as the label on the edge from `pred` to `ref`. This label describes the nature of // the relationship between `pred` and `ref`. - use(base, src) and pred = trackUseNode(src) + useExpr(node, base) | // // Referring to an attribute on a node that is a use of `base`: // pred = `Rails` part of `Rails::Whatever` // lbl = `Whatever` // ref = `Rails::Whatever` - exists(ExprNodes::ConstantAccessCfgNode c, DataFlow::Node node, ConstantReadAccess read | + exists(ExprNodes::ConstantAccessCfgNode c, ConstantReadAccess read | not exists(resolveTopLevel(read)) and - pred.flowsTo(node) and - node.asExpr() = c.getScopeExpr() and + node = c.getScopeExpr() and lbl = Label::member(read.getName()) and ref.asExpr() = c and read = c.getExpr() ) or // Calling a method on a node that is a use of `base` - exists(ExprNodes::MethodCallCfgNode call, DataFlow::Node node, string name | - pred.flowsTo(node) and - node.asExpr() = call.getReceiver() and + exists(ExprNodes::MethodCallCfgNode call, string name | + node = call.getReceiver() and name = call.getExpr().getMethodName() and lbl = Label::return(name) and name != "new" and @@ -311,9 +309,8 @@ module API { ) or // Calling the `new` method on a node that is a use of `base`, which creates a new instance - exists(ExprNodes::MethodCallCfgNode call, DataFlow::Node node | - pred.flowsTo(node) and - node.asExpr() = call.getReceiver() and + exists(ExprNodes::MethodCallCfgNode call | + node = call.getReceiver() and lbl = Label::instance() and call.getExpr().getMethodName() = "new" and ref.asExpr() = call @@ -321,6 +318,15 @@ module API { ) } + pragma[nomagic] + private predicate useExpr(ExprCfgNode node, TApiNode base) { + exists(DataFlow::LocalSourceNode src, DataFlow::LocalSourceNode pred | + use(base, src) and + pred = trackUseNode(src) and + pred.flowsTo(any(DataFlow::ExprNode n | n.getExprNode() = node)) + ) + } + /** * Holds if `ref` is a use of node `nd`. */ @@ -332,7 +338,10 @@ module API { * * The flow from `src` to that node may be inter-procedural. */ - private DataFlow::LocalSourceNode trackUseNode(DataFlow::LocalSourceNode src, TypeTracker t) { + private DataFlow::LocalSourceNode trackUseNode(DataFlow::Node src, TypeTracker t) { + // Declaring `src` to be a `SourceNode` currently causes a redundant check in the + // recursive case, so instead we check it explicitly here. + src instanceof DataFlow::LocalSourceNode and t.start() and use(_, src) and result = src