Skip to content

Conversation

R3x
Copy link

@R3x R3x commented Apr 6, 2023

Added 3 Queries, that track flows from -

  1. Action Input to Sinks
  2. Action Github Context to Sinks
  3. Action Environment to Sinks

@R3x R3x requested a review from a team as a code owner April 6, 2023 14:06
@github-actions github-actions bot added the JS label Apr 6, 2023
@calumgrant calumgrant requested a review from asgerf April 6, 2023 14:08
@R3x
Copy link
Author

R3x commented Apr 6, 2023

@kevinbackhouse @JarLob - I have added the sinks that are of higher priority here. During the analysis - we also marked some lower-priority sinks (things such as fs.write etc). Which basically provides primitives such as an arbitrary file write - but not as severe as the ones here. Do you prefer that I add them as well?

@kevinbackhouse
Copy link
Contributor

@kevinbackhouse @JarLob - I have added the sinks that are of higher priority here. During the analysis - we also marked some lower-priority sinks (things such as fs.write etc). Which basically provides primitives such as an arbitrary file write - but not as severe as the ones here. Do you prefer that I add them as well?

I think it's better to start with just the high-priority ones, so that you have a lower false-positive rate.

Comment on lines +59 to +61
srcidx = source.getNode().asExpr() and envname = srcidx.getPropertyName()
or
srcdot = source.getNode().asExpr() and envname = srcdot.getPropertyName()

Check warning

Code scanning / CodeQL

Var only used in one side of disjunct.

The [variable srcidx](1) is only used in one side of disjunct. The [variable srcdot](2) is only used in one side of disjunct.
Copy link
Contributor

@JarLob JarLob left a comment

Choose a reason for hiding this comment

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

Reformatting of the added lines may be needed.

R3x and others added 3 commits April 21, 2023 15:45
Co-authored-by: Jaroslav Lobačevski <jarlob@github.com>
…ToSink.ql

Co-authored-by: Jaroslav Lobačevski <jarlob@github.com>
…ToSink.ql

Co-authored-by: Jaroslav Lobačevski <jarlob@github.com>
R3x and others added 2 commits April 21, 2023 19:41
…ToSink.ql

Co-authored-by: Jaroslav Lobačevski <jarlob@github.com>
@asgerf
Copy link
Contributor

asgerf commented May 1, 2023

Hi @R3x and @JarLob 👋

First of all, sorry for the long radio silence.

These models seem very valuable, and I'd like to get them into the mainline outside experimental. However, I foresee some issues with code duplication, since this PR adds new variants of existing queries containing sinks we already have models for. It's difficult for us to maintain code with such duplication. Rather than merging to experimental and then do a round of clean-up afterwards, I've opened a PR adding the new sources as well as @actions/exec to mainline.

I haven't ported the "Actions Environment to Sink" query as I'm still not quite sure what the threat model is, but I'm thinking it could be part of js/shell-command-injection-from-environment

@R3x
Copy link
Author

R3x commented May 3, 2023

Closing this. since we have #12978

@R3x R3x closed this May 3, 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.

4 participants