Skip to content

Fix critical GHA vulnerability#8100

Closed
Inverle wants to merge 1 commit intoFreshRSS:edgefrom
Inverle:actions-security-fix
Closed

Fix critical GHA vulnerability#8100
Inverle wants to merge 1 commit intoFreshRSS:edgefrom
Inverle:actions-security-fix

Conversation

@Inverle
Copy link
Member

@Inverle Inverle commented Oct 12, 2025

See #8099

The commands action is currently disabled, needs some more improvement.
We need to make sure that the code within the PR cannot access GITHUB_TOKEN.

I am not happy with having to grant contents: write permission so that commits can be added into the current PR, so some workaround is needed if possible. Otherwise it is safer to restrict permissions to just owner/member, and check PR contents before running /fix-all.

Current zizmor output:

info[template-injection]: code injection via template expansion
   --> ./commands.yml:140:37
    |
136 |         uses: actions/github-script@v8
    |         ------------------------------ action accepts arbitrary code
...
139 |           script: |
    |           ------ via this input
140 |             const makeFailed = '${{ steps.fix.outputs.make_failed }}' === 'true';
    |                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ may expand into attacker-controllable code
    |
    = note: audit confidence → Low

info[template-injection]: code injection via template expansion
   --> ./commands.yml:141:36
    |
136 |         uses: actions/github-script@v8
    |         ------------------------------ action accepts arbitrary code
...
139 |           script: |
    |           ------ via this input
140 |             const makeFailed = '${{ steps.fix.outputs.make_failed }}' === 'true';
141 |             const noChanges = '${{ steps.commit.outputs.no_changes || 'false' }}' === 'true';
    |                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ may expand into attacker-controllable code
    |
    = note: audit confidence → Low

info[template-injection]: code injection via template expansion
   --> ./commands.yml:142:33
    |
136 |         uses: actions/github-script@v8
    |         ------------------------------ action accepts arbitrary code
...
139 |           script: |
    |           ------ via this input
...
142 |             const pushed = '${{ steps.commit.outputs.pushed || 'false' }}' === 'true';
    |                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^ may expand into attacker-controllable code
    |
    = note: audit confidence → Low

info[template-injection]: code injection via template expansion
   --> ./commands.yml:143:36
    |
136 |         uses: actions/github-script@v8
    |         ------------------------------ action accepts arbitrary code
...
139 |           script: |
    |           ------ via this input
...
143 |             const commitSha = '${{ steps.commit.outputs.commit_sha }}';
    |                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ may expand into attacker-controllable code
    |
    = note: audit confidence → Low

info[template-injection]: code injection via template expansion
   --> ./commands.yml:144:36
    |
136 |         uses: actions/github-script@v8
    |         ------------------------------ action accepts arbitrary code
...
139 |           script: |
    |           ------ via this input
...
144 |             const jobStatus = '${{ job.status }}';
    |                                    ^^^^^^^^^^ may expand into attacker-controllable code
    |
    = note: audit confidence → Low

15 findings (10 suppressed): 5 informational, 0 low, 0 medium, 0 high

@Inverle
Copy link
Member Author

Inverle commented Oct 12, 2025 •
edited
Loading

This is still potentially unsafe.
Untrusted code had already run, is it possible to access the GITHUB_TOKEN from memory / environment variables from a background process that spawned earlier? is it possible to just make git run something else?:

FreshRSS/.github/workflows/commands.yml

Lines 98 to 106 in b733715

- name: Commit and push changes
id: commit
if: success()
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
HEAD_REPO: ${{ fromJSON(steps.pr.outputs.result).repo.full_name }}
HEAD_BRANCH: ${{ fromJSON(steps.pr.outputs.result).ref }}
run: |
git add -A

Also no idea if this is safe either:

FreshRSS/.github/workflows/commands.yml

Lines 35 to 44 in b733715

- name: Get PR details
uses: actions/github-script@v8
id: pr
with:
script: |
const pr = await github.rest.pulls.get({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: context.issue.number
});

(and other actions/github-script@v8 runs, is it a separate environment?)

@Inverle Inverle mentioned this pull request Oct 12, 2025
Closed
@Alkarex Alkarex added this to the 1.28.0 milestone Oct 12, 2025
@Alkarex Alkarex added Security 🛡️ System care Everything related to system care labels Oct 12, 2025
@Inverle
Copy link
Member Author

Inverle commented Oct 12, 2025 •
edited
Loading

Also no idea if this is safe either:

https://docs.github.com/en/actions/reference/security/secure-use#use-an-action-instead-of-an-inline-script

maybe?

But then this is concerning:

https://docs.github.com/en/actions/how-tos/write-workflows/choose-where-workflows-run/run-jobs-in-a-container#:~:text=If%20you%20do%20not%20set%20a%20container%2C%20all%20steps%20will%20run%20directly%20on%20the%20host%20specified%20by%20runs%2Don%20unless%20a%20step%20refers%20to%20an%20action%20configured%20to%20run%20in%20a%20container.

@Frenzie
Copy link
Member

Frenzie commented Oct 13, 2025

(and other actions/github-script@v8 runs, is it a separate environment?)

I don't believe they can access environment variables from another step.

There isn't some kind of "it can push to the PR but only the PR permission"? Or just make the bot post 'hey, please run make fix-all' instead of doing it unsupervised?

@Alkarex Alkarex mentioned this pull request Oct 13, 2025
Closed
@Alkarex
Copy link
Member

Alkarex commented Oct 13, 2025

I have made another attempt in #8106
Keeping write permissions only for issues and using GitHub API instead of git.
Not tested at all yet.

@Inverle
Copy link
Member Author

Inverle commented Oct 15, 2025

I don't believe they can access environment variables from another step.

All steps run on the same host in the context of the runner user as a different process, which means they can access each others environment variables via /proc/[pid]/environ

There isn't some kind of "it can push to the PR but only the PR permission"?

Unfortunately there isn't

@Frenzie
Copy link
Member

Frenzie commented Oct 15, 2025

All steps run on the same host in the context of the runner user as a different process, which means they can access each others environment variables via /proc/[pid]/environ

I'd find that quite surprising. You mean this is what you found after testing? Of course it should be tested and not assumed, but my expectations would be:

  1. there is no pid in the first place because it already finished
  2. permission denied

@Inverle
Copy link
Member Author

Inverle commented Oct 17, 2025

replaced by #8123

@Inverle Inverle closed this Oct 17, 2025
@Inverle Inverle deleted the actions-security-fix branch December 6, 2025 16:47
@Inverle Inverle restored the actions-security-fix branch December 6, 2025 16:47
@Inverle Inverle deleted the actions-security-fix branch December 6, 2025 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Security 🛡️ System care Everything related to system care

Projects

None yet

Milestone

1.28.0

Development

Successfully merging this pull request may close these issues.

3 participants

@Inverle @Frenzie @Alkarex