Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use golang.org/x/sys/execabs #2950

Merged
merged 2 commits into from
Jan 27, 2021
Merged

Use golang.org/x/sys/execabs #2950

merged 2 commits into from
Jan 27, 2021

Conversation

tiborvass
Copy link
Collaborator

@tiborvass tiborvass commented Jan 25, 2021

On Windows, the os/exec.{Command,CommandContext,LookPath} functions
resolve command names that have neither path separators nor file extension
(e.g., "git") by first looking in the current working directory before
looking in the PATH environment variable.
Go maintainers intended to match cmd.exe's historical behavior.

However, this is pretty much never the intended behavior and as an abundance of precaution
this patch prevents that when executing commands.
Example of commands that docker.exe may execute: git, docker-buildx (or other cli plugin), docker-credential-wincred, docker.

Note that this was prompted by the Go 1.15.7 security fixes, but unlike in go.exe,
the windows path lookups in docker are not in a code path allowing remote code execution, thus there is no security impact on docker.

Signed-off-by: Tibor Vass [email protected]

vendor.conf Outdated
@@ -13,8 +13,8 @@ github.com/creack/pty 2a38352e8b4d7ab6c336eef107e4
github.com/davecgh/go-spew 8991bc29aa16c548c550c7ff78260e27b9ab7c73 # v1.1.1
github.com/docker/compose-on-kubernetes 78e6a00beda64ac8ccb9fec787e601fe2ce0d5bb # v0.5.0-alpha1
github.com/docker/distribution 0d3efadf0154c2b8a4e7b6621fff9809655cc580
github.com/docker/docker f0014860c1b3345e1fcc7ed81c491298de2633fb # v20.10.1
github.com/docker/docker-credential-helpers 54f0238b6bf101fc3ad3b34114cb5520beb562f5 # v0.6.3
github.com/docker/docker 7ca0cb7ffafc5339ac5fa575ce3f8b479c3643bf https://github.com/tiborvass/docker
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wip, updating when moby/moby#41927 is merged

Tibor Vass added 2 commits January 26, 2021 17:18
On Windows, the os/exec.{Command,CommandContext,LookPath} functions
resolve command names that have neither path separators nor file extension
(e.g., "git") by first looking in the current working directory before
looking in the PATH environment variable.
Go maintainers intended to match cmd.exe's historical behavior.

However, this is pretty much never the intended behavior and as an abundance of precaution
this patch prevents that when executing commands.
Example of commands that docker.exe may execute: `git`, `docker-buildx` (or other cli plugin), `docker-credential-wincred`, `docker`.

Note that this was prompted by the [Go 1.15.7 security fixes](https://blog.golang.org/path-security), but unlike in `go.exe`,
the windows path lookups in docker are not in a code path allowing remote code execution, thus there is no security impact on docker.

Signed-off-by: Tibor Vass <[email protected]>
@tiborvass tiborvass marked this pull request as ready for review January 26, 2021 17:20
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

github.com/docker/docker f0014860c1b3345e1fcc7ed81c491298de2633fb # v20.10.1
github.com/docker/docker-credential-helpers 54f0238b6bf101fc3ad3b34114cb5520beb562f5 # v0.6.3
github.com/docker/docker d5209b29b9777e0b9713d87847a5dc8ce9d93da6
github.com/docker/docker-credential-helpers 38bea2ce277ad0c9d2a6230692b0606ca5286526
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think we should tag a release for the creds helpers?

@thaJeztah thaJeztah merged commit d26bdfd into docker:master Jan 27, 2021
@thaJeztah thaJeztah added this to the 21.xx milestone Feb 2, 2021
@thaJeztah
Copy link
Member

These changes were cherry-picked into the 20.10 release branch, and released as part of v20.10.3; v20.10.2...v20.10.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants