From ca92e7831abe1ef26b7680c66f8ba06ddc68f076 Mon Sep 17 00:00:00 2001 From: Leah Rowe Date: Sun, 22 Mar 2026 21:08:18 +0000 Subject: cleanup WIP Signed-off-by: Leah Rowe --- util/nvmutil/include/common.h | 13 ++- util/nvmutil/lib/file.c | 213 +++++++++++++++++------------------------- util/nvmutil/lib/io.c | 3 +- util/nvmutil/lib/state.c | 3 +- 4 files changed, 98 insertions(+), 134 deletions(-) diff --git a/util/nvmutil/include/common.h b/util/nvmutil/include/common.h index b47e5f4b..e856664f 100644 --- a/util/nvmutil/include/common.h +++ b/util/nvmutil/include/common.h @@ -478,8 +478,7 @@ const char *getnvmprogname(void); /* libc hardening */ -char *new_tmpfile(int *fd); -char *new_tmplate(int *fd); +int new_tmpfile(int *fd, char **path); static int mkhtemp_try_create(int dirfd, struct stat *st_dir_initial, char *fname_copy, @@ -497,9 +496,13 @@ int same_dir(const char *a, const char *b); int tmpdir_policy(const char *path, int *allow_noworld_unsticky); char *env_tmpdir(int always_sticky); -int secure_file(int *fd, struct stat *st, - int bad_flags, int check_seek, - int do_lock, mode_t mode); +int secure_file(int *fd, + struct stat *st, + struct stat *expected, + int bad_flags, + int check_seek, + int do_lock, + mode_t mode); int close_on_eintr(int fd); int fsync_on_eintr(int fd); int fs_rename_at(int olddirfd, const char *old, diff --git a/util/nvmutil/lib/file.c b/util/nvmutil/lib/file.c index ee849b73..50b48065 100644 --- a/util/nvmutil/lib/file.c +++ b/util/nvmutil/lib/file.c @@ -30,6 +30,9 @@ /* check that a file changed */ +#define MKHTEMP_RETRY_MAX 512 +#define MKHTEMP_SPIN_THRESHOLD 32 + int same_file(int fd, struct stat *st_old, int check_size) @@ -234,8 +237,10 @@ err_fsync_dir: /* hardened tmpfile creation */ -char * -new_tmpfile(int *fd) +/* returns opened fd, sets fd and *path at pointers *fd and *path */ +/* also sets external stat */ +int +new_tmpfile(int *fd, char **path) { /* TODO: * directory support (currently only files) @@ -253,7 +258,6 @@ new_tmpfile(int *fd) char *tmpdir = NULL; size_t dirlen; - size_t pathlen; size_t destlen; size_t baselen; char *dest = NULL; /* final path */ @@ -261,43 +265,26 @@ new_tmpfile(int *fd) int dirfd = -1; const char *fname = NULL; - /* open the directory early, - * check via fd throughout, - * for directory replacement - * attack mitigation - */ struct stat st_dir_initial; char *dir = NULL; char *base = NULL; - if (fd == NULL) { - + if (path == NULL || fd == NULL) { + errno = EFAULT; + goto err_new_tmpfile; + } else if (*path == NULL) { errno = EFAULT; goto err_new_tmpfile; } - /* block operating on - * someone elses file - */ - if (*fd >= 0) { - + if (*fd >= 0) { /* file already opend */ errno = EEXIST; goto err_new_tmpfile; - - /* they might - * want it later! - */ } - /* but now it's *mine*: - */ *fd = -1; - /* base dir e.g. /tmp - */ - #if defined(PERMIT_NON_STICKY_ALWAYS) && \ ((PERMIT_NON_STICKY_ALWAYS) > 0) - tmpdir = env_tmpdir(PERMIT_NON_STICKY_ALWAYS); #else tmpdir = env_tmpdir(0); @@ -305,22 +292,13 @@ new_tmpfile(int *fd) if (tmpdir == NULL) goto err_new_tmpfile; - if (slen(tmpdir, maxlen, &dirlen) < 0) goto err_new_tmpfile; - pathlen = 0; - - /* now we want the base dir, - * with the file appended, - * and the XXXXXXXXXX suffix - */ - /* using sizeof (not slen) adds an extra byte, * useful because we either want '.' or '/' */ - destlen = dirlen + pathlen + sizeof(suffix); - + destlen = dirlen + sizeof(suffix); if (destlen > maxlen - 1) { /* -1 for NULL */ errno = EOVERFLOW; @@ -334,34 +312,22 @@ new_tmpfile(int *fd) goto err_new_tmpfile; } - /* As you see above, we only allow - * either a base tmpdir and suffix, - * or a user-supplied file and we - * suffix that. - */ - if (dirlen > 0 && pathlen > 0) { - - errno = EINVAL; - goto err_new_tmpfile; /* pre-emptive fix */ - } - - *(dest + dirlen) = '/'; memcpy(dest, tmpdir, dirlen); - memcpy(dest + dirlen + 1, suffix, - sizeof(suffix) - 1); + *(dest + dirlen) = '/'; + memcpy(dest + dirlen + 1, suffix, sizeof(suffix) - 1); + *(dest + destlen) = '\0'; + + /* new tmpfile name */ + fname = dest + dirlen + 1; dirfd = fs_open(tmpdir, O_RDONLY | O_DIRECTORY); if (dirfd < 0) goto err_new_tmpfile; - fname = dest + dirlen + 1; - if (fstat(dirfd, &st_dir_initial) < 0) goto err_new_tmpfile; - *(dest + destlen) = '\0'; - *fd = mkhtemp(fd, &st, dest, dirfd, fname, &st_dir_initial); if (*fd < 0) goto err_new_tmpfile; @@ -370,14 +336,16 @@ new_tmpfile(int *fd) (void) close_on_eintr(dirfd); dirfd = -1; } - if (dir != NULL) { free(dir); dir = NULL; } errno = saved_errno; - return dest; + + *path = dest; + + return 0; err_new_tmpfile: @@ -410,7 +378,7 @@ err_new_tmpfile: errno = saved_errno; - return NULL; + return -1; } /* hardened TMPDIR parsing @@ -712,7 +680,6 @@ mkhtemp(int *fd, size_t xc = 0; size_t fname_len = 0; - char *template_copy = NULL; char *fname_copy = NULL; char *p; @@ -726,6 +693,7 @@ mkhtemp(int *fd, #else size_t max_len = 4096; #endif + int r; if (fd == NULL || template == NULL || @@ -793,29 +761,21 @@ mkhtemp(int *fd, return -1; } - template_copy = malloc(len + 1); - if (template_copy == NULL) { - errno = ENOMEM; - goto err; - } - fname_copy = malloc(fname_len + 1); if (fname_copy == NULL) { errno = ENOMEM; goto err; } - memcpy(template_copy, template, len + 1); - + /* fname_copy = suffix region only; p points to trailing XXXXXX */ memcpy(fname_copy, - template_copy + len - fname_len, + template + len - fname_len, fname_len + 1); - p = fname_copy + fname_len - xc; - for (retries = 0; retries < 300; retries++) { + for (retries = 0; retries < MKHTEMP_RETRY_MAX; retries++) { - int r = mkhtemp_try_create( + r = mkhtemp_try_create( dirfd, st_dir_initial, fname_copy, @@ -824,17 +784,16 @@ mkhtemp(int *fd, fd, st); - if (r == 1) { - /* success: copy final name back */ - memcpy(template + len - fname_len, - fname_copy, fname_len); + /* DoS mitigation; add jitter delay (0–1023 µs) */ + if (r != 1 && retries >= MKHTEMP_SPIN_THRESHOLD) + usleep((useconds_t)rlong() & 0x3FF); - errno = saved_errno; - goto success; - } + /* success: copy final name back */ + memcpy(template + len - fname_len, + fname_copy, fname_len); - if (r == -1) - goto err; + errno = saved_errno; + goto success; } errno = EEXIST; @@ -846,8 +805,6 @@ err: } success: - if (template_copy != NULL) - free(template_copy); if (fname_copy != NULL) free(fname_copy); @@ -865,6 +822,9 @@ mkhtemp_try_create(int dirfd, struct stat *st) { struct stat st_dir_now; + struct stat st_open; + int saved_errno = errno; + int close_errno; if (mkhtemp_fill_random(p, xc) < 0) return -1; @@ -885,15 +845,35 @@ mkhtemp_try_create(int dirfd, if (*fd >= 0) { - if (secure_file(fd, st, O_APPEND, 1, 1, 0600) < 0) + if (fstat(*fd, &st_open) < 0) + return -1; + + if (fstat(dirfd, &st_dir_now) < 0) + return -1; + + if (st_dir_now.st_dev != st_dir_initial->st_dev || + st_dir_now.st_ino != st_dir_initial->st_ino) { + + close_errno = errno; + (void) close_on_eintr(*fd); + errno = close_errno; + + errno = ESTALE; return -1; + } + if (secure_file(fd, st, &st_open, O_APPEND, 1, 1, 0600) < 0) + return -1; + + errno = saved_errno; return 1; /* success */ } if (errno == EEXIST || errno == EINTR || - errno == EAGAIN) + errno == EAGAIN || + errno == EWOULDBLOCK || + errno == ETXTBSY) return 0; /* retry */ return -1; /* fatal */ @@ -964,31 +944,16 @@ err_mkhtemp_fill_random: return -1; } -/* why doesn't literally - every libc have this? - - TODO: consider set_flags, - complementing bad_flags, - for setting new flags; - with another option to - set it before the bad_flags - check, or after it (because - some callers may be setting - flags given to them with - theirs OR'd in, yet still - want to filter bad flags, - whereas others may not want - to modify flags on a file - that already contains - specific flags) - */ -int -secure_file(int *fd, - struct stat *st, int bad_flags, - int check_seek, int do_lock, +int secure_file(int *fd, + struct stat *st, + struct stat *expected, + int bad_flags, + int check_seek, + int do_lock, mode_t mode) { int flags; + struct stat st_now; int saved_errno = errno; if (fd == NULL) { @@ -1012,16 +977,8 @@ secure_file(int *fd, if (bad_flags > 0) { - /* - * For example: - * O_APPEND would permit offsets - * to be ignored, which breaks - * positional read/write - * - * You might provide that as - * the mask to this function, - * before doing positional i/o - */ + /* e.g. O_APPEND breaks pwrite/pread + * by allowing offsets to be ignored */ if (flags & bad_flags) { errno = EPERM; goto err_secure_file; @@ -1031,12 +988,20 @@ secure_file(int *fd, if (fstat(*fd, st) == -1) goto err_secure_file; - /* - * Extremely defensive - * likely pointless checks - */ + /* verify inode/device stability */ + if (expected != NULL) { + + if (st_now.st_dev != expected->st_dev || + st_now.st_ino != expected->st_ino) { + + errno = ESTALE; + goto err_secure_file; + } + } + + /* now copy to caller */ + *st = st_now; - /* check if it's a file */ if (!S_ISREG(st->st_mode)) { errno = EBADF; goto err_secure_file; @@ -1044,23 +1009,17 @@ secure_file(int *fd, if (check_seek) { - /* check if it's seekable - */ + /* check if it's seekable */ if (lseek(*fd, 0, SEEK_CUR) == (off_t)-1) goto err_secure_file; } - /* tmpfile re-linked, or - unlinked, while opened - */ + /* tmpfile re/un linked while open */ if (st->st_nlink != 1) { errno = ELOOP; goto err_secure_file; } - /* block if we don't own the file - * (exception made for root) - */ if (st->st_uid != geteuid() && /* someone else's file */ geteuid() != 0) { /* override for root */ diff --git a/util/nvmutil/lib/io.c b/util/nvmutil/lib/io.c index e95be73c..94bde87e 100644 --- a/util/nvmutil/lib/io.c +++ b/util/nvmutil/lib/io.c @@ -473,7 +473,8 @@ gbe_mv(void) /* create replacement temp in target directory */ - dest_tmp = new_tmpfile(&dest_fd); + if (new_tmpfile(&dest_fd, &f->fname) < 1) + goto ret_gbe_mv; if (dest_tmp == NULL) goto ret_gbe_mv; diff --git a/util/nvmutil/lib/state.c b/util/nvmutil/lib/state.c index 836c6bc7..19d5cd8c 100644 --- a/util/nvmutil/lib/state.c +++ b/util/nvmutil/lib/state.c @@ -112,7 +112,8 @@ xstart(int argc, char *argv[]) us.argv0 = argv[0]; us.f.fname = argv[1]; - us.f.tname = new_tmpfile(&us.f.tmp_fd); + if (new_tmpfile(&us.f.tmp_fd, &us.f.tname) < 0) + err_no_cleanup(errno, "xstart: cannot create tmpfile"); /* parse user command */ /* TODO: CHECK ACCESSES VIA xstatus() */ -- cgit v1.2.1