interp: fix cd builtin: call a system function to determine access instead of reverse-engineering it ourselves
#1034
Conversation
|
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.) |
|
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 |
|
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 |
|
Actually performing |
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. |
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".
|
Doesn't open and a possible read afterwards mimic the same behavior, without actually changing the directory? I would bet that it does. |
|
No. For --x and -wx, I don't see any Go call that would specifically examine the "execute" bit of a directory. |
|
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! |
|
id: uid lmc, gid staff, other guids: contains admin, but not daemon test directory: 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. |
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.
theclapp
changed the title
cd builtin: check all user groupscd builtin: call a system function to determine access instead of reverse-engineering it ourselves
Since os_notunix.go is Windows-only, my "goimports" didn't run, so it didn't remove the now-unnecessary import of "os".
|
Finally got all checks to pass. No code changes, I just kept re-running the checks. I dunno if that was the right approach. 🤷♂️ 😆 |
There was a problem hiding this comment.
TIL about the access syscall:
access() checks whether the calling process can access the file pathname.
R_OK, W_OK, and X_OK test whether the file exists and grants read, write, and execute permissions, respectively.
I am also personally surprised that "execute" permission is enough to cd into a directory, and "read" is unnecessary, but you've clearly tested it :)
Either way, love to delete all this code. Depending on x/sys/unix is always a bit tricky with portability, but this is already os_unix.go.
|
It's better than it was, but is still wrong for set-uid programs.
I saw that in the 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 I think I need to use But as I said, it's still better than it was. |
Fixes #1033.