From 0bd985efbcee7329bb03e059e264124877bdc884 Mon Sep 17 00:00:00 2001 From: Martin Fouilleul Date: Tue, 13 Jun 2023 15:56:19 +0200 Subject: [PATCH] [io] restrict rights of files open with file_open_at to the rights of the directory handle --- src/platform/platform_io.h | 41 ++++--- src/platform/platform_io_common.c | 10 +- src/platform/platform_io_internal.c | 5 + src/platform/platform_io_internal.h | 1 + src/platform/posix_io.c | 146 +++++++++++++++--------- test/files/main.c | 171 ++++++++++++++++++++++++++-- 6 files changed, 293 insertions(+), 81 deletions(-) diff --git a/src/platform/platform_io.h b/src/platform/platform_io.h index 9531149..014f16b 100644 --- a/src/platform/platform_io.h +++ b/src/platform/platform_io.h @@ -17,20 +17,29 @@ typedef struct { u64 h; } file_handle; -typedef u32 file_open_flags; -enum { - FILE_OPEN_READ = 1<<0, - FILE_OPEN_WRITE = 1<<1, - FILE_OPEN_APPEND = 1<<2, - FILE_OPEN_TRUNCATE = 1<<3, - FILE_OPEN_CREATE = 1<<4, +typedef u16 file_open_flags; +enum _file_open_flags +{ + FILE_OPEN_NONE = 0, + FILE_OPEN_APPEND = 1<<1, + FILE_OPEN_TRUNCATE = 1<<2, + FILE_OPEN_CREATE = 1<<3, - FILE_OPEN_SYMLINK = 1<<5, - FILE_OPEN_NO_FOLLOW = 1<<6, - FILE_OPEN_RESTRICT = 1<<7, + FILE_OPEN_SYMLINK = 1<<4, + FILE_OPEN_NO_FOLLOW = 1<<5, + FILE_OPEN_RESTRICT = 1<<6, //... }; +typedef u16 file_access_rights; +enum _file_access_rights +{ + FILE_ACCESS_NONE = 0, + FILE_ACCESS_READ = 1<<1, + FILE_ACCESS_WRITE = 1<<2, +}; + + typedef enum { FILE_SEEK_SET, FILE_SEEK_END, FILE_SEEK_CURRENT } file_whence; typedef u64 io_req_id; @@ -67,7 +76,12 @@ typedef struct io_req union { - file_open_flags openFlags; + struct + { + file_access_rights rights; + file_open_flags flags; + } open; + file_whence whence; }; @@ -126,9 +140,8 @@ MP_API io_cmp io_wait_single_req(io_req* req); // File IO wrapper API //---------------------------------------------------------------- -MP_API file_handle file_open(str8 path, file_open_flags flags); -MP_API file_handle file_open_at(file_handle dir, str8 path, file_open_flags flags); -MP_API file_handle file_open_relative(file_handle base, str8 path, file_open_flags flags); +MP_API file_handle file_open(str8 path, file_access_rights rights, file_open_flags flags); +MP_API file_handle file_open_at(file_handle dir, str8 path, file_access_rights rights, file_open_flags flags); MP_API void file_close(file_handle file); MP_API i64 file_pos(file_handle file); diff --git a/src/platform/platform_io_common.c b/src/platform/platform_io_common.c index 9db1a0c..c01366e 100644 --- a/src/platform/platform_io_common.c +++ b/src/platform/platform_io_common.c @@ -11,12 +11,13 @@ // File stream read/write API //------------------------------------------------------------------------------ -file_handle file_open(str8 path, file_open_flags flags) +file_handle file_open(str8 path, file_access_rights rights, file_open_flags flags) { io_req req = {.op = IO_OP_OPEN_AT, .size = path.len, .buffer = path.ptr, - .openFlags = flags}; + .open.rights = rights, + .open.flags = flags }; io_cmp cmp = io_wait_single_req(&req); @@ -25,13 +26,14 @@ file_handle file_open(str8 path, file_open_flags flags) return(cmp.handle); } -file_handle file_open_at(file_handle dir, str8 path, file_open_flags flags) +file_handle file_open_at(file_handle dir, str8 path, file_access_rights rights, file_open_flags flags) { io_req req = {.op = IO_OP_OPEN_AT, .handle = dir, .size = path.len, .buffer = path.ptr, - .openFlags = flags}; + .open.rights = rights, + .open.flags = flags,}; io_cmp cmp = io_wait_single_req(&req); return(cmp.handle); diff --git a/src/platform/platform_io_internal.c b/src/platform/platform_io_internal.c index 8be45e9..22d97e6 100644 --- a/src/platform/platform_io_internal.c +++ b/src/platform/platform_io_internal.c @@ -20,6 +20,11 @@ file_slot* file_slot_alloc(file_table* table) slot->fd = -1; table->nextSlot++; } + + u32 tmpGeneration = slot->generation; + memset(slot, 0, sizeof(file_slot)); + slot->generation = tmpGeneration; + return(slot); } diff --git a/src/platform/platform_io_internal.h b/src/platform/platform_io_internal.h index 69181de..d5fa461 100644 --- a/src/platform/platform_io_internal.h +++ b/src/platform/platform_io_internal.h @@ -26,6 +26,7 @@ typedef struct file_slot bool fatal; list_elt freeListElt; + file_access_rights rights; PLATFORM_IO_NATIVE_MEMBER; } file_slot; diff --git a/src/platform/posix_io.c b/src/platform/posix_io.c index 53915ef..74c52e8 100644 --- a/src/platform/posix_io.c +++ b/src/platform/posix_io.c @@ -108,13 +108,13 @@ io_error io_convert_errno(int e) return(error); } -int io_convert_open_flags(file_open_flags flags) +int io_convert_access_rights(file_access_rights rights) { int oflags = 0; - if(flags & FILE_OPEN_READ) + if(rights & FILE_ACCESS_READ) { - if(flags & FILE_OPEN_WRITE) + if(rights & FILE_ACCESS_WRITE) { oflags = O_RDWR; } @@ -123,10 +123,16 @@ int io_convert_open_flags(file_open_flags flags) oflags = O_RDONLY; } } - else if(flags & FILE_OPEN_WRITE) + else if(rights & FILE_ACCESS_WRITE) { oflags = O_WRONLY; } + return(oflags); +} + +int io_convert_open_flags(file_open_flags flags) +{ + int oflags = 0; if(flags & FILE_OPEN_TRUNCATE) { @@ -151,6 +157,27 @@ int io_convert_open_flags(file_open_flags flags) return(oflags); } +int io_update_dir_flags_at(int dirFd, char* path, int flags) +{ + struct stat s; + if(!fstatat(dirFd, path, &s, AT_SYMLINK_NOFOLLOW)) + { + if((s.st_mode & S_IFMT) == S_IFDIR) + { + if(flags & O_WRONLY) + { + flags &= ~O_WRONLY; + } + else if(flags & O_RDWR) + { + flags &= ~O_RDWR; + flags |= O_RDONLY; + } + } + } + return(flags); +} + typedef struct io_open_restrict_result { io_error error; @@ -355,74 +382,89 @@ io_cmp io_open_at(file_slot* atSlot, io_req* req, file_table* table) } else { + slot->fd = -1; + cmp.handle = file_handle_from_slot(table, slot); - int flags = io_convert_open_flags(req->openFlags); - - //TODO: allow specifying access - mode_t mode = S_IRUSR - | S_IWUSR - | S_IRGRP - | S_IWGRP - | S_IROTH - | S_IWOTH; - - //NOTE: open - mem_arena_scope scratch = mem_scratch_begin(); - - str8 path = str8_from_buffer(req->size, req->buffer); - - - //TODO: if FILE_OPEN_RESTRICT, do the file traversal to check that path is in the - // subtree rooted at atSlot->fd - slot->fd = -1; + slot->rights = req->open.rights; if(atSlot) { - if(path.len && path.ptr[0] == '/') - { - //NOTE: if path is absolute, change for a relative one, otherwise openat ignores fd. - str8_list list = {0}; - str8_list_push(scratch.arena, &list, STR8(".")); - str8_list_push(scratch.arena, &list, path); - path = str8_list_join(scratch.arena, list); - } - char* pathCStr = str8_to_cstring(scratch.arena, path); + slot->rights &= atSlot->rights; + } - if(req->openFlags & FILE_OPEN_RESTRICT) + if(slot->rights != req->open.rights) + { + slot->error = IO_ERR_PERM; + slot->fatal = true; + } + else + { + int flags = io_convert_access_rights(slot->rights); + flags |= io_convert_open_flags(req->open.flags); + + mode_t mode = S_IRUSR + | S_IWUSR + | S_IRGRP + | S_IWGRP + | S_IROTH + | S_IWOTH; + + mem_arena_scope scratch = mem_scratch_begin(); + str8 path = str8_from_buffer(req->size, req->buffer); + + slot->fd = -1; + if(atSlot) { - io_open_restrict_result res = io_open_restrict(atSlot->fd, path, flags, mode); - if(res.error == IO_OK) + if(path.len && path.ptr[0] == '/') { - slot->fd = res.fd; + //NOTE: if path is absolute, change for a relative one, otherwise openat ignores fd. + str8_list list = {0}; + str8_list_push(scratch.arena, &list, STR8(".")); + str8_list_push(scratch.arena, &list, path); + path = str8_list_join(scratch.arena, list); + } + char* pathCStr = str8_to_cstring(scratch.arena, path); + + if(req->open.flags & FILE_OPEN_RESTRICT) + { + io_open_restrict_result res = io_open_restrict(atSlot->fd, path, flags, mode); + if(res.error == IO_OK) + { + slot->fd = res.fd; + } + else + { + slot->fatal = true; + slot->error = res.error; + } } else { - slot->fatal = true; - slot->error = res.error; + flags = io_update_dir_flags_at(atSlot->fd, pathCStr, flags); + + slot->fd = openat(atSlot->fd, pathCStr, flags, mode); + if(slot->fd < 0) + { + slot->fatal = true; + slot->error = io_convert_errno(errno); + } } } else { - slot->fd = openat(atSlot->fd, pathCStr, flags, mode); + char* pathCStr = str8_to_cstring(scratch.arena, path); + + flags = io_update_dir_flags_at(AT_FDCWD, pathCStr, flags); + + slot->fd = open(pathCStr, flags, mode); if(slot->fd < 0) { slot->fatal = true; slot->error = io_convert_errno(errno); } } + mem_scratch_end(scratch); } - else - { - char* pathCStr = str8_to_cstring(scratch.arena, path); - slot->fd = open(pathCStr, flags, mode); - if(slot->fd < 0) - { - slot->fatal = true; - slot->error = io_convert_errno(errno); - } - } - - mem_scratch_end(scratch); cmp.error = slot->error; } @@ -552,6 +594,7 @@ io_cmp io_read(file_slot* slot, io_req* req) if(cmp.result < 0) { slot->error = io_convert_errno(errno); + cmp.result = 0; cmp.error = slot->error; } @@ -567,6 +610,7 @@ io_cmp io_write(file_slot* slot, io_req* req) if(cmp.result < 0) { slot->error = io_convert_errno(errno); + cmp.result = 0; cmp.error = slot->error; } diff --git a/test/files/main.c b/test/files/main.c index 3d940f8..9cb05df 100644 --- a/test/files/main.c +++ b/test/files/main.c @@ -13,7 +13,7 @@ int test_write(mem_arena* arena, str8 path, str8 test_string) { log_info("writing\n"); - file_handle f = file_open(path, FILE_OPEN_CREATE|FILE_OPEN_WRITE|FILE_OPEN_TRUNCATE); + file_handle f = file_open(path, FILE_ACCESS_WRITE, FILE_OPEN_CREATE|FILE_OPEN_TRUNCATE); if(file_last_error(f)) { log_error("Can't create/open file %.*s for writing\n", (int)path.len, path.ptr); @@ -52,7 +52,7 @@ int test_read(mem_arena* arena, str8 path, str8 test_string) { log_info("reading\n"); - file_handle f = file_open(path, FILE_OPEN_READ); + file_handle f = file_open(path, FILE_ACCESS_READ, 0); if(file_last_error(f)) { log_error("Can't open file %.*s for reading\n", (int)path.len, path.ptr); @@ -81,7 +81,7 @@ int test_stat_size(str8 path, u64 size) { log_info("stat size\n"); - file_handle f = file_open(path, 0); + file_handle f = file_open(path, 0, 0); if(file_last_error(f)) { log_error("Can't open file\n"); @@ -119,7 +119,7 @@ int test_stat_type(mem_arena* arena, str8 dataDir) log_info("stat type, regular\n"); - file_handle f = file_open(regular, 0); + file_handle f = file_open(regular, 0, 0); if(file_last_error(f)) { log_error("Can't open file\n"); @@ -141,7 +141,7 @@ int test_stat_type(mem_arena* arena, str8 dataDir) log_info("stat type, directory\n"); - f = file_open(dir, 0); + f = file_open(dir, 0, 0); if(file_last_error(f)) { log_error("Can't open file\n"); @@ -163,7 +163,7 @@ int test_stat_type(mem_arena* arena, str8 dataDir) log_info("stat type, symlink\n"); - f = file_open(link, FILE_OPEN_SYMLINK); + f = file_open(link, FILE_ACCESS_NONE, FILE_OPEN_SYMLINK); if(file_last_error(f)) { log_error("Can't open file\n"); @@ -190,7 +190,7 @@ int test_jail() { log_info("test jail\n"); - file_handle jail = file_open(STR8("./data/jail"), FILE_OPEN_READ); + file_handle jail = file_open(STR8("./data/jail"), FILE_ACCESS_READ, 0); if(file_last_error(jail)) { log_error("Can't open jail directory\n"); @@ -198,7 +198,7 @@ int test_jail() } // Check legitimates open - file_handle f = file_open_at(jail, STR8("/test.txt"), FILE_OPEN_READ | FILE_OPEN_RESTRICT); + file_handle f = file_open_at(jail, STR8("/test.txt"), FILE_ACCESS_READ, FILE_OPEN_RESTRICT); if(file_last_error(f) != IO_OK) { log_error("Can't open jail/test.txt\n"); @@ -206,7 +206,7 @@ int test_jail() } file_close(f); - f = file_open_at(jail, STR8("/dir1/../test.txt"), FILE_OPEN_READ | FILE_OPEN_RESTRICT); + f = file_open_at(jail, STR8("/dir1/../test.txt"), FILE_ACCESS_READ, FILE_OPEN_RESTRICT); if(file_last_error(f) != IO_OK) { log_error("Can't open jail/dir1/../test.txt\n"); @@ -215,7 +215,7 @@ int test_jail() file_close(f); // Check escapes - f = file_open_at(jail, STR8(".."), FILE_OPEN_READ | FILE_OPEN_RESTRICT); + f = file_open_at(jail, STR8(".."), FILE_ACCESS_READ, FILE_OPEN_RESTRICT); if(file_last_error(f) != IO_ERR_WALKOUT) { log_error("Escaped jail with relative path ..\n"); @@ -223,7 +223,7 @@ int test_jail() } file_close(f); - f = file_open_at(jail, STR8(".."), FILE_OPEN_READ | FILE_OPEN_RESTRICT); + f = file_open_at(jail, STR8(".."), FILE_ACCESS_READ, FILE_OPEN_RESTRICT); if(file_last_error(f) != IO_ERR_WALKOUT) { log_error("Escaped jail with relative path dir1/../..\n"); @@ -231,7 +231,7 @@ int test_jail() } file_close(f); - f = file_open_at(jail, STR8("/escape"), FILE_OPEN_READ | FILE_OPEN_RESTRICT); + f = file_open_at(jail, STR8("/escape"), FILE_ACCESS_READ, FILE_OPEN_RESTRICT); if(file_last_error(f) != IO_ERR_WALKOUT) { log_error("Escaped jail with symlink\n"); @@ -242,6 +242,152 @@ int test_jail() return(0); } +int test_rights(mem_arena* arena, str8 dirPath) +{ + log_info("test rights\n"); + + //-------------------------------------------------------------------------------------- + // base dir with no access + //-------------------------------------------------------------------------------------- + { + file_handle dir = file_open(dirPath, FILE_ACCESS_NONE, 0); + if(file_last_error(dir)) + { + log_error("Couldn't open ./dir1 with no access rights\n"); + return(-1); + } + + file_handle f = file_open_at(dir, STR8("./regular.txt"), FILE_ACCESS_READ, 0); + if(file_last_error(f) != IO_ERR_PERM) + { + log_error("Incorrect check when opening file with read access in dir with no access\n"); + return(-1); + } + file_close(f); + file_close(dir); + } + //-------------------------------------------------------------------------------------- + // base dir with read access + //-------------------------------------------------------------------------------------- + { + file_handle dir = file_open(dirPath, FILE_ACCESS_READ, 0); + if(file_last_error(dir)) + { + log_error("Couldn't open ./dir1 with read rights\n"); + return(-1); + } + + // check that we _can't_ open a file with write access + file_handle f = file_open_at(dir, STR8("./regular.txt"), FILE_ACCESS_WRITE, 0); + if(file_last_error(f) != IO_ERR_PERM) + { + log_error("Incorrect check when opening file with write access in dir with read access\n"); + return(-1); + } + file_close(f); + + // check that we _can_ open a file with read access + f = file_open_at(dir, STR8("./regular.txt"), FILE_ACCESS_READ, 0); + if(file_last_error(f)) + { + log_error("Couldn't open file with read access in dir with read access\n"); + return(-1); + } + + // check that we _can't_ write to that file + str8 testWrite = STR8("Hello, world!\n"); + if(file_write(f, testWrite.len, testWrite.ptr) != 0) + { + log_error("Incorrectly wrote to read-only file\n"); + return(-1); + } + if(file_last_error(f) != IO_ERR_PERM) + { + log_error("Incorrect error returned from writing to read-only file\n"); + return(-1); + } + + file_close(f); + file_close(dir); + } + //-------------------------------------------------------------------------------------- + // base dir with write access + //-------------------------------------------------------------------------------------- + { + file_handle dir = file_open(dirPath, FILE_ACCESS_WRITE, 0); + if(file_last_error(dir)) + { + log_error("Couldn't open ./dir1 with write rights\n"); + return(-1); + } + + // check that we _can't_ open a file with read access + file_handle f = file_open_at(dir, STR8("./regular.txt"), FILE_ACCESS_READ, 0); + if(file_last_error(f) != IO_ERR_PERM) + { + log_error("Incorrect check when opening file with read access in dir with write access\n"); + return(-1); + } + file_close(f); + + // check that we _can_ open a file with write access + f = file_open_at(dir, STR8("./regular.txt"), FILE_ACCESS_WRITE, 0); + if(file_last_error(f)) + { + log_error("Couldn't open file with write access in dir with write access\n"); + return(-1); + } + + // check that we _can't_ read that file + char testRead[512]; + if(file_read(f, 512, testRead) != 0) + { + log_error("Incorrectly read write-only file\n"); + return(-1); + } + if(file_last_error(f) != IO_ERR_PERM) + { + log_error("Incorrect error returned from reading write-only file\n"); + return(-1); + } + + file_close(f); + file_close(dir); + } + //-------------------------------------------------------------------------------------- + // base dir with read/write access + //-------------------------------------------------------------------------------------- + { + file_handle dir = file_open(dirPath, FILE_ACCESS_READ|FILE_ACCESS_WRITE, 0); + if(file_last_error(dir)) + { + log_error("Couldn't open ./dir1 with read rights\n"); + return(-1); + } + + // check that we can open file with read access + file_handle f = file_open_at(dir, STR8("./regular.txt"), FILE_ACCESS_READ, 0); + if(file_last_error(f)) + { + log_error("Incorrect check when opening file with read access in dir with read/write access\n"); + return(-1); + } + file_close(f); + + // check that we can open file with write access + f = file_open_at(dir, STR8("./regular.txt"), FILE_ACCESS_WRITE, 0); + if(file_last_error(f)) + { + log_error("Couldn't open file with write access in dir with read/write access\n"); + return(-1); + } + + file_close(f); + file_close(dir); + } + return(0); +} + int main(int argc, char** argv) { mem_arena* arena = mem_scratch(); @@ -255,6 +401,7 @@ int main(int argc, char** argv) if(test_read(arena, path, test_string)) { return(-1); } if(test_stat_size(path, test_string.len)) { return(-1); } if(test_stat_type(arena, dataDir)) { return(-1); } + if(test_rights(arena, dataDir)) { return(-1); } if(test_jail()) { return(-1); } remove("./test.txt");