interp: fix `cd` builtin: call a system function to determine access instead of reverse-engineering it ourselves

[theclapp]

Fixes #1033.

[theclapp]

It seems like if you are user id 1 & group id 2, and a directory is also user id 1 & group id 2, and the directory is 070 or 007, then you should be able to cd into the directory -- the "group" or "other" perms should let you in even if the "owner" perms don't, but you can't & they don't.

I don't know if this is "right" or not, but it is what both bash and zsh do on both macOS and Linux, so I'm sticking with it.

(To be clear: This package already did it that way, this comment was just to explain it a little, and this PR was just to make it look at all your groups, not just your "main" group.)

[mvdan]

I'm starting to wonder if this is the wrong approach entirely - it feels like we're trying to reverse-engineer the Unix-like semantics. Which are probably pointless or more likely wrong on Windows or Plan9.

What if we were to open the directory and try to read from it? That should get the OS to tell us whether we can use the directory or not. Opening the directory alone might be enough to tell; if not, then we could call e.g. https://pkg.go.dev/os#File.Readdirnames with an argument 1 to get just the first directory entry name, to just perform the cheapest read operation possible.

[theclapp]

I thought about that, and would prefer that approach. I wasn't sure what operations to try to verity the exact permissions required. It may take some experimentation.

There is os.Chdir (et al) but that doesn't seem right, either.

[mvdan]

Actually performing os.Chdir would be correct if we can assume we own the entire process per https://github.com/mvdan/sh/issues/171, but in general we can't assume that, e.g. if a Go process is interpreting multiple scripts they can't be fighting each other over os.Chdir or other global state like os.Setenv.

[theclapp]

but in general we can't assume that, e.g. if a Go process is interpreting multiple scripts

Yes, exactly. My app might be doing exactly that. Now, that's all it does, so if sh used a lock surrounding the global Chdir, and exported it (or made some api public), then apps that used sh and were aware of the problem could cooperate.

That still seems suboptimal. I'd rather figure out some other (more local/specific) system call (or combination thereof) that exactly mirrors the pass/fail semantics of chdir.

[mvdan]

Doesn't open and a possible read afterwards mimic the same behavior, without actually changing the directory? I would bet that it does.

[theclapp]

No. For --x and -wx, cd should succeed, but Open/OpenFile fails (so there's nothing to Read). For r-- and rw-, cd should fail, but Open and Readdirnames both succeed. Ditto ReadDir and Readdir. So Open and Read* look at the "read" bit.

I don't see any Go call that would specifically examine the "execute" bit of a directory.

[theclapp]

It looks like golang.org/x/sys/unix.Access (https://pkg.go.dev/golang.org/x/sys@v0.12.0/unix#Access) does the trick!

[theclapp]

id: uid lmc, gid staff, other guids: contains admin, but not daemon

test directory:

d---------  3 daemon  staff   96 Sep 28 10:59 g0/
d-----x---  3 daemon  staff   96 Sep 28 10:59 g1/
d----w----  3 daemon  staff   96 Sep 28 10:59 g2/
d----wx---  3 daemon  staff   96 Sep 28 10:59 g3/
d---r-----@ 3 daemon  staff   96 Sep 28 10:59 g4/
d---r-x---@ 3 daemon  staff   96 Sep 28 10:59 g5/
d---rw----@ 3 daemon  staff   96 Sep 28 10:59 g6/
d---rwx---@ 3 daemon  staff   96 Sep 28 10:59 g7/
d---------  2 daemon  admin   64 Sep 28 11:36 g_0/
d-----x---  2 daemon  admin   64 Sep 28 11:36 g_1/
d----w----  2 daemon  admin   64 Sep 28 11:36 g_2/
d----wx---  2 daemon  admin   64 Sep 28 11:36 g_3/
d---r-----@ 2 daemon  admin   64 Sep 28 11:36 g_4/
d---r-x---@ 2 daemon  admin   64 Sep 28 11:36 g_5/
d---rw----@ 2 daemon  admin   64 Sep 28 11:36 g_6/
d---rwx---@ 2 daemon  admin   64 Sep 28 11:36 g_7/
d---------  3 daemon  daemon  96 Sep 28 10:59 o0/
d--------x  3 daemon  daemon  96 Sep 28 10:59 o1/
d-------w-  3 daemon  daemon  96 Sep 28 10:59 o2/
d-------wx  3 daemon  daemon  96 Sep 28 10:59 o3/
d------r--@ 3 daemon  daemon  96 Sep 28 10:59 o4/
d------r-x@ 3 daemon  daemon  96 Sep 28 10:59 o5/
d------rw-@ 3 daemon  daemon  96 Sep 28 10:59 o6/
d------rwx@ 3 daemon  daemon  96 Sep 28 10:59 o7/
d---------  3 lmc     staff   96 Sep 28 10:59 u0/
d--x------  3 lmc     staff   96 Sep 28 10:59 u1/
d-w-------  3 lmc     staff   96 Sep 28 10:59 u2/
d-wx------  3 lmc     staff   96 Sep 28 10:59 u3/
dr--------@ 3 lmc     staff   96 Sep 28 10:59 u4/
dr-x------@ 3 lmc     staff   96 Sep 28 10:59 u5/
drw-------@ 3 lmc     staff   96 Sep 28 10:59 u6/
drwx------@ 3 lmc     staff   96 Sep 28 10:59 u7/

Test sh code:

for f in * ; do if cd $f >& /dev/null ; then echo $f worked ; cd .. ; else echo $f failed ; fi ; done
g0 failed
g1 worked
g2 failed
g3 worked
[...]

test Go code:

func (r *Runner) changeDir(ctx context.Context, path string) int {
    if path == "" {
        path = "."
    }
    path = r.absPath(path)
    info, err := r.stat(ctx, path)
    if err != nil || !info.IsDir() {
        return 1
    }
    accessErr := unix.Access(path, unix.X_OK)
    hptd := hasPermissionToDir(info)
    if hptd != (accessErr == nil) {
        log.Printf("%s: hptd: %v, accessErr: %v", path, hptd, accessErr)
    }
    if !hptd {
        return 1
    }
    r.Dir = path
    r.setVarString("OLDPWD", r.envGet("PWD"))
    r.setVarString("PWD", path)
    return 0
}

So the Go code runs both unix.Access and our already-defined hasPermissionToDir and prints stuff when they differ, and they never did.

[theclapp]

Finally got all checks to pass. No code changes, I just kept re-running the checks. I dunno if that was the right approach. 🤷‍♂️ 😆

[theclapp]

It's better than it was, but is still wrong for set-uid programs.

The check is done using the calling process's real UID and GID, rather than the effective IDs as is done when actually attempting an operation (e.g., open(2)) on the file.

I saw that in the access manpage, but, to be honest, just didn't understand it till now.

So, for example, if I (user "lmc") run gosh as set-uid root, I should be able to cd into any directory with any of the execute bits (user, group, other) set (since I'm running as root), but since Access checks the real UID ("lmc") not the effective UID ("root"), I can't. (I think. I'm not sure I've built my test-case correctly.)

I think I need to use Faccessat with a mode including AT_EACCESS.

But as I said, it's still better than it was.