Differential D29323
Implement O_PATH Authored by kib on Mar 18 2021, 12:29 PM. Tags None Referenced Files
Details
This was done from reading the Linux man page for open(2). I did not performed experiments on Linux to see how is this implementation is compatible with it. As I understand, it was asked for by samba devs/porters. Test program is available at https://gist.github.com/35e8084c99fcc9a2af0b1ae79dd845d7
Diff Detail
Event TimelineThere are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions Disable adv locking.
Comment Actions Man page editings. Comment Actions I started writing some test cases for O_PATH/AT_EMPTY_PATH BTW.
Comment Actions Add MAC and access checks for O_EXEC | O_PATH case.
Comment Actions Enable EVFILT_VNODE kqfilter for O_PATH files. Comment Actions Would it be possible to include a basic test that does things like checks for EBADF? Comment Actions I am able to open 600 root:wheel files as ordinary user with O_PATH flag added. Is it intended behavior? Comment Actions Yes intended. O_PATH should not result in any operation on the file not available through the path, but that's all.
Can you formulate exactly what should we deny? I do not see how to limit this usefully, unless you can provide a criteria, I will roll back enablement of the kqueue' vnode filter. Comment Actions At the present state of patch we can open() e.g. /etc/master.password as unprivileged user and start listening for kevents generated by superuser actions.
I don't have a strong opinion which option is better. Comment Actions If we have x access to the directory, we can stat files in it, or we can open files with O_PATH. In either case, we can notice:
So for me, it seems that we already get the same knowledge just from the ability to do stat(2). Two quirks are that
Do we still consider it wrong? If yes, I can add some global sysctl knob to enable/disable vnode event filter on O_PATH files, but IMO it is overkill. Comment Actions
I do not have enough level of competence in security area to give solid answer to this question. IMO albeit "Security through obscurity" principle exists, it is better to have this feature than not to have it at all even at the price of realtime notification. Also after some playing with O_PATH in libinotify, I found pair of issues:
Comment Actions Should be fixed.
Can you elaborate more? I am not aware what faccess() is, and there is no Linux man page either. Is this something like faccessat(fd, NULL, mode, AT_EMPTY_PATH) ? Comment Actions
Linux checks for access rights to a file when establish an inotify watch on it (but ignores access rights for subwatches) We misses that ability with O_PATH at least in raceless way as VOP_ACCESS stage is skipped at open() and there is no access() variant that have fd parameter passed in.
It is really called faccessx(), not faccess(). My bad, sorry: https://www.ibm.com/docs/en/i/7.4?topic=ssw_ibm_i_74/apis/faccessx.htm
AT_EMPTY_PATH looks promising. I should test it. Does it work when fd is not a directory? Comment Actions You mean, with O_PATH we should check the snapshot of current access rights when fd is passed to kqueue for vnode filter?
I cannot find faccessx() on my Fedora 33 VM either. The reference seems to be about AIX?
Yes, it should work when fd is arbitrary file, this is the point of AT_EMPTY_PATH. It should be "" and not NULL in the second arg, now that I look at the snippet. Comment Actions An attempt to fchdir() using an O_PATH | O_DIRECTORY descriptor yields: VNASSERT failed: old > 0 not true at /usr/home/markj/src/freebsd-dev/sys/kern/vfs_subr.c:3031 (vrefact)
0xfffff8051d419000: type VDIR
usecount 1, writecount 0, refcount 2 seqc users 0 mountedhere 0
hold count flags ()
flags (VMP_LAZYLIST)
lock type ufs: UNLOCKED
nlink=2, effnlink=2, size=512, extsize 0
generation=67b06701, uid=1001, gid=0, flags=0x0
ino 7867288, on dev ada2s1
panic: vrefact: wrong use count 0
cpuid = 21
time = 1617808572
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe01ae1cf980
vpanic() at vpanic+0x181/frame 0xfffffe01ae1cf9d0
panic() at panic+0x43/frame 0xfffffe01ae1cfa30
vrefact() at vrefact+0x5e/frame 0xfffffe01ae1cfa50
sys_fchdir() at sys_fchdir+0x83/frame 0xfffffe01ae1cfac0
amd64_syscall() at amd64_syscall+0x12e/frame 0xfffffe01ae1cfbf0
fast_syscall_common() at fast_syscall_common+0xf8/frame 0xfffffe01ae1cfbf0
Comment Actions
Yes! It would be ideal as it would eliminate any security concerns and make faccess* stuff unneeded. We also can cache result of VOP_ACCESS performed at open() time with VWRITE & VREAD flags kept and than use it at kevent() times to keep traditional kqueue access rights checking style but it could be an overkill.
Yes, it is AIX and OS/400 specific function. Comment Actions Why VWRITE? I added the check on each VOP, but I probably drop it. It is there for now if you want to play with it and for others to state their opinion. I do not see a problem with allowing kevent(2) on O_PATH.
I believe faccessat(AT_EMPTY_PATH) gives this semantic.
Comment Actions Fix fchdir() Comment Actions
It depends on orthogonality of O_PATH
To catch the result of check at open time is what I offered in second sentence of my previous reply. We can add second VOP_ACCESS(VREAD) call at open() and use its returned value in knfilter later.
I tested It and found it working as expected after fixing a typo in VOP_ACCESS parameters (s/FREAD/VREAD/). Thanks! Comment Actions Check access on open and allow kevent vnode filter for O_PATH files if VOP_ACCESS(VREAD) passed.
Comment Actions
kevent() is always failing now on registering of O_PATH file descriptors. Debugging printfs show that FKQALLOWED flag is not preserved between open() and kevent() calls. Comment Actions
FKQALLOWED flag is still unset in vn_kqfilter_opath() Comment Actions
$ cat test.c #include <sys/types.h>
#include <sys/event.h>
#include <sys/time.h>
#include <fcntl.h>
#include <err.h>
#include <errno.h>
int
main(int argc, char *argv[])
{
int fd, kq, nev;
struct kevent ev;
static const struct timespec tout = { 1, 0 };
if ((fd = open(argv[1], O_PATH | O_NOFOLLOW | O_NONBLOCK)) == -1)
err(1, "Cannot open `%s'", argv[1]);
if ((kq = kqueue()) == -1)
err(1, "Cannot create kqueue");
EV_SET(&ev, fd, EVFILT_VNODE, EV_ADD | EV_ENABLE | EV_CLEAR,
NOTE_DELETE|NOTE_WRITE|NOTE_EXTEND|NOTE_ATTRIB|NOTE_LINK|
NOTE_RENAME|NOTE_REVOKE|NOTE_READ|NOTE_OPEN|NOTE_CLOSE|
NOTE_CLOSE_WRITE, 0, 0);
if (kevent(kq, &ev, 1, NULL, 0, &tout) == -1)
err(1, "kevent %d", errno);
}$ cc test.c -o test Comment Actions I would suggest explicitly prohibiting AIO operations on O_PATH descriptors, in aio_aqueue(). Today reads and writes are prohibited already, but fsyncs are not and who knows what might be added in the future. We do not always return EBADF in response to invalid system calls, e.g., mmap() returns ENODEV. I am not sure if it is worth addressing.
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
If this is the only use, I would actually prefer to kill 'data' entirely to avoid someone trying to use additional bytes and instead do something like:
char query; ... query = fubyte(ifreqp->ifr_data); if (query == -1) return (EFAULT); switch (query) {Also, I wonder if we should prefer fuebyte() in new code instead? (Maybe kib@ has an opinion on that)