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

Pass inputs to env before reading #66

Merged
merged 10 commits into from
Nov 6, 2022
Merged

Conversation

dotboris
Copy link
Contributor

@dotboris dotboris commented Sep 1, 2022

👋 Hello there. My organization's security team has found that this GitHub action is vulnerable to script injection. This kind of vulnerability is described here: https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#understanding-the-risk-of-script-injections

The issue is present because GitHub expressions (${{ ... }}) are used inside run: ... values. If uncontrolled user input is passes in to this action, it would allow an attacker to inject a shell script and execute arbitrary code. The exploitability of this issue depends entire on how users of this action call it. To avoid any potential vulnerabilities for users of this action, I have fixed all script injection issues.

The fix consists of using intermediary environment variables. This ensures that the usual shell variable quoting rules apply correctly and no input can be interpreted as code by bash.

@ludeeus ludeeus changed the title Fix script injection vulnerabilities Pass inputs to env before reading Sep 1, 2022
action.yaml Outdated Show resolved Hide resolved
action.yaml Outdated Show resolved Hide resolved
action.yaml Outdated Show resolved Hide resolved
action.yaml Outdated Show resolved Hide resolved
action.yaml Outdated Show resolved Hide resolved
Comment on lines +159 to +160
exclude_args: ${{ steps.exclude.outputs.excludes }}
additional_file_args: ${{ steps.additional.outputs.files }}
Copy link
Owner

Choose a reason for hiding this comment

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

No need to change outputs.

Copy link
Contributor Author

@dotboris dotboris Sep 1, 2022

Choose a reason for hiding this comment

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

I think that this depends on the output. For example, in the case of steps.exclude.outputs.excludes and steps.additional.outputs.files, their values are a transformation of inputs.ignore_* and inputs.additional_files respectively.

If someone adds a malicious payload into one of those inputs, it can eventually executed when those outputs are injected.

Some of the outputs aren't function of inputs so they could be reverted back but I wanted to be safe. This ensures that scripts can't be injected with reflection.

Copy link
Owner

Choose a reason for hiding this comment

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

They do not?
They are now passed as env?

Copy link
Contributor Author

@dotboris dotboris Sep 1, 2022

Choose a reason for hiding this comment

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

I was referring to the hypothetical case where the code would use an intermediate env var for the input but not the output. I believe that's what you're suggesting here. My understanding is that you're saying that outputs don't need to pass through intermediate variables. I believe that they do need to passed through an intermediate variable.

Here's simplified snippet of what I mean. I'll take the steps.options.outputs.options example here.

    # ...
    - name: Set options
      shell: bash
      id: options
      env:
        # Note: Inputs passed through intermediate env vars
        severity: ${{ inputs.severity }}
        format: ${{ inputs.format }}
      run: |
        declare -a options
        if [[ -n "${severity}" ]]; then
          options+=("-S ${severity}")
        fi
        options+=("--format=${format}")
        echo "::set-output name=options::${options[@]}"

    # ...

    - name: Print information
      shell: bash
      # Note: Outputs passed as is with no intermediate variable as you're suggesting
      run: |
        echo "Files: ${{steps.filepaths.outputs.filepaths}}"
        echo "Excluded: ${{ steps.exclude.outputs.excludes }}"
        echo "Options: ${{ steps.options.outputs.options }}"
        echo "Status code: ${{steps.check.outputs.statuscode}}"
        exit ${{steps.check.outputs.statuscode}}

Getting code execution in this context can be done by calling the action like this:

use: ludeeus/action-shellcheck@master
with:
  format: 'gcc";echo "injected code";echo "'

The injection would happen on the following line:

echo "Options: ${{ steps.options.outputs.options }}"

which would be interpreted as

echo "Options: --format=";echo "injected code"; echo ""

This example is theoretical as I haven't tested it. It may require an extra quote or backslash here and there.

I hope that illustrates my point that outputs that may contain user inputs need to be passed through intermediate env variables as well. For the protection against this kind of attack to work, intermediate variables need to be used for every value that may have included user input.

action.yaml Outdated
Comment on lines 211 to 212
shellcheck_options: ${{ steps.options.outputs.options }}
filepaths: ${{ steps.filepaths.outputs.filepaths }}
Copy link
Owner

Choose a reason for hiding this comment

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

No need to change outputs

action.yaml Outdated
Comment on lines 233 to 237
env:
filepaths: ${{ steps.filepaths.outputs.filepaths }}
exclude_args: ${{ steps.exclude.outputs.excludes }}
shellcheck_options: ${{ steps.options.outputs.options }}
statuscode: ${{ steps.check.outputs.statuscode }}
Copy link
Owner

Choose a reason for hiding this comment

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

No need to change outputs

Comment on lines +61 to +62
format: ${{ inputs.format }}
disable_matcher: ${{ inputs.disable_matcher }}
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the PR 👍

There are no problems here with the expected usage of the inputs, you really need to go out of your way for this to be a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally believe that most people will pass in hard-coded strings to this action so they won't be vulnerable. I think that people can be very creative when it comes to their CI and GitHub actions. I don't them to be accidentally vulnerable to script injection issues.

Also, in it's security recommendations, GH states that it's safe to pass untrusted values to actions is safe: https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#using-an-action-instead-of-an-inline-script-recommended

With that in mind, I think that it's fair to think that people will pass in untrusted data into this action.

Copy link
Owner

Choose a reason for hiding this comment

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

My point was that none of the inputs for this action make sense to have a dynamic based on PR/commit title/content, which is what the goal of this PR was.

BTW that recommendation is laughable, that is not true at all...
GitHub even hosted a CTF around it last year.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My point was that none of the inputs for this action make sense to have a dynamic based on PR/commit title/content, which is what the goal of this PR was.

One example that comes to mind is that someone might want to expose the severity input in such a way that someone writing a PR may opt-into running shellcheck in a more aggressive mode. This could be exposed by having a previous step read the PR title or description to extract a special value that is then passed into this action. For example, a previous step could look for force_shellcheck_severity=... in the description.

Another simpler approach would be to create a manual workflow that exposes the severity option as an input. This workflow could be used by someone who wants to allow devs to run shellcheck in a more aggressive mode in special cases.

These theoretical examples are likely not going to get implemented often but I think that there's reasonable enough that someone may want to something like that. In which case, they would be vulnerable to a script injection attack through this action.

Perhaps it's a question of philosophy. In my mind, when I use an action or a library, I expect it to be secure by default and to treat all inputs as untrusted. For example, if someone passes $(echo hi) as the scandir input, I expect it to look into a directory named $(echo hi) and fail. I don't expect it to actually run echo hi. Here I'm framing this as a security issue but it could also be seen as a regular bug because $(echo hi) is technically a valid directory name and it's not being handled correctly. The example here is convoluted but it's not completely impossible for someone to use $(...) somewhere in a directory name.

@ludeeus
Copy link
Owner

ludeeus commented Sep 3, 2022

A conflict appeared and CI is failing.

@dotboris
Copy link
Contributor Author

dotboris commented Sep 8, 2022

@ludeeus I have fixed the merge conflicts. I believe that I need your approval for the CI to run so that I can figure if it's still breaking and why.

action.yaml Outdated Show resolved Hide resolved
dotboris and others added 2 commits September 8, 2022 09:47
Co-authored-by: Joakim Sørensen <hi@ludeeus.dev>
Now that we're passing in values through environment variables, we don't
need the extra layer of escaping that was present in the code because
the shell script was getting templated by GH actions.

Also, since this layer is now gone, we stop the shell from globbing in
the `check` step to avoid globbing the find arguments.
@dotboris
Copy link
Contributor Author

dotboris commented Sep 8, 2022

@ludeeus I believe that I've fixed all the CI issues. I tested locally and everything worked as expected. I'll need your approval to run the CI again.

Copy link
Owner

@ludeeus ludeeus left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@ludeeus ludeeus merged commit 45e81d0 into ludeeus:master Nov 6, 2022
@dotboris dotboris deleted the fix-injections branch November 7, 2022 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants