From 5e8978406c97076aaacfd30d6b078e52e638070c Mon Sep 17 00:00:00 2001 From: Larry Clapp Date: Wed, 27 Sep 2023 15:31:12 -0400 Subject: [PATCH 1/4] interp: fix `cd` builtin: check all user groups Fixes #1033. --- interp/os_unix.go | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/interp/os_unix.go b/interp/os_unix.go index e393399c..e3cce97d 100644 --- a/interp/os_unix.go +++ b/interp/os_unix.go @@ -43,13 +43,26 @@ func hasPermissionToDir(info os.FileInfo) bool { return true } + // Group perms only apply if you're not the owner of the directory. gid, _ := strconv.Atoi(user.Gid) - // other users in group (g) - if perm&0o010 != 0 && st.Uid != uint32(uid) && st.Gid == uint32(gid) { - return true + inGroup := st.Gid == uint32(gid) + if st.Uid != uint32(uid) { + gids, _ := user.GroupIds() + for _, gid := range gids { + gid, _ := strconv.Atoi(gid) + if st.Gid == uint32(gid) { + // other users in group (g) + if perm&0o010 != 0 { + return true + } + inGroup = true + } + } } - // remaining users (o) - if perm&0o001 != 0 && st.Uid != uint32(uid) && st.Gid != uint32(gid) { + + // remaining users (o) -- only apply if you're not the owner and none of + // your groups match its group. + if perm&0o001 != 0 && st.Uid != uint32(uid) && !inGroup { return true } From d8a48c253e3d107322c5e09cfda5e7c4f5f7cdb9 Mon Sep 17 00:00:00 2001 From: Larry Clapp Date: Thu, 28 Sep 2023 11:57:37 -0400 Subject: [PATCH 2/4] Streamline hasPermissionsToDir If the user id matches the file owner id, then only the "user" permission bits apply, so just return true or false immediately. And then similarly for group & "others". --- interp/os_unix.go | 54 +++++++++++++++++++++++++---------------------- 1 file changed, 29 insertions(+), 25 deletions(-) diff --git a/interp/os_unix.go b/interp/os_unix.go index e3cce97d..fd963ad2 100644 --- a/interp/os_unix.go +++ b/interp/os_unix.go @@ -18,8 +18,8 @@ func mkfifo(path string, mode uint32) error { return unix.Mkfifo(path, mode) } -// hasPermissionToDir returns if the OS current user has execute permission -// to the given directory +// hasPermissionToDir returns true if the OS current user has execute +// permission to the given directory func hasPermissionToDir(info os.FileInfo) bool { user, err := user.Current() if err != nil { @@ -38,33 +38,37 @@ func hasPermissionToDir(info os.FileInfo) bool { panic("unexpected info.Sys type") } perm := info.Mode().Perm() + // user (u) - if perm&0o100 != 0 && st.Uid == uint32(uid) { - return true + if st.Uid == uint32(uid) { + return perm&0o100 != 0 } - // Group perms only apply if you're not the owner of the directory. - gid, _ := strconv.Atoi(user.Gid) - inGroup := st.Gid == uint32(gid) - if st.Uid != uint32(uid) { - gids, _ := user.GroupIds() - for _, gid := range gids { - gid, _ := strconv.Atoi(gid) - if st.Gid == uint32(gid) { - // other users in group (g) - if perm&0o010 != 0 { - return true - } - inGroup = true - } - } + // group (g) -- check the users's actual group, and then all the other + // groups they're in. + gid, err := strconv.Atoi(user.Gid) + if err != nil { + return false // on POSIX systems, Gid should always be a decimal number } - - // remaining users (o) -- only apply if you're not the owner and none of - // your groups match its group. - if perm&0o001 != 0 && st.Uid != uint32(uid) && !inGroup { - return true + if st.Gid == uint32(gid) { + return perm&0o010 != 0 + } + gids, err := user.GroupIds() + if err != nil { + // If we can't get the list of group IDs, we can't know if the group + // permissions, so default to false/no access. + return false + } + for _, gid := range gids { + gid, err := strconv.Atoi(gid) + if err != nil { + return false + } + if st.Gid == uint32(gid) { + return perm&0o010 != 0 + } } - return false + // remaining users (o) + return perm&0o001 != 0 } From c5b3b2f64e249850e874d9c852184ea97b95a1e4 Mon Sep 17 00:00:00 2001 From: Larry Clapp Date: Thu, 28 Sep 2023 15:07:50 -0400 Subject: [PATCH 3/4] Replace body of hasPermissionToDir with unix.Access Rip out the body of interp.hasPermissionToDir and replace it with just a call to unix.Access(path, unix.X_OK). Update the function signature of hasPermissionToDir in os_notunix.go, too. --- interp/builtin.go | 2 +- interp/os_notunix.go | 2 +- interp/os_unix.go | 58 ++------------------------------------------ 3 files changed, 4 insertions(+), 58 deletions(-) diff --git a/interp/builtin.go b/interp/builtin.go index 8dc45cb4..b24a3d6a 100644 --- a/interp/builtin.go +++ b/interp/builtin.go @@ -963,7 +963,7 @@ func (r *Runner) changeDir(ctx context.Context, path string) int { if err != nil || !info.IsDir() { return 1 } - if !hasPermissionToDir(info) { + if !hasPermissionToDir(path) { return 1 } r.Dir = path diff --git a/interp/os_notunix.go b/interp/os_notunix.go index f2409321..8aef4a56 100644 --- a/interp/os_notunix.go +++ b/interp/os_notunix.go @@ -15,6 +15,6 @@ func mkfifo(path string, mode uint32) error { } // hasPermissionToDir is a no-op on Windows. -func hasPermissionToDir(info os.FileInfo) bool { +func hasPermissionToDir(string) bool { return true } diff --git a/interp/os_unix.go b/interp/os_unix.go index fd963ad2..3a82d69b 100644 --- a/interp/os_unix.go +++ b/interp/os_unix.go @@ -6,11 +6,6 @@ package interp import ( - "os" - "os/user" - "strconv" - "syscall" - "golang.org/x/sys/unix" ) @@ -20,55 +15,6 @@ func mkfifo(path string, mode uint32) error { // hasPermissionToDir returns true if the OS current user has execute // permission to the given directory -func hasPermissionToDir(info os.FileInfo) bool { - user, err := user.Current() - if err != nil { - return false // unknown user; assume no permissions - } - uid, err := strconv.Atoi(user.Uid) - if err != nil { - return false // on POSIX systems, Uid should always be a decimal number - } - if uid == 0 { - return true // super-user - } - - st, _ := info.Sys().(*syscall.Stat_t) - if st == nil { - panic("unexpected info.Sys type") - } - perm := info.Mode().Perm() - - // user (u) - if st.Uid == uint32(uid) { - return perm&0o100 != 0 - } - - // group (g) -- check the users's actual group, and then all the other - // groups they're in. - gid, err := strconv.Atoi(user.Gid) - if err != nil { - return false // on POSIX systems, Gid should always be a decimal number - } - if st.Gid == uint32(gid) { - return perm&0o010 != 0 - } - gids, err := user.GroupIds() - if err != nil { - // If we can't get the list of group IDs, we can't know if the group - // permissions, so default to false/no access. - return false - } - for _, gid := range gids { - gid, err := strconv.Atoi(gid) - if err != nil { - return false - } - if st.Gid == uint32(gid) { - return perm&0o010 != 0 - } - } - - // remaining users (o) - return perm&0o001 != 0 +func hasPermissionToDir(path string) bool { + return unix.Access(path, unix.X_OK) == nil } From f25d6d68e131c6705ee74c18ae27d68a22923033 Mon Sep 17 00:00:00 2001 From: Larry Clapp Date: Thu, 28 Sep 2023 15:22:49 -0400 Subject: [PATCH 4/4] Fix Windows compilation error Since os_notunix.go is Windows-only, my "goimports" didn't run, so it didn't remove the now-unnecessary import of "os". --- interp/os_notunix.go | 1 - 1 file changed, 1 deletion(-) diff --git a/interp/os_notunix.go b/interp/os_notunix.go index 8aef4a56..66ed777a 100644 --- a/interp/os_notunix.go +++ b/interp/os_notunix.go @@ -7,7 +7,6 @@ package interp import ( "fmt" - "os" ) func mkfifo(path string, mode uint32) error {