Skip to content

Commit

Permalink
merge #44 into cyphar/filepath-securejoin:main
Browse files Browse the repository at this point in the history
Aleksa Sarai (2):
  gha: add GOARCH=386 build check
  mkdirall: switch to os.FileMode argument

LGTMs: cyphar kolyshkin
  • Loading branch information
cyphar committed Jan 13, 2025
2 parents f3a512c + ea4e5b6 commit e410d4a
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 29 deletions.
8 changes: 8 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ jobs:
- windows-latest
- ubuntu-latest
- macos-latest
go-arch:
- amd64
go-version:
- "1.18"
- "1.19"
Expand All @@ -27,13 +29,19 @@ 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
- uses: actions/setup-go@v5
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
Expand Down
14 changes: 14 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 ###
Expand Down
49 changes: 35 additions & 14 deletions mkdir_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -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:
//
Expand All @@ -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.
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
35 changes: 20 additions & 15 deletions mkdir_linux_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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)
}
}

Expand All @@ -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()
Expand Down

0 comments on commit e410d4a

Please sign in to comment.