Skip to content

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Nov 9, 2022

⚠️ for reviewers: only go1.19 and up have this fix in os/exec, so if we expect go1.18 and before to be used with containerd 1.7, we should hold off on this.

This is effectively a revert of 2ac9968 (#5906), which switched from os/exec to the golang.org/x/sys/execabs package to mitigate security issues (mainly on Windows) with lookups resolving to binaries in the current directory.

from the go1.19 release notes https://go.dev/doc/go1.19#os-exec-path

PATH lookups

Command and LookPath no longer allow results from a PATH search to be found
relative to the current directory. This removes a common source of security
problems but may also break existing programs that depend on using, say,
exec.Command("prog") to run a binary named prog (or, on Windows, prog.exe) in
the current directory. See the os/exec package documentation for information
about how best to update such programs.

On Windows, Command and LookPath now respect the NoDefaultCurrentDirectoryInExePath
environment variable, making it possible to disable the default implicit search
of “.” in PATH lookups on Windows systems.

@AkihiroSuda
Copy link
Member

Distros may package containerd 1.7 with Go 1.18, so probably this should be postponed to v2.0

@dcantah
Copy link
Member

dcantah commented Jul 10, 2023

@thaJeztah I was reviewing #8789 when I noticed the import for one of the execs was execabs, and then wound up down the same rabbit hole as you 🤣. I think we can swap now given we're on the road to 2.0

@thaJeztah
Copy link
Member Author

Oh! Saw your comment, and then forgot about it; I just rebased this one. Looks like the only consumer of that package is now golang.org/x/tools. If I find some time, I'll open a pull request there to have it removed.

@thaJeztah

This comment was marked as resolved.

@thaJeztah
Copy link
Member Author

thaJeztah commented Nov 1, 2023

@thaJeztah
Copy link
Member Author

Rebased after #9316 was merged (which caused one minor conflict in integration/client)

@AkihiroSuda @dcantah this one good to go?

@thaJeztah
Copy link
Member Author

oh! needs a rebase again; let me do so

This is effectively a revert of 2ac9968, which
switched from os/exec to the golang.org/x/sys/execabs package to mitigate
security issues (mainly on Windows) with lookups resolving to binaries in the
current directory.

from the go1.19 release notes https://go.dev/doc/go1.19#os-exec-path

> ## PATH lookups
>
> Command and LookPath no longer allow results from a PATH search to be found
> relative to the current directory. This removes a common source of security
> problems but may also break existing programs that depend on using, say,
> exec.Command("prog") to run a binary named prog (or, on Windows, prog.exe) in
> the current directory. See the os/exec package documentation for information
> about how best to update such programs.
>
> On Windows, Command and LookPath now respect the NoDefaultCurrentDirectoryInExePath
> environment variable, making it possible to disable the default implicit search
> of “.” in PATH lookups on Windows systems.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Member Author

done; should be good again 👍

@AkihiroSuda AkihiroSuda enabled auto-merge November 3, 2023 06:56
@AkihiroSuda AkihiroSuda added this pull request to the merge queue Nov 3, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 3, 2023
@AkihiroSuda AkihiroSuda added this pull request to the merge queue Nov 3, 2023
Merged via the queue into containerd:main with commit 33fab02 Nov 3, 2023
@thaJeztah thaJeztah deleted the no_execabs branch November 3, 2023 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status/needs-major-release Can only be accepted into a major release

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants