I am also experimenting with having a separate binary for the chrootarchive reexecs as I described in #42402 (comment) which has both compile-time and runtime assertions to make really, really sure that it can only be built as a static binary without cgo. I have opened a draft PR for your consideration: #43186

@tonistiigi @kolyshkin @cpuguy83 PTAL

In view of this, perhaps it might make sense to close golang/go#50102 since we can achieve a similar thing without too much effort.

I think the proposal should be left open. This PR implements the suggestion from golang/go#50102 (comment) and reimplements the loading of (some of) the other filesystem info in userspace, so to speak. The proposed tar.FileInfoNames interface would reduce the amount of moby code that needs to be maintained to support new platforms, and would be beneficial to others in a similar situation who also require having atime and ctime set in their tar file headers.

The problems with this is that it:

• Makes assumptions about the stdlib FileInfoHeader implementation. That the current implementation has custom code for *tar.Header and statUnix implementation in stdlib returns early before it reaches the code that can invoke the libraries that do the loading of usernames. Potentially this can change without notice in stdlib.
• We need to make sure that the new NoLookups function is used always. If one place forgets to call it (eg. after some code change) it breaks the security for the rest. This is also true for the proposal in archive/tar: add FileInfoNames interface golang/go#50102

If we are ok with these risks then the implementation SGTM

The problems with this is that it:

• Makes assumptions about the stdlib FileInfoHeader implementation. That the current implementation has custom code for *tar.Header and statUnix implementation in stdlib returns early before it reaches the code that can invoke the libraries that do the loading of usernames. Potentially this can change without notice in stdlib.

The only assumption being made is that the stdlib tar.FileInfoHeader implementation loads the file's username and group name iff the file's UID and GID are known to it. The fs.FileInfo interface cannot be widened without breaking the Go 1 Compatibility Promise so it seems highly unlikely for the mechanism used to get the UID and GID from the FileInfo value to change without notice. That being said, I can guard against such an occurrence by adding a unit test which asserts that tar.FileInfoHeader(nosysFileInfo{fi}, "") returns a tar.Header with zero-valued Uname and Gname fields when fi is a value returned from os.Stat.

• We need to make sure that the new NoLookups function is used always. If one place forgets to call it (eg. after some code change) it breaks the security for the rest. This is also true for the proposal in archive/tar: add FileInfoNames interface golang/go#50102

We need to make sure that the new function is always used in code executed inside a chroot. Or to be more precise, we need to make sure that dlopen() is never executed while the process is chrooted. Loading shared objects from the host's system library paths does not break security when the process's filesystem root is the host's filesystem root. And conversely, archive/tar is not the only package which potentially leads to dlopen() via glibc. Even with the existing solution of patching archive/tar, security could be broken by a call to e.g. user.Current() or net.Dial() while chrooted. Although, as I mentioned in the PR description, there is another layer of mitigation which prevents such oversights from actually breaking security.

Looks like gosec linting is complaining. We had quite some "false positives" on that linter, but we should check. We updated the linter to a new version Yesterday; if G305 is not useful for us, we can either disable it globally or if "useful", but not for all cases, add some //nolint: gosec comments, e.g.

//nolint: gosec // Ignore G305: File traversal when extracting zip/tar archive. 

INFO [runner] linters took 1m25.662021744s with stages: goanalysis_metalinter: 1m25.57620577s
pkg/archive/archive.go:732:17: G305: File traversal when extracting zip/tar archive (gosec)
        targetPath := filepath.Join(extractDir, hdr.Linkname)
                      ^
pkg/archive/archive.go:744:17: G305: File traversal when extracting zip/tar archive (gosec)
        targetPath := filepath.Join(filepath.Dir(path), hdr.Linkname)
                      ^
pkg/archive/archive.go:1097:11: G305: File traversal when extracting zip/tar archive (gosec)
        path := filepath.Join(dest, hdr.Name)
                ^
pkg/archive/archive.go:1161:11: G305: File traversal when extracting zip/tar archive (gosec)
        path := filepath.Join(dest, hdr.Name)
                ^
pkg/archive/archive_linux.go:54:17: G305: File traversal when extracting zip/tar archive (gosec)
                Name:       filepath.Join(hdr.Name, WhiteoutOpaqueDir),
                            ^
pkg/archive/diff.go:116:11: G305: File traversal when extracting zip/tar archive (gosec)
        path := filepath.Join(dest, hdr.Name)
                ^
pkg/archive/diff.go:212:11: G305: File traversal when extracting zip/tar archive (gosec)
        path := filepath.Join(dest, hdr.Name)
                ^
pkg/tarsum/tarsum.go:249:36: G305: File traversal when extracting zip/tar archive (gosec)
            ts.currentFile = path.Join(".", path.Join("/", currentHeader.Name))
                                            ^

@corhere thanks again! We discussed this PR in Yesterday's maintainers meeting with @tonistiigi @samuelkarp @cpuguy83 @tianon and @crazy-max and from that discussion, I think we're good to move forward with this change.

Could you rebase the PR (and have a look at the linter failures? #43185 (comment))

I can guard against such an occurrence by adding a unit test which asserts that tar.FileInfoHeader(nosysFileInfo{fi}, "") returns a tar.Header with zero-valued Uname and Gname fields when fi is a value returned from os.Stat.

It probably doesn't hurt to have a test (just to be sure); that said (as mentioned) there may be other areas that could potentially do a dlopen() vis glib; I recall we had discussions about that at the time, and there was no "real" solution other than the separate binary, which we still could consider as a follow-up.

there is another layer of mitigation which prevents such oversights from actually breaking security.

I was actually trying to recall why we added back the vendored pkg/archive (after we added the other mitigation); I found #35739 (comment) on that topic, and some internal tickets, which may have been related to Docker Enterprise versions being on different go versions (not supporting the osusergo build tag) and/or FIPS-compliant builds of Docker Enterprise, but my memory is failing me.

Let me know if you need help!

I hope I didn't miss anything (if I did, can you add details, @tonistiigi ?)

Could you rebase the PR (and have a look at the linter failures? #43185 (comment))

On it. Taking a peek under the hood at the gosec linter, it indiscriminately flags any path.Join call with an argument derived from a tar.Header without checking if it is followed by a mitigation for path-traversal so lots of potential for false positives. I'll review the code to verify that all the usages are safe and annotate them with //#nosec suppressions as they are more auditable than //nolint suppressions.

It probably doesn't hurt to have a test (just to be sure)

Done.

I was actually trying to recall why we added back the vendored pkg/archive (after we added the other mitigation); I found #35739 (comment) on that topic, and some internal tickets, which may have been related to Docker Enterprise versions being on different go versions (not supporting the osusergo build tag) and/or FIPS-compliant builds of Docker Enterprise, but my memory is failing me.

Thankfully it was documented at the time! Tl;dr it was a workaround required specifically for static builds on Go 1.10 with CGO_ENABLED=1 which was made obsolete by Go 1.11 with the osusergo build tag. (Although in hindsight, nosysFileInfo might have been just as effective a workaround back then.) I'll follow up internally with the rest of the Mirantis Container Runtime team to verify that this change won't give us any trouble with FIPS-compliant builds, but otherwise it appears that the reasons for adding the back the vendored archive/tar have long reached their end of life.

Thankfully it was documented at the time!

🤦 ah, good find! I didn't look there!

Although in hindsight, nosysFileInfo might have been just as effective a workaround back then

If my memory serves me well, the osusergo option was added before the archive/tar issues existed (as it already cases grievance when using os/user); only before, it would happen when it was used explicitly, and now it became an issue when used implicitly (through archive/tar)

@tonistiigi @cpuguy83 PTAL

I'll go ahead and bring this one in. I assume @tonistiigi is good with this (otherwise let me know)