Validate compose project name before using it in docker-compose command lines - #829
Open
carfeii wants to merge 1 commit into
Open
Validate compose project name before using it in docker-compose command lines#829carfeii wants to merge 1 commit into
carfeii wants to merge 1 commit into
Conversation
…nd lines The auto-detected local project name and any explicit Service/Project ProjectName can originate from the com.docker.compose.project Docker label, which (unlike a project name declared via docker-compose.yml) has no format restriction: a plain `docker run --label` can set it to any string, including one containing extra command-line flags. That value was being concatenated unescaped into the docker-compose command prefix used to build every generated command, allowing argument injection. Reject label-derived values that don't match the character class Compose itself requires for a project name before adopting them for auto-detection, and before using them to build a command line, so a stray or malicious label on an unrelated container can no longer influence the commands run against the user's own project. See jesseduffield#828.
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.
Fixes #828.
Summary
The auto-detected local project name, and any explicit
Service/ProjectProjectName, can originate from thecom.docker.compose.projectDockerlabel. Unlike a project name declared via
docker-compose.yml, a rawcontainer label has no format restriction:
docker run --labelacceptsany string, including one containing extra command-line flags. That value
was concatenated unescaped into the docker-compose command prefix used to
build every generated command (restart, stop, logs, etc.), allowing
argument injection from any container on the host whose
com.docker.compose.servicelabel happened to match one of the project'sservice names.
Fix
Adds
isValidComposeProjectName, matching the character class the Composespec itself requires for a project name, and uses it in two places:
RefreshContainersAndServices: a container'sProjectNamelabel isskipped during auto-detection if it doesn't pass validation, so the
loop continues to a legitimate match instead of adopting it.
NewCommandObject: the-pflag is only appended if the project namepasses validation, as defense in depth in case an invalid value reaches
this point through another path.
Testing
go build ./...andgo test ./...pass with no changes to existingtest expectations.
a container with a
com.docker.compose.projectlabel containing--project-directory, matching service nameweb) and confirmed theresolved
RestartServicecommand's argv contained the injected flag.Repeated the same setup against this branch and confirmed the label is
now rejected, detection falls through to the legitimate project name,
and the resolved command's argv is unaffected by the malicious
container's label.
Happy to add an automated regression test for this in
pkg/commandsifyou'd like one included, following the existing test style in that
package. I kept this PR to the minimal fix in the meantime.