Use securejoin.SecureJoin when forming userns paths

[rcmadhankumar]

No description provided.

[dcermak]

Fails to build:

❯ make
go build "-buildmode=pie" -ldflags '-X main.gitCommit=64d6052f792e360c4159aae53e462e189244145c ' -gcflags "" -tags "   " -o bin/skopeo ./cmd/skopeo
vendor/github.com/containers/storage/userns.go:16:2: cannot find module providing package github.com/moby/sys/user: import lookup disabled by -mod=vendor
	(Go version in go.mod is at least 1.14 and vendor directory exists.)
make: *** [Makefile:135: bin/skopeo] Error 1

[danishprakash]

Do we plan to hand wrangle this and avoid make vendor?

[dcermak]

I think we can re-vendor, otherwise this is going to turn into a nightmare

[rcmadhankumar]

Make output

madhankumar@localhost:~/repos/skopeo-play/new/skopeo> make
go build "-buildmode=pie" -ldflags '-X main.gitCommit=812c8c2874790b91c45789b04d741109899a0965 ' -gcflags "" -tags "btrfs_noversion exclude_graphdriver_btrfs  " -o bin/skopeo ./cmd/skopeo
madhankumar@localhost:~/repos/skopeo-play/new/skopeo> 

madhankumar@localhost:~/repos/skopeo-play/new/skopeo> ./bin/skopeo --version
skopeo version 1.14.4 commit: 812c8c2874790b91c45789b04d741109899a0965

[danishprakash]

Just found out that we don't really have to bump filepath-securejoin to 0.3.0 in order to use OpenInRoot, instead, we could've used the older API documented at https://github.com/cyphar/filepath-securejoin/blob/07ceaf03cda4b79346f5e1de82143db6e12cb8c5/open_linux.go#L30C1-L31C64. In fact, the old API was used to backport CVE-2024-9676 for containers/[email protected]

// Effectively, OpenInRoot(root, unsafePath) is equivalent to
//
//	path, _ := securejoin.SecureJoin(root, unsafePath)
//	handle, err := os.OpenFile(path, unix.O_PATH|unix.O_CLOEXEC)

Bottomline, a simple c/storage bump from v1.51.0 to v1.51.2 should suffice in this case.

Additionally, I'm planning to do the same for podman essentially reverting the old change which has the huge patch to the old API.

[danishprakash]

So we have 3 options now:

1. First is the usual bump (this PR at the time of this comment)
2. Bump c/storage to 1.51.2 which uses the old API, and that closes the CVE problem. But as mentioned by @cyphar, this is not race-safe if an attacker changes the filesystem between the SecureJoin op and obtaining the handle.
3. Workaround to ^2, additionally check whether the obtained handle is the path we expected using procfs. Copying the snippet shared by Aleksa:

func somewhatSafeOpen(root, unsafePath string, mode int) (_ *os.File, Err error) {
	if mode&(os.O_CREAT|os.O_TRUNC) != 0 {
		return nil, fmt.Errorf("O_CREAT|O_TRUNC cannot be used safely")
	}
	path, err := securejoin.SecureJoin(root, unsafePath)
	if err != nil {
		return nil, err
	}
	handle, err := os.OpenFile(path, mode)
	if err != nil {
		return nil, err
	}
	defer func() {
		if Err != nil {
			_ = handle.Close()
		}
	}()
	// At this point, we aren't sure that the handle we grabbed is the same
	// path we expected. So double-check it using procfs.
	actualPath, err := os.Readlink(fmt.Sprintf("/proc/self/fd/%d", handle.Fd()))
	if err != nil {
		return nil, fmt.Errorf("failed to check handle path for %q: %w", path, err)
	}
	if path != actualPath {
		return nil, fmt.Errorf("container breakout detected: tried to open %q but instead opened %q", path, actualPath)
	}
	return handle, nil
}

I'd recommend the first option, being the most idiomatic despite being verbose. Thoughts?