From b0dddc98fcf189727c9fedc0a62c6bad6e10d614 Mon Sep 17 00:00:00 2001 From: Chris Sawer Date: Tue, 23 Jan 2024 17:03:29 +0000 Subject: [PATCH 1/5] Add tests for zip file permissions issues --- extractor_test.go | 71 +++++++++++++++++++++++++++++++++++++++++++++++ go.mod | 1 + go.sum | 2 ++ 3 files changed, 74 insertions(+) diff --git a/extractor_test.go b/extractor_test.go index 9943d2d..db8ef48 100644 --- a/extractor_test.go +++ b/extractor_test.go @@ -9,11 +9,14 @@ import ( "os" "path/filepath" "runtime" + "strconv" + "strings" "testing" "github.com/arduino/go-paths-helper" "github.com/codeclysm/extract/v4" "github.com/stretchr/testify/require" + "golang.org/x/sys/unix" ) func TestExtractors(t *testing.T) { @@ -259,6 +262,74 @@ func TestSymLinkMazeHardening(t *testing.T) { }) } +func TestUnixPermissions(t *testing.T) { + // Disable user's umask to enable creation of files with any permission, restore it after the test + userUmask := unix.Umask(0) + defer unix.Umask(userUmask) + + archiveFilenames := []string{ + "testdata/permissions.zip", + "testdata/permissions.tar", + } + for _, archiveFilename := range archiveFilenames { + tmp, err := paths.MkTempDir("", "") + require.NoError(t, err) + defer tmp.RemoveAll() + + f, err := paths.New(archiveFilename).Open() + require.NoError(t, err) + err = extract.Archive(context.Background(), f, tmp.String(), nil) + require.NoError(t, err) + + filepath.Walk(tmp.String(), func(path string, info os.FileInfo, _ error) error { + filename := filepath.Base(path) + // Desired permissions indicated by part of the filenames inside the zip/tar files + if strings.HasPrefix(filename, "dir") { + desiredPermString, _ := strings.CutPrefix(filename, "dir") + desiredPerms, _ := strconv.ParseUint(desiredPermString, 8, 32) + require.Equal(t, os.ModeDir|os.FileMode(desiredPerms), info.Mode()) + } else if strings.HasPrefix(filename, "file") { + desiredPermString, _ := strings.CutPrefix(filename, "file") + desiredPerms, _ := strconv.ParseUint(desiredPermString, 8, 32) + require.Equal(t, os.FileMode(desiredPerms), info.Mode()) + } + return nil + }) + } +} + +func TestZipDirectoryPermissions(t *testing.T) { + // Disable user's umask to enable creation of files with any permission, restore it after the test + userUmask := unix.Umask(0) + defer unix.Umask(userUmask) + + // This arduino library has files before their containing directories in the zip, + // so a good test case that these directory permissions are created correctly + archive := paths.New("testdata/filesbeforedirectories.zip") + download(t, "https://downloads.arduino.cc/libraries/github.com/arduino-libraries/LiquidCrystal-1.0.7.zip", archive) + + tmp, err := paths.MkTempDir("", "") + require.NoError(t, err) + defer tmp.RemoveAll() + + f, err := archive.Open() + require.NoError(t, err) + err = extract.Archive(context.Background(), f, tmp.String(), nil) + require.NoError(t, err) + + filepath.Walk(tmp.String(), func(path string, info os.FileInfo, _ error) error { + // Test files and directories (excluding the parent) match permissions from the zip file + if path != tmp.String() { + if info.IsDir() { + require.Equal(t, os.ModeDir|os.FileMode(0755), info.Mode()) + } else { + require.Equal(t, os.FileMode(0644), info.Mode()) + } + } + return nil + }) +} + // MockDisk is a disk that chroots to a directory type MockDisk struct { Base string diff --git a/go.mod b/go.mod index 5c8f60d..0a6bad5 100644 --- a/go.mod +++ b/go.mod @@ -11,6 +11,7 @@ require ( github.com/klauspost/compress v1.15.13 github.com/stretchr/testify v1.9.0 github.com/ulikunitz/xz v0.5.12 + golang.org/x/sys v0.16.0 ) require ( diff --git a/go.sum b/go.sum index 1c42629..b190457 100644 --- a/go.sum +++ b/go.sum @@ -31,6 +31,8 @@ github.com/ulikunitz/xz v0.5.12/go.mod h1:nbz6k7qbPmH4IRqmfOplQw/tblSgqTqBwxkY0o golang.org/x/crypto v0.0.0-20180214000028-650f4a345ab4/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4= golang.org/x/net v0.0.0-20180406214816-61147c48b25b/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +golang.org/x/sys v0.16.0 h1:xWw16ngr6ZMtmxDyKyIgsE93KNKz5HKmMa3b8ALHidU= +golang.org/x/sys v0.16.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= gopkg.in/check.v1 v1.0.0-20160105164936-4f90aeace3a2/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f h1:BLraFXnmrev5lT+xlilqcH8XK9/i0At2xKjWk4p6zsU= gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= From 848ee6c48a07eeddbc4b25fc262fc024e33ca218 Mon Sep 17 00:00:00 2001 From: Chris Sawer Date: Tue, 23 Jan 2024 17:04:55 +0000 Subject: [PATCH 2/5] Add test data for zip file permissions issues --- testdata/.gitignore | 2 +- testdata/permissions.tar | Bin 0 -> 10240 bytes testdata/permissions.zip | Bin 0 -> 1726 bytes 3 files changed, 1 insertion(+), 1 deletion(-) create mode 100644 testdata/permissions.tar create mode 100644 testdata/permissions.zip diff --git a/testdata/.gitignore b/testdata/.gitignore index 0121aea..95bb67c 100644 --- a/testdata/.gitignore +++ b/testdata/.gitignore @@ -1,3 +1,3 @@ big.tar.gz big.zip - +filesbeforedirectories.zip diff --git a/testdata/permissions.tar b/testdata/permissions.tar new file mode 100644 index 0000000000000000000000000000000000000000..06c03c2e0f06b1652b991e48b93298c197887dc2 GIT binary patch literal 10240 zcmeH~(Q3mm3`PAZ`GZ-GWq;3DI~Leey8ZtoyM{Fs=7&l!di7$GP$|8~S5{8j&N03O z%ZL(EB*;RQZ&rPz5hydSrSeJ~B2g?H7Pfq;k^B4d^|~)hc>CD5%eZ-G{pxf4WRz79{i=D&+#vvKm6~3KQ#W;!T(i!J>~xd{x0Nuk@xNMoaOwr3_awv#ti?J zdjHST|83vu`j4M{{#X9Rfxp(6<4?{1IfB2XCr9qaU&OWy^?#8XQ~YJ{*ZH4U@V8_# za(DiDT$-$%f1cu>bWRZd75q(D9FsQw#eu)nnDKwo?*!#9^S^CZx+;P5|6EErZi|Or vH~&kGIsURwh?0y8;QvqxNCpL<02F`%Pyh-*0Vn_kpa2wr0#E=7JgWjfErH+C literal 0 HcmV?d00001 diff --git a/testdata/permissions.zip b/testdata/permissions.zip new file mode 100644 index 0000000000000000000000000000000000000000..b63aa8acfdd12d1f97bf6e43c2939d718d635c38 GIT binary patch literal 1726 zcmajgy-LGS6u|M*ruDOfZVsZ4V1_nX4eB5+9Ta>3D-@grL3A+=4nBi|N(C)ws9%$! zAR;2-Yjp7uM9;Y&xx>BZrs0Gv_s6{?f7)CMDkDI@-h8|&9@K3RCSk3;*$Bg#mE~aw zzIL+K^lsML+8%{cr5^x@TQdjF-9W5?dj#fuK!*VuY6rJPz!?WLIapFrm5_huMT4^ERro4G(6Qp|LS|Nk~IX zc7hmn;=$}n4AxKf6(*WNjGFOab};viyYx_F)Ffig9t~y}X|RX%Gsmz`#GGwb2k>iz N8eT{E-&4Z@_y<=kBR&8C literal 0 HcmV?d00001 From b540998c0914d0d83c6ed73f9ee0a20e6309c163 Mon Sep 17 00:00:00 2001 From: Chris Sawer Date: Tue, 23 Jan 2024 17:05:24 +0000 Subject: [PATCH 3/5] Fix zip file permissions issues --- extract.go | 8 ++++++++ extractor.go | 17 +++++++++++++++-- extractor_test.go | 10 ++++++++++ loggingfs_test.go | 31 +++++++++++++++++++++++++++++++ 4 files changed, 64 insertions(+), 2 deletions(-) diff --git a/extract.go b/extract.go index 973abdb..9ab7ef9 100644 --- a/extract.go +++ b/extract.go @@ -110,3 +110,11 @@ func (f fs) OpenFile(name string, flag int, perm os.FileMode) (*os.File, error) func (f fs) Remove(path string) error { return os.Remove(path) } + +func (f fs) Stat(name string) (os.FileInfo, error) { + return os.Stat(name) +} + +func (f fs) Chmod(name string, mode os.FileMode) error { + return os.Chmod(name, mode) +} diff --git a/extractor.go b/extractor.go index d472d3d..c9f28c6 100644 --- a/extractor.go +++ b/extractor.go @@ -38,6 +38,13 @@ type Extractor struct { // Remove removes the named file or (empty) directory. Remove(path string) error + + // Stat returns a FileInfo describing the named file. + Stat(name string) (os.FileInfo, error) + + // Chmod changes the mode of the named file to mode. + // If the file is a symbolic link, it changes the mode of the link's target. + Chmod(name string, mode os.FileMode) error } } @@ -346,7 +353,13 @@ func (e *Extractor) Zip(ctx context.Context, body io.Reader, location string, re switch { case info.IsDir() || forceDir: - if err := e.FS.MkdirAll(path, info.Mode()|os.ModeDir|100); err != nil { + dirMode := info.Mode() | os.ModeDir | 0100 + if _, err := e.FS.Stat(path); err == nil { + // directory already created, update permissions + if err := e.FS.Chmod(path, dirMode); err != nil { + return errors.Annotatef(err, "Set permissions %s", path) + } + } else if err := e.FS.MkdirAll(path, dirMode); err != nil { return errors.Annotatef(err, "Create directory %s", path) } // We only check for symlinks because hard links aren't possible @@ -379,7 +392,7 @@ func (e *Extractor) Zip(ctx context.Context, body io.Reader, location string, re func (e *Extractor) copy(ctx context.Context, path string, mode os.FileMode, src io.Reader) error { // We add the execution permission to be able to create files inside it - err := e.FS.MkdirAll(filepath.Dir(path), mode|os.ModeDir|100) + err := e.FS.MkdirAll(filepath.Dir(path), mode|os.ModeDir|0100) if err != nil { return err } diff --git a/extractor_test.go b/extractor_test.go index db8ef48..f817d64 100644 --- a/extractor_test.go +++ b/extractor_test.go @@ -360,3 +360,13 @@ func (m MockDisk) OpenFile(name string, flag int, perm os.FileMode) (*os.File, e func (m MockDisk) Remove(path string) error { return os.Remove(filepath.Join(m.Base, path)) } + +func (m MockDisk) Stat(name string) (os.FileInfo, error) { + name = filepath.Join(m.Base, name) + return os.Stat(name) +} + +func (m MockDisk) Chmod(name string, mode os.FileMode) error { + name = filepath.Join(m.Base, name) + return os.Chmod(name, mode) +} diff --git a/loggingfs_test.go b/loggingfs_test.go index 05017ee..b13048c 100644 --- a/loggingfs_test.go +++ b/loggingfs_test.go @@ -16,6 +16,7 @@ type LoggedOp struct { Path string OldPath string Mode os.FileMode + Info os.FileInfo Flags int Err error } @@ -33,6 +34,10 @@ func (op *LoggedOp) String() string { res += fmt.Sprintf("open %v %s (flags=%04x)", op.Mode, op.Path, op.Flags) case "remove": res += fmt.Sprintf("remove %v", op.Path) + case "stat": + res += fmt.Sprintf("stat %v -> %v", op.Path, op.Info) + case "chmod": + res += fmt.Sprintf("chmod %v %s", op.Mode, op.Path) default: panic("unknown LoggedOP " + op.Op) } @@ -108,6 +113,32 @@ func (m *LoggingFS) Remove(path string) error { return err } +func (m *LoggingFS) Stat(path string) (os.FileInfo, error) { + info, err := os.Stat(path) + op := &LoggedOp{ + Op: "stat", + Path: path, + Info: info, + Err: err, + } + m.Journal = append(m.Journal, op) + fmt.Println("FS>", op) + return info, err +} + +func (m *LoggingFS) Chmod(path string, mode os.FileMode) error { + err := os.Chmod(path, mode) + op := &LoggedOp{ + Op: "chmod", + Path: path, + Mode: mode, + Err: err, + } + m.Journal = append(m.Journal, op) + fmt.Println("FS>", op) + return err +} + func (m *LoggingFS) String() string { res := "" for _, op := range m.Journal { From eeefea2a5a4c0e58a56e159696a22b311afeeae1 Mon Sep 17 00:00:00 2001 From: Chris Sawer Date: Wed, 24 Jan 2024 16:12:34 +0000 Subject: [PATCH 4/5] Fix building/running of permissions tests on Windows --- extractor_test.go | 17 ++++++++--------- umask_unix_test.go | 21 +++++++++++++++++++++ umask_windows_test.go | 21 +++++++++++++++++++++ 3 files changed, 50 insertions(+), 9 deletions(-) create mode 100644 umask_unix_test.go create mode 100644 umask_windows_test.go diff --git a/extractor_test.go b/extractor_test.go index f817d64..dd5a239 100644 --- a/extractor_test.go +++ b/extractor_test.go @@ -16,7 +16,6 @@ import ( "github.com/arduino/go-paths-helper" "github.com/codeclysm/extract/v4" "github.com/stretchr/testify/require" - "golang.org/x/sys/unix" ) func TestExtractors(t *testing.T) { @@ -264,8 +263,8 @@ func TestSymLinkMazeHardening(t *testing.T) { func TestUnixPermissions(t *testing.T) { // Disable user's umask to enable creation of files with any permission, restore it after the test - userUmask := unix.Umask(0) - defer unix.Umask(userUmask) + userUmask := UnixUmaskZero() + defer UnixUmask(userUmask) archiveFilenames := []string{ "testdata/permissions.zip", @@ -287,11 +286,11 @@ func TestUnixPermissions(t *testing.T) { if strings.HasPrefix(filename, "dir") { desiredPermString, _ := strings.CutPrefix(filename, "dir") desiredPerms, _ := strconv.ParseUint(desiredPermString, 8, 32) - require.Equal(t, os.ModeDir|os.FileMode(desiredPerms), info.Mode()) + require.Equal(t, os.ModeDir|os.FileMode(OsDirPerms(desiredPerms)), info.Mode()) } else if strings.HasPrefix(filename, "file") { desiredPermString, _ := strings.CutPrefix(filename, "file") desiredPerms, _ := strconv.ParseUint(desiredPermString, 8, 32) - require.Equal(t, os.FileMode(desiredPerms), info.Mode()) + require.Equal(t, os.FileMode(OsFilePerms(desiredPerms)), info.Mode()) } return nil }) @@ -300,8 +299,8 @@ func TestUnixPermissions(t *testing.T) { func TestZipDirectoryPermissions(t *testing.T) { // Disable user's umask to enable creation of files with any permission, restore it after the test - userUmask := unix.Umask(0) - defer unix.Umask(userUmask) + userUmask := UnixUmaskZero() + defer UnixUmask(userUmask) // This arduino library has files before their containing directories in the zip, // so a good test case that these directory permissions are created correctly @@ -321,9 +320,9 @@ func TestZipDirectoryPermissions(t *testing.T) { // Test files and directories (excluding the parent) match permissions from the zip file if path != tmp.String() { if info.IsDir() { - require.Equal(t, os.ModeDir|os.FileMode(0755), info.Mode()) + require.Equal(t, os.ModeDir|os.FileMode(OsDirPerms(0755)), info.Mode()) } else { - require.Equal(t, os.FileMode(0644), info.Mode()) + require.Equal(t, os.FileMode(OsFilePerms(0644)), info.Mode()) } } return nil diff --git a/umask_unix_test.go b/umask_unix_test.go new file mode 100644 index 0000000..ad0f155 --- /dev/null +++ b/umask_unix_test.go @@ -0,0 +1,21 @@ +//go:build !windows + +package extract_test + +import "golang.org/x/sys/unix" + +func UnixUmaskZero() int { + return unix.Umask(0) +} + +func UnixUmask(userUmask int) { + unix.Umask(userUmask) +} + +func OsFilePerms(unixPerms uint64) uint64 { + return unixPerms +} + +func OsDirPerms(unixPerms uint64) uint64 { + return unixPerms +} diff --git a/umask_windows_test.go b/umask_windows_test.go new file mode 100644 index 0000000..e0e0ac2 --- /dev/null +++ b/umask_windows_test.go @@ -0,0 +1,21 @@ +//go:build windows + +package extract_test + +func UnixUmaskZero() int { + return 0 +} + +func UnixUmask(userUmask int) { +} + +func OsFilePerms(unixPerms uint64) uint64 { + // Go on Windows just uses 666/444 for files depending on whether "read only" is set + globalPerms := unixPerms >> 6 + return globalPerms | (globalPerms << 3) | (globalPerms << 6) +} + +func OsDirPerms(unixPerms uint64) uint64 { + // Go on Windows just uses 777 for directories + return 0777 +} From 432fe339b8ead04414187ad0a9088587c19145ab Mon Sep 17 00:00:00 2001 From: Chris Sawer Date: Fri, 1 Mar 2024 14:42:07 +0000 Subject: [PATCH 5/5] Fix tests to work on Go 1.17 --- extractor_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/extractor_test.go b/extractor_test.go index dd5a239..0d8011c 100644 --- a/extractor_test.go +++ b/extractor_test.go @@ -284,11 +284,11 @@ func TestUnixPermissions(t *testing.T) { filename := filepath.Base(path) // Desired permissions indicated by part of the filenames inside the zip/tar files if strings.HasPrefix(filename, "dir") { - desiredPermString, _ := strings.CutPrefix(filename, "dir") + desiredPermString := strings.Split(filename, "dir")[1] desiredPerms, _ := strconv.ParseUint(desiredPermString, 8, 32) require.Equal(t, os.ModeDir|os.FileMode(OsDirPerms(desiredPerms)), info.Mode()) } else if strings.HasPrefix(filename, "file") { - desiredPermString, _ := strings.CutPrefix(filename, "file") + desiredPermString := strings.Split(filename, "file")[1] desiredPerms, _ := strconv.ParseUint(desiredPermString, 8, 32) require.Equal(t, os.FileMode(OsFilePerms(desiredPerms)), info.Mode()) }