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

Add new script injection input #332

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

Conversation

hugo-syn
Copy link
Contributor

@hugo-syn hugo-syn commented Aug 3, 2023

I've added a new entry in BuiltinUntrustedInputs because I already saw something like this where It's possible to inject code in the runner from an opened issue:

name: Test

on:
  issues:
    types: [opened]

jobs:
  dummyjob:
    runs-on: ubuntu-latest
    steps:
      - name: parse issue Body
        id: prepareBody
        shell: pwsh
        run: |
          # this script parse issue and set env variables
          .\scripts\issue-body-parser.ps1 $env:BODY
        env:
          BODY: ${{ github.event.issue.body }}
      
      - name: Injection step
        shell: pwsh
        run: |
          
          # InjectionEntry comes from issue-body-parser.ps1 
          # which parse the body and do something like this
          # Write-Output "InjectionEntry=$dataparsedFromIssue" | Out-File -FilePath $Env:GITHUB_ENV -Encoding utf8 -Append

          .\scripts\my-script.ps1 -param ${{ env.InjectionEntry}}
        
./main env-inject.yml       
env-inject.yml :22:265: "env.*" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions for more details [expression]

I changed a lot of tests to make it work I hope it will be ok.

@rhysd
Copy link
Owner

rhysd commented Aug 4, 2023

Unfortunately this PR is not acceptable straight forward because

  • The NewUntrustedInputMap("env", NewUntrustedInputMap("*")) filter is not correct. * indicates array elements and the env context is an object, not array.
  • Unit tests are modified without understanding what they are doing. These modifications can avoid test failures, but they don't fix the failures. You removes many correct test cases.

But it's good point that accessing env context can cause XSS. The problem you want to fix by this PR is that referring env context in run: script may contain external inputs. And your solution is simply banning env context access. Am I correct?

@hugo-syn
Copy link
Contributor Author

hugo-syn commented Aug 4, 2023

Sorry for the valid tests that I removed.

The problem I want to fix with this PR is the use of this expression in run scripts ${{env.CustomVariable}} as it can be used to inject arbitrary commands from other tasks that write to the environment variable. It's the same idea with ${{ github.event.pull_request.title }} but here untrusted data comes from other tasks that write to environment.

The solution is not to ban ${{env.CustomVariable}} but to use classical environment access in bash $CustomVariable or in PowerShell $ENV:CustomVariable otherwise the workflow templating re-introduce the command injection vulnerability.

We could change the * character in the UntrustedInputMap, one to match an array and one wildcard character ?

@hugo-syn
Copy link
Contributor Author

hugo-syn commented Aug 4, 2023

I propose a new solution for matching all element like in env I'm using ** but I think we can use other chars If you want. I was able to restore most of the tests tell me if it's better.

Sorry for all the commits I was confused with my branches and my local repo.

@rhysd
Copy link
Owner

rhysd commented Aug 5, 2023

I think banning env context requires discussion. I'll make an issue for it since PR should focus on the implementation.

@hugo-syn
Copy link
Contributor Author

Hi @rhysd any update on this PR ? I've opened an issue with more details as you asked :)

@hugo-syn
Copy link
Contributor Author

hugo-syn commented Jun 20, 2024

Hi @rhysd , I've reverted the env context from BuiltinUntrustedInputs since you don't want to ban the following syntax (which I understand):

echo {{ env.EnvVariable}}

However, I've kept the possibility to add wildcard entry to this UntrustedInputMap. For example, I'm using actionlint as a library and I can add entries to the BuiltinUntrustedInputs like this:

var envUntrustedInput = actionlint.NewUntrustedInputMap("env",
	actionlint.NewUntrustedInputMap("**"),
)

...
untrustedInputSearchRoots := actionlint.BuiltinUntrustedInputs
untrustedInputSearchRoots.AddRoot(envUntrustedInput)

This way it doesn't impact actionlint but offer the possibility to catch other untrusted inputs.

In my context I can catch this:

test/test.yml:58:24: Expression injection, "env.**" is potentially untrusted. [expression-injection]
   |
58 |         run: echo "${{ env.FOO }}"
   |                        ^~~~~~~

I hope this suits you better. All the tests are OK so this should be better for you :) Don't hesitate if you have any remarks !

@hugo-syn
Copy link
Contributor Author

Hi @rhysd any comment since my last update ? Do you need additional information or something to help you with this PR ?

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