From bfc81fd062b720b3c227f48405d3d3adc4387ac2 Mon Sep 17 00:00:00 2001 From: Martin Fouilleul Date: Thu, 11 May 2023 18:09:26 +0200 Subject: [PATCH] [io] check that file path doesn't escape app's local data folder when opening files --- samples/pong/build.sh | 2 +- samples/pong/dir1/test_read.txt | 1 + samples/pong/src/main.c | 4 +- scripts/mkapp.py | 30 ++- src/io_common.h | 29 ++- src/io_impl.c | 378 +++++++++++++++++++++++++------- src/main.c | 2 +- 7 files changed, 345 insertions(+), 101 deletions(-) create mode 100644 samples/pong/dir1/test_read.txt diff --git a/samples/pong/build.sh b/samples/pong/build.sh index 74fe697..f5fb063 100755 --- a/samples/pong/build.sh +++ b/samples/pong/build.sh @@ -12,4 +12,4 @@ wasmFlags="--target=wasm32 \ /usr/local/opt/llvm/bin/clang $wasmFlags -o ./module.wasm ../../sdk/orca.c src/main.c -python3 ../../scripts/mkapp.py --orca-dir ../.. --name Pong --icon icon.png module.wasm +python3 ../../scripts/mkapp.py --orca-dir ../.. --name Pong --icon icon.png --data-dir dir1 module.wasm diff --git a/samples/pong/dir1/test_read.txt b/samples/pong/dir1/test_read.txt new file mode 100644 index 0000000..5549f68 --- /dev/null +++ b/samples/pong/dir1/test_read.txt @@ -0,0 +1 @@ +Hello, ressource file diff --git a/samples/pong/src/main.c b/samples/pong/src/main.c index 0463c46..65ec5ff 100644 --- a/samples/pong/src/main.c +++ b/samples/pong/src/main.c @@ -41,13 +41,13 @@ void OnInit(void) surface = mg_surface_main(); canvas = mg_canvas_create(); - file_handle file = file_open(STR8("test_file.txt") , IO_OPEN_CREATE | IO_OPEN_WRITE); + file_handle file = file_open(STR8("/test_write.txt") , IO_OPEN_CREATE | IO_OPEN_WRITE); str8 string = STR8("Hello, file!\n"); file_write(file, string.len, string.ptr); file_close(file); - file = file_open(STR8("test_file.txt") , IO_OPEN_READ); + file = file_open(STR8("/dir1/test_read.txt") , IO_OPEN_READ); u64 size = file_size(file); char* buffer = mem_arena_alloc(mem_scratch(), size); file_read(file, size, buffer); diff --git a/scripts/mkapp.py b/scripts/mkapp.py index 6dcb3f3..b4220e8 100644 --- a/scripts/mkapp.py +++ b/scripts/mkapp.py @@ -18,10 +18,10 @@ from argparse import ArgumentParser #---------------------------------------------------------------------------------------------- parser = ArgumentParser(prog='mkapp') -parser.add_argument("-r", "--res-file", nargs='+', action='append', dest='res_files') -parser.add_argument("-R", "--res-dir", nargs='+', action='append', dest='res_dirs') +parser.add_argument("-d", "--data-file", action='append', dest='data_files') +parser.add_argument("-D", "--data-dir", action='append', dest='data_dirs') parser.add_argument("-i", "--icon") -parser.add_argument("-D", "--out-dir", default=os.getcwd()) +parser.add_argument("-C", "--out-dir", default=os.getcwd()) parser.add_argument("-n", "--name", default='out') parser.add_argument("-O", "--orca-dir", default='.') parser.add_argument("--version", default='0.0.0') @@ -38,8 +38,9 @@ bundle_path = args.out_dir + '/' + bundle_name contents_dir = bundle_path + '/Contents' exe_dir = contents_dir + '/MacOS' res_dir = contents_dir + '/resources' -wasm_dir = contents_dir + '/wasm' -data_dir = contents_dir + '/data' +guest_dir = contents_dir + '/app' +wasm_dir = guest_dir + '/wasm' +data_dir = guest_dir + '/data' if os.path.exists(bundle_path): shutil.rmtree(bundle_path) @@ -47,6 +48,7 @@ os.mkdir(bundle_path) os.mkdir(contents_dir) os.mkdir(exe_dir) os.mkdir(res_dir) +os.mkdir(guest_dir) os.mkdir(wasm_dir) os.mkdir(data_dir) @@ -66,23 +68,27 @@ shutil.copy(egl_lib, exe_dir) shutil.copy(renderer_lib, exe_dir) #----------------------------------------------------------- -#NOTE: copy wasm module and resources +#NOTE: copy wasm module and data #----------------------------------------------------------- shutil.copy(args.module, wasm_dir + '/module.wasm') -if args.res_files != None: - for res in args.res_files: - shutil.copy(res, res_dir) +if args.data_files != None: + for data in args.data_files: + shutil.copy(data, data_dir) -if args.res_dirs != None: - for res in args.res_dirs: - shutil.copytree(res, res_dir) +if args.data_dirs != None: + for data in args.data_dirs: + shutil.copytree(data, data_dir + '/' + os.path.basename(data), dirs_exist_ok=True) +#----------------------------------------------------------- +#NOTE: copy runtime resources +#----------------------------------------------------------- # default fonts shutil.copy(args.orca_dir + '/resources/OpenSansLatinSubset.ttf', res_dir) shutil.copy(args.orca_dir + '/resources/Menlo.ttf', res_dir) shutil.copy(args.orca_dir + '/resources/Menlo Bold.ttf', res_dir) shutil.copy(args.orca_dir + '/resources/Menlo Italics.ttf', res_dir) + #----------------------------------------------------------- #NOTE make icon #----------------------------------------------------------- diff --git a/src/io_common.h b/src/io_common.h index 3187b1c..f8925da 100644 --- a/src/io_common.h +++ b/src/io_common.h @@ -70,20 +70,27 @@ typedef i32 io_error; enum { IO_OK = 0, IO_ERR_UNKNOWN, - IO_ERR_OP, // unsupported operation - IO_ERR_HANDLE, // invalid handle - IO_ERR_PREV, // previously had a fatal error (last error stored on handle) - IO_ERR_ARG, // invalid argument or argument combination - IO_ERR_PERM, // access denied - IO_ERR_SPACE, // no space left - IO_ERR_NO_FILE, // file does not exist - IO_ERR_EXISTS, // file already exists - IO_ERR_MAX_FILES, // max open files reached + IO_ERR_OP, // unsupported operation + IO_ERR_HANDLE, // invalid handle + IO_ERR_PREV, // previously had a fatal error (last error stored on handle) + IO_ERR_ARG, // invalid argument or argument combination + IO_ERR_PERM, // access denied + IO_ERR_SPACE, // no space left + IO_ERR_NO_ENTRY, // file or directory does not exist + IO_ERR_EXISTS, // file already exists + IO_ERR_NOT_DIR, // path element is not a directory + IO_ERR_DIR, // attempted to write directory + IO_ERR_MAX_FILES, // max open files reached + IO_ERR_MAX_LINKS, // too many symbolic links in path IO_ERR_PATH_LENGTH, // path too long IO_ERR_FILE_SIZE, // file too big IO_ERR_OVERFLOW, // offset too big - IO_ERR_NOT_READY, // no data ready to be read/written - IO_ERR_MEM, // failed to allocate memory + IO_ERR_NOT_READY, // no data ready to be read/written + IO_ERR_MEM, // failed to allocate memory + IO_ERR_INTERRUPT, // operation interrupted by a signal + IO_ERR_PHYSICAL, // physical IO error + IO_ERR_NO_DEVICE, // device not found + IO_ERR_WALKOUT, // attempted to walk out of root directory //... }; diff --git a/src/io_impl.c b/src/io_impl.c index 302fcfb..01d08a5 100644 --- a/src/io_impl.c +++ b/src/io_impl.c @@ -9,6 +9,7 @@ #include #include #include +#include #include"io_common.h" //TODO: - file_handle to FILE* association @@ -84,6 +85,278 @@ file_slot* file_slot_from_handle(file_table* table, file_handle handle) return(slot); } +io_error io_convert_errno(int e) +{ + io_error error; + switch(e) + { + case EPERM: + case EACCES: + case EROFS: + error = IO_ERR_PERM; + break; + + case ENOENT: + error = IO_ERR_NO_ENTRY; + break; + + case EINTR: + error = IO_ERR_INTERRUPT; + break; + + case EIO: + error = IO_ERR_PHYSICAL; + break; + + case ENXIO: + error = IO_ERR_NO_DEVICE; + break; + + case EBADF: + // this should only happen when user tries to write/read to a file handle + // opened with readonly/writeonly access + error = IO_ERR_PERM; + break; + + case ENOMEM: + error = IO_ERR_MEM; + break; + + case EFAULT: + case EINVAL: + case EDOM: + error = IO_ERR_ARG; + break; + + case EBUSY: + case EAGAIN: + error = IO_ERR_NOT_READY; + break; + + case EEXIST: + error = IO_ERR_EXISTS; + break; + + case ENOTDIR: + error = IO_ERR_NOT_DIR; + break; + + case EISDIR: + error = IO_ERR_DIR; + break; + + case ENFILE: + case EMFILE: + error = IO_ERR_MAX_FILES; + break; + + case EFBIG: + error = IO_ERR_FILE_SIZE; + break; + + case ENOSPC: + case EDQUOT: + error = IO_ERR_SPACE; + break; + + case ELOOP: + error = IO_ERR_MAX_LINKS; + break; + + case ENAMETOOLONG: + error = IO_ERR_PATH_LENGTH; + break; + + case EOVERFLOW: + error = IO_ERR_OVERFLOW; + break; + + default: + error = IO_ERR_UNKNOWN; + break; + } + return(error); +} + +typedef struct io_path_walk_result +{ + io_error error; + int fd; + +} io_path_walk_result; + +io_path_walk_result io_path_walk(str8 path) +{ + io_path_walk_result result = {.fd = -1}; + + mem_arena* scratch = mem_scratch(); + mem_arena_marker mark = mem_arena_mark(scratch); + + str8_list sep = {0}; + str8_list_push(scratch, &sep, STR8("/")); + str8_list pathElements = str8_split(scratch, path, sep); + + str8 execPath = mp_path_directory(mp_app_get_executable_path(scratch)); + str8_list list = {0}; + str8_list_push(scratch, &list, execPath); + str8_list_push(scratch, &list, STR8("../app/data")); + str8 dataPath = str8_list_join(scratch, list); + + result.fd = open(str8_to_cstring(scratch, dataPath), O_RDONLY); + if(result.fd < 0) + { + result.error = io_convert_errno(errno); + goto error; + } + struct stat dirStat; + if(fstat(result.fd, &dirStat)) + { + result.error = io_convert_errno(errno); + goto error; + } + ino_t baseInode = dirStat.st_ino; + ino_t currentInode = baseInode; + + for_list(&pathElements.list, elt, str8_elt, listElt) + { + str8 name = elt->string; + + if(!str8_cmp(name, STR8("."))) + { + //NOTE: skip + continue; + } + else if(!str8_cmp(name, STR8(".."))) + { + //NOTE: check that we don't escape 'root' dir + if(currentInode == baseInode) + { + result.error = IO_ERR_WALKOUT; + goto error; + } + else + { + // open that dir and continue + int nextFd = openat(result.fd, "..", O_RDONLY); + if(!nextFd) + { + result.error = io_convert_errno(errno); + goto error; + } + else + { + close(result.fd); + result.fd = nextFd; + if(fstat(result.fd, &dirStat)) + { + result.error = io_convert_errno(errno); + goto error; + } + currentInode = dirStat.st_ino; + } + } + } + else + { + char* nameCStr = str8_to_cstring(scratch, name); + struct stat st; + + if(fstatat(result.fd, nameCStr, &st, AT_SYMLINK_NOFOLLOW)) + { + if(&elt->listElt != list_last(&pathElements.list)) + { + result.error = io_convert_errno(errno); + goto error; + } + else + { + continue; + } + } + + int type = st.st_mode & S_IFMT; + + if(type == S_IFLNK) + { + // symlink, check that it's relative, and insert it in elements + char buff[PATH_MAX+1]; + int r = readlinkat(result.fd, nameCStr, buff, PATH_MAX); + if(r<0) + { + result.error = io_convert_errno(errno); + goto error; + } + else if(r == 0) + { + //NOTE: skip + } + else if(buff[0] == '/') + { + result.error = IO_ERR_WALKOUT; + goto error; + } + else + { + buff[r] = '\0'; + + str8_list linkElements = str8_split(scratch, str8_from_buffer(r, buff), sep); + if(!list_empty(&linkElements.list)) + { + //NOTE: insert linkElements into pathElements after elt + list_elt* tmp = elt->listElt.next; + elt->listElt.next = linkElements.list.first; + linkElements.list.last->next = tmp; + if(!tmp) + { + pathElements.list.last = linkElements.list.last; + } + } + } + } + else if(type == S_IFDIR) + { + // dir, open it and continue + int nextFd = openat(result.fd, nameCStr, O_RDONLY); + if(!nextFd) + { + result.error = io_convert_errno(errno); + goto error; + } + else + { + close(result.fd); + result.fd = nextFd; + currentInode = st.st_ino; + } + } + else if(type == S_IFREG) + { + // regular file, check that we're at the last element + if(&elt->listElt != list_last(&pathElements.list)) + { + result.error = IO_ERR_NOT_DIR; + goto error; + } + } + else + { + result.error = IO_ERR_NOT_DIR; + goto error; + } + } + } + goto end; + + error: + { + close(result.fd); + result.fd = -1; + } + end: + mem_arena_clear_to(scratch, mark); + return(result); +} + io_cmp io_open(io_req* req) { io_cmp cmp = {0}; @@ -129,6 +402,8 @@ io_cmp io_open(io_req* req) flags |= O_CREAT; } + flags |= O_NOFOLLOW; + mode_t mode = S_IRUSR | S_IWUSR | S_IRGRP @@ -136,88 +411,43 @@ io_cmp io_open(io_req* req) | S_IROTH | S_IWOTH; - //NOTE: build path - //////////////////////////////////////////////////////////////////////////////////// - //TODO: canonicalize directory path & check that it's inside local app folder - //////////////////////////////////////////////////////////////////////////////////// - - mem_arena* scratch = mem_scratch(); - mem_arena_marker mark = mem_arena_mark(scratch); - - str8_list list = {0}; - - str8 execPath = mp_app_get_executable_path(scratch); - str8 execDir = mp_path_directory(execPath); - - str8_list_push(scratch, &list, execDir); - str8_list_push(scratch, &list, STR8("/../data/")); - str8_list_push(scratch, &list, str8_from_buffer(req->size, req->buffer)); - - str8 absPath = str8_list_join(scratch, list); - char* absCString = str8_to_cstring(scratch, absPath); - - //NOTE: open - int fd = open(absCString, flags, mode); - - if(fd >= 0) - { - slot->fd = fd; - file_handle handle = file_handle_from_slot(&__globalFileTable, slot); - cmp.result = handle.h; - } - else + //NOTE: walk path (ensuring it's inside app's data directory subtree) + str8 path = str8_from_buffer(req->size, req->buffer); + io_path_walk_result walkRes = io_path_walk(path); + if(walkRes.error != IO_OK) { slot->fd = -1; slot->fatal = true; + slot->error = walkRes.error; + } + else + { + //NOTE: open + mem_arena* scratch = mem_scratch(); + mem_arena_marker mark = mem_arena_mark(scratch); - switch(errno) + str8 name = mp_path_base_name(path); + char* nameCStr = str8_to_cstring(scratch, name); + int fd = openat(walkRes.fd, nameCStr, flags, mode); + close(walkRes.fd); + + mem_arena_clear_to(scratch, mark); + + if(fd >= 0) { - case EACCES: - case EROFS: - slot->error = IO_ERR_PERM; - break; - - case EDQUOT: - case ENOSPC: - slot->error = IO_ERR_SPACE; - break; - - case EEXIST: - slot->error = IO_ERR_EXISTS; - break; - - case EFAULT: - case EINVAL: - case EISDIR: - slot->error = IO_ERR_ARG; - break; - - case EMFILE: - case ENFILE: - slot->error = IO_ERR_MAX_FILES; - break; - - case ENAMETOOLONG: - slot->error = IO_ERR_PATH_LENGTH; - break; - - case ENOENT: - case ENOTDIR: - slot->error = IO_ERR_NO_FILE; - break; - - case EOVERFLOW: - slot->error = IO_ERR_FILE_SIZE; - break; - - default: - slot->error = IO_ERR_UNKNOWN; - break; + slot->fd = fd; + file_handle handle = file_handle_from_slot(&__globalFileTable, slot); + cmp.result = handle.h; + } + else + { + slot->fd = -1; + slot->fatal = true; + slot->error = io_convert_errno(errno); } - cmp.error = slot->error; } - mem_arena_clear_to(scratch, mark); + cmp.error = slot->error; } return(cmp); diff --git a/src/main.c b/src/main.c index 2669f9e..1de0011 100644 --- a/src/main.c +++ b/src/main.c @@ -363,7 +363,7 @@ void* orca_runloop(void* user) //NOTE: loads wasm module const char* bundleNameCString = "module"; - str8 modulePath = mp_app_get_resource_path(mem_scratch(), "../wasm/module.wasm"); + str8 modulePath = mp_app_get_resource_path(mem_scratch(), "../app/wasm/module.wasm"); const char* modulePathCString = str8_to_cstring(mem_scratch(), modulePath); FILE* file = fopen(modulePathCString, "rb");