fix: avoid shell interpolation of SLURM_NODELIST (CVE-2024-27763) - #765
Open
christopher5106 wants to merge 1 commit into
Open
fix: avoid shell interpolation of SLURM_NODELIST (CVE-2024-27763)#765christopher5106 wants to merge 1 commit into
christopher5106 wants to merge 1 commit into
Conversation
_init_dist_slurm passed the SLURM_NODELIST environment variable into a shell
command string via subprocess.getoutput, so a crafted value could execute
arbitrary commands:
SLURM_NODELIST='x; touch /tmp/PWNED; echo done'
Pass the node list as an argv element instead, and take the first line in
Python rather than piping through `head -n1`.
Output is unchanged for well-formed node lists. Combined stdout/stderr and the
non-raising behaviour of getoutput are preserved, with one difference worth
noting: a missing `scontrol` binary now raises FileNotFoundError instead of
silently assigning a shell error message to MASTER_ADDR.
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.
Fixes CVE-2024-27763 / GHSA-86w8-vhw6-q9qq, reported in #729.
Problem
_init_dist_slurminterpolates theSLURM_NODELISTenvironment variable into ashell command string:
subprocess.getoutputruns its argument through/bin/sh -c, so any shellmetacharacter in
SLURM_NODELISTis interpreted rather than passed toscontrol. This is the onlygetoutputcall in the codebase.Fix
Pass the node list as an argv element so no shell is involved, and take the
first line in Python instead of piping through
head -n1.Verification
With a stub
scontrolonPATHthat echoes its arguments:SLURM_NODELISTnode[1-4]'node-show''node-show'(unchanged)x; touch /tmp/PWNED; echo done/tmp/PWNEDcreated, output'node-show\nnode-hostname\nnode-x\ndone'/tmp/PWNEDnot created, output'node-show'Note the injected command also defeated the
head -n1truncation before the fix.Compatibility
Output is unchanged for well-formed node lists, and
getoutput's combinedstdout/stderr and non-raising behaviour are preserved. One difference worth
calling out: a missing
scontrolbinary now raisesFileNotFoundErrorratherthan silently assigning a shell error string to
MASTER_ADDR. That only affectsthe
launcher='slurm'path, wherescontrolis expected to exist, and failingloudly seemed preferable to proceeding with a bogus master address — happy to
restore the previous silent behaviour if you would rather keep the diff purely
mechanical.