Skip to content

JS: Add more sources, more unit tests, fixes to the GitHub Actions injection query #12748

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

Merged
merged 35 commits into from
May 15, 2023

Conversation

JarLob
Copy link
Contributor

@JarLob JarLob commented Apr 3, 2023

No description provided.

@JarLob JarLob requested a review from a team as a code owner April 3, 2023 13:04
@github-actions github-actions bot added the JS label Apr 3, 2023
@owen-mc owen-mc changed the title Add more sources, more unit tests, fixes to the GitHub Actions injection query JS: Add more sources, more unit tests, fixes to the GitHub Actions injection query Apr 4, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 2023

QHelp previews:

javascript/ql/src/Security/CWE-094/ExpressionInjection.qhelp

Expression injection in Actions

Using user-controlled input in GitHub Actions may lead to code injection in contexts like run: or script:.

Code injection in GitHub Actions may allow an attacker to exfiltrate any secrets used in the workflow and the temporary GitHub repository authorization token. The token might have write access to the repository, allowing an attacker to use the token to make changes to the repository.

Recommendation

The best practice to avoid code injection vulnerabilities in GitHub workflows is to set the untrusted input value of the expression to an intermediate environment variable and then use the environment variable using the native syntax of the shell/script interpreter (that is, not ${{ env.VAR }}).

It is also recommended to limit the permissions of any tokens used by a workflow such as the GITHUB_TOKEN.

Example

The following example lets a user inject an arbitrary shell command:

on: issue_comment

jobs:
  echo-body:
    runs-on: ubuntu-latest
    steps:
    - run: |
        echo '${{ github.event.comment.body }}'

The following example uses an environment variable, but still allows the injection because of the use of expression syntax:

on: issue_comment

jobs:
  echo-body:
    runs-on: ubuntu-latest
    steps:
    -  env:
        BODY: ${{ github.event.issue.body }}
      run: |
        echo '${{ env.BODY }}'

The following example uses shell syntax to read the environment variable and will prevent the attack:

on: issue_comment

jobs:
  echo-body:
    runs-on: ubuntu-latest
    steps:
    - env:
        BODY: ${{ github.event.issue.body }}
      run: |
        echo '$BODY'

References

@JarLob
Copy link
Contributor Author

JarLob commented Apr 5, 2023

Sorry for keep pushing new changes. I think I'm done now. Ready for review.

@JarLob
Copy link
Contributor Author

JarLob commented Apr 5, 2023

I made the last commit separate on purpose. Maybe it needs to reverted. I wanted to make the message more clear: instead of injection from github.event.comment.body, injection from ${{ github.event.comment.body }}. However I have noticed that VSCode doubles the curly braces so it becomes ${{{{ github.event.comment.body }}}}. I thought I found the way to display it as I need, but just noticed it is now ${ github.event.comment.body } in the terminal where I ran tests.
I'm open to suggestions how to properly display ${{ }} everywhere... Or is it better to revert?

@JarLob
Copy link
Contributor Author

JarLob commented Apr 6, 2023

Since nobody is reviewing :) I have added support for composite actions.

Copy link
Collaborator

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

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

Good stuff. I'm particularly glad to see the modelling of Actions concepts growing, and to see more tests! Some high-level comments about the modelling, and a few minor suggestions on the details.

Copy link
Collaborator

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

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

Good stuff. I'm particularly glad to see the modelling of Actions concepts growing, and to see more tests! Some high-level comments about the modelling, and a few minor suggestions on the details.

@JarLob
Copy link
Contributor Author

JarLob commented Apr 21, 2023

Good to be merged?

@calumgrant calumgrant requested a review from aibaars May 2, 2023 08:49
@calumgrant calumgrant requested a review from asgerf May 9, 2023 09:03
Copy link
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

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

Sorry for the delayed review, @aibaars and I will try to get this merged soon.

JarLob and others added 3 commits May 9, 2023 12:23
Co-authored-by: Asger F <asgerf@github.com>
Co-authored-by: Asger F <asgerf@github.com>
@asgerf asgerf self-assigned this May 15, 2023
@asgerf asgerf merged commit 20e8ee8 into github:main May 15, 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