From 2509f30be4c82c124da6d1d266cd4409ca64569e Mon Sep 17 00:00:00 2001 From: vimist Date: Mon, 8 May 2023 12:49:27 +0100 Subject: [PATCH 01/12] Enable ACL support for libfuse3 --- configure.ac | 2 +- src/bindfs.c | 235 ++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 232 insertions(+), 5 deletions(-) mode change 100644 => 100755 src/bindfs.c diff --git a/configure.ac b/configure.ac index 7764e5b..6ce1d78 100644 --- a/configure.ac +++ b/configure.ac @@ -36,7 +36,7 @@ AM_CONDITIONAL(BUILD_OS_IS_DARWIN, [test x"$build_os" = darwin]) my_CPPFLAGS="-D_REENTRANT -D_FILE_OFFSET_BITS=64 -D_XOPEN_SOURCE=700 -D__BSD_VISIBLE=1 -D_BSD_SOURCE -D_DEFAULT_SOURCE -D_DARWIN_BETTER_REALPATH" my_CFLAGS="-std=c99 -Wall -Wpedantic -fno-common" -my_LDFLAGS="-pthread" +my_LDFLAGS="-pthread -lacl" AC_SUBST([my_CPPFLAGS]) AC_SUBST([my_CFLAGS]) AC_SUBST([my_LDFLAGS]) diff --git a/src/bindfs.c b/src/bindfs.c old mode 100644 new mode 100755 index 00a7592..3855eb1 --- a/src/bindfs.c +++ b/src/bindfs.c @@ -63,6 +63,8 @@ #ifdef HAVE_SETXATTR #include #endif +#include +#include #ifdef HAVE_FUSE_3 #ifndef __NR_renameat2 @@ -772,6 +774,234 @@ static int bindfs_fgetattr(const char *path, struct stat *stbuf, } #endif +/** + * Convert an ACL permset to a bitmask. + * + * @param permset The permset to convert. + * + * @return The permset represented as an `access(2)` compatible bitmask. + */ +unsigned int permset_to_bits(acl_permset_t permset) { + unsigned int bits = 0; + + if (acl_get_perm(permset, ACL_READ) == 1) + bits |= R_OK; + + if (acl_get_perm(permset, ACL_WRITE) == 1) + bits |= W_OK; + + if (acl_get_perm(permset, ACL_EXECUTE) == 1) + bits |= X_OK; + + return bits; +} + +/** + * Determine whether the effective user has access to a given file system + * object. + * + * Access check algorithm documented here: https://www.usenix.org/legacy/publications/library/proceedings/usenix03/tech/freenix03/full_papers/gruenbacher/gruenbacher_html/main.html + * + * This function performs POSIX ACL access checking, which is a superset of the + * standard Unix permissions. + * + * @param path The path to the file system object. + * @param wants The bitwise-inclusive OR of the access permissions to be + * checked (R_OK, W_OK, X_OK). + * + * @return 0 if the effective user has access, -EACCES otherwise. + * + * */ +static int bindfs_access(const char *path, int wants) +{ + DPRINTF("Performing access check for '%s'", path); + + char *real_path = process_path(path, true); + + struct stat st; + if (lstat(real_path, &st) == -1) { + DPRINTF("Could not lstat '%s': %s", real_path, strerror(errno)); + free(real_path); + return -errno; + } + + uid_t euid = geteuid(); + gid_t egid = getegid(); + + // TODO: There are `root` cases still needing to be accounted for. + // This is not the correct implementation! + //if (euid == 0) + // return 0; + + struct passwd *pwd = getpwuid(euid); + + // We can perform this check early, Unsubstantiated, but I would imagine + // it's one of the more likely cases, so it makes sense to optimise for it. + // + // If the user ID of the process is the owner, the owner entry determines + // access. + if (euid == st.st_uid) { + return (((st.st_mode & 0700) >> 6) & wants) == wants + ? 0 + : -EACCES; + } + + // Get the groups the effective user is a part of + int ngroups = 8; + gid_t *groups = malloc(sizeof(*groups) * ngroups); + + while (getgrouplist(pwd->pw_name, egid, groups, &ngroups) == -1) { + DPRINTF("Reallocating space for groups to %d entries", ngroups); + groups = realloc(groups, sizeof(*groups) * ngroups); + } + + // Notes on the below variables: + // We do not need to store the owner permissions as we can exit immediately + // if we find them and do not need to do further processing. + + // We do not need to store group ACL permissions separately from the + // standard group permissions. We can merge all the group permissions + // together and treat them as one. TODO: I think, need to thoroughly test. + // NOTE: This only holds if an object that has ACLs where `group_a = r--`, + // `group_b = --x` and the request is for `r-x` (`wants` = 5) we should + // grant access, if we should disallow access, we can't aggregate + // permissions in this way. + + unsigned int group_perms = 0; + unsigned int other_perms = 0; + + unsigned int user_acl_perms = 0; + unsigned int acl_mask = 0; + + unsigned int has_user_acl = 0; + unsigned int has_group = 0; + unsigned int has_mask = 0; + + acl_t acl = acl_get_file(real_path, ACL_TYPE_ACCESS); + acl_entry_t acl_entry; + for ( + int result = acl_get_entry(acl, ACL_FIRST_ENTRY, &acl_entry); + result > 0; + result = acl_get_entry(acl, ACL_NEXT_ENTRY, &acl_entry) + ) { + acl_tag_t acl_tag; + if (acl_get_tag_type(acl_entry, &acl_tag) != 0) { + DPRINTF("Could not get ACL tag type: %s", strerror(errno)); + continue; + } + + acl_permset_t acl_permset; + if (acl_get_permset(acl_entry, &acl_permset) != 0) { + DPRINTF("Could not get ACL permset: %s", strerror(errno)); + continue; + } + + unsigned int permset_as_bits = permset_to_bits(acl_permset); + + void *acl_qualifier = acl_get_qualifier(acl_entry); + switch(acl_tag) { + case ACL_USER_OBJ: + if (euid == st.st_uid) { + acl_free(acl_qualifier); + acl_free(acl); + free(real_path); + + return (permset_as_bits & wants) == wants + ? 0 + : -EACCES; + } + break; + + case ACL_USER: + if (euid == *(uid_t*)acl_qualifier) { + has_user_acl = 1; + user_acl_perms = permset_as_bits; + } + break; + + case ACL_GROUP_OBJ: + for (int i = 0; i < ngroups; i++) { + // Aggregate all group permissions, we only care if we have + // the permissions, not where they come from + if (st.st_gid == groups[i]) { + DPRINTF("Has ACL_GROUP_OBJ of %d", groups[i]); + has_group = 1; + group_perms |= permset_as_bits; + } + } + break; + + case ACL_GROUP: + for (int i = 0; i < ngroups; i++) { + // Aggregate all group permissions, we only care if we have + // the permissions, not where they come from + if (*(gid_t*)acl_qualifier == groups[i]) { + DPRINTF("Has ACL_GROUP of %d", groups[i]); + has_group = 1; + group_perms |= permset_as_bits; + } + } + break; + + case ACL_MASK: + has_mask = 1; + acl_mask = permset_as_bits; + break; + + case ACL_OTHER: + other_perms = permset_as_bits; + break; + } + acl_free(acl_qualifier); + } + + acl_free(acl); + free(real_path); + + // If there is no mask entry, it doesn't restrict anything. + if (has_mask == 0) { + DPRINTF("ACL has no ACL_MASK entry, setting mask to rwx"); + acl_mask = R_OK | W_OK | X_OK; + } + + //If the user ID of the process is the owner, the owner entry determines + //access - This check is performed above, very early in the function. + + // If the user ID of the process matches the qualifier in one + // of the named user entries, this entry determines access + if (has_user_acl == 1) { + DPRINTF("Using ACL_USER entry to determine access"); + return (user_acl_perms & acl_mask & wants) == wants + ? 0 + : -EACCES; + } + + // If one of the group IDs of the process matches the owning group and the + // owning group entry contains the requested permissions, this entry + // determines access + + // If one of the group IDs of the process matches the qualifier of one of + // the named group entries and this entry contains the requested + // permissions, this entry determines access + + // If one of the group IDs of the process matches the owning group or any + // of the named group entries, but neither the owning group entry nor any + // of the matching named group entries contains the requested permissions, + // this determines that access is denied + if (has_group == 1) { + DPRINTF("Using ACL_GROUP_OBJ or ACL_GROUP entry to determine access"); + return (group_perms & acl_mask & wants) == wants + ? 0 + : -EACCES; + } + + // Else the other entry determines access. + DPRINTF("Using ACL_OTHER entry to determine access"); + return (other_perms & wants) == wants + ? 0 + : -EACCES; +} + static int bindfs_readlink(const char *path, char *buf, size_t size) { int res; @@ -1665,7 +1895,7 @@ static struct fuse_operations bindfs_oper = { #ifndef HAVE_FUSE_3 .fgetattr = bindfs_fgetattr, #endif - /* no access() since we always use -o default_permissions */ + .access = bindfs_access, .readlink = bindfs_readlink, .readdir = bindfs_readdir, .mknod = bindfs_mknod, @@ -2775,9 +3005,6 @@ int main(int argc, char *argv[]) fuse_opt_add_arg(&args, "-oallow_other"); } - /* We want the kernel to do our access checks for us based on what getattr gives it. */ - fuse_opt_add_arg(&args, "-odefault_permissions"); - // With FUSE 3 we set this in bindfs_init #ifndef HAVE_FUSE_3 /* We want to mirror inodes. */ From 23b9a919f14cedc246c7eac3a0ee25ea58e66382 Mon Sep 17 00:00:00 2001 From: vimist Date: Mon, 5 Jun 2023 10:57:22 +0100 Subject: [PATCH 02/12] Fix types of a few variables --- src/bindfs.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/bindfs.c b/src/bindfs.c index 3855eb1..467d23f 100755 --- a/src/bindfs.c +++ b/src/bindfs.c @@ -867,15 +867,15 @@ static int bindfs_access(const char *path, int wants) // grant access, if we should disallow access, we can't aggregate // permissions in this way. - unsigned int group_perms = 0; - unsigned int other_perms = 0; + mode_t group_perms = 0; + mode_t other_perms = 0; - unsigned int user_acl_perms = 0; - unsigned int acl_mask = 0; + mode_t user_acl_perms = 0; + mode_t acl_mask = 0; - unsigned int has_user_acl = 0; - unsigned int has_group = 0; - unsigned int has_mask = 0; + bool has_user_acl = false; + bool has_group = false; + bool has_mask = false; acl_t acl = acl_get_file(real_path, ACL_TYPE_ACCESS); acl_entry_t acl_entry; @@ -914,7 +914,7 @@ static int bindfs_access(const char *path, int wants) case ACL_USER: if (euid == *(uid_t*)acl_qualifier) { - has_user_acl = 1; + has_user_acl = true; user_acl_perms = permset_as_bits; } break; @@ -925,7 +925,7 @@ static int bindfs_access(const char *path, int wants) // the permissions, not where they come from if (st.st_gid == groups[i]) { DPRINTF("Has ACL_GROUP_OBJ of %d", groups[i]); - has_group = 1; + has_group = true; group_perms |= permset_as_bits; } } @@ -937,14 +937,14 @@ static int bindfs_access(const char *path, int wants) // the permissions, not where they come from if (*(gid_t*)acl_qualifier == groups[i]) { DPRINTF("Has ACL_GROUP of %d", groups[i]); - has_group = 1; + has_group = true; group_perms |= permset_as_bits; } } break; case ACL_MASK: - has_mask = 1; + has_mask = true; acl_mask = permset_as_bits; break; @@ -959,7 +959,7 @@ static int bindfs_access(const char *path, int wants) free(real_path); // If there is no mask entry, it doesn't restrict anything. - if (has_mask == 0) { + if (!has_mask) { DPRINTF("ACL has no ACL_MASK entry, setting mask to rwx"); acl_mask = R_OK | W_OK | X_OK; } @@ -969,7 +969,7 @@ static int bindfs_access(const char *path, int wants) // If the user ID of the process matches the qualifier in one // of the named user entries, this entry determines access - if (has_user_acl == 1) { + if (has_user_acl) { DPRINTF("Using ACL_USER entry to determine access"); return (user_acl_perms & acl_mask & wants) == wants ? 0 @@ -988,7 +988,7 @@ static int bindfs_access(const char *path, int wants) // of the named group entries, but neither the owning group entry nor any // of the matching named group entries contains the requested permissions, // this determines that access is denied - if (has_group == 1) { + if (has_group) { DPRINTF("Using ACL_GROUP_OBJ or ACL_GROUP entry to determine access"); return (group_perms & acl_mask & wants) == wants ? 0 From aaf6c48c2ae936652f2c6cc8f4e803b096e70bcc Mon Sep 17 00:00:00 2001 From: vimist Date: Tue, 6 Jun 2023 16:01:01 +0100 Subject: [PATCH 03/12] Add getattr_common call to access check --- src/bindfs.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/bindfs.c b/src/bindfs.c index 467d23f..ba4342a 100755 --- a/src/bindfs.c +++ b/src/bindfs.c @@ -825,6 +825,9 @@ static int bindfs_access(const char *path, int wants) return -errno; } + // Update the stat struct to take into account the bindfs flags + getattr_common(real_path, &st); + uid_t euid = geteuid(); gid_t egid = getegid(); From 2fc4b3d1484d3bbcb824db85021db2a264cf1b0b Mon Sep 17 00:00:00 2001 From: vimist Date: Tue, 6 Jun 2023 16:01:26 +0100 Subject: [PATCH 04/12] Remove ACL_USER_OBJ case from ACL access check --- src/bindfs.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/bindfs.c b/src/bindfs.c index ba4342a..c2c3e00 100755 --- a/src/bindfs.c +++ b/src/bindfs.c @@ -903,17 +903,13 @@ static int bindfs_access(const char *path, int wants) void *acl_qualifier = acl_get_qualifier(acl_entry); switch(acl_tag) { - case ACL_USER_OBJ: - if (euid == st.st_uid) { - acl_free(acl_qualifier); - acl_free(acl); - free(real_path); + // This check is already performed above. As ACLs are a superset of + // the standard Unix permissions, we can perform this permission + // check against the non-ACL permissions and potentially return + // early. - return (permset_as_bits & wants) == wants - ? 0 - : -EACCES; - } - break; + //case ACL_USER_OBJ: + //break; case ACL_USER: if (euid == *(uid_t*)acl_qualifier) { From aa6a532a281ff723b3f2adc979f0ffaf8717364d Mon Sep 17 00:00:00 2001 From: vimist Date: Sat, 10 Jun 2023 18:47:56 +0100 Subject: [PATCH 05/12] Basic (non-working, incorrect, broken) access checks --- src/bindfs.c | 226 +++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 217 insertions(+), 9 deletions(-) diff --git a/src/bindfs.c b/src/bindfs.c index c2c3e00..71767b8 100755 --- a/src/bindfs.c +++ b/src/bindfs.c @@ -247,6 +247,11 @@ static int unapply_gid_offset(gid_t *gid); static size_t round_up_buffer_size_for_direct_io(size_t size); #endif +static unsigned int permset_to_bits(acl_permset_t permset); +static int access_check(const char *real_path, int wants); +static int dirname_access_check(const char *path, int wants); +static int check_path_search_perms(const char *path); + /* FUSE callbacks */ #ifdef HAVE_FUSE_3 static void *bindfs_init(struct fuse_conn_info *conn, struct fuse_config *cfg); @@ -560,6 +565,8 @@ static int chown_new_file(const char *path, struct fuse_context *fc, int (*chown static int delete_file(const char *path, int (*target_delete_func)(const char *)) { int res; char *real_path; + int access_check_result; + int path_search_permission; struct stat st; char *also_try_delete = NULL; char *unlink_first = NULL; @@ -572,6 +579,14 @@ static int delete_file(const char *path, int (*target_delete_func)(const char *) if (real_path == NULL) return -errno; + access_check_result = dirname_access_check(real_path, W_OK); + if (access_check_result != 0) + return access_check_result; + + path_search_permission = check_path_search_perms(real_path); + if (path_search_permission != 0) + return path_search_permission; + if (settings.resolve_symlinks) { if (lstat(real_path, &st) == -1) { free(real_path); @@ -781,7 +796,7 @@ static int bindfs_fgetattr(const char *path, struct stat *stbuf, * * @return The permset represented as an `access(2)` compatible bitmask. */ -unsigned int permset_to_bits(acl_permset_t permset) { +static unsigned int permset_to_bits(acl_permset_t permset) { unsigned int bits = 0; if (acl_get_perm(permset, ACL_READ) == 1) @@ -812,11 +827,9 @@ unsigned int permset_to_bits(acl_permset_t permset) { * @return 0 if the effective user has access, -EACCES otherwise. * * */ -static int bindfs_access(const char *path, int wants) +static int access_check(const char *real_path, int wants) { - DPRINTF("Performing access check for '%s'", path); - - char *real_path = process_path(path, true); + DPRINTF("Performing access check for '%s'", real_path); struct stat st; if (lstat(real_path, &st) == -1) { @@ -1001,15 +1014,78 @@ static int bindfs_access(const char *path, int wants) : -EACCES; } +/** + * Perform an access check against the dirname of the given path. + * + * @param path The path to get the dirname of and performe the check against. + * @param wants The bitwise-inclusive OR of the access permissions to be + * checked (R_OK, W_OK, X_OK). + * + * @return 0 if the dirname has the requested permissions, -EACCES otherwise. + */ +static int dirname_access_check(const char *path, int wants) { + size_t path_length = strlen(path); + char *part = malloc(path_length + 1); + memcpy(part, path, path_length + 1); + + part = dirname(part); + return access_check(part, wants); +} + +static int bindfs_access(const char *path, int wants) +{ + char *real_path = process_path(path, true); + return access_check(real_path, wants); +} + +/** + * Iterate over the given path and check whether each component has the + * search permission. + * + * @param path The path to iterate over. + * + * @return 0 if all path components have the search permission, -EACCES + * otherwise. + */ +static int check_path_search_perms(const char *path) { + size_t path_length = strlen(path); + char *part = malloc(path_length + 1); + memcpy(part, path, path_length + 1); + + int access_check_result; + do { + part = dirname(part); + DPRINTF( + "Checking path component for required search permissions: %s", + part); + + access_check_result = access_check(part, X_OK); + if (access_check_result != 0) { + DPRINTF( + "Path component doesn't have required search permission: %s", + part); + return access_check_result; + } + } while(strcmp(part, "/") != 0); + + DPRINTF("Path has search required permissions: %s", path); + return 0; +} + static int bindfs_readlink(const char *path, char *buf, size_t size) { int res; char *real_path; + int path_search_permission; real_path = process_path(path, true); if (real_path == NULL) return -errno; + path_search_permission = check_path_search_perms(real_path); + if (path_search_permission != 0) + return path_search_permission; + /* No need to check for access to the link itself, since symlink permissions don't matter. Access to the path components of the symlink are automatically queried by FUSE. */ @@ -1126,11 +1202,21 @@ static int bindfs_mknod(const char *path, mode_t mode, dev_t rdev) int res; struct fuse_context *fc; char *real_path; + int access_check_result; + int path_search_permission; real_path = process_path(path, true); if (real_path == NULL) return -errno; + access_check_result = dirname_access_check(real_path, W_OK); + if (access_check_result != 0) + return access_check_result; + + path_search_permission = check_path_search_perms(real_path); + if (path_search_permission != 0) + return path_search_permission; + mode = permchain_apply(settings.create_permchain, mode); if (S_ISFIFO(mode)) @@ -1154,11 +1240,21 @@ static int bindfs_mkdir(const char *path, mode_t mode) int res; struct fuse_context *fc; char *real_path; + int access_check_result; + int path_search_permission; real_path = process_path(path, true); if (real_path == NULL) return -errno; + access_check_result = dirname_access_check(real_path, W_OK); + if (access_check_result != 0) + return access_check_result; + + path_search_permission = check_path_search_perms(real_path); + if (path_search_permission != 0) + return path_search_permission; + mode |= S_IFDIR; /* tell permchain_apply this is a directory */ mode = permchain_apply(settings.create_permchain, mode); @@ -1190,6 +1286,8 @@ static int bindfs_symlink(const char *from, const char *to) int res; struct fuse_context *fc; char *real_to; + int access_check_result; + int path_search_permission; if (settings.resolve_symlinks) return -EPERM; @@ -1198,6 +1296,14 @@ static int bindfs_symlink(const char *from, const char *to) if (real_to == NULL) return -errno; + access_check_result = dirname_access_check(real_to, W_OK); + if (access_check_result != 0) + return access_check_result; + + path_search_permission = check_path_search_perms(real_to); + if (path_search_permission != 0) + return path_search_permission; + res = symlink(from, real_to); if (res == -1) { free(real_to); @@ -1219,6 +1325,9 @@ static int bindfs_rename(const char *from, const char *to) { int res; char *real_from, *real_to; + int access_check_result; + int path_search_permission; + struct stat st; if (settings.rename_deny) return -EPERM; @@ -1233,6 +1342,31 @@ static int bindfs_rename(const char *from, const char *to) return -errno; } + access_check_result = dirname_access_check(real_from, W_OK); + if (access_check_result != 0) + return access_check_result; + + access_check_result = dirname_access_check(real_to, W_OK); + if (access_check_result != 0) + return access_check_result; + + path_search_permission = check_path_search_perms(real_from); + if (path_search_permission != 0) + return path_search_permission; + + path_search_permission = check_path_search_perms(real_to); + if (path_search_permission != 0) + return path_search_permission; + + if(lstat(real_from, &st) != 0) + return -errno; + + if ((st.st_mode & S_IFDIR) == S_IFDIR) { + access_check_result = dirname_access_check(real_from, W_OK); + if (access_check_result != 0) + return access_check_result; + } + #ifdef HAVE_FUSE_3 if (flags == 0) { @@ -1264,6 +1398,8 @@ static int bindfs_link(const char *from, const char *to) { int res; char *real_from, *real_to; + int access_check_result; + int path_search_permission; real_from = process_path(from, true); if (real_from == NULL) @@ -1275,6 +1411,18 @@ static int bindfs_link(const char *from, const char *to) return -errno; } + access_check_result = dirname_access_check(real_to, W_OK); + if (access_check_result != 0) + return access_check_result; + + path_search_permission = check_path_search_perms(real_from); + if (path_search_permission != 0) + return path_search_permission; + + path_search_permission = check_path_search_perms(real_to); + if (path_search_permission != 0) + return path_search_permission; + res = link(real_from, real_to); free(real_from); free(real_to); @@ -1294,11 +1442,16 @@ static int bindfs_chmod(const char *path, mode_t mode) struct stat st; mode_t diff = 0; char *real_path; + int path_search_permission; real_path = process_path(path, true); if (real_path == NULL) return -errno; + path_search_permission = check_path_search_perms(real_path); + if (path_search_permission != 0) + return path_search_permission; + if (settings.chmod_allow_x) { /* Get the old permission bits and see which bits would change. */ if (lstat(real_path, &st) == -1) { @@ -1359,6 +1512,15 @@ static int bindfs_chown(const char *path, uid_t uid, gid_t gid) { int res; char *real_path; + int path_search_permission; + + real_path = process_path(path, true); + if (real_path == NULL) + return -errno; + + path_search_permission = check_path_search_perms(real_path); + if (path_search_permission != 0) + return path_search_permission; if (uid != -1) { switch (settings.chown_policy) { @@ -1393,10 +1555,6 @@ static int bindfs_chown(const char *path, uid_t uid, gid_t gid) } if (uid != -1 || gid != -1) { - real_path = process_path(path, true); - if (real_path == NULL) - return -errno; - res = lchown(real_path, uid, gid); free(real_path); if (res == -1) @@ -1415,11 +1573,16 @@ static int bindfs_truncate(const char *path, off_t size) { int res; char *real_path; + int path_search_permission; real_path = process_path(path, true); if (real_path == NULL) return -errno; + path_search_permission = check_path_search_perms(real_path); + if (path_search_permission != 0) + return path_search_permission; + res = truncate(real_path, size); free(real_path); if (res == -1) @@ -1435,6 +1598,10 @@ static int bindfs_ftruncate(const char *path, off_t size, int res; (void) path; + // TODO: Implement access check with `access_check` function (though it + // doesn't currently take a file descriptor, so may need refactoring to + // support this). + res = ftruncate(fi->fh, size); if (res == -1) return -errno; @@ -1451,11 +1618,21 @@ static int bindfs_utimens(const char *path, const struct timespec ts[2]) { int res; char *real_path; + int access_check_result; + int path_search_permission; real_path = process_path(path, true); if (real_path == NULL) return -errno; + access_check_result = dirname_access_check(real_path, W_OK); + if (access_check_result != 0) + return access_check_result; + + path_search_permission = check_path_search_perms(real_path); + if (path_search_permission != 0) + return path_search_permission; + #ifdef HAVE_UTIMENSAT res = utimensat(settings.mntsrc_fd, real_path, ts, AT_SYMLINK_NOFOLLOW); #elif HAVE_LUTIMES @@ -1481,11 +1658,24 @@ static int bindfs_create(const char *path, mode_t mode, struct fuse_file_info *f int fd; struct fuse_context *fc; char *real_path; + int access_check_result; + int path_search_permission; real_path = process_path(path, true); if (real_path == NULL) return -errno; + access_check_result = dirname_access_check(real_path, W_OK); + if (access_check_result != 0) + return access_check_result; + + path_search_permission = check_path_search_perms(real_path); + if (path_search_permission != 0) + return path_search_permission; + + // TODO: Pretty sure we need more access checks here. Will let the tests + // highlight the issues. + mode |= S_IFREG; /* tell permchain_apply this is a regular file */ mode = permchain_apply(settings.create_permchain, mode); @@ -1514,11 +1704,24 @@ static int bindfs_open(const char *path, struct fuse_file_info *fi) { int fd; char *real_path; + int access_check_result; + int path_search_permission; real_path = process_path(path, true); if (real_path == NULL) return -errno; + access_check_result = dirname_access_check(real_path, W_OK); + if (access_check_result != 0) + return access_check_result; + + path_search_permission = check_path_search_perms(real_path); + if (path_search_permission != 0) + return path_search_permission; + + // TODO: Pretty sure we need more access checks here. Will let the tests + // highlight the issues. + int flags = fi->flags; #ifdef __linux__ if (!settings.forward_odirect) { @@ -1653,11 +1856,16 @@ static int bindfs_statfs(const char *path, struct statvfs *stbuf) { int res; char *real_path; + int path_search_permission; real_path = process_path(path, true); if (real_path == NULL) return -errno; + path_search_permission = check_path_search_perms(real_path); + if (path_search_permission != 0) + return path_search_permission; + res = statvfs(real_path, stbuf); free(real_path); if (res == -1) From fb8853d7884828412350dab93f2e8fedc1d1da43 Mon Sep 17 00:00:00 2001 From: vimist Date: Sun, 11 Jun 2023 12:24:56 +0100 Subject: [PATCH 06/12] Refactor based on PR comments --- src/bindfs.c | 238 ++++++++++++++++++++------------------------------- 1 file changed, 92 insertions(+), 146 deletions(-) diff --git a/src/bindfs.c b/src/bindfs.c index 71767b8..afc8aa3 100755 --- a/src/bindfs.c +++ b/src/bindfs.c @@ -249,8 +249,8 @@ static size_t round_up_buffer_size_for_direct_io(size_t size); static unsigned int permset_to_bits(acl_permset_t permset); static int access_check(const char *real_path, int wants); -static int dirname_access_check(const char *path, int wants); -static int check_path_search_perms(const char *path); +static bool dirname_access_check(const char *path, int wants); +static bool path_has_search_perms(const char *path); /* FUSE callbacks */ #ifdef HAVE_FUSE_3 @@ -565,8 +565,6 @@ static int chown_new_file(const char *path, struct fuse_context *fc, int (*chown static int delete_file(const char *path, int (*target_delete_func)(const char *)) { int res; char *real_path; - int access_check_result; - int path_search_permission; struct stat st; char *also_try_delete = NULL; char *unlink_first = NULL; @@ -579,13 +577,11 @@ static int delete_file(const char *path, int (*target_delete_func)(const char *) if (real_path == NULL) return -errno; - access_check_result = dirname_access_check(real_path, W_OK); - if (access_check_result != 0) - return access_check_result; - - path_search_permission = check_path_search_perms(real_path); - if (path_search_permission != 0) - return path_search_permission; + if ( + !dirname_access_check(real_path, W_OK) + || !path_has_search_perms(real_path) + ) + return -EACCES; if (settings.resolve_symlinks) { if (lstat(real_path, &st) == -1) { @@ -1014,6 +1010,12 @@ static int access_check(const char *real_path, int wants) : -EACCES; } +static int bindfs_access(const char *path, int wants) +{ + char *real_path = process_path(path, true); + return access_check(real_path, wants); +} + /** * Perform an access check against the dirname of the given path. * @@ -1021,21 +1023,16 @@ static int access_check(const char *real_path, int wants) * @param wants The bitwise-inclusive OR of the access permissions to be * checked (R_OK, W_OK, X_OK). * - * @return 0 if the dirname has the requested permissions, -EACCES otherwise. + * @return true if the dirname has the requested permissions, false otherwise. */ -static int dirname_access_check(const char *path, int wants) { - size_t path_length = strlen(path); - char *part = malloc(path_length + 1); - memcpy(part, path, path_length + 1); - +static bool dirname_access_check(const char *path, int wants) { + char *part = strdup(path); part = dirname(part); - return access_check(part, wants); -} -static int bindfs_access(const char *path, int wants) -{ - char *real_path = process_path(path, true); - return access_check(real_path, wants); + int access_check_result = access_check(part, wants); + + free(part); + return access_check_result == 0; } /** @@ -1044,47 +1041,52 @@ static int bindfs_access(const char *path, int wants) * * @param path The path to iterate over. * - * @return 0 if all path components have the search permission, -EACCES + * @return true if all path components have the search permission, false * otherwise. */ -static int check_path_search_perms(const char *path) { - size_t path_length = strlen(path); - char *part = malloc(path_length + 1); - memcpy(part, path, path_length + 1); +static bool path_has_search_perms(const char *path) { + char *part = strdup(path); - int access_check_result; + bool parent_dir = true; do { part = dirname(part); + + if (parent_dir && access_check(part, W_OK) != 0) { + free(part); + return false; + } + DPRINTF( "Checking path component for required search permissions: %s", part); - access_check_result = access_check(part, X_OK); - if (access_check_result != 0) { + if (access_check(part, X_OK) != 0) { DPRINTF( "Path component doesn't have required search permission: %s", part); - return access_check_result; + free(part); + return false; } + + parent_dir = false; } while(strcmp(part, "/") != 0); DPRINTF("Path has search required permissions: %s", path); - return 0; + free(part); + return true; } static int bindfs_readlink(const char *path, char *buf, size_t size) { int res; char *real_path; - int path_search_permission; real_path = process_path(path, true); if (real_path == NULL) return -errno; - path_search_permission = check_path_search_perms(real_path); - if (path_search_permission != 0) - return path_search_permission; + if(!path_has_search_perms(real_path)) + return -EACCES; /* No need to check for access to the link itself, since symlink permissions don't matter. Access to the path components of the symlink @@ -1202,20 +1204,16 @@ static int bindfs_mknod(const char *path, mode_t mode, dev_t rdev) int res; struct fuse_context *fc; char *real_path; - int access_check_result; - int path_search_permission; real_path = process_path(path, true); if (real_path == NULL) return -errno; - access_check_result = dirname_access_check(real_path, W_OK); - if (access_check_result != 0) - return access_check_result; - - path_search_permission = check_path_search_perms(real_path); - if (path_search_permission != 0) - return path_search_permission; + if ( + dirname_access_check(real_path, W_OK) + || !path_has_search_perms(real_path) + ) + return -EACCES; mode = permchain_apply(settings.create_permchain, mode); @@ -1240,20 +1238,16 @@ static int bindfs_mkdir(const char *path, mode_t mode) int res; struct fuse_context *fc; char *real_path; - int access_check_result; - int path_search_permission; real_path = process_path(path, true); if (real_path == NULL) return -errno; - access_check_result = dirname_access_check(real_path, W_OK); - if (access_check_result != 0) - return access_check_result; - - path_search_permission = check_path_search_perms(real_path); - if (path_search_permission != 0) - return path_search_permission; + if ( + dirname_access_check(real_path, W_OK) + || !path_has_search_perms(real_path) + ) + return -EACCES; mode |= S_IFDIR; /* tell permchain_apply this is a directory */ mode = permchain_apply(settings.create_permchain, mode); @@ -1286,8 +1280,6 @@ static int bindfs_symlink(const char *from, const char *to) int res; struct fuse_context *fc; char *real_to; - int access_check_result; - int path_search_permission; if (settings.resolve_symlinks) return -EPERM; @@ -1296,13 +1288,11 @@ static int bindfs_symlink(const char *from, const char *to) if (real_to == NULL) return -errno; - access_check_result = dirname_access_check(real_to, W_OK); - if (access_check_result != 0) - return access_check_result; - - path_search_permission = check_path_search_perms(real_to); - if (path_search_permission != 0) - return path_search_permission; + if ( + dirname_access_check(real_to, W_OK) + || !path_has_search_perms(real_to) + ) + return -EACCES; res = symlink(from, real_to); if (res == -1) { @@ -1325,8 +1315,6 @@ static int bindfs_rename(const char *from, const char *to) { int res; char *real_from, *real_to; - int access_check_result; - int path_search_permission; struct stat st; if (settings.rename_deny) @@ -1342,30 +1330,19 @@ static int bindfs_rename(const char *from, const char *to) return -errno; } - access_check_result = dirname_access_check(real_from, W_OK); - if (access_check_result != 0) - return access_check_result; - - access_check_result = dirname_access_check(real_to, W_OK); - if (access_check_result != 0) - return access_check_result; - - path_search_permission = check_path_search_perms(real_from); - if (path_search_permission != 0) - return path_search_permission; - - path_search_permission = check_path_search_perms(real_to); - if (path_search_permission != 0) - return path_search_permission; + if ( + !dirname_access_check(real_from, W_OK) + || !dirname_access_check(real_to, W_OK) + || !path_has_search_perms(real_from) + || !path_has_search_perms(real_to) + ) + return -EACCES; if(lstat(real_from, &st) != 0) return -errno; - if ((st.st_mode & S_IFDIR) == S_IFDIR) { - access_check_result = dirname_access_check(real_from, W_OK); - if (access_check_result != 0) - return access_check_result; - } + if ((st.st_mode & S_IFDIR) == S_IFDIR && access_check(real_from, W_OK) != 0) + return -EACCES; #ifdef HAVE_FUSE_3 @@ -1398,8 +1375,6 @@ static int bindfs_link(const char *from, const char *to) { int res; char *real_from, *real_to; - int access_check_result; - int path_search_permission; real_from = process_path(from, true); if (real_from == NULL) @@ -1411,17 +1386,12 @@ static int bindfs_link(const char *from, const char *to) return -errno; } - access_check_result = dirname_access_check(real_to, W_OK); - if (access_check_result != 0) - return access_check_result; - - path_search_permission = check_path_search_perms(real_from); - if (path_search_permission != 0) - return path_search_permission; - - path_search_permission = check_path_search_perms(real_to); - if (path_search_permission != 0) - return path_search_permission; + if ( + !dirname_access_check(real_to, W_OK) + || !path_has_search_perms(real_from) + || !path_has_search_perms(real_to) + ) + return -EACCES; res = link(real_from, real_to); free(real_from); @@ -1442,15 +1412,13 @@ static int bindfs_chmod(const char *path, mode_t mode) struct stat st; mode_t diff = 0; char *real_path; - int path_search_permission; real_path = process_path(path, true); if (real_path == NULL) return -errno; - path_search_permission = check_path_search_perms(real_path); - if (path_search_permission != 0) - return path_search_permission; + if(!path_has_search_perms(real_path)) + return -EACCES; if (settings.chmod_allow_x) { /* Get the old permission bits and see which bits would change. */ @@ -1512,15 +1480,13 @@ static int bindfs_chown(const char *path, uid_t uid, gid_t gid) { int res; char *real_path; - int path_search_permission; real_path = process_path(path, true); if (real_path == NULL) return -errno; - path_search_permission = check_path_search_perms(real_path); - if (path_search_permission != 0) - return path_search_permission; + if(!path_has_search_perms(real_path)) + return -EACCES; if (uid != -1) { switch (settings.chown_policy) { @@ -1573,15 +1539,13 @@ static int bindfs_truncate(const char *path, off_t size) { int res; char *real_path; - int path_search_permission; real_path = process_path(path, true); if (real_path == NULL) return -errno; - path_search_permission = check_path_search_perms(real_path); - if (path_search_permission != 0) - return path_search_permission; + if(!path_has_search_perms(real_path)) + return -EACCES; res = truncate(real_path, size); free(real_path); @@ -1598,10 +1562,6 @@ static int bindfs_ftruncate(const char *path, off_t size, int res; (void) path; - // TODO: Implement access check with `access_check` function (though it - // doesn't currently take a file descriptor, so may need refactoring to - // support this). - res = ftruncate(fi->fh, size); if (res == -1) return -errno; @@ -1618,20 +1578,16 @@ static int bindfs_utimens(const char *path, const struct timespec ts[2]) { int res; char *real_path; - int access_check_result; - int path_search_permission; real_path = process_path(path, true); if (real_path == NULL) return -errno; - access_check_result = dirname_access_check(real_path, W_OK); - if (access_check_result != 0) - return access_check_result; - - path_search_permission = check_path_search_perms(real_path); - if (path_search_permission != 0) - return path_search_permission; + if ( + !dirname_access_check(real_path, W_OK) + || !path_has_search_perms(real_path) + ) + return -EACCES; #ifdef HAVE_UTIMENSAT res = utimensat(settings.mntsrc_fd, real_path, ts, AT_SYMLINK_NOFOLLOW); @@ -1658,20 +1614,16 @@ static int bindfs_create(const char *path, mode_t mode, struct fuse_file_info *f int fd; struct fuse_context *fc; char *real_path; - int access_check_result; - int path_search_permission; real_path = process_path(path, true); if (real_path == NULL) return -errno; - access_check_result = dirname_access_check(real_path, W_OK); - if (access_check_result != 0) - return access_check_result; - - path_search_permission = check_path_search_perms(real_path); - if (path_search_permission != 0) - return path_search_permission; + if ( + !dirname_access_check(real_path, W_OK) + || !path_has_search_perms(real_path) + ) + return -EACCES; // TODO: Pretty sure we need more access checks here. Will let the tests // highlight the issues. @@ -1704,20 +1656,16 @@ static int bindfs_open(const char *path, struct fuse_file_info *fi) { int fd; char *real_path; - int access_check_result; - int path_search_permission; real_path = process_path(path, true); if (real_path == NULL) return -errno; - access_check_result = dirname_access_check(real_path, W_OK); - if (access_check_result != 0) - return access_check_result; - - path_search_permission = check_path_search_perms(real_path); - if (path_search_permission != 0) - return path_search_permission; + if ( + !dirname_access_check(real_path, W_OK) + || !path_has_search_perms(real_path) + ) + return -EACCES; // TODO: Pretty sure we need more access checks here. Will let the tests // highlight the issues. @@ -1856,15 +1804,13 @@ static int bindfs_statfs(const char *path, struct statvfs *stbuf) { int res; char *real_path; - int path_search_permission; real_path = process_path(path, true); if (real_path == NULL) return -errno; - path_search_permission = check_path_search_perms(real_path); - if (path_search_permission != 0) - return path_search_permission; + if(!path_has_search_perms(real_path)) + return -EACCES; res = statvfs(real_path, stbuf); free(real_path); From d4a56e8fea2c11e349edfeefb678fb017f801804 Mon Sep 17 00:00:00 2001 From: vimist Date: Sun, 11 Jun 2023 19:35:32 +0100 Subject: [PATCH 07/12] Resolve my sloppy mistakes, I should have spotted these... --- src/bindfs.c | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/src/bindfs.c b/src/bindfs.c index afc8aa3..9173710 100755 --- a/src/bindfs.c +++ b/src/bindfs.c @@ -1026,12 +1026,12 @@ static int bindfs_access(const char *path, int wants) * @return true if the dirname has the requested permissions, false otherwise. */ static bool dirname_access_check(const char *path, int wants) { - char *part = strdup(path); - part = dirname(part); + char *dup_path = strdup(path); + char *part = dirname(dup_path); int access_check_result = access_check(part, wants); - free(part); + free(dup_path); return access_check_result == 0; } @@ -1045,17 +1045,11 @@ static bool dirname_access_check(const char *path, int wants) { * otherwise. */ static bool path_has_search_perms(const char *path) { - char *part = strdup(path); + char *dup_path = strdup(path); + char *part = dup_path; - bool parent_dir = true; do { part = dirname(part); - - if (parent_dir && access_check(part, W_OK) != 0) { - free(part); - return false; - } - DPRINTF( "Checking path component for required search permissions: %s", part); @@ -1064,15 +1058,13 @@ static bool path_has_search_perms(const char *path) { DPRINTF( "Path component doesn't have required search permission: %s", part); - free(part); + free(dup_path); return false; } - - parent_dir = false; } while(strcmp(part, "/") != 0); DPRINTF("Path has search required permissions: %s", path); - free(part); + free(dup_path); return true; } @@ -1210,7 +1202,7 @@ static int bindfs_mknod(const char *path, mode_t mode, dev_t rdev) return -errno; if ( - dirname_access_check(real_path, W_OK) + !dirname_access_check(real_path, W_OK) || !path_has_search_perms(real_path) ) return -EACCES; @@ -1244,7 +1236,7 @@ static int bindfs_mkdir(const char *path, mode_t mode) return -errno; if ( - dirname_access_check(real_path, W_OK) + !dirname_access_check(real_path, W_OK) || !path_has_search_perms(real_path) ) return -EACCES; @@ -1289,7 +1281,7 @@ static int bindfs_symlink(const char *from, const char *to) return -errno; if ( - dirname_access_check(real_to, W_OK) + !dirname_access_check(real_to, W_OK) || !path_has_search_perms(real_to) ) return -EACCES; From 40b59c104e541b79abfe34091e8165ae8000f39e Mon Sep 17 00:00:00 2001 From: vimist Date: Mon, 12 Jun 2023 19:38:01 +0100 Subject: [PATCH 08/12] Convert access functions to return bool and set errno --- src/bindfs.c | 75 ++++++++++++++++++++++++++-------------------------- 1 file changed, 38 insertions(+), 37 deletions(-) diff --git a/src/bindfs.c b/src/bindfs.c index 9173710..b182a92 100755 --- a/src/bindfs.c +++ b/src/bindfs.c @@ -248,7 +248,7 @@ static size_t round_up_buffer_size_for_direct_io(size_t size); #endif static unsigned int permset_to_bits(acl_permset_t permset); -static int access_check(const char *real_path, int wants); +static bool access_check(const char *real_path, int wants); static bool dirname_access_check(const char *path, int wants); static bool path_has_search_perms(const char *path); @@ -581,7 +581,7 @@ static int delete_file(const char *path, int (*target_delete_func)(const char *) !dirname_access_check(real_path, W_OK) || !path_has_search_perms(real_path) ) - return -EACCES; + return -errno; if (settings.resolve_symlinks) { if (lstat(real_path, &st) == -1) { @@ -820,10 +820,10 @@ static unsigned int permset_to_bits(acl_permset_t permset) { * @param wants The bitwise-inclusive OR of the access permissions to be * checked (R_OK, W_OK, X_OK). * - * @return 0 if the effective user has access, -EACCES otherwise. - * + * @return true if the effective user has access or false on any other error. + * `errno` is set to the specific error number if one occurred. * */ -static int access_check(const char *real_path, int wants) +static bool access_check(const char *real_path, int wants) { DPRINTF("Performing access check for '%s'", real_path); @@ -831,7 +831,7 @@ static int access_check(const char *real_path, int wants) if (lstat(real_path, &st) == -1) { DPRINTF("Could not lstat '%s': %s", real_path, strerror(errno)); free(real_path); - return -errno; + return false; } // Update the stat struct to take into account the bindfs flags @@ -853,9 +853,8 @@ static int access_check(const char *real_path, int wants) // If the user ID of the process is the owner, the owner entry determines // access. if (euid == st.st_uid) { - return (((st.st_mode & 0700) >> 6) & wants) == wants - ? 0 - : -EACCES; + errno = EACCES; + return (((st.st_mode & 0700) >> 6) & wants) == wants; } // Get the groups the effective user is a part of @@ -979,9 +978,8 @@ static int access_check(const char *real_path, int wants) // of the named user entries, this entry determines access if (has_user_acl) { DPRINTF("Using ACL_USER entry to determine access"); - return (user_acl_perms & acl_mask & wants) == wants - ? 0 - : -EACCES; + errno = EACCES; + return (user_acl_perms & acl_mask & wants) == wants; } // If one of the group IDs of the process matches the owning group and the @@ -998,22 +996,24 @@ static int access_check(const char *real_path, int wants) // this determines that access is denied if (has_group) { DPRINTF("Using ACL_GROUP_OBJ or ACL_GROUP entry to determine access"); - return (group_perms & acl_mask & wants) == wants - ? 0 - : -EACCES; + errno = EACCES; + return (group_perms & acl_mask & wants) == wants; } // Else the other entry determines access. DPRINTF("Using ACL_OTHER entry to determine access"); - return (other_perms & wants) == wants - ? 0 - : -EACCES; + errno = EACCES; + return (other_perms & wants) == wants; } static int bindfs_access(const char *path, int wants) { char *real_path = process_path(path, true); - return access_check(real_path, wants); + + if (!access_check(real_path, wants)) + return -errno; + else + return 0; } /** @@ -1024,15 +1024,16 @@ static int bindfs_access(const char *path, int wants) * checked (R_OK, W_OK, X_OK). * * @return true if the dirname has the requested permissions, false otherwise. + * `errno` is set to the specific error number if one occurred. */ static bool dirname_access_check(const char *path, int wants) { char *dup_path = strdup(path); char *part = dirname(dup_path); - int access_check_result = access_check(part, wants); + bool access_check_result = access_check(part, wants); free(dup_path); - return access_check_result == 0; + return access_check_result; } /** @@ -1054,7 +1055,7 @@ static bool path_has_search_perms(const char *path) { "Checking path component for required search permissions: %s", part); - if (access_check(part, X_OK) != 0) { + if (!access_check(part, X_OK)) { DPRINTF( "Path component doesn't have required search permission: %s", part); @@ -1078,7 +1079,7 @@ static int bindfs_readlink(const char *path, char *buf, size_t size) return -errno; if(!path_has_search_perms(real_path)) - return -EACCES; + return -errno; /* No need to check for access to the link itself, since symlink permissions don't matter. Access to the path components of the symlink @@ -1205,7 +1206,7 @@ static int bindfs_mknod(const char *path, mode_t mode, dev_t rdev) !dirname_access_check(real_path, W_OK) || !path_has_search_perms(real_path) ) - return -EACCES; + return -errno; mode = permchain_apply(settings.create_permchain, mode); @@ -1239,7 +1240,7 @@ static int bindfs_mkdir(const char *path, mode_t mode) !dirname_access_check(real_path, W_OK) || !path_has_search_perms(real_path) ) - return -EACCES; + return -errno; mode |= S_IFDIR; /* tell permchain_apply this is a directory */ mode = permchain_apply(settings.create_permchain, mode); @@ -1284,7 +1285,7 @@ static int bindfs_symlink(const char *from, const char *to) !dirname_access_check(real_to, W_OK) || !path_has_search_perms(real_to) ) - return -EACCES; + return -errno; res = symlink(from, real_to); if (res == -1) { @@ -1328,13 +1329,13 @@ static int bindfs_rename(const char *from, const char *to) || !path_has_search_perms(real_from) || !path_has_search_perms(real_to) ) - return -EACCES; + return -errno; if(lstat(real_from, &st) != 0) return -errno; - if ((st.st_mode & S_IFDIR) == S_IFDIR && access_check(real_from, W_OK) != 0) - return -EACCES; + if ((st.st_mode & S_IFDIR) == S_IFDIR && !access_check(real_from, W_OK)) + return -errno; #ifdef HAVE_FUSE_3 @@ -1383,7 +1384,7 @@ static int bindfs_link(const char *from, const char *to) || !path_has_search_perms(real_from) || !path_has_search_perms(real_to) ) - return -EACCES; + return -errno; res = link(real_from, real_to); free(real_from); @@ -1410,7 +1411,7 @@ static int bindfs_chmod(const char *path, mode_t mode) return -errno; if(!path_has_search_perms(real_path)) - return -EACCES; + return -errno; if (settings.chmod_allow_x) { /* Get the old permission bits and see which bits would change. */ @@ -1478,7 +1479,7 @@ static int bindfs_chown(const char *path, uid_t uid, gid_t gid) return -errno; if(!path_has_search_perms(real_path)) - return -EACCES; + return -errno; if (uid != -1) { switch (settings.chown_policy) { @@ -1537,7 +1538,7 @@ static int bindfs_truncate(const char *path, off_t size) return -errno; if(!path_has_search_perms(real_path)) - return -EACCES; + return -errno; res = truncate(real_path, size); free(real_path); @@ -1579,7 +1580,7 @@ static int bindfs_utimens(const char *path, const struct timespec ts[2]) !dirname_access_check(real_path, W_OK) || !path_has_search_perms(real_path) ) - return -EACCES; + return -errno; #ifdef HAVE_UTIMENSAT res = utimensat(settings.mntsrc_fd, real_path, ts, AT_SYMLINK_NOFOLLOW); @@ -1615,7 +1616,7 @@ static int bindfs_create(const char *path, mode_t mode, struct fuse_file_info *f !dirname_access_check(real_path, W_OK) || !path_has_search_perms(real_path) ) - return -EACCES; + return -errno; // TODO: Pretty sure we need more access checks here. Will let the tests // highlight the issues. @@ -1657,7 +1658,7 @@ static int bindfs_open(const char *path, struct fuse_file_info *fi) !dirname_access_check(real_path, W_OK) || !path_has_search_perms(real_path) ) - return -EACCES; + return -errno; // TODO: Pretty sure we need more access checks here. Will let the tests // highlight the issues. @@ -1802,7 +1803,7 @@ static int bindfs_statfs(const char *path, struct statvfs *stbuf) return -errno; if(!path_has_search_perms(real_path)) - return -EACCES; + return -errno; res = statvfs(real_path, stbuf); free(real_path); From 6c60f403797576dd38ba019bb831179901e03dc7 Mon Sep 17 00:00:00 2001 From: vimist Date: Mon, 12 Jun 2023 20:04:33 +0100 Subject: [PATCH 09/12] Integrate path_has_search_perms into path_access_check --- src/bindfs.c | 57 +++++++++++++--------------------------------------- 1 file changed, 14 insertions(+), 43 deletions(-) diff --git a/src/bindfs.c b/src/bindfs.c index b182a92..1a82793 100755 --- a/src/bindfs.c +++ b/src/bindfs.c @@ -249,7 +249,7 @@ static size_t round_up_buffer_size_for_direct_io(size_t size); static unsigned int permset_to_bits(acl_permset_t permset); static bool access_check(const char *real_path, int wants); -static bool dirname_access_check(const char *path, int wants); +static bool path_access_check(const char *path, int wants); static bool path_has_search_perms(const char *path); /* FUSE callbacks */ @@ -577,10 +577,7 @@ static int delete_file(const char *path, int (*target_delete_func)(const char *) if (real_path == NULL) return -errno; - if ( - !dirname_access_check(real_path, W_OK) - || !path_has_search_perms(real_path) - ) + if (!path_access_check(real_path, W_OK)) return -errno; if (settings.resolve_symlinks) { @@ -1017,7 +1014,8 @@ static int bindfs_access(const char *path, int wants) } /** - * Perform an access check against the dirname of the given path. + * Check the full path has search permissions and the immediate parent + * directory has `wants` permission. * * @param path The path to get the dirname of and performe the check against. * @param wants The bitwise-inclusive OR of the access permissions to be @@ -1026,14 +1024,14 @@ static int bindfs_access(const char *path, int wants) * @return true if the dirname has the requested permissions, false otherwise. * `errno` is set to the specific error number if one occurred. */ -static bool dirname_access_check(const char *path, int wants) { +static bool path_access_check(const char *path, int wants) { char *dup_path = strdup(path); char *part = dirname(dup_path); bool access_check_result = access_check(part, wants); free(dup_path); - return access_check_result; + return access_check_result && path_has_search_perms(path); } /** @@ -1202,10 +1200,7 @@ static int bindfs_mknod(const char *path, mode_t mode, dev_t rdev) if (real_path == NULL) return -errno; - if ( - !dirname_access_check(real_path, W_OK) - || !path_has_search_perms(real_path) - ) + if (!path_access_check(real_path, W_OK)) return -errno; mode = permchain_apply(settings.create_permchain, mode); @@ -1236,10 +1231,7 @@ static int bindfs_mkdir(const char *path, mode_t mode) if (real_path == NULL) return -errno; - if ( - !dirname_access_check(real_path, W_OK) - || !path_has_search_perms(real_path) - ) + if (!path_access_check(real_path, W_OK)) return -errno; mode |= S_IFDIR; /* tell permchain_apply this is a directory */ @@ -1281,10 +1273,7 @@ static int bindfs_symlink(const char *from, const char *to) if (real_to == NULL) return -errno; - if ( - !dirname_access_check(real_to, W_OK) - || !path_has_search_perms(real_to) - ) + if (!path_access_check(real_to, W_OK)) return -errno; res = symlink(from, real_to); @@ -1323,12 +1312,7 @@ static int bindfs_rename(const char *from, const char *to) return -errno; } - if ( - !dirname_access_check(real_from, W_OK) - || !dirname_access_check(real_to, W_OK) - || !path_has_search_perms(real_from) - || !path_has_search_perms(real_to) - ) + if (!path_access_check(real_from, W_OK) || !path_access_check(real_to, W_OK)) return -errno; if(lstat(real_from, &st) != 0) @@ -1379,11 +1363,7 @@ static int bindfs_link(const char *from, const char *to) return -errno; } - if ( - !dirname_access_check(real_to, W_OK) - || !path_has_search_perms(real_from) - || !path_has_search_perms(real_to) - ) + if (!path_access_check(real_to, W_OK) || !path_has_search_perms(real_from)) return -errno; res = link(real_from, real_to); @@ -1576,10 +1556,7 @@ static int bindfs_utimens(const char *path, const struct timespec ts[2]) if (real_path == NULL) return -errno; - if ( - !dirname_access_check(real_path, W_OK) - || !path_has_search_perms(real_path) - ) + if (!path_access_check(real_path, W_OK)) return -errno; #ifdef HAVE_UTIMENSAT @@ -1612,10 +1589,7 @@ static int bindfs_create(const char *path, mode_t mode, struct fuse_file_info *f if (real_path == NULL) return -errno; - if ( - !dirname_access_check(real_path, W_OK) - || !path_has_search_perms(real_path) - ) + if (!path_access_check(real_path, W_OK)) return -errno; // TODO: Pretty sure we need more access checks here. Will let the tests @@ -1654,10 +1628,7 @@ static int bindfs_open(const char *path, struct fuse_file_info *fi) if (real_path == NULL) return -errno; - if ( - !dirname_access_check(real_path, W_OK) - || !path_has_search_perms(real_path) - ) + if (!path_access_check(real_path, W_OK)) return -errno; // TODO: Pretty sure we need more access checks here. Will let the tests From 18fdc54cebef5fec0b6ba1696ecbfec25a11ff2a Mon Sep 17 00:00:00 2001 From: vimist Date: Mon, 12 Jun 2023 20:06:43 +0100 Subject: [PATCH 10/12] Re-order function definitions (slightly more logical placement) --- src/bindfs.c | 207 ++++++++++++++++++++++++++------------------------- 1 file changed, 104 insertions(+), 103 deletions(-) diff --git a/src/bindfs.c b/src/bindfs.c index 1a82793..878d019 100755 --- a/src/bindfs.c +++ b/src/bindfs.c @@ -247,6 +247,7 @@ static int unapply_gid_offset(gid_t *gid); static size_t round_up_buffer_size_for_direct_io(size_t size); #endif +/* Access checking helper functions */ static unsigned int permset_to_bits(acl_permset_t permset); static bool access_check(const char *real_path, int wants); static bool path_access_check(const char *path, int wants); @@ -689,99 +690,6 @@ static size_t round_up_buffer_size_for_direct_io(size_t size) } #endif -#ifdef HAVE_FUSE_3 -static void *bindfs_init(struct fuse_conn_info *conn, struct fuse_config *cfg) -#else -static void *bindfs_init() -#endif -{ - #ifdef HAVE_FUSE_3 - (void) conn; - cfg->use_ino = 1; - - // Disable caches so changes in base FS are visible immediately. - // Especially the attribute cache must be disabled when different users - // might see different file attributes, such as when mirroring users. - cfg->entry_timeout = 0; - cfg->attr_timeout = 0; - cfg->negative_timeout = 0; -#ifdef __linux__ - cfg->direct_io = settings.direct_io; -#endif - #endif - - assert(settings.permchain != NULL); - assert(settings.mntsrc_fd > 0); - - maybe_stdout_stderr_to_file(); - - if (fchdir(settings.mntsrc_fd) != 0) { - fprintf( - stderr, - "Could not change working directory to '%s': %s\n", - settings.mntsrc, - strerror(errno) - ); - bindfs_init_failed = true; -#ifdef __OpenBSD__ - exit(1); -#else - fuse_exit(fuse_get_context()->fuse); -#endif - } - - return NULL; -} - -static void bindfs_destroy(void *private_data) -{ -} - -#ifdef HAVE_FUSE_3 -static int bindfs_getattr(const char *path, struct stat *stbuf, - struct fuse_file_info *fi) -#else -static int bindfs_getattr(const char *path, struct stat *stbuf) -#endif -{ - int res; - char *real_path; - - real_path = process_path(path, true); - if (real_path == NULL) - return -errno; - - if (lstat(real_path, stbuf) == -1) { - free(real_path); - return -errno; - } - - res = getattr_common(real_path, stbuf); - free(real_path); - return res; -} - -#ifndef HAVE_FUSE_3 -static int bindfs_fgetattr(const char *path, struct stat *stbuf, - struct fuse_file_info *fi) -{ - int res; - char *real_path; - - real_path = process_path(path, true); - if (real_path == NULL) - return -errno; - - if (fstat(fi->fh, stbuf) == -1) { - free(real_path); - return -errno; - } - res = getattr_common(real_path, stbuf); - free(real_path); - return res; -} -#endif - /** * Convert an ACL permset to a bitmask. * @@ -1003,16 +911,6 @@ static bool access_check(const char *real_path, int wants) return (other_perms & wants) == wants; } -static int bindfs_access(const char *path, int wants) -{ - char *real_path = process_path(path, true); - - if (!access_check(real_path, wants)) - return -errno; - else - return 0; -} - /** * Check the full path has search permissions and the immediate parent * directory has `wants` permission. @@ -1067,6 +965,109 @@ static bool path_has_search_perms(const char *path) { return true; } +#ifdef HAVE_FUSE_3 +static void *bindfs_init(struct fuse_conn_info *conn, struct fuse_config *cfg) +#else +static void *bindfs_init() +#endif +{ + #ifdef HAVE_FUSE_3 + (void) conn; + cfg->use_ino = 1; + + // Disable caches so changes in base FS are visible immediately. + // Especially the attribute cache must be disabled when different users + // might see different file attributes, such as when mirroring users. + cfg->entry_timeout = 0; + cfg->attr_timeout = 0; + cfg->negative_timeout = 0; +#ifdef __linux__ + cfg->direct_io = settings.direct_io; +#endif + #endif + + assert(settings.permchain != NULL); + assert(settings.mntsrc_fd > 0); + + maybe_stdout_stderr_to_file(); + + if (fchdir(settings.mntsrc_fd) != 0) { + fprintf( + stderr, + "Could not change working directory to '%s': %s\n", + settings.mntsrc, + strerror(errno) + ); + bindfs_init_failed = true; +#ifdef __OpenBSD__ + exit(1); +#else + fuse_exit(fuse_get_context()->fuse); +#endif + } + + return NULL; +} + +static void bindfs_destroy(void *private_data) +{ +} + +#ifdef HAVE_FUSE_3 +static int bindfs_getattr(const char *path, struct stat *stbuf, + struct fuse_file_info *fi) +#else +static int bindfs_getattr(const char *path, struct stat *stbuf) +#endif +{ + int res; + char *real_path; + + real_path = process_path(path, true); + if (real_path == NULL) + return -errno; + + if (lstat(real_path, stbuf) == -1) { + free(real_path); + return -errno; + } + + res = getattr_common(real_path, stbuf); + free(real_path); + return res; +} + +#ifndef HAVE_FUSE_3 +static int bindfs_fgetattr(const char *path, struct stat *stbuf, + struct fuse_file_info *fi) +{ + int res; + char *real_path; + + real_path = process_path(path, true); + if (real_path == NULL) + return -errno; + + if (fstat(fi->fh, stbuf) == -1) { + free(real_path); + return -errno; + } + res = getattr_common(real_path, stbuf); + free(real_path); + return res; +} +#endif + +static int bindfs_access(const char *path, int wants) +{ + char *real_path = process_path(path, true); + + if (!access_check(real_path, wants)) + return -errno; + else + return 0; +} + static int bindfs_readlink(const char *path, char *buf, size_t size) { int res; From c059947a2ac43da8567fd4193d56e32f8a90f43f Mon Sep 17 00:00:00 2001 From: vimist Date: Mon, 12 Jun 2023 21:26:27 +0100 Subject: [PATCH 11/12] Fix incorrect setting of errno in access_check --- src/bindfs.c | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/src/bindfs.c b/src/bindfs.c index 878d019..adbbb58 100755 --- a/src/bindfs.c +++ b/src/bindfs.c @@ -758,8 +758,12 @@ static bool access_check(const char *real_path, int wants) // If the user ID of the process is the owner, the owner entry determines // access. if (euid == st.st_uid) { - errno = EACCES; - return (((st.st_mode & 0700) >> 6) & wants) == wants; + if ((((st.st_mode & 0700) >> 6) & wants) == wants) { + return true; + } else { + errno = EACCES; + return false; + } } // Get the groups the effective user is a part of @@ -883,8 +887,12 @@ static bool access_check(const char *real_path, int wants) // of the named user entries, this entry determines access if (has_user_acl) { DPRINTF("Using ACL_USER entry to determine access"); - errno = EACCES; - return (user_acl_perms & acl_mask & wants) == wants; + if ((user_acl_perms & acl_mask & wants) == wants) { + return true; + } else { + errno = EACCES; + return false; + } } // If one of the group IDs of the process matches the owning group and the @@ -901,14 +909,22 @@ static bool access_check(const char *real_path, int wants) // this determines that access is denied if (has_group) { DPRINTF("Using ACL_GROUP_OBJ or ACL_GROUP entry to determine access"); - errno = EACCES; - return (group_perms & acl_mask & wants) == wants; + if ((group_perms & acl_mask & wants) == wants) { + return true; + } else { + errno = EACCES; + return false; + } } // Else the other entry determines access. DPRINTF("Using ACL_OTHER entry to determine access"); - errno = EACCES; - return (other_perms & wants) == wants; + if ((other_perms & wants) == wants) { + return true; + } else { + errno = EACCES; + return false; + } } /** From 736c57ac82faeb1c2f691095283bb2dca44133fa Mon Sep 17 00:00:00 2001 From: vimist Date: Mon, 12 Jun 2023 21:27:06 +0100 Subject: [PATCH 12/12] Fix a couple of bugs --- src/bindfs.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/bindfs.c b/src/bindfs.c index adbbb58..d2fb1e3 100755 --- a/src/bindfs.c +++ b/src/bindfs.c @@ -872,7 +872,6 @@ static bool access_check(const char *real_path, int wants) } acl_free(acl); - free(real_path); // If there is no mask entry, it doesn't restrict anything. if (!has_mask) { @@ -974,7 +973,7 @@ static bool path_has_search_perms(const char *path) { free(dup_path); return false; } - } while(strcmp(part, "/") != 0); + } while(strcmp(part, "/") != 0 && strcmp(part, ".") != 0); DPRINTF("Path has search required permissions: %s", path); free(dup_path);