From daa870860139645333f541a4332a080c892532dd Mon Sep 17 00:00:00 2001 From: Bjorn Neergaard Date: Mon, 26 Sep 2022 07:51:44 -0600 Subject: [PATCH 1/2] refactor(pkg/archive): factor out createImpliedDirectories helper This code was duplicated in two places -- factor it out, add documentation, and move magic numbers into a constant. Additionally, use the same permissions (0755) in both code paths, and ensure that the ID map is used in both code paths. Co-authored-by: Vasiliy Ulyanov Signed-off-by: Bjorn Neergaard Signed-off-by: Vasiliy Ulyanov (cherry picked from commit 4831ff9f27a8611b3b5308ef9895f2e444f72ee3) --- pkg/archive/archive.go | 60 +++++++++++++++++++++++++++++++----------- pkg/archive/diff.go | 18 +++---------- 2 files changed, 49 insertions(+), 29 deletions(-) diff --git a/pkg/archive/archive.go b/pkg/archive/archive.go index 6d8464b60b..a1386c2c18 100644 --- a/pkg/archive/archive.go +++ b/pkg/archive/archive.go @@ -31,6 +31,18 @@ import ( exec "golang.org/x/sys/execabs" ) +// ImpliedDirectoryMode represents the mode (Unix permissions) applied to directories that are implied by files in a +// tar, but that do not have their own header entry. +// +// The permissions mask is stored in a constant instead of locally to ensure that magic numbers do not +// proliferate in the codebase. The default value 0755 has been selected based on the default umask of 0022, and +// a convention of mkdir(1) calling mkdir(2) with permissions of 0777, resulting in a final value of 0755. +// +// This value is currently implementation-defined, and not captured in any cross-runtime specification. Thus, it is +// subject to change in Moby at any time -- image authors who require consistent or known directory permissions +// should explicitly control them by ensuring that header entries exist for any applicable path. +const ImpliedDirectoryMode = 0755 + type ( // Compression is the state represents if compressed or not. Compression int @@ -840,7 +852,6 @@ func Tar(path string, compression Compression) (io.ReadCloser, error) { // TarWithOptions creates an archive from the directory at `path`, only including files whose relative // paths are included in `options.IncludeFiles` (if non-nil) or not in `options.ExcludePatterns`. func TarWithOptions(srcPath string, options *TarOptions) (io.ReadCloser, error) { - // Fix the source path to work with long path names. This is a no-op // on platforms other than Windows. srcPath = fixVolumePathPrefix(srcPath) @@ -1048,7 +1059,6 @@ func Unpack(decompressedArchive io.Reader, dest string, options *TarOptions) err defer pools.BufioReader32KPool.Put(trBuf) var dirs []*tar.Header - rootIDs := options.IDMap.RootPair() whiteoutConverter, err := getWhiteoutConverter(options.WhiteoutFormat, options.InUserNS) if err != nil { return err @@ -1083,19 +1093,10 @@ loop: } } - // After calling filepath.Clean(hdr.Name) above, hdr.Name will now be in - // the filepath format for the OS on which the daemon is running. Hence - // the check for a slash-suffix MUST be done in an OS-agnostic way. - if !strings.HasSuffix(hdr.Name, string(os.PathSeparator)) { - // Not the root directory, ensure that the parent directory exists - parent := filepath.Dir(hdr.Name) - parentPath := filepath.Join(dest, parent) - if _, err := os.Lstat(parentPath); err != nil && os.IsNotExist(err) { - err = idtools.MkdirAllAndChownNew(parentPath, 0755, rootIDs) - if err != nil { - return err - } - } + // Ensure that the parent directory exists. + err = createImpliedDirectories(dest, hdr, options) + if err != nil { + return err } // #nosec G305 -- The joined path is checked for path traversal. @@ -1173,6 +1174,35 @@ loop: return nil } +// createImpliedDirectories will create all parent directories of the current path with default permissions, if they do +// not already exist. This is possible as the tar format supports 'implicit' directories, where their existence is +// defined by the paths of files in the tar, but there are no header entries for the directories themselves, and thus +// we most both create them and choose metadata like permissions. +// +// The caller should have performed filepath.Clean(hdr.Name), so hdr.Name will now be in the filepath format for the OS +// on which the daemon is running. This precondition is required because this function assumes a OS-specific path +// separator when checking that a path is not the root. +func createImpliedDirectories(dest string, hdr *tar.Header, options *TarOptions) error { + // Not the root directory, ensure that the parent directory exists + if !strings.HasSuffix(hdr.Name, string(os.PathSeparator)) { + parent := filepath.Dir(hdr.Name) + parentPath := filepath.Join(dest, parent) + if _, err := os.Lstat(parentPath); err != nil && os.IsNotExist(err) { + // RootPair() is confined inside this loop as most cases will not require a call, so we can spend some + // unneeded function calls in the uncommon case to encapsulate logic -- implied directories are a niche + // usage that reduces the portability of an image. + rootIDs := options.IDMap.RootPair() + + err = idtools.MkdirAllAndChownNew(parentPath, ImpliedDirectoryMode, rootIDs) + if err != nil { + return err + } + } + } + + return nil +} + // Untar reads a stream of bytes from `archive`, parses it as a tar archive, // and unpacks it into the directory at `dest`. // The archive may be compressed with one of the following algorithms: diff --git a/pkg/archive/diff.go b/pkg/archive/diff.go index f83d126faf..62409d827e 100644 --- a/pkg/archive/diff.go +++ b/pkg/archive/diff.go @@ -72,20 +72,10 @@ func UnpackLayer(dest string, layer io.Reader, options *TarOptions) (size int64, } } - // Note as these operations are platform specific, so must the slash be. - if !strings.HasSuffix(hdr.Name, string(os.PathSeparator)) { - // Not the root directory, ensure that the parent directory exists. - // This happened in some tests where an image had a tarfile without any - // parent directories. - parent := filepath.Dir(hdr.Name) - parentPath := filepath.Join(dest, parent) - - if _, err := os.Lstat(parentPath); err != nil && os.IsNotExist(err) { - err = system.MkdirAll(parentPath, 0600) - if err != nil { - return 0, err - } - } + // Ensure that the parent directory exists. + err = createImpliedDirectories(dest, hdr, options) + if err != nil { + return 0, err } // Skip AUFS metadata dirs From f6ebfaea198fe85a237ef952d2f5afe308c719a8 Mon Sep 17 00:00:00 2001 From: Bjorn Neergaard Date: Mon, 26 Sep 2022 07:54:06 -0600 Subject: [PATCH 2/2] test(pkg/archive): add TestImpliedDirectoryPermissions Co-authored-by: Cory Snider Signed-off-by: Bjorn Neergaard (cherry picked from commit 5dff494b87ca140626f2b722f78fd729db33fd22) --- pkg/archive/archive_test.go | 50 ++++++++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/pkg/archive/archive_test.go b/pkg/archive/archive_test.go index 22078cc83f..e12e75f747 100644 --- a/pkg/archive/archive_test.go +++ b/pkg/archive/archive_test.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "io" + "io/fs" "os" "os/exec" "path/filepath" @@ -1216,7 +1217,7 @@ func TestTempArchiveCloseMultipleTimes(t *testing.T) { } } -// TestXGlobalNoParent is a regression test to check parent directories are not crated for PAX headers +// TestXGlobalNoParent is a regression test to check parent directories are not created for PAX headers func TestXGlobalNoParent(t *testing.T) { buf := &bytes.Buffer{} w := tar.NewWriter(buf) @@ -1236,6 +1237,53 @@ func TestXGlobalNoParent(t *testing.T) { assert.Check(t, errors.Is(err, os.ErrNotExist)) } +// TestImpliedDirectoryPermissions ensures that directories implied by paths in the tar file, but without their own +// header entries are created recursively with the default mode (permissions) stored in ImpliedDirectoryMode. This test +// also verifies that the permissions of explicit directories are respected. +func TestImpliedDirectoryPermissions(t *testing.T) { + skip.If(t, runtime.GOOS == "windows", "skipping test that requires Unix permissions") + + buf := &bytes.Buffer{} + headers := []tar.Header{{ + Name: "deeply/nested/and/implied", + }, { + Name: "explicit/", + Mode: 0644, + }, { + Name: "explicit/permissions/", + Mode: 0600, + }, { + Name: "explicit/permissions/specified", + Mode: 0400, + }} + + w := tar.NewWriter(buf) + for _, header := range headers { + err := w.WriteHeader(&header) + assert.NilError(t, err) + } + + tmpDir := t.TempDir() + + err := Untar(buf, tmpDir, nil) + assert.NilError(t, err) + + assertMode := func(path string, expected uint32) { + t.Helper() + stat, err := os.Lstat(filepath.Join(tmpDir, path)) + assert.Check(t, err) + assert.Check(t, is.Equal(stat.Mode().Perm(), fs.FileMode(expected))) + } + + assertMode("deeply", ImpliedDirectoryMode) + assertMode("deeply/nested", ImpliedDirectoryMode) + assertMode("deeply/nested/and", ImpliedDirectoryMode) + + assertMode("explicit", 0644) + assertMode("explicit/permissions", 0600) + assertMode("explicit/permissions/specified", 0400) +} + func TestReplaceFileTarWrapper(t *testing.T) { filesInArchive := 20 testcases := []struct {