Skip to content

Ruby: Content flow through captured variables #10844

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Oct 14, 2022

We were previously supporting direct flow through captured variables:

x = taint1
[1, 2, 3].each do |i|
    sink x # `taint1` flows in
end

[1, 2, 3].each do |i|
    y = taint2
end

sink y # `taint2` flows out

However, we were not supporting flow through contents (e.g. fields) on captured variables:

class C
    def set_field x
        @field = x
    end
    def get_field
        return @field
    end
end

x = C.new
x.set_field(taint1)
[1, 2, 3].each do |i|
    sink(x.get_field) # `taint1` flows in
end

y = C.new
[1, 2, 3].each do |i|
    y.set_field(taint2)
end

sink(y.get_field) # `taint2` flows out

Luckily, the existing SSA library provides the bits that we need to also cover the cases above:

  • The first call to each already gives rise to an implicit read of c, so instead of limiting flow in to be the SSA definition for the implicit read, we do like with normal SSA flow and consider all adjacent accesses (reads or the definition itself) to flow in (that is, it is no longer x = C.new that flows in, but instead the post-update node for x in x.set_field(taint1)).
  • Again, the second call to each already gives rise to an implicit read of c, so we simply need to add flow from the post-update of y in y.set_field(taint2) to all adjacent reads/phi nodes that immediately follow the implicit read at the call to each.

@github-actions github-actions bot added the Ruby label Oct 14, 2022
@hvitved hvitved force-pushed the ruby/capture-flow-side-effects branch 3 times, most recently from c085d9f to dabf77c Compare October 17, 2022 10:03
@hvitved hvitved force-pushed the ruby/capture-flow-side-effects branch from dabf77c to d88744f Compare October 17, 2022 10:57
@hvitved hvitved force-pushed the ruby/capture-flow-side-effects branch 5 times, most recently from 689c527 to 4f80b4c Compare October 18, 2022 14:00
@hvitved hvitved force-pushed the ruby/capture-flow-side-effects branch from 4f80b4c to 30c8417 Compare December 7, 2022 13:45
@@ -7,6 +7,8 @@
import codeql.ruby.dataflow.internal.DataFlowImplForRegExp
import PathGraph

predicate stats = stageStats/8;

Check warning

Code scanning / CodeQL

Dead code

This code is never used, and it's not publicly exported.
@hvitved hvitved force-pushed the ruby/capture-flow-side-effects branch from 30c8417 to 5b50d20 Compare December 7, 2022 14:04
@hvitved
Copy link
Contributor Author

hvitved commented Aug 30, 2023

Superseded by #11725

@hvitved hvitved closed this Aug 30, 2023
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.

1 participant