From 5735bb698dc7ca94d8f4e432028c7fdf8f094360 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Mon, 22 Nov 2021 14:21:43 +0100 Subject: [PATCH] Ruby: Hide desugared nodes in data-flow paths --- .../codeql/ruby/ast/internal/Synthesis.qll | 17 ++++++++++++++ .../dataflow/internal/DataFlowPrivate.qll | 3 +++ ruby/ql/lib/codeql/ruby/printAst.qll | 22 ++----------------- ruby/ql/test/library-tests/ast/AstDesugar.ql | 2 +- .../security/cwe-079/ReflectedXSS.expected | 4 +--- 5 files changed, 24 insertions(+), 24 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/ast/internal/Synthesis.qll b/ruby/ql/lib/codeql/ruby/ast/internal/Synthesis.qll index 1afef58cd49..83c0f997215 100644 --- a/ruby/ql/lib/codeql/ruby/ast/internal/Synthesis.qll +++ b/ruby/ql/lib/codeql/ruby/ast/internal/Synthesis.qll @@ -126,6 +126,23 @@ private class Desugared extends AstNode { */ int desugarLevel(AstNode n) { result = count(Desugared desugared | n = desugared.getADescendant()) } +/** + * Holds if `n` appears in a context that is desugared. That is, a + * transitive, reflexive parent of `n` is a desugared node. + */ +predicate isInDesugeredContext(AstNode n) { n = any(AstNode sugar).getDesugared().getAChild*() } + +/** + * Holds if `n` is a node that only exists as a result of desugaring some + * other node. + */ +predicate isDesugarNode(AstNode n) { + n = any(AstNode sugar).getDesugared() + or + isInDesugeredContext(n) and + forall(AstNode parent | parent = n.getParent() | parent.isSynthesized()) +} + /** * Use this predicate in `Synthesis::child` to generate an assignment of `value` to * synthesized variable `v`, where the assignment is a child of `assignParent` at diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll index e5b8e1af85e..ee6c6442ec5 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll @@ -1,4 +1,5 @@ private import ruby +private import codeql.ruby.ast.internal.Synthesis private import codeql.ruby.CFG private import codeql.ruby.dataflow.SSA private import DataFlowPublic @@ -274,6 +275,8 @@ predicate nodeIsHidden(Node n) { def instanceof Ssa::PhiNode ) or + isDesugarNode(n.(ExprNode).getExprNode().getExpr()) + or n instanceof SummaryNode or n instanceof SummaryParameterNode diff --git a/ruby/ql/lib/codeql/ruby/printAst.qll b/ruby/ql/lib/codeql/ruby/printAst.qll index 2f8da6c75ae..b04a9fec50a 100644 --- a/ruby/ql/lib/codeql/ruby/printAst.qll +++ b/ruby/ql/lib/codeql/ruby/printAst.qll @@ -8,13 +8,7 @@ private import AST private import codeql.ruby.security.performance.RegExpTreeView as RETV - -/** Holds if `n` appears in the desugaring of some other node. */ -predicate isDesugared(AstNode n) { - n = any(AstNode sugar).getDesugared() - or - isDesugared(n.getParent()) -} +private import codeql.ruby.ast.internal.Synthesis /** * The query can extend this class to control which nodes are printed. @@ -25,19 +19,7 @@ class PrintAstConfiguration extends string { /** * Holds if the given node should be printed. */ - predicate shouldPrintNode(AstNode n) { - not isDesugared(n) - or - not n.isSynthesized() - or - n.isSynthesized() and - not n = any(AstNode sugar).getDesugared() and - exists(AstNode parent | - parent = n.getParent() and - not parent.isSynthesized() and - not n = parent.getDesugared() - ) - } + predicate shouldPrintNode(AstNode n) { not isDesugarNode(n) } predicate shouldPrintAstEdge(AstNode parent, string edgeName, AstNode child) { child = parent.getAChild(edgeName) and diff --git a/ruby/ql/test/library-tests/ast/AstDesugar.ql b/ruby/ql/test/library-tests/ast/AstDesugar.ql index ab7036adc48..d10b0c80071 100644 --- a/ruby/ql/test/library-tests/ast/AstDesugar.ql +++ b/ruby/ql/test/library-tests/ast/AstDesugar.ql @@ -8,7 +8,7 @@ import codeql.ruby.ast.internal.Synthesis class DesugarPrintAstConfiguration extends PrintAstConfiguration { override predicate shouldPrintNode(AstNode n) { - isDesugared(n) + isInDesugeredContext(n) or exists(n.getDesugared()) } diff --git a/ruby/ql/test/query-tests/security/cwe-079/ReflectedXSS.expected b/ruby/ql/test/query-tests/security/cwe-079/ReflectedXSS.expected index 9f9f21e346d..0678f3896df 100644 --- a/ruby/ql/test/query-tests/security/cwe-079/ReflectedXSS.expected +++ b/ruby/ql/test/query-tests/security/cwe-079/ReflectedXSS.expected @@ -1,8 +1,7 @@ edges | app/controllers/foo/bars_controller.rb:9:12:9:17 | call to params : | app/controllers/foo/bars_controller.rb:9:12:9:29 | ...[...] : | | app/controllers/foo/bars_controller.rb:9:12:9:29 | ...[...] : | app/views/foo/bars/show.html.erb:47:5:47:13 | call to user_name | -| app/controllers/foo/bars_controller.rb:13:5:13:37 | ... = ... : | app/views/foo/bars/show.html.erb:51:5:51:18 | call to user_name_memo | -| app/controllers/foo/bars_controller.rb:13:20:13:25 | call to params : | app/controllers/foo/bars_controller.rb:13:5:13:37 | ... = ... : | +| app/controllers/foo/bars_controller.rb:13:20:13:25 | call to params : | app/views/foo/bars/show.html.erb:51:5:51:18 | call to user_name_memo | | app/controllers/foo/bars_controller.rb:17:21:17:26 | call to params : | app/controllers/foo/bars_controller.rb:17:21:17:36 | ...[...] : | | app/controllers/foo/bars_controller.rb:17:21:17:36 | ...[...] : | app/views/foo/bars/show.html.erb:2:18:2:30 | @user_website | | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params : | app/controllers/foo/bars_controller.rb:19:22:19:23 | dt : | @@ -21,7 +20,6 @@ edges nodes | app/controllers/foo/bars_controller.rb:9:12:9:17 | call to params : | semmle.label | call to params : | | app/controllers/foo/bars_controller.rb:9:12:9:29 | ...[...] : | semmle.label | ...[...] : | -| app/controllers/foo/bars_controller.rb:13:5:13:37 | ... = ... : | semmle.label | ... = ... : | | app/controllers/foo/bars_controller.rb:13:20:13:25 | call to params : | semmle.label | call to params : | | app/controllers/foo/bars_controller.rb:17:21:17:26 | call to params : | semmle.label | call to params : | | app/controllers/foo/bars_controller.rb:17:21:17:36 | ...[...] : | semmle.label | ...[...] : |