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

Fix false-positive script injection warnings when using builtin functions with stable outputs #459

Merged
merged 2 commits into from
Oct 18, 2024

Conversation

IlyaGulya
Copy link
Contributor

Problem

Currently there are cases when actionlint falsely assumes expression to be unsafe when used inside scripts and warns about possible script injection.

For instance, following step description:

- name: Determine if release branch
  run: |
    echo "is_release_branch=${{ contains(github.head_ref, 'release') }}" >> $GITHUB_OUTPUT

would lead to warning:

.github/workflows/test.yaml:17:51: "github.head_ref" 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]

Why do I assume this to be false positive

The only place which I suppose relevant to the issue is this: https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#understanding-the-risk-of-script-injections
Thus, if the point of this lint rule is to ensure that script injection is not possible, example above is definitely false positive.
It's false positive because there's no way for attacker to get some arbitrary output from the contains function.

What is done in this PR?

  1. I've added a parent field to every expression AST node
  2. I'm using this parent field to traverse the AST and search for FunctionCallNode parents
  3. If parent is found and it's Callee considered safe for script injection attacks - we skip all further insecure input checks in this AST leaf.

Which functions considered safe?

Basically, I've chosen only functions which are returning booleans. Maybe there's more, but I decided to start with these:

  • contains
  • startsWith
  • endsWith

P.S.

If there's a better way to implement this - I would love to apply any fixes required for this PR to get into main.
I've added the parent to not pollute the calls to sema.check with additional argument just for this case.
I hope additional memory usage would not be that high, since it's just an additional pointer per AST node.

@rhysd
Copy link
Owner

rhysd commented Oct 18, 2024

Thank you for this patch. It sounds a good addition. I'll take a look at more details later.

"github.event.",
"github.event.",
},
},
Copy link
Owner

@rhysd rhysd Oct 18, 2024

Choose a reason for hiding this comment

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

Instead of removing these tests, would you replace contains with some other unsafe functions? These cases test detecting insecure properties usage inside function calls and should remain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, replaced contains with fromJson

@IlyaGulya IlyaGulya force-pushed the bugfix/false-positive-script-injection branch from 244fe5a to a950ac3 Compare October 18, 2024 06:37
Copy link
Owner

@rhysd rhysd left a comment

Choose a reason for hiding this comment

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

I may do some followup after merging this PR, but changes basically look good to me. Thanks!

@rhysd rhysd merged commit bbe63e9 into rhysd:main Oct 18, 2024
14 checks passed
@IlyaGulya IlyaGulya deleted the bugfix/false-positive-script-injection branch October 18, 2024 12:13
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