Skip to content

Conversation

joefarebrother
Copy link
Contributor

This PR models attributes and methods from an UploadedFile that comes from a field of params.
This allows an additional result for command injection in railsgoat to be found.
Closes https://github.com/github/codeql-team/issues/2363.

@joefarebrother joefarebrother changed the title [Draft] Ruby: Model ActiveDispatch::Http::UploadedFile Ruby: Model ActiveDispatch::Http::UploadedFile Mar 14, 2024
@joefarebrother joefarebrother marked this pull request as ready for review March 14, 2024 22:51
@joefarebrother joefarebrother requested a review from a team as a code owner March 14, 2024 22:51
@hvitved hvitved force-pushed the ruby-uploaded-file branch from 84cb919 to e7b00a7 Compare March 15, 2024 09:47
Copy link
Contributor

@alexrford alexrford left a comment

Choose a reason for hiding this comment

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

Just one comment. I think I'm fine with a just a predicate rename & qldoc update for this as I don't think that higher precision in modeling params field accesses will make a difference in practice.

Comment on lines +513 to +514
paramsInstance(),
paramsInstance().getAMethodCall(methodReturnsTaintFromSelf()).getAnElementRead*()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably a bit broader than it needs to be, as it will include field accesses like params[:foo] and params.dig(:foo, :bar, :baz), but also other things like hashes and arrays that are tainted by request parameters (e.g. params, params.deep_dup(), params.values_at(:a, :b)).

This should work fine in practice, but the name and documentation is misleading - this is really describing anything that might be tainted by params in a way that our standard taint tracking might not detect.

@joefarebrother joefarebrother merged commit 4177c38 into github:main Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants