diff options
| -rw-r--r-- | util/libreboot-utils/include/common.h | 5 | ||||
| -rw-r--r-- | util/libreboot-utils/lib/file.c | 83 | ||||
| -rw-r--r-- | util/libreboot-utils/lib/io.c | 33 | ||||
| -rw-r--r-- | util/libreboot-utils/lib/mkhtemp.c | 68 | ||||
| -rw-r--r-- | util/libreboot-utils/lib/rand.c | 2 | ||||
| -rw-r--r-- | util/libreboot-utils/lib/string.c | 23 |
6 files changed, 107 insertions, 107 deletions
diff --git a/util/libreboot-utils/include/common.h b/util/libreboot-utils/include/common.h index d9f327ee..1932b2da 100644 --- a/util/libreboot-utils/include/common.h +++ b/util/libreboot-utils/include/common.h @@ -346,7 +346,6 @@ int fd_verify_dir_identity(int fd, int is_owner(struct stat *st); int lock_file(int fd, int flags); int same_file(int fd, struct stat *st_old, int check_size); -void xopen(int *fd, const char *path, int flags, struct stat *st); /* Read GbE file and verify checksums */ @@ -490,6 +489,7 @@ int try_err(int loop_err, int errval); */ void usage(void); +int set_errno(int saved_errno, int fallback); void err_exit(int nvm_errval, const char *msg, ...); func_t errhook(func_t ptr); /* hook function for cleanup on err */ const char *getnvmprogname(void); @@ -545,7 +545,8 @@ int fs_rename_at(int olddirfd, const char *old, int newdirfd, const char *new); int fs_open(const char *path, int flags); void free_and_set_null(char **buf); -void open_on_eintr(char *path, int *fd, int flags, mode_t mode); +void open_on_eintr(const char *path, int *fd, int flags, mode_t mode, + struct stat *st); struct filesystem *rootfs(void); int fs_resolve_at(int dirfd, const char *path, int flags); int fs_next_component(const char **p, diff --git a/util/libreboot-utils/lib/file.c b/util/libreboot-utils/lib/file.c index 5d656e0e..3620f425 100644 --- a/util/libreboot-utils/lib/file.c +++ b/util/libreboot-utils/lib/file.c @@ -62,27 +62,7 @@ same_file(int fd, struct stat *st_old, return 0; err_same_file: - - if (errno == saved_errno) - errno = ESTALE; - - return -1; -} - -void -xopen(int *fd_ptr, const char *path, int flags, struct stat *st) -{ - if ((*fd_ptr = open(path, flags)) < 0) - err_exit(errno, "%s", path); - - if (fstat(*fd_ptr, st) < 0) - err_exit(errno, "%s: stat", path); - - if (!S_ISREG(st->st_mode)) - err_exit(errno, "%s: not a regular file", path); - - if (lseek_on_eintr(*fd_ptr, 0, SEEK_CUR, 1, 1) == (off_t)-1) - err_exit(errno, "%s: file not seekable", path); + return set_errno(saved_errno, ESTALE); } int @@ -155,13 +135,11 @@ fsync_dir(const char *path) err_fsync_dir: - if (errno == saved_errno) - errno = EIO; free_and_set_null(&dirbuf); close_on_eintr(&dirfd); - return -1; + return set_errno(saved_errno, EIO); } /* rw_file_exact() - Read perfectly or die @@ -264,10 +242,7 @@ rw_file_exact(int fd, unsigned char *mem, size_t nrw, err_rw_file_exact: - if (errno == saved_errno) - errno = EIO; - - return -1; + return set_errno(saved_errno, EIO); } /* prw() - portable read-write with more @@ -446,11 +421,7 @@ real_pread_pwrite: #endif err_prw: - - if (errno == saved_errno) - errno = EIO; - - return -1; + return set_errno(saved_errno, EIO); } int @@ -472,10 +443,7 @@ io_args(int fd, void *mem, size_t nrw, return 0; err_io_args: - if (errno == saved_errno) - errno = EINVAL; - - return -1; + return set_errno(saved_errno, EINVAL); } int @@ -493,10 +461,7 @@ check_file(int fd, struct stat *st) return 0; err_is_file: - if (errno == saved_errno) - errno = EINVAL; - - return -1; + return set_errno(saved_errno, EINVAL); } /* POSIX can say whatever it wants. @@ -518,10 +483,7 @@ rw_over_nrw(ssize_t r, size_t nrw) return r; err_rw_over_nrw: - if (errno == saved_errno) - errno = EIO; - - return -1; + return set_errno(saved_errno, EIO); } off_t @@ -590,8 +552,9 @@ free_and_set_null(char **buf) } void -open_on_eintr(char *path, - int *fd, int flags, mode_t mode) +open_on_eintr(const char *path, + int *fd, int flags, mode_t mode, + struct stat *st) { int r = -1; int saved_errno = errno; @@ -616,6 +579,17 @@ open_on_eintr(char *path, *fd = r; + if (st != NULL) { + if (fstat(*fd, st) < 0) + err_exit(errno, "%s: stat", path); + + if (!S_ISREG(st->st_mode)) + err_exit(errno, "%s: not a regular file", path); + } + + if (lseek_on_eintr(*fd, 0, SEEK_CUR, 1, 1) == (off_t)-1) + err_exit(errno, "%s: file not seekable", path); + errno = saved_errno; } @@ -705,7 +679,7 @@ rootfs(void) global_fs.rootfd = -1; open_on_eintr("/", &global_fs.rootfd, - O_RDONLY | O_DIRECTORY | O_CLOEXEC, 0400); + O_RDONLY | O_DIRECTORY | O_CLOEXEC, 0400, NULL); if (global_fs.rootfd < 0) return NULL; @@ -717,6 +691,10 @@ rootfs(void) } /* filesystem sandboxing in userspace + * TODO: + missing length bound check. + potential CPU DoS on very long paths, spammed repeatedly. + perhaps cap at PATH_LEN? */ int fs_resolve_at(int dirfd, const char *path, int flags) @@ -780,6 +758,15 @@ err: return -1; } +/* NOTE: + rejects . and .. but not empty strings + after normalisation. edge case: + ////// + + normalised implicitly, but might be good + to add a defensive check regardless. code + probably not exploitable in current state. + */ int fs_next_component(const char **p, char *name, size_t namesz) diff --git a/util/libreboot-utils/lib/io.c b/util/libreboot-utils/lib/io.c index f69954c8..eac6073e 100644 --- a/util/libreboot-utils/lib/io.c +++ b/util/libreboot-utils/lib/io.c @@ -4,6 +4,10 @@ * I/O functions specific to nvmutil. */ +/* TODO: local tmpfiles not being deleted + when flags==O_RDONLY e.g. dump command + */ + #include <sys/types.h> #include <sys/stat.h> @@ -27,9 +31,12 @@ open_gbe_file(void) int _flags; - xopen(&f->gbe_fd, f->fname, - cmd->flags | O_BINARY | - O_NOFOLLOW | O_CLOEXEC | O_NOCTTY, &f->gbe_st); + f->gbe_fd = -1; + + open_on_eintr(f->fname, &f->gbe_fd, + O_NOFOLLOW | O_CLOEXEC | O_NOCTTY, + ((cmd->flags & O_ACCMODE) == O_RDONLY) ? 0400 : 0600, + &f->gbe_st); if (f->gbe_st.st_nlink > 1) err_exit(EINVAL, @@ -62,8 +69,11 @@ open_gbe_file(void) err_exit(EINVAL, "File size must be 8KB, 16KB or 128KB"); } +/* currently fails (EBADF), locks are advisory anyway: */ +/* if (lock_file(f->gbe_fd, cmd->flags) == -1) err_exit(errno, "%s: can't lock", f->fname); +*/ } void @@ -439,10 +449,8 @@ gbe_mv(void) tmp_gbe_bin_exists = 0; ret_gbe_mv: - if (f->gbe_fd > -1) { close_on_eintr(&f->gbe_fd); - f->gbe_fd = -1; if (fsync_dir(f->fname) < 0) { f->io_err_gbe_bin = 1; @@ -462,17 +470,12 @@ ret_gbe_mv: tmp_gbe_bin_exists = 0; } - if (rval < 0) { - /* if nothing set errno, - * we assume EIO, or we - * use what was set - */ - if (errno == saved_errno) - errno = EIO; - } else { - errno = saved_errno; - } + if (rval >= 0) + goto out; + return set_errno(saved_errno, EIO); +out: + errno = saved_errno; return rval; } diff --git a/util/libreboot-utils/lib/mkhtemp.c b/util/libreboot-utils/lib/mkhtemp.c index 2726cb02..61479b25 100644 --- a/util/libreboot-utils/lib/mkhtemp.c +++ b/util/libreboot-utils/lib/mkhtemp.c @@ -206,8 +206,8 @@ env_tmpdir(int bypass_all_sticky_checks, char **tmpdir, int allow_noworld_unsticky; int saved_errno = errno; - char tmp[] = "/tmp"; - char vartmp[] = "/var/tmp"; + static const char tmp[] = "/tmp"; + static const char vartmp[] = "/var/tmp"; /* tmpdir is a user override, if set */ if (override_tmpdir == NULL) @@ -238,26 +238,26 @@ env_tmpdir(int bypass_all_sticky_checks, char **tmpdir, allow_noworld_unsticky = 0; - if (world_writeable_and_sticky("/tmp", + if (world_writeable_and_sticky(tmp, allow_noworld_unsticky, bypass_all_sticky_checks)) { if (tmpdir != NULL) - *tmpdir = tmp; + *tmpdir = (char *)tmp; errno = saved_errno; - return "/tmp"; + return (char *)tmp; } - if (world_writeable_and_sticky("/var/tmp", + if (world_writeable_and_sticky(vartmp, allow_noworld_unsticky, bypass_all_sticky_checks)) { if (tmpdir != NULL) - *tmpdir = vartmp; + *tmpdir = (char *)vartmp; errno = saved_errno; - return "/var/tmp"; + return (char *)vartmp; } return NULL; @@ -295,11 +295,7 @@ tmpdir_policy(const char *path, return 0; err_tmpdir_policy: - - if (errno == saved_errno) - errno = EIO; - - return -1; + return set_errno(saved_errno, EIO); } int @@ -331,11 +327,11 @@ same_dir(const char *a, const char *b) if (rval_scmp == 0) goto success_same_dir; - fd_a = fs_open(a, O_RDONLY | O_DIRECTORY); + fd_a = fs_open(a, O_RDONLY | O_DIRECTORY | O_NOFOLLOW); if (fd_a < 0) goto err_same_dir; - fd_b = fs_open(b, O_RDONLY | O_DIRECTORY); + fd_b = fs_open(b, O_RDONLY | O_DIRECTORY | O_NOFOLLOW); if (fd_b < 0) goto err_same_dir; @@ -377,10 +373,7 @@ err_same_dir: close_on_eintr(&fd_a); close_on_eintr(&fd_b); - if (errno == saved_errno) - errno = EIO; - - return -1; + return set_errno(saved_errno, EIO); } /* bypass_all_sticky_checks: if set, @@ -467,19 +460,15 @@ world_writeable_and_sticky( goto sticky_hell; /* heaven visa denied */ sticky_heaven: - close_on_eintr(&dirfd); errno = saved_errno; return 1; sticky_hell: - - if (errno == saved_errno) - errno = EPERM; - close_on_eintr(&dirfd); + (void) set_errno(saved_errno, EPERM); return 0; } @@ -524,6 +513,17 @@ sticky_hell: * file ownership under hardened mode * (only lets you touch your own files/dirs) */ +/* + TODO: + some variables e.g. template vs suffix, + assumes they match. + we should test this explicitly, + but the way this is called is + currently safe - this would however + be nice for future library use + by outside projects. + this whole code needs to be reorganised +*/ int mkhtemp(int *fd, struct stat *st, @@ -832,6 +832,12 @@ err: } #endif +/* TODO: potential infinite loop under entropy failure. + * e.g. keeps returning low quality RNG, or atacker + * has control (DoS attack potential). + * possible solution: add a timeout (and abort if + * the timeout is reached) + */ int mkhtemp_fill_random(char *p, size_t xc) { @@ -911,6 +917,8 @@ int secure_file(int *fd, if (lock_file(*fd, flags) == -1) goto err_demons; + /* TODO: why would this be NULL? audit + * to find out. we should always verify! */ if (expected != NULL) if (fd_verify_identity(*fd, expected, &st_now) < 0) goto err_demons; @@ -923,11 +931,7 @@ int secure_file(int *fd, return 0; err_demons: - - if (errno == saved_errno) - errno = EIO; - - return -1; + return set_errno(saved_errno, EIO); } int @@ -1037,9 +1041,5 @@ lock_file(int fd, int flags) return 0; err_lock_file: - - if (errno == saved_errno) - errno = EIO; - - return -1; + return set_errno(saved_errno, EIO); } diff --git a/util/libreboot-utils/lib/rand.c b/util/libreboot-utils/lib/rand.c index 030ca5ec..9edc8a5b 100644 --- a/util/libreboot-utils/lib/rand.c +++ b/util/libreboot-utils/lib/rand.c @@ -142,7 +142,7 @@ rset(void *buf, size_t n) #if defined(USE_URANDOM) && \ ((USE_URANDOM) > 0) int fd = -1; - open_on_eintr("/dev/urandom", &fd, O_RDONLY, 0400); + open_on_eintr("/dev/urandom", &fd, O_RDONLY, 0400, NULL); retry_rand: if ((rc = read(fd, (unsigned char *)buf + off, n - off)) < 0) { #elif defined(__linux__) diff --git a/util/libreboot-utils/lib/string.c b/util/libreboot-utils/lib/string.c index 9e38a9e9..76141c58 100644 --- a/util/libreboot-utils/lib/string.c +++ b/util/libreboot-utils/lib/string.c @@ -201,14 +201,10 @@ scatn(ssize_t sc, const char **sv, errno = saved_errno; return 0; err: - if (ct != NULL) - free(ct); - if (size != NULL) - free(size); - if (errno == saved_errno) - errno = EFAULT; + free_and_set_null(&ct); + free_and_set_null((char **)&size); - return -1; + return set_errno(saved_errno, EFAULT); } /* strict strcat */ @@ -285,6 +281,19 @@ err: return -1; } +/* on functions that return with errno, + * i sometimes have a default fallback, + * which is set if errno wasn't changed, + * under error condition. + */ +int +set_errno(int saved_errno, int fallback) +{ + if (errno == saved_errno) + errno = fallback; + return -1; +} + /* the one for nvmutil state is in state.c */ /* this one just exits */ void |