should use eaccess instead of access to check temporary file directory
(1) By jjm2473 on 2020-08-16 03:38:25 [link]
The 'access' syscall uses REAL uid to check aceess, we should use EFFECTIVE uid with 'eaccess' call. See man page for their difference. ## Detail: Issue in forked-daapd in OpenWRT, before patched, forked-daapd scanning multi files or doing database vacuum, 'unable to open database file' may appears in log. Using strace to trace it, shows that: ``` setresgid(-1, 3003, -1) = 0 ... setresuid(-1, 190, -1) = 0 ... newfstatat(AT_FDCWD, "/var/tmp", {st_mode=S_IFDIR|0755, st_size=40, ...}, 0) = 0 faccessat(AT_FDCWD, "/var/tmp", W_OK|X_OK) = 0 faccessat(AT_FDCWD, "/var/tmp/etilqs_d4c84886ce52e72a", F_OK) = -1 ENOENT (No such file or directory) openat(AT_FDCWD, "/var/tmp/etilqs_d4c84886ce52e72a", O_RDWR|O_CREAT|O_EXCL|O_LARGEFILE|O_NOFOLLOW|O_CLOEXEC, 0600) = -1 EACCES (Permission denied) openat(AT_FDCWD, "/var/tmp/etilqs_d4c84886ce52e72a", O_RDONLY|O_EXCL|O_LARGEFILE|O_NOFOLLOW|O_CLOEXEC) = -1 ENOENT (No such file or directory) ``` As shown above, forked-daapd was run by root, then it sets its effective uid to 190, then sqlite trying to check '/var/tmp' writeable with `faccessat(AT_FDCWD, "/var/tmp", W_OK|X_OK)`, kernel says ok, but failed when actually create file with `openat`. Because `faccessat` using real uid to check access by default, but `openat` using effective uid. PS, The information of '/var/tmp' : ``` drwxr-xr-x 2 root root 40 Aug 16 10:29 tmp ``` ## Patch: ``` --- a/sqlite3.c +++ b/sqlite3.c @@ -33842,7 +33842,7 @@ static struct unix_syscall { { "close", (sqlite3_syscall_ptr)close, 0 }, #define osClose ((int(*)(int))aSyscall[1].pCurrent) - { "access", (sqlite3_syscall_ptr)access, 0 }, + { "access", (sqlite3_syscall_ptr)eaccess, 0 }, #define osAccess ((int(*)(const char*,int))aSyscall[2].pCurrent) { "getcwd", (sqlite3_syscall_ptr)getcwd, 0 }, ```
(2) By Stephan Beal (stephan) on 2020-08-16 03:43:49 in reply to 1 [link]
`man eaccess` says it's not a standard API... ``` NAME euidaccess, eaccess - check effective user's permissions for a file SYNOPSIS #define _GNU_SOURCE /* See feature_test_macros(7) */ #include <unistd.h> ... CONFORMING TO These functions are nonstandard. Some other systems have an eaccess() function. ```
(3) By jjm2473 on 2020-08-16 10:13:40 in reply to 2
No, eaccess is NOT portable. Maybe we should find solution in libc. In musl libc, eaccess is define as: euidaccess.c ``` #define _GNU_SOURCE #include <unistd.h> #include <fcntl.h> #include "libc.h" int euidaccess(const char *filename, int amode) { return faccessat(AT_FDCWD, filename, amode, AT_EACCESS); } weak_alias(euidaccess, eaccess); ``` faccessat.c: ``` #include <unistd.h> #include <fcntl.h> #include <sys/wait.h> #include "syscall.h" #include "pthread_impl.h" struct ctx { int fd; const char *filename; int amode; int p; }; static int checker(void *p) { struct ctx *c = p; int ret; if (__syscall(SYS_setregid, __syscall(SYS_getegid), -1) || __syscall(SYS_setreuid, __syscall(SYS_geteuid), -1)) __syscall(SYS_exit, 1); ret = __syscall(SYS_faccessat, c->fd, c->filename, c->amode, 0); __syscall(SYS_write, c->p, &ret, sizeof ret); return 0; } int faccessat(int fd, const char *filename, int amode, int flag) { if (!flag || (flag==AT_EACCESS && getuid()==geteuid() && getgid()==getegid())) return syscall(SYS_faccessat, fd, filename, amode, flag); if (flag != AT_EACCESS) return __syscall_ret(-EINVAL); char stack[1024]; sigset_t set; pid_t pid; int status; int ret, p[2]; if (pipe2(p, O_CLOEXEC)) return __syscall_ret(-EBUSY); struct ctx c = { .fd = fd, .filename = filename, .amode = amode, .p = p[1] }; __block_all_sigs(&set); pid = __clone(checker, stack+sizeof stack, 0, &c); __syscall(SYS_close, p[1]); if (pid<0 || __syscall(SYS_read, p[0], &ret, sizeof ret) != sizeof(ret)) ret = -EBUSY; __syscall(SYS_close, p[0]); __syscall(SYS_wait4, pid, &status, __WCLONE, 0); __restore_sigs(&set); return __syscall_ret(ret); } ``` So, it calls clone to run checker. But is `clone` a good way? (Some system may strict clone syscall) Do you have other ways? Can we just change `unixTempFileDir` to really create a temporary file to check the directory is writable?