-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
[20.10 backport] Remove local fork of archive/tar package #46695
Conversation
Linter is complaining; you probably need to cherry-pick 65e1adc (part of #43765)
|
The only CI failures are the usual Windows Server hcsshim timeouts. PTAL @thaJeztah |
hack/vendor.sh
Outdated
@@ -1,28 +0,0 @@ | |||
#!/usr/bin/env bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you removed a bit too much here; the hack/vendor.sh
script itself should probably still be there (or Jenkins to be updated);
[2023-10-23T17:06:17.709Z] + docker run --rm -t --privileged -v /home/ubuntu/workspace/moby_PR-46695/.git:/go/src/github.com/docker/docker/.git --name docker-pr2 -e DOCKER_EXPERIMENTAL -e DOCKER_GITCOMMIT=d6cbc6b37d407d5bc8573bcfbbcf803b8fcb91e4 -e DOCKER_GRAPHDRIVER -e TEST_FORCE_VALIDATE -e VALIDATE_REPO=https://github.com/moby/moby.git -e VALIDATE_BRANCH=20.10 docker:d6cbc6b37d407d5bc8573bcfbbcf803b8fcb91e4 hack/validate/vendor
[2023-10-23T17:06:17.969Z] hack/validate/vendor: line 14: ./hack/vendor.sh: No such file or directory
script returned exit code 127
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, missed that one. Those pesky horizontal scroll bars...
A copy of Go's archive/tar packge was vendored with a patch applied to mitigate CVE-2019-14271. Vendoring standard library packages is not supported by Go in module-aware mode, which is getting in the way of maintenance. A different approach to mitigate the vulnerability is needed which does not involve vendoring parts of the standard library. glibc implements name service lookups such as users, groups and DNS using a scheme known as Name Service Switch. The services are implemented as modules, shared libraries which glibc dynamically links into the process the first time a function requiring the module is called. This is the crux of the vulnerability: if a process linked against glibc chroots, then calls one of the functions implemented with NSS for the first time, glibc may load NSS modules out of the chrooted filesystem. The API underlying the `docker cp` command is implemented by forking a new process which chroots into the container's rootfs and writes a tar stream of files from the container over standard output. It utilizes the Go standard library's archive/tar package to write the tar stream. It makes use of the tar.FileInfoHeader function to construct a tar.Header value from an fs.FileInfo value. In modern versions of Go on *nix platforms, FileInfoHeader will attempt to resolve the file's UID and GID to their respective user and group names by calling the os/user functions LookupId and LookupGroupId. The cgo implementation of os/user on *nix performs lookups by calling the corresponding libc functions. So when linked against glibc, calls to tar.FileInfoHeader after the process has chrooted into the container's rootfs can have the side effect of loading NSS modules from the container! Without any mitigations, a malicious container image author can trivially get arbitrary code execution by leveraging this vulnerability and escape the chroot (which is not a sandbox) into the host. Mitigate the vulnerability without patching or forking archive/tar by hiding the OS-dependent file info from tar.FileInfoHeader which it needs to perform the lookups. Without that information available it falls back to populating the tar.Header with only the information obtainable directly from the FileInfo value without making any calls into os/user. Fixes moby#42402 Signed-off-by: Cory Snider <csnider@mirantis.com> (cherry picked from commit e9bbc41) Signed-off-by: Cory Snider <csnider@mirantis.com>
The recently-upgraded gosec linter has a rule for archive extraction code which may be vulnerable to directory traversal attacks, a.k.a. Zip Slip. Gosec's detection is unfortunately prone to false positives, however: it flags any filepath.Join call with an argument derived from a tar.Header value, irrespective of whether the resultant path is used for filesystem operations or if directory traversal attacks are guarded against. All of the lint errors reported by gosec appear to be false positives. Signed-off-by: Cory Snider <csnider@mirantis.com> (cherry picked from commit 833139f) Signed-off-by: Cory Snider <csnider@mirantis.com>
6c3e402
to
6c523aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The hack of forking archive/tar is incompatible with the version of the package in Go 1.20.
Backport the proper fix rather than hacking on the hack.
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)