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

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

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

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 (i.e. NOT the ${{ 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.

or
(exists(on.getNode("discussion")) or exists(on.getNode("discussion_comment"))) and
isExternalUserControlledDiscussion(context)
exists(Actions::Env env | isEnvTainted(env, injection, context))

Check warning

Code scanning / CodeQL

Omittable 'exists' variable Warning

This exists variable can be omitted by using a don't-care expression
in this argument
.
(
injection = context
or
exists(Actions::Env env | isEnvTainted(env, injection, context))

Check warning

Code scanning / CodeQL

Omittable 'exists' variable Warning

This exists variable can be omitted by using a don't-care expression
in this argument
.
Job getJob() { result = job }
Job getJob() { result = parent.(Job) }

Runs getRuns() { result = parent.(Runs) }

Check warning

Code scanning / CodeQL

Redundant cast Warning

Redundant cast to
Runs
.
/**
* The env variable name in `${{ env.name }}`
* is where the external user controlled value was assigned to.
*/

Check warning

Code scanning / CodeQL

Predicate QLDoc style. Warning

The QLDoc for a predicate without a result should start with 'Holds'.
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.

None yet

1 participant