diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index f113130..ff14b03 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -5,6 +5,38 @@ All notable changes to restoHack will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [1.1.7] - 2026-06-05 + +### Fixed + +- **SECURITY**: Format string vulnerabilities in inventory display + - `pline(xprname(...))` in `hack.invent.c` — user-named objects containing `%` became format strings + - `pline(buf)` in `hack.options.c` — option state buffer passed directly as format string +- **SECURITY**: `free()` on arena-allocated `toptenentry` in `prscore()` (hack.end.c) + - `newttentry()` returns pointers into a static arena; the old `free()` was undefined behavior + - Removed; `reset_topten_arena()` at the top of each caller handles cleanup +- **SECURITY**: `getenv("HOME")` NULL dereference in `child()` (hack.pager.c) + - Crashes in stripped environments (containers, setuid, cron) where HOME is unset + - Now falls back to `/tmp` with a warning to stderr +- **SECURITY**: Missing `setuid(getuid())` privilege drop before `exec` in `child()` + - Only `setgid` was being dropped; a comment at the call site already claimed `setuid` was done + - Added `setuid(getuid())` after `setgid(getgid())` so the pager exec runs unprivileged +- **STABILITY**: Out-of-bounds read in `dowhatis()` (hack.pager.c) + - `ep[-1]` when `ep == NULL` (no newline found by `index()`) caused a null pointer deref + - Guard is now `ep && ep > buf && ep[-1] == ';'` +- **STABILITY**: Three file descriptor leaks in `dorecover()` on early-return paths (hack.save.c) + - Struct size mismatch, unexpected version, and pre-versioned format rejection paths all leaked fd +- **STABILITY**: `check_save_header()` used panic-backed `mread()` for post-magic header fields + - Truncated save files triggered `panic("dungeon collapses")` instead of a graceful rejection + - Replaced `sr_u16`/`sr_u32` calls with a single `read()` that returns 0 on short read; removed now-dead `sr_u16` +- **STABILITY**: `save_pos` typed `long` instead of `off_t` in `dorecover()` (hack.save.c) + - `lseek()` returns `off_t`; assignment to `long` truncates on ILP32 large-file platforms + - Initial `lseek` now also checked for failure +- **STABILITY**: lseek I/O failure and "too old" save format were indistinguishable (hack.save.c) + - Seek failure now branches to its own error path with a distinct message +- **CORRECTNESS**: `resize_pending` signal flag typed `volatile int` instead of `volatile sig_atomic_t` + - Written from SIGWINCH handler; `sig_atomic_t` is required by the C standard for signal safety + ## [1.1.6] 2026-03-15 ### Fixed diff --git a/src/hack.end.c b/src/hack.end.c index ea69eb2..4375163 100644 --- a/src/hack.end.c +++ b/src/hack.end.c @@ -837,7 +837,7 @@ void prscore(int argc, char **argv) { break; } } - free((char *)t1); + /* Modern: Removed free() - t1 is arena-allocated, reset_topten_arena() handles cleanup */ } #ifdef nonsense totchars[totcharct] = 0; diff --git a/src/hack.invent.c b/src/hack.invent.c index 5d454c1..53f8f30 100644 --- a/src/hack.invent.c +++ b/src/hack.invent.c @@ -572,7 +572,7 @@ int max; if (ckfn && !(*ckfn)(otmp)) continue; if (!allflag) { - pline(xprname(otmp, ilet)); + pline("%s", xprname(otmp, ilet)); /* Modern: Fix format string vulnerability */ addtopl(" [nyaq]? "); sym = readchar(); } else @@ -615,7 +615,7 @@ struct obj *obj; void prinv(obj) struct obj *obj; { - pline(xprname(obj, obj_to_let(obj))); + pline("%s", xprname(obj, obj_to_let(obj))); /* Modern: Fix format string vulnerability */ } static char *xprname(struct obj *obj, char let) /* ANSI C conversion from K&R style */ diff --git a/src/hack.main.c b/src/hack.main.c index 55d28ee..f4ba204 100644 --- a/src/hack.main.c +++ b/src/hack.main.c @@ -62,7 +62,7 @@ void hangup(int sig); * dynamic terminal sizing in modern window managers. Maintains game state * integrity during resize events. */ -static volatile int resize_pending = 0; +static volatile sig_atomic_t resize_pending = 0; /* Modern: sig_atomic_t required for signal handler writes */ static int original_CO = 0, original_LI = 0; void handle_resize(int sig); diff --git a/src/hack.options.c b/src/hack.options.c index 338456d..e5ae945 100644 --- a/src/hack.options.c +++ b/src/hack.options.c @@ -307,7 +307,7 @@ int doset(void) { buf[len - 1] = 0; } } - pline(buf); + pline("%s", buf); /* Modern: Fix format string vulnerability */ } else parseoptions(buf, FALSE); diff --git a/src/hack.pager.c b/src/hack.pager.c index bdd703c..64b076b 100644 --- a/src/hack.pager.c +++ b/src/hack.pager.c @@ -80,7 +80,7 @@ int dowhatis(void) { (void)strncpy(buf + 1, " ", 7); } pline("%s", buf); /* MODERN: Fix format string vulnerability */ - if (ep[-1] == ';') { + if (ep && ep > buf && ep[-1] == ';') { /* Modern: ep can be NULL; ep>buf guards UB if ep==buf */ pline("More info? "); if (readchar() == 'y') { page_more(fp, 1); /* does fclose() */ @@ -448,6 +448,9 @@ int child(int wt) { /* Privilege dropping failed, but continue - it's advisory */ perror("setgid warning"); } + /* Modern: Drop UID before exec - comment at caller claimed this was already done */ + if (setuid(getuid()) != 0) + perror("setuid warning"); #ifdef CHDIR #if 0 @@ -456,9 +459,14 @@ int child(int wt) { #endif /** * MODERN ADDITION (2025): Improved chdir error handling*/ - if (chdir(getenv("HOME")) != 0) { - /* Failed to change to HOME directory, warn but continue */ - perror("chdir to HOME warning"); + { + const char *home = getenv("HOME"); /* Modern: NULL-safe; HOME unset in containers/setuid */ + if (!home) { + fprintf(stderr, "warning: HOME not set, using /tmp as working directory\n"); + home = "/tmp"; + } + if (chdir(home) != 0) + perror("chdir to HOME warning"); } #endif /* CHDIR */ return (1); diff --git a/src/hack.save.c b/src/hack.save.c index 6dd6015..aafad8d 100644 --- a/src/hack.save.c +++ b/src/hack.save.c @@ -59,11 +59,6 @@ static void sw_u16(int fd, rh_u16_t val) { bwrite(fd, (char *)buf, 2); } -static rh_u16_t sr_u16(int fd) { - unsigned char buf[2]; - mread(fd, (char *)buf, 2); - return ((rh_u16_t)buf[0] << 8) | buf[1]; -} static void sw_u32(int fd, rh_u32_t val) { unsigned char buf[4]; @@ -229,9 +224,18 @@ static int check_save_header(int fd, rh_hdr_t *hdr) { /* Copy magic to header */ memcpy(hdr->magic, test_magic, 4); - hdr->version = sr_u16(fd); - hdr->endiantag = sr_u32(fd); - hdr->reserved = sr_u32(fd); + /* Modern: Use direct read() not sr_u16/sr_u32 — mread() panics on short read, + * but this function is supposed to return 0 gracefully on truncated files. */ + { + unsigned char rem[10]; + if (read(fd, rem, 10) != 10) + return 0; + hdr->version = ((rh_u16_t)rem[0] << 8) | rem[1]; + hdr->endiantag = ((rh_u32_t)rem[2] << 24) | ((rh_u32_t)rem[3] << 16) + | ((rh_u32_t)rem[4] << 8) | rem[5]; + hdr->reserved = ((rh_u32_t)rem[6] << 24) | ((rh_u32_t)rem[7] << 16) + | ((rh_u32_t)rem[8] << 8) | rem[9]; + } /* Check endianness */ if (hdr->endiantag != RH_ENDIANTAG) { @@ -505,7 +509,13 @@ int dorecover(int fd) { /* Check if this is a versioned save file */ rh_hdr_t hdr; - long save_pos = lseek(fd, 0, SEEK_CUR); /* Save current position */ + off_t save_pos = lseek(fd, 0, SEEK_CUR); /* Modern: off_t not long; lseek returns off_t */ + if (save_pos == (off_t)-1) { + perror("lseek: cannot determine save file position"); + (void)close(fd); + restoring = FALSE; + return (0); + } if (check_save_header(fd, &hdr)) { /* Versioned format detected */ @@ -530,6 +540,7 @@ int dorecover(int fd) { if (you_size != sizeof(struct you)) { puts("Save file struct size mismatch."); restoring = FALSE; + (void)close(fd); /* Modern: Fix fd leak on early return */ return (0); } sr_bytes(fd, &u, sizeof(struct you)); @@ -597,6 +608,7 @@ int dorecover(int fd) { /* Unknown version (shouldn't happen due to check_save_header) */ impossible("Unexpected save version", hdr.version, 0); restoring = FALSE; + (void)close(fd); /* Modern: Fix fd leak on early return */ return (0); } @@ -616,9 +628,16 @@ int dorecover(int fd) { * * For canonical hosting safety, these are intentionally rejected. */ - lseek(fd, save_pos, SEEK_SET); + if (lseek(fd, save_pos, SEEK_SET) == (off_t)-1) { /* Modern: Branch I/O errors separately */ + perror("lseek on save file"); + puts("I/O error reading save file. Cannot restore."); + restoring = FALSE; + (void)close(fd); + return (0); + } puts("Save file is too old (pre-Version 1). Cannot load safely."); restoring = FALSE; + (void)close(fd); return (0); } restnames(fd);