Skip to content

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented May 1, 2023

Adds sources and sinks based on this PR from @R3x with comments from @JarLob.

This PR is essentially a port of that PR with the following differences:

  • Use API graphs, to ensure global tracking of intermediate values
  • Deduplicate code using set literals and intermediate predicates
  • Populate concepts rather than adding new queries
  • Target mainline instead of experimental
  • Write tests closer to how we normally do it

I couldn't foresee a practical way to get the original PR to this state via code reviews, hence this new PR.

But I'd like to give credit for @R3x and @JarLob for authoring the models. These were just some changes needed to get the models into a format where we can use it outside of experimental.

@asgerf asgerf force-pushed the js/github-actions-sources branch from b24595f to 5eaaa7e Compare May 1, 2023 09:43
@asgerf asgerf marked this pull request as ready for review May 2, 2023 09:03
@asgerf asgerf requested review from a team as code owners May 2, 2023 09:03
@R3x
Copy link

R3x commented May 2, 2023

Hi @asgerf, Thanks for the updates.

Is it possible to not combine these sources? I noticed that the same taint source is used for both the inputs as well as context. I feel that splitting these up makes more sense.

Github Context directly to JS sink is of higher severity, than the Action Input to Sink or the Environment variable to the sink.

If there is a Github Context to Sink vulnerability then it affects all the workflows that are used by the action, whereas if it's the latter, then it affects only the workflows that are passing tainted input into these Actions either as an Input or as an environment variable.

Also, can we also taint the environment variables? I used a list to filter out the names of the ones that are set by the runner, the others can still be tainted. Note that this query can suffer from some loss in precision - if the action itself is setting the environment variable and then using it (although I haven't seen it in practice).

Copy link

@R3x R3x left a comment

Choose a reason for hiding this comment

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

Should this be in javascript/ql/src/change-notes instead - it was added in ruby?

@github-actions github-actions bot removed the Ruby label May 3, 2023
@asgerf
Copy link
Contributor Author

asgerf commented May 3, 2023

Is it possible to not combine these sources? I noticed that the same taint source is used for both the inputs as well as context. I feel that splitting these up makes more sense.

There is some work in progress that would enable a more fine-grained classification of taint sources, but for the moment we have to either include these taint sources or not. But I've made it more clear in the source code that there is a difference in severity so it's easier to review in the future.

Also, can we also taint the environment variables?

As mentioned above, we don't currently have fine-grained classification of sources. For now I've updated IndirectCommandInjection to block flow through the environment variables you listed. This query treats environment variables as taint sources.

Should this be in javascript/ql/src/change-notes instead - it was added in ruby?

Thanks for pointing that out, obviously it should be in the JS folder

@asgerf
Copy link
Contributor Author

asgerf commented May 4, 2023

After reviewing some more of the results this change would produce, we're going to restrict the getInput() source to just the indirect command-injection query for now.

@calumgrant calumgrant requested a review from erik-krogh May 9, 2023 09:00
erik-krogh
erik-krogh previously approved these changes May 9, 2023
Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

Single optional comment, there was one predicate you had not made private.

…CommandInjectionCustomizations.qll

Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>
@asgerf asgerf merged commit c376eeb into github:main May 10, 2023
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