Fix shell injection via workspace path in _peek_file_lines - #119
Merged
Conversation
$source is a submitter-chosen workspace path -- input_file, dna_file,
rna_file or msa_file straight from the job parameters -- and it was
interpolated into a shell string:
my $cmd = "p3-cat '$source' 2>/dev/null | head -n $n_lines";
@lines = `$cmd`;
A single quote in the workspace object name escapes the quoting and the
rest of the name runs as the service user. Confirmed with a harmless
payload against the real sub:
/ws/home/x'; touch /tmp/PWNED; echo '.fasta
Replaced with list-form open, so the path is one argv element and there
is no shell to escape from. Behaviour is preserved: reading only
$n_lines and closing early sends SIGPIPE to p3-cat, which is what stops
the transfer -- previously the job of the piped `head`.
Found by coconut-stabilinnator, who hit the same pattern in
App-StabiliNNator.pl (a converter invoked through backticks with a path
ending in a workspace basename) and suggested checking here.
Adds tests/test_service_script.py: one asserting the code carries no
shell form, one executing the sub with a hostile path and a stub p3-cat
and checking the payload did not run. Both fail against the old line.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DEWo4xvL59PHC1QUvtTV9F
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
1 participant
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.
$sourceis a submitter-chosen workspace path —input_file,dna_file,rna_fileormsa_filestraight from the job parameters — and it was interpolated into a shell string:A single quote in the workspace object name escapes the quoting and the remainder runs as the service user. Confirmed against the real sub with a harmless payload:
Replaced with list-form
open(my $ph, "-|", "p3-cat", $source), so the path is a single argv element with no shell involved. Behaviour preserved: reading only$n_linesand closing early sends SIGPIPE top3-cat, which is what actually stops the transfer — previously the job of the pipedhead.Credit: found by the coconut-stabilinnator session, which hit the same pattern in
App-StabiliNNator.pl(a converter invoked through backticks with a path ending in a workspace basename, fixed before merge) and suggested checking here.New
tests/test_service_script.py— one test asserts the code carries no shell form (ignoring comments), one executes the extracted sub with a hostile path and a stubp3-catand asserts the payload did not run. Mutation-tested: restoring the old line fails both.