diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2e5bebe..461d2d2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -19,6 +19,8 @@ jobs: - windows-latest - ubuntu-latest - macos-latest + go-arch: + - amd64 go-version: - "1.18" - "1.19" @@ -27,6 +29,10 @@ jobs: - "1.22" - "1.23" - "^1" + include: + - os: ubuntu-latest + go-arch: "386" + go-version: "^1" runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v4 @@ -34,6 +40,8 @@ jobs: with: go-version: ${{ matrix.go-version }} check-latest: true + - name: set GOOARCH + run: echo "GOARCH=${{ matrix.go-arch }}" >>"$GITHUB_ENV" - name: go build check run: go build ./... - name: go test build check diff --git a/CHANGELOG.md b/CHANGELOG.md index b437ee6..e7b01ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,20 @@ and this project adheres to [Semantic Versioning](http://semver.org/). necessarily a breaking API change (though we expect no real users to be affected by it). +- `MkdirAll` and `MkdirHandle` now take an `os.FileMode`-style mode argument + instead of a raw `unix.S_*`-style mode argument, which may cause compile-time + type errors depending on how you use `filepath-securejoin`. For most users, + there will be no change in behaviour aside from the type change (as the + bottom `0o777` bits are the same in both formats, and most users are probably + only using those bits). + + However, if you were using `unix.S_ISVTX` to set the sticky bit with + `MkdirAll(Handle)` you will need to switch to `os.ModeSticky` otherwise you + will get a runtime error with this update. In addition, the error message you + will get from passing `unix.S_ISUID` and `unix.S_ISGID` will be different as + they are treated as invalid bits now (note that previously passing said bits + was also an error). + ## [0.3.6] - 2024-12-17 ## ### Compatibility ### diff --git a/mkdir_linux.go b/mkdir_linux.go index 5e559bb..a17ae3b 100644 --- a/mkdir_linux.go +++ b/mkdir_linux.go @@ -21,6 +21,33 @@ var ( errPossibleAttack = errors.New("possible attack detected") ) +// modePermExt is like os.ModePerm except that it also includes the set[ug]id +// and sticky bits. +const modePermExt = os.ModePerm | os.ModeSetuid | os.ModeSetgid | os.ModeSticky + +//nolint:cyclop // this function needs to handle a lot of cases +func toUnixMode(mode os.FileMode) (uint32, error) { + sysMode := uint32(mode.Perm()) + if mode&os.ModeSetuid != 0 { + sysMode |= unix.S_ISUID + } + if mode&os.ModeSetgid != 0 { + sysMode |= unix.S_ISGID + } + if mode&os.ModeSticky != 0 { + sysMode |= unix.S_ISVTX + } + // We don't allow file type bits. + if mode&os.ModeType != 0 { + return 0, fmt.Errorf("%w %+.3o (%s): type bits not permitted", errInvalidMode, mode, mode) + } + // We don't allow other unknown modes. + if mode&^modePermExt != 0 || sysMode&unix.S_IFMT != 0 { + return 0, fmt.Errorf("%w %+.3o (%s): unknown mode bits", errInvalidMode, mode, mode) + } + return sysMode, nil +} + // MkdirAllHandle is equivalent to [MkdirAll], except that it is safer to use // in two respects: // @@ -39,17 +66,17 @@ var ( // a brand new lookup of unsafePath (such as with [SecureJoin] or openat2) after // doing [MkdirAll]. If you intend to open the directory after creating it, you // should use MkdirAllHandle. -func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err error) { - // Make sure there are no os.FileMode bits set. - if mode&^0o7777 != 0 { - return nil, fmt.Errorf("%w for mkdir 0o%.3o", errInvalidMode, mode) +func MkdirAllHandle(root *os.File, unsafePath string, mode os.FileMode) (_ *os.File, Err error) { + unixMode, err := toUnixMode(mode) + if err != nil { + return nil, err } // On Linux, mkdirat(2) (and os.Mkdir) silently ignore the suid and sgid // bits. We could also silently ignore them but since we have very few // users it seems more prudent to return an error so users notice that // these bits will not be set. - if mode&^0o1777 != 0 { - return nil, fmt.Errorf("%w for mkdir 0o%.3o: suid and sgid are ignored by mkdir", errInvalidMode, mode) + if unixMode&^0o1777 != 0 { + return nil, fmt.Errorf("%w for mkdir %+.3o: suid and sgid are ignored by mkdir", errInvalidMode, mode) } // Try to open as much of the path as possible. @@ -104,9 +131,6 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err return nil, fmt.Errorf("%w: yet-to-be-created path %q contains '..' components", unix.ENOENT, remainingPath) } - // Make sure the mode doesn't have any type bits. - mode &^= unix.S_IFMT - // Create the remaining components. for _, part := range remainingParts { switch part { @@ -123,7 +147,7 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err // directory at the same time as us. In that case, just continue on as // if we created it (if the created inode is not a directory, the // following open call will fail). - if err := unix.Mkdirat(int(currentDir.Fd()), part, uint32(mode)); err != nil && !errors.Is(err, unix.EEXIST) { + if err := unix.Mkdirat(int(currentDir.Fd()), part, unixMode); err != nil && !errors.Is(err, unix.EEXIST) { err = &os.PathError{Op: "mkdirat", Path: currentDir.Name() + "/" + part, Err: err} // Make the error a bit nicer if the directory is dead. if deadErr := isDeadInode(currentDir); deadErr != nil { @@ -196,10 +220,7 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err // If you plan to open the directory after you have created it or want to use // an open directory handle as the root, you should use [MkdirAllHandle] instead. // This function is a wrapper around [MkdirAllHandle]. -// -// NOTE: The mode argument must be set the unix mode bits (unix.S_I...), not -// the Go generic mode bits ([os.FileMode]...). -func MkdirAll(root, unsafePath string, mode int) error { +func MkdirAll(root, unsafePath string, mode os.FileMode) error { rootDir, err := os.OpenFile(root, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) if err != nil { return err diff --git a/mkdir_linux_test.go b/mkdir_linux_test.go index 99e2ee3..bcc989b 100644 --- a/mkdir_linux_test.go +++ b/mkdir_linux_test.go @@ -20,14 +20,14 @@ import ( "golang.org/x/sys/unix" ) -type mkdirAllFunc func(t *testing.T, root, unsafePath string, mode int) error +type mkdirAllFunc func(t *testing.T, root, unsafePath string, mode os.FileMode) error -var mkdirAll_MkdirAll mkdirAllFunc = func(t *testing.T, root, unsafePath string, mode int) error { +var mkdirAll_MkdirAll mkdirAllFunc = func(t *testing.T, root, unsafePath string, mode os.FileMode) error { // We can't check expectedPath here. return MkdirAll(root, unsafePath, mode) } -var mkdirAll_MkdirAllHandle mkdirAllFunc = func(t *testing.T, root, unsafePath string, mode int) error { +var mkdirAll_MkdirAllHandle mkdirAllFunc = func(t *testing.T, root, unsafePath string, mode os.FileMode) error { // Same logic as MkdirAll. rootDir, err := os.OpenFile(root, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) if err != nil { @@ -55,7 +55,7 @@ var mkdirAll_MkdirAllHandle mkdirAllFunc = func(t *testing.T, root, unsafePath s return nil } -func checkMkdirAll(t *testing.T, mkdirAll mkdirAllFunc, root, unsafePath string, mode, expectedMode int, expectedErr error) { +func checkMkdirAll(t *testing.T, mkdirAll mkdirAllFunc, root, unsafePath string, mode os.FileMode, expectedMode int, expectedErr error) { rootDir, err := os.OpenFile(root, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) require.NoError(t, err) defer rootDir.Close() @@ -248,31 +248,36 @@ func TestMkdirAllHandle_AsRoot(t *testing.T) { func testMkdirAll_InvalidMode(t *testing.T, mkdirAll mkdirAllFunc) { for _, test := range []struct { - mode int + mode os.FileMode expectedErr error }{ - // os.FileMode bits are invalid. - {int(os.ModeDir | 0o777), errInvalidMode}, - {int(os.ModeSticky | 0o777), errInvalidMode}, - {int(os.ModeIrregular | 0o777), errInvalidMode}, + // unix.S_IS* bits are invalid. + {unix.S_ISUID | 0o777, errInvalidMode}, + {unix.S_ISGID | 0o777, errInvalidMode}, + {unix.S_ISVTX | 0o777, errInvalidMode}, + {unix.S_ISUID | unix.S_ISGID | unix.S_ISVTX | 0o777, errInvalidMode}, // unix.S_IFMT bits are also invalid. {unix.S_IFDIR | 0o777, errInvalidMode}, {unix.S_IFREG | 0o777, errInvalidMode}, {unix.S_IFIFO | 0o777, errInvalidMode}, + // os.FileType bits are also invalid. + {os.ModeDir | 0o777, errInvalidMode}, + {os.ModeNamedPipe | 0o777, errInvalidMode}, + {os.ModeIrregular | 0o777, errInvalidMode}, // suid/sgid bits are silently ignored by mkdirat and so we return an // error explicitly. - {unix.S_ISUID | 0o777, errInvalidMode}, - {unix.S_ISGID | 0o777, errInvalidMode}, - {unix.S_ISUID | unix.S_ISGID | unix.S_ISVTX | 0o777, errInvalidMode}, + {os.ModeSetuid | 0o777, errInvalidMode}, + {os.ModeSetgid | 0o777, errInvalidMode}, + {os.ModeSetuid | os.ModeSetgid | os.ModeSticky | 0o777, errInvalidMode}, // Proper sticky bit should work. - {unix.S_ISVTX | 0o777, nil}, + {os.ModeSticky | 0o777, nil}, // Regular mode bits. {0o777, nil}, {0o711, nil}, } { root := t.TempDir() err := mkdirAll(t, root, "a/b/c", test.mode) - assert.ErrorIsf(t, err, test.expectedErr, "mkdirall 0o%.3o", test.mode) + assert.ErrorIsf(t, err, test.expectedErr, "mkdirall %+.3o (%s)", test.mode, test.mode) } } @@ -295,7 +300,7 @@ func newRacingMkdirMeta() *racingMkdirMeta { } } -func (m *racingMkdirMeta) checkMkdirAllHandle_Racing(t *testing.T, root, unsafePath string, mode int, allowedErrs []error) { +func (m *racingMkdirMeta) checkMkdirAllHandle_Racing(t *testing.T, root, unsafePath string, mode os.FileMode, allowedErrs []error) { rootDir, err := os.OpenFile(root, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) require.NoError(t, err, "open root") defer rootDir.Close()