Fix shell command injection via sequence range in Database#retrieve - #844
Open
yannickwurm wants to merge 1 commit into
Open
Fix shell command injection via sequence range in Database#retrieve#844yannickwurm wants to merge 1 commit into
yannickwurm wants to merge 1 commit into
Conversation
Database#retrieve interpolates the user supplied range into a shell command (`blastdbcmd ... -range <coords>`), but validated it with the unanchored regex /[0-9]+-[0-9]*/. Any string containing a digit-dash pair passed, so a range such as `1-2;echo INJECTED` was accepted and the trailing command was executed by the shell. The range is reachable, without authentication in standalone SequenceServer, through `GET /searchdata.json?query=<accession>:<range>`. The range is now validated against BLAST::VALID_SEQUENCE_RANGE, which is anchored at both ends (\A[0-9]+-[0-9]*\z) so only "start-stop" or "start-" can reach the command line. Specs cover an appended command, a command hidden behind a newline, and a non-numeric prefix. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T22rUQt69nPsbSUDFxhuq3
|
|
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
3 participants
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.
Summary
Database#retrieve(lib/sequenceserver/database.rb) builds the shell commandblastdbcmd -db <db> -entry '<accession>' -range <coords>withcoordsinterpolated unquoted. The only guard was the unanchored regex/[0-9]+-[0-9]*/, so any string containing a digit-dash pair passed, e.g.1-2;echo INJECTED, and the trailing command was executed by the shell.Reachable via
GET /searchdata.json?query=<accession>:1-2;<cmd>. This is pre-authentication in standalone SequenceServer and behind login in the cloud deployment. Present in 3.1.3 (the commit this branch is based on is the one deployed as thesequenceserversubmodule ofsequenceserver-cloud).Changes
lib/sequenceserver/blast.rb: addBLAST::VALID_SEQUENCE_RANGE = /\A[0-9]+-[0-9]*\z/, anchored at both ends, next to the existingVALID_SEQUENCE_ID.lib/sequenceserver/database.rb: validatecoordsagainst that constant instead of the unanchored literal. Onlystart-stoporstart-(open ended) can now reach the command line; everything else raisesInvalidParameterErroras before.spec/database_spec.rb: regression specs for an appended command (1-2;echo INJECTED), a command hidden behind a newline (1-2\n;echo INJECTED, which^/$anchors would have missed), and a non-numeric prefix (x1-2).Accessions were already validated by the anchored
VALID_SEQUENCE_IDand are not affected.Testing
Run locally with BLAST+ 2.12 (NCBI FTP is blocked from the sandbox, so
BLAST_VERSIONwas overridden for the local run only; nothing of that is in the diff). RuboCop reports no new offences on the changed files.🤖 Generated with Claude Code
https://claude.ai/code/session_01T22rUQt69nPsbSUDFxhuq3
Generated by Claude Code