diff --git a/CHANGELOG.md b/CHANGELOG.md index 0960cfb..12b6f70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,13 @@ and this project adheres to [Semantic Versioning](http://semver.org/). users thinking their code sets these bits when it doesn't. Programs that need to deal with compatibility can mask the bits themselves. (#23, #25) +## Fixes ## +- If a directory has `S_ISGID` set, then all child directories will have + `S_ISGID` set when created and a different gid will be used for any inode + created under the directory. Previously, the "expected owner and mode" + validation in `securejoin.MkdirAll` did not correctly handle this. We now + correctly handle this case. (#24, #25) + ## [0.3.1] - 2024-07-23 ## ### Changed ### diff --git a/mkdir_linux.go b/mkdir_linux.go index 0c537ba..49ffdbe 100644 --- a/mkdir_linux.go +++ b/mkdir_linux.go @@ -127,6 +127,17 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err expectedGid = uint32(unix.Getegid()) ) + // The setgid bit (S_ISGID = 0o2000) is inherited to child directories and + // affects the group of any inodes created in said directory, so if the + // starting directory has it set we need to adjust our expected mode and + // owner to match. + if st, err := fstatFile(currentDir); err != nil { + return nil, fmt.Errorf("failed to stat starting path for mkdir %q: %w", currentDir.Name(), err) + } else if st.Mode&unix.S_ISGID == unix.S_ISGID { + expectedMode |= unix.S_ISGID + expectedGid = st.Gid + } + // Create the remaining components. for _, part := range remainingParts { switch part { diff --git a/mkdir_linux_test.go b/mkdir_linux_test.go index 5d3f059..b32df3f 100644 --- a/mkdir_linux_test.go +++ b/mkdir_linux_test.go @@ -54,7 +54,7 @@ var mkdirAll_MkdirAllHandle mkdirAllFunc = func(t *testing.T, root, unsafePath s return nil } -func checkMkdirAll(t *testing.T, partialLookupFn partialLookupFunc, root, unsafePath string, mode, expectedMode int, expectedErr error) { +func checkMkdirAll(t *testing.T, mkdirAll mkdirAllFunc, root, unsafePath string, mode, 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() @@ -73,9 +73,6 @@ func checkMkdirAll(t *testing.T, partialLookupFn partialLookupFunc, root, unsafe } }() - // This mode is different to the one set up by createTree. - const expectedMode = 0o711 - // Actually make the tree. err = mkdirAll(t, root, unsafePath, mode) assert.ErrorIsf(t, err, expectedErr, "MkdirAll(%q, %q)", root, unsafePath) @@ -128,6 +125,9 @@ func testMkdirAll_Basic(t *testing.T, mkdirAll mkdirAllFunc) { // Symlink loop. "dir loop", "symlink loop/link ../loop/link", + // S_ISGID directory. + "dir sgid-self ::2755", + "dir sgid-sticky-self ::3755", } withWithoutOpenat2(t, true, func(t *testing.T) { @@ -182,13 +182,14 @@ func testMkdirAll_Basic(t *testing.T, mkdirAll mkdirAllFunc) { "loop-basic": {unsafePath: "loop/link/foo", expectedErr: unix.ELOOP}, "loop-dotdot": {unsafePath: "loop/link/../foo", expectedErr: unix.ELOOP}, // Make sure the S_ISGID handling is correct. - "sgid-self": {unsafePath: "sgid-self/"} + "sgid-dir-ownedbyus": {unsafePath: "sgid-self/foo/bar/baz", expectedModeBits: unix.S_ISGID}, + "sgid-sticky-dir-ownedbyus": {unsafePath: "sgid-sticky-self/foo/bar/baz", expectedModeBits: unix.S_ISGID}, } { test := test // copy iterator t.Run(name, func(t *testing.T) { root := createTree(t, tree...) const mode = 0o711 - checkMkdirAll(t, mkdirAll, root, test.unsafePath, mode, test.expectedModeBits|mode) + checkMkdirAll(t, mkdirAll, root, test.unsafePath, mode, test.expectedModeBits|mode, test.expectedErr) }) } }) @@ -202,7 +203,49 @@ func TestMkdirAllHandle_Basic(t *testing.T) { testMkdirAll_Basic(t, mkdirAll_MkdirAllHandle) } -func testMkdirAll_InvalidMode(t *testing.T, mkdirAll func(t *testing.T, root, unsafePath string, mode int) error) { +func testMkdirAll_AsRoot(t *testing.T, mkdirAll mkdirAllFunc) { + requireRoot(t) // chown + + // We create a new tree for each test, but the template is the same. + tree := []string{ + // S_ISGID directories. + "dir sgid-self ::2755", + "dir sgid-other 1000:1000:2755", + "dir sgid-sticky-self ::3755", + "dir sgid-sticky-other 1000:1000:3755", + } + + withWithoutOpenat2(t, true, func(t *testing.T) { + for name, test := range map[string]struct { + unsafePath string + expectedErr error + expectedModeBits int + }{ + // Make sure the S_ISGID handling is correct. + "sgid-dir-ownedbyus": {unsafePath: "sgid-self/foo/bar/baz", expectedModeBits: unix.S_ISGID}, + "sgid-dir-ownedbyother": {unsafePath: "sgid-other/foo/bar/baz", expectedModeBits: unix.S_ISGID}, + "sgid-sticky-dir-ownedbyus": {unsafePath: "sgid-sticky-self/foo/bar/baz", expectedModeBits: unix.S_ISGID}, + "sgid-sticky-dir-ownedbyother": {unsafePath: "sgid-sticky-other/foo/bar/baz", expectedModeBits: unix.S_ISGID}, + } { + test := test // copy iterator + t.Run(name, func(t *testing.T) { + root := createTree(t, tree...) + const mode = 0o711 + checkMkdirAll(t, mkdirAll, root, test.unsafePath, mode, test.expectedModeBits|mode, test.expectedErr) + }) + } + }) +} + +func TestMkdirAll_AsRoot(t *testing.T) { + testMkdirAll_AsRoot(t, mkdirAll_MkdirAll) +} + +func TestMkdirAllHandle_AsRoot(t *testing.T) { + testMkdirAll_AsRoot(t, mkdirAll_MkdirAllHandle) +} + +func testMkdirAll_InvalidMode(t *testing.T, mkdirAll mkdirAllFunc) { for _, test := range []struct { mode int expectedErr error diff --git a/openat_linux.go b/openat_linux.go index 949fb5f..ac083f2 100644 --- a/openat_linux.go +++ b/openat_linux.go @@ -42,6 +42,10 @@ func fstatatFile(dir *os.File, path string, flags int) (unix.Stat_t, error) { return stat, nil } +func fstatFile(fd *os.File) (unix.Stat_t, error) { + return fstatatFile(fd, "", unix.AT_EMPTY_PATH) +} + func readlinkatFile(dir *os.File, path string) (string, error) { size := 4096 for {