Sitelet https://web.archive.org/web/20260528175614/https://github.com/github/codeql/pull/7568
Skip to content

Ruby: taint steps for pattern matches#7568

Merged
aibaars merged 13 commits into
github:mainfrom
aibaars:ruby-pattern-matching-taint
Jan 26, 2022
Merged

Ruby: taint steps for pattern matches#7568
aibaars merged 13 commits into
github:mainfrom
aibaars:ruby-pattern-matching-taint

Conversation

@aibaars
Copy link
Copy Markdown
Contributor

@aibaars aibaars commented Jan 11, 2022 •

This pull request adds taint steps from the value of a case expression to the variables in the patterns the value is matched against.

@github-actions github-actions Bot added the Ruby label Jan 11, 2022
@aibaars aibaars marked this pull request as ready for review January 17, 2022 11:38
@aibaars aibaars requested a review from a team as a code owner January 17, 2022 11:38
Copy link
Copy Markdown
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Could you please start a DCA run as well?

Comment thread ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll Outdated
Comment thread ruby/ql/lib/codeql/ruby/dataflow/internal/TaintTrackingPrivate.qll Outdated
Comment thread ruby/ql/test/library-tests/dataflow/local/local_dataflow.rb
Comment thread ruby/ql/lib/codeql/ruby/ast/internal/Variable.qll
@aibaars aibaars force-pushed the ruby-pattern-matching-taint branch from 9987006 to 139e8a0 Compare January 18, 2022 14:42
Comment thread ruby/ql/test/library-tests/dataflow/local/local_dataflow.rb
@aibaars aibaars force-pushed the ruby-pattern-matching-taint branch 2 times, most recently from d85f630 to ca2540c Compare January 19, 2022 15:19
Copy link
Copy Markdown
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Some trivial comments.

Comment thread ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll Outdated
Comment thread ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll Outdated
Comment thread ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll Outdated
Comment thread ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll Outdated
Comment thread ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll Outdated
Comment thread ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll Outdated
Comment thread ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll Outdated
Comment thread ruby/ql/lib/codeql/ruby/dataflow/internal/TaintTrackingPrivate.qll Outdated
Comment thread ruby/ql/test/library-tests/dataflow/local/local_dataflow.rb Outdated
@aibaars aibaars force-pushed the ruby-pattern-matching-taint branch from 2987d0c to 91a3dca Compare January 24, 2022 09:34
Comment thread ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll Outdated
@aibaars aibaars force-pushed the ruby-pattern-matching-taint branch from 91a3dca to 78b4d7c Compare January 24, 2022 10:27
Joining on variable name alone is a bad thing:

```
[2022-01-25 11:13:20] (228s) Tuple counts for Variable::Cached::access#ff#shared/3@868b54tu after 3m37s:
                      112554    ~0%     {3} r1 = JOIN Variable::VariableReal::getNameImpl_dispred#ff WITH Variable::VariableReal::getDeclaringScopeImpl_dispred#ff ON FIRST 1 OUTPUT Lhs.1, Lhs.0 'arg2', Rhs.1 'arg1'
                      561015756 ~1%     {3} r2 = JOIN r1 WITH Variable::variableName#ff_10#join_rhs ON FIRST 1 OUTPUT Rhs.1 'arg0', Lhs.2 'arg1', Lhs.1 'arg2'
                                        return r2
```

This change ensures that we join on name and scope simultaneously.
Copy link
Copy Markdown
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance looks good now.

@aibaars aibaars merged commit 948ebe4 into github:main Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants