Merge pull request #23 from chrissawer/zip-permissions-fixes

Zip permissions fixes

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)
+}

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
 	}

extractor_test.go

@@ -9,6 +9,8 @@ import (
 	"os"
 	"path/filepath"
 	"runtime"
+	"strconv"
+	"strings"
 	"testing"
 
 	"github.com/arduino/go-paths-helper"
@@ -259,6 +261,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 := UnixUmaskZero()
+	defer UnixUmask(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.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.Split(filename, "file")[1]
+				desiredPerms, _ := strconv.ParseUint(desiredPermString, 8, 32)
+				require.Equal(t, os.FileMode(OsFilePerms(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 := 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
+	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(OsDirPerms(0755)), info.Mode())
+			} else {
+				require.Equal(t, os.FileMode(OsFilePerms(0644)), info.Mode())
+			}
+		}
+		return nil
+	})
+}
+
 // MockDisk is a disk that chroots to a directory
 type MockDisk struct {
 	Base string
@@ -289,3 +359,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)
+}

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 (

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=

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 {

testdata/.gitignore

@@ -1,3 +1,3 @@
 big.tar.gz
 big.zip
-
+filesbeforedirectories.zip

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
+}

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
+}