The Wayback Machine - https://web.archive.org/web/20221128090328/https://github.com/github/codeql/pull/11398
Skip to content
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

Rb: add some more flow through splat parameters #11398

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Nov 23, 2022

This was something I needed in my second-order-command-injection WIP branch.

Precise flow through splat parameters is hard in the general case.
But if we special-case to the situration where both the argument and the parameter are in the first position, then it's easy.

Evaluation looks OK.
The easiest way to see the impact is to look at the new call-edges. Those new edges mostly appear from the receiver being tracked more precisely.
E.g. here where args is a splat parameter that is tracked more precisely with this change.

This is my first venture into the inner workings of the dataflow library, so I hope I got it right.

@github-actions github-actions bot added the Ruby label Nov 23, 2022
@erik-krogh erik-krogh added the no-change-note-required This PR does not need a change note label Nov 24, 2022
@erik-krogh erik-krogh marked this pull request as ready for review Nov 24, 2022
@erik-krogh erik-krogh requested a review from a team as a code owner Nov 24, 2022
Copy link
Contributor

@hvitved hvitved left a comment

I am not convinced that this adds the type of flow that we want; I think it would be better to add support a la how we do it for hash splats.

args = taint(26)
def splatstuff(*x)
sink x # $ hasValueFlow=26
Copy link
Contributor

@hvitved hvitved Nov 24, 2022

Choose a reason for hiding this comment

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

I don't think we want flow in this case; we only want flow to x[n].

Copy link
Contributor Author

@erik-krogh erik-krogh Nov 28, 2022

Choose a reason for hiding this comment

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

I've made it similar to how we handle hash-splat, except I still restrict it to the first argument/parameter.

It still gives me the flow I want in the second-order-command-injection PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note Ruby
2 participants