Repository navigation
ci: pass PR title and changed files through env in scoring workflow - #2481
Open
arpitjain099 wants to merge 1 commit into
Open
arpitjain099 wants to merge 1 commit into
arpitjain099 wants to merge 1 commit into
Conversation
The scoring workflow interpolates ${{ github.event.pull_request.title }} and
${{ env.CHANGED_FILES }} directly into run: scripts. Both are attacker
controlled: the title is free text, and CHANGED_FILES is built from
git diff --name-only, so it carries filenames taken from the pull request.
A title containing a single quote closes the surrounding quotes in the
BS_UID and BS_PUBLIC steps and the remainder is executed by the shell.
Filenames behave the same way in the Get plugin info step: git quotes a path
containing a double quote and escapes it, but leaves single quotes, backticks
and $( ) untouched, so a single quote closes the python -c string. Python
then raises a SyntaxError, but only after the injected command has run, so the
failure looks like a malformed filename in the log.
This matters because Configure AWS Credentials runs earlier in the same job,
so anything executing here does so with those credentials in the environment.
Pass both values through env: blocks and reference them as shell variables,
which GitHub does not evaluate as script. PLUGIN_INFO is a step output rather
than an environment variable, so the steps that consume it get explicit env
entries. No behaviour change otherwise.
Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Following up on the report I sent Kartik privately, opening it here as asked.
What
score_new_plugins.ymlinterpolates${{ github.event.pull_request.title }}and${{ env.CHANGED_FILES }}directly intorun:scripts. Both are attacker controlled. The title is free text;CHANGED_FILESis built fromgit diff --name-only, so it carries filenames taken from the pull request.GitHub substitutes those expressions as literal text into the script before bash runs, so a value that closes the surrounding quotes is executed rather than read.
Verified rather than assumed
Both tested locally on reconstructions of these exact shell lines. Nothing was run against Brain-Score infrastructure.
PR title. In the
Update PLUGIN_INFO based on labelstep:A title of
x'; touch /tmp/title_pwn; echo 'y (user:bob) (public:True)closed the quote and thetouchran, while thesedstill produced a plausibleBS_UID, so the log looks normal afterwards. Titles have no character restrictions.Filename. In
Get plugin info. Worth noting git's quoting is less protective than it looks: it quotes and escapes a path containing a double quote, so that payload fails, but it leaves single quotes, backticks and$( )untouched. A file nameda'; touch bsx_marker; echo 'b.pyclosed thepython -cstring and ran. Python then raises a SyntaxError, but only after the injected command has executed, so the failure reads as a malformed filename. This one is bounded, since filenames cannot contain/.Why it is worth fixing
Configure AWS Credentialsruns earlier in the same job, so anything executing in these steps does so with those credentials already in the environment.The workflow is
pull_request_targeton closed withif: merged == true, so a PR has to be merged first and an outside contributor cannot self-label. The route that remains is a brain-score.org submission, which arrives already carryingautomerge-web.The change
Nine interpolation sites moved into
env:blocks and referenced as shell variables, which GitHub does not evaluate as script.PLUGIN_INFOis a step output rather than an environment variable, so the steps consuming it get explicitenv:entries.CHANGED_FILESis already in$GITHUB_ENV. No behaviour change otherwise, and the YAML validates.Also worth a look
brain-score/languagehas the same pattern inplugin_submission_orchestrator.yml:PR_TITLE="${{ github.event.pull_request.title }}"at lines 251, 823, 868, 1044 and 1080, andCHANGED_FILESat line 106. The double-quoted form is injectable the same way, which I also confirmed locally. Happy to send a matching PR there if useful.Separately and unrelated to injection:
hmarr/auto-approve-action@v3.1.0andplm9606/automerge_actions@1.2.2inautomerge_plugin-only_prs.ymlare mutable tags rather than commit SHAs, and the second is handedWORKFLOW_TOKEN. Left out of this PR to keep it focused, but happy to pin them in a follow-up.I used AI tooling while investigating and drafting this; the findings were reproduced and verified by hand before opening.