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.

Sign up for GitHub

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

Jump to bottom

Pass inputs to env before reading #66

Merged
merged 10 commits into from
Nov 6, 2022
Merged

Pass inputs to env before reading #66

Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
27a08b3
Use GITHUB_ACTION_PATH variable instead of ${{ github.action_path }}
dotboris Sep 1, 2022
7bc2356
Use $RUNNER_OS instead of ${{ runner.os }}
dotboris Sep 1, 2022
046cc62
Use ${...} syntax for env vars
dotboris Sep 1, 2022
1abc20e
Inject all inputs through environment variables
dotboris Sep 1, 2022
9efe230
Pass all step outputs as env vars
dotboris Sep 1, 2022
1b56179
Revert to using github.action_path and runner.os expr
dotboris Sep 1, 2022
f93c360
Merge branch 'master' into fix-injections
dotboris Sep 8, 2022
3c8c1f0
Use output directly for status code
dotboris Sep 8, 2022
f2b0110
Adjust escaping and globbing to work with env vars passing
dotboris Sep 8, 2022
5e9d5c0
Merge branch 'master' into fix-injections
ludeeus Nov 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 53 additions & 30 deletions action.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,22 +58,26 @@ runs:
steps:
- name: Enable problem-matcher
shell: bash
env:
format: ${{ inputs.format }}
disable_matcher: ${{ inputs.disable_matcher }}
Comment on lines +62 to +63
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.

run: |
problem_matcher_file="${{ github.action_path }}/.github/problem-matcher-${{ inputs.format }}.json"
if [[ ${{ inputs.disable_matcher }} != "true" && -f "$problem_matcher_file" ]]; then
problem_matcher_file="${{ github.action_path }}/.github/problem-matcher-${format}.json"
if [[ "${disable_matcher}" != "true" && -f "$problem_matcher_file" ]]; then
echo "::add-matcher::$problem_matcher_file"
fi

- name: Download shellcheck
shell: bash
env:
scversion: ${{ inputs.version }}
run: |
if [[ "${{ runner.os }}" == "macOS" ]]; then
osvariant="darwin"
else
osvariant="linux"
fi

scversion="${{ inputs.version }}"
baseurl="https://github.com/koalaman/shellcheck/releases/download"

curl -Lso "${{ github.action_path }}/sc.tar.xz" \
Expand All @@ -91,42 +95,49 @@ runs:
- name: Set options
shell: bash
id: options
env:
severity: ${{ inputs.severity }}
format: ${{ inputs.format }}
run: |
declare -a options
if [[ -n "${{ inputs.severity }}" ]]; then
options+=("-S ${{ inputs.severity }}")
if [[ -n "${severity}" ]]; then
options+=("-S ${severity}")
fi
options+=("--format=${{ inputs.format }}")
options+=("--format=${format}")
echo "options=${options[@]}" >> $GITHUB_OUTPUT

- name: Gather excluded paths
shell: bash
id: exclude
env:
ignore: ${{ inputs.ignore }}
ignore_paths: ${{ inputs.ignore_paths }}
ignore_names: ${{ inputs.ignore_names }}
run: |
declare -a excludes
set -f # temporarily disable globbing so that globs in input aren't expanded

excludes+=("! -path \"*./.git/*\"")
excludes+=("! -path \"*.go\"")
excludes+=("! -path \"*/mvnw\"")
if [[ -n "${{ inputs.ignore }}" ]]; then
excludes+=("! -path *./.git/*")
excludes+=("! -path *.go")
excludes+=("! -path */mvnw")
if [[ -n "${ignore}" ]]; then
echo "::warning::ignore is deprecated. Please use ignore_paths instead"
for path in ${{ inputs.ignore }}; do
for path in ${ignore}; do
echo "::debug:: Adding '$path' to excludes"
excludes+=("! -path \"*./$path/*\"")
excludes+=("! -path \"*/$path/*\"")
excludes+=("! -path \"$path\"")
excludes+=("! -path *./$path/*")
excludes+=("! -path */$path/*")
excludes+=("! -path $path")
done
else
for path in ${{ inputs.ignore_paths }}; do
for path in ${ignore_paths}; do
echo "::debug:: Adding '$path' to excludes"
excludes+=("! -path \"*./$path/*\"")
excludes+=("! -path \"*/$path/*\"")
excludes+=("! -path \"$path\"")
excludes+=("! -path *./$path/*")
excludes+=("! -path */$path/*")
excludes+=("! -path $path")
done
fi

for name in ${{ inputs.ignore_names }}; do
for name in ${ignore_names}; do
echo "::debug:: Adding '$name' to excludes"
excludes+=("! -name $name")
done
Expand All @@ -137,26 +148,36 @@ runs:
- name: Gather additional files
shell: bash
id: additional
env:
additional_files: ${{ inputs.additional_files }}
run: |
declare -a files
for file in ${{ inputs.additional_files }}; do
echo "::debug:: Adding '$file' to excludes"
files+=("-o -name \"*$file\"")
for file in ${additional_files}; do
echo "::debug:: Adding '$file' to additional files"
files+=("-o -name *$file")
done
echo "files=${files[@]}" >> $GITHUB_OUTPUT

- name: Run the check
shell: bash
id: check
env:
scandir: ${{ inputs.scandir }}
check_together: ${{ inputs.check_together }}
exclude_args: ${{ steps.exclude.outputs.excludes }}
additional_file_args: ${{ steps.additional.outputs.files }}
Comment on lines +167 to +168
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 •
edited
Loading

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 •
edited
Loading

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.

shellcheck_options: ${{ steps.options.outputs.options }}
run: |
statuscode=0
declare -a filepaths
shebangregex="^#! */[^ ]*/(env *)?[abk]*sh"

set -f # temporarily disable globbing so that globs in inputs aren't expanded

while IFS= read -r -d '' file; do
filepaths+=("$file")
done < <(find "${{ inputs.scandir }}" \
${{ steps.exclude.outputs.excludes }} \
done < <(find "${scandir}" \
${exclude_args} \
-type f \
'(' \
-name '*.bash' \
Expand Down Expand Up @@ -185,34 +206,36 @@ runs:
-o -path '*/.profile' \
-o -path '*/profile' \
-o -name '*.shlib' \
${{ steps.additional.outputs.files }} \
${additional_file_args} \
')' \
-print0)

while IFS= read -r -d '' file; do
head -n1 "$file" | grep -Eqs "$shebangregex" || continue
filepaths+=("$file")
done < <(find "${{ inputs.scandir }}" \
${{ steps.exclude.outputs.excludes }} \
done < <(find "${scandir}" \
${exclude_args} \
-type f ! -name '*.*' -perm /111 \
-print0)

if [[ -n "${{ inputs.check_together }}" ]]; then
if [[ -n "${check_together}" ]]; then
"${{ github.action_path }}/shellcheck" \
${{ steps.options.outputs.options }} \
${shellcheck_options} \
"${filepaths[@]}" || statuscode=$?
else
for file in "${filepaths[@]}"; do
echo "::debug::Checking '$file'"
"${{ github.action_path }}/shellcheck" \
${{ steps.options.outputs.options }} \
${shellcheck_options} \
"$file" || statuscode=$?
done
fi

echo "filepaths=${filepaths[@]}" >> $GITHUB_OUTPUT
echo "statuscode=$statuscode" >> $GITHUB_OUTPUT

set +f # re-enable globbing

- name: Exit action
shell: bash
run: exit ${{steps.check.outputs.statuscode}}