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

Dangerous Workflow: some user input are not being detected as untrusted input. #3915

Open
diogoteles08 opened this issue Mar 4, 2024 · 2 comments
Labels

Comments

@diogoteles08
Copy link
Contributor

diogoteles08 commented Mar 4, 2024

Is your feature request related to a problem? Please describe.
The current Dangerous Workflow check looks for places where untrusted user inputs could be used for Script Injection, such as:

- name: Check title
  run: |
    title="${{ github.event.issue.title }}"
    if [[ ! $title =~ ^.*:\ .*$ ]]; then
      echo "Bad issue title"
      exit 1
    fi

The problem is that Scorecard is not considering the whole range of user input that might lead to Script Injection. The piece of code that lists the user inputs is the following:

func containsUntrustedContextPattern(variable string) bool {
        // GitHub event context details that may be attacker controlled.
        // See https://securitylab.github.com/research/github-actions-untrusted-input/
        untrustedContextPattern := regexp.MustCompile(
                `.*(issue\.title|` +
                        `issue\.body|` +
                        `pull_request\.title|` +
                        `pull_request\.body|` +
                        `comment\.body|` +
                        `review\.body|` +
                        `review_comment\.body|` +
                        `pages.*\.page_name|` +
                        `commits.*\.message|` +
                        `head_commit\.message|` +
                        `head_commit\.author\.email|` +
                        `head_commit\.author\.name|` +
                        `commits.*\.author\.email|` +
                        `commits.*\.author\.name|` +
                        `pull_request\.head\.ref|` +
                        `pull_request\.head\.label|` +
                        `pull_request\.head\.repo\.default_branch).*`)


        if strings.Contains(variable, "github.head_ref") {
                return true
        }
        return strings.Contains(variable, "github.event.") && untrustedContextPattern.MatchString(variable)
}

And I can point that it isn't able to detect the following inputs, for example:

  • ${{ github.event.issue_comment.comment.body }}, described in github's doc
  • ${{ github.event.commit_comment.comment.body }}, described in github's doc
  • ${{ github.event.fork.forkee.name }}

Describe the solution you'd like
As Dangerous Workflow is a very important check, Scorecard should be able to identify as many Script Injection risks as possible.

We should study more types of GitHub Events, their payloads, and make Scorecard as reliable as possible to identify different ways that Script Injections could take place.

Additional context
I'd be happy to complete my search and also raise a PR to fix this, but I won't be able to do this in a near future. However, I'm marking this as "Good First issue", as the biggest effort to solve this would be actually a research on GitHub's API. The changes on Scorecard would be simple.

@spencerschrock
Copy link
Contributor

And I can point that it isn't able to detect the following inputs, for example:

  • ${{ github.event.issue_comment.comment.body }}, described in github's doc
  • ${{ github.event.commit_comment.comment.body }}, described in github's doc

We detect the first two at least:
https://go.dev/play/p/Saz4FZ7j2A-

But yeah, there are probably more to consider. #3554, #2236, etc.

@diogoteles08
Copy link
Contributor Author

Thanks Spencer! I failed on the regex mental compilation ><
@pnacht also pointed out that ${{ github.event.fork.forkee.name }} shouldn't be considered dangerous, because the rules for repo names make it impossible to craft a malicious name (no spaces, no slashes, etc).

But I've found the following ones that are probably dangerous and not covered by scorecard:

  • "${{ github.event.discussion.body }}"
  • "${{ github.event.discussion.title }}"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Looked at during triage meetings
Development

No branches or pull requests

2 participants