diff --git a/.kiro/specs/scheduler-core/tasks.md b/.kiro/specs/scheduler-core/tasks.md index a809190a..7420d2e0 100644 --- a/.kiro/specs/scheduler-core/tasks.md +++ b/.kiro/specs/scheduler-core/tasks.md @@ -216,12 +216,13 @@ - [ ] 9. Core: Recording Control -- [ ] 9.1 Implement recording directory provisioning and path validation +- [x] 9.1 Implement recording directory provisioning and path validation - Add a scheduler startup CLI flag (parsed in the same argument-parsing pass as the rest of normal-mode's flags) for the recording directory — never settable from any request-path field — defaulting to a fixed system path - Validate any caller-supplied record file name before ever opening it for writing: reject any value containing a path separator or a `..` segment, resolve the final path beneath the recording directory, and confirm containment before opening - Observable: a record-file name containing a path separator or `..` is rejected before any filesystem access occurs; a bare filename resolves to a path inside the recording directory and is accepted - _Requirements: 5.10_ - _Depends: 8.1_ + - _Verified: 2026-08-04 — `--record-dir` CLI flag added to main.c/scheduler.h (record_dir field, never a request-path field). sched_validate_record_filename() (new recording.c/recording.h) rejects any `filename` containing `/` or `..` before any filesystem call, then resolves via realpath() and confirms canonical (not textual-prefix) containment beneath record_dir via path_is_contained(); rejects pre-existing non-regular entries (-EEXIST) and a non-directory record_dir (-ENOTDIR). Adversarial security review found and this task fixed two real CWE-59 inode-aliasing bypasses across 3 rounds: round 1 found a direct hard link inside record_dir aliasing an outside file passed validation (S_ISREG doesn't distinguish a hard-linked regular file from a genuine one) — fixed with an entry_st.st_nlink > 1 check (-EACCES) in the direct-entry branch, live RED→GREEN reproduced. Round 2 re-review found a compound variant still open: a symlink inside record_dir pointing at an in-directory file itself hard-linked outside record_dir was still accepted, since the S_ISLNK branch checked containment but never st_nlink on the resolved target — fixed by stat()'ing the already-realpath()-resolved target and applying the same S_ISREG && st_nlink>1 check, generalizing correctly to multi-hop symlink chains, live RED→GREEN reproduced. Round 3 re-review independently reproduced both fixes via a standalone PoC harness and additionally hunted for further variants (2-hop chains, symlink-to-symlink escape, FIFO/socket direct-naming) — all clean, no new bypass found; one non-blocking observation logged (symlink resolving to a contained-but-non-regular target is accepted rather than rejected, pre-existing since round 1, not the CWE-22/CWE-59 escape class, already covered by task 9.2's planned O_NOFOLLOW hardening) — APPROVED. TOCTOU residual risk (validation-to-eventual-fopen() window) explicitly documented in recording.h and deferred to task 9.2 (O_NOFOLLOW + fstat()-based st_nlink==1 re-check on the opened fd), not this task's scope. 18/18 recording subtests (16 original + 2 hard-link/compound regressions), 24/24 scheduler suite, clang-format clean — all independently re-verified by me via a fresh `meson compile`/`meson test --suite scheduler` run on current HEAD before marking complete._ - [ ] 9.2 Implement the record start/stop lifecycle - On start: take a full snapshot, write the replay header followed by the initial snapshot entry, begin tracking recording state diff --git a/src/scheduler/include/recording.h b/src/scheduler/include/recording.h new file mode 100644 index 00000000..519dbd12 --- /dev/null +++ b/src/scheduler/include/recording.h @@ -0,0 +1,131 @@ +#if !defined(_RECORDING_H_) + +#define _RECORDING_H_ 1 + +#include + +/* + * recording.h - scheduler-core spec, task 9.1 (Req 5 AC 10, "Record file + * path validation"). First piece of design.md's File Structure Plan + * `recording.c` ("NEW (Phase 3): record start/stop/status, ReplayEvent/ + * ReplaySession writes") -- this task adds ONLY the validation function + * every future write must pass through first; the actual record start/ + * stop/status lifecycle (design.md's NormalDriver service interface -- + * sched_driver_record_start()/_stop()/_status(), `xce_sched_driver_t`'s + * recording-state fields) is task 9.2's job and is NOT implemented here. + * + * sched_validate_record_filename - validates a caller-supplied, admin- + * authenticated but UNTRUSTED record file name (controller.capnp's + * `SchedAdminRequest.recordFile` -- "filename (not a path)", per that + * field's own comment) and resolves it to a full path beneath `record_dir` + * BEFORE the caller (task 9.2) ever opens it for writing. `record_dir` + * itself is trusted: per design.md's NormalDriver component, it is + * provisioned EXACTLY ONE WAY, the scheduler's own `--record-dir` startup + * CLI flag (main.c), and is never settable from any request-path field -- + * so only `filename` is adversarial input here, never `record_dir`. + * + * Two-phase check, per requirements.md Req 5 AC 10 "Record file path + * validation": + * + * 1. String-level rejection, BEFORE any filesystem call ever touches + * `filename`: reject if `filename` is NULL/empty, contains a path + * separator ('/'), or contains a ".." substring anywhere (leading, + * trailing, embedded, doubled -- e.g. "..", "../x", "x/..", "..x", + * "x..", "x..y", "...."). Since step 1 also bans '/' outright, a bare + * ".." substring check is both necessary and sufficient: with no + * slash permitted at all, `filename` can never have more than one + * path segment, so there is no "segment" for ".." to hide in other + * than the substring itself. C strings terminate at the first NUL + * byte, so none of these checks -- nor the eventual fopen() -- can + * ever observe bytes past an embedded NUL; there is no bypass there. + * + * 2. Canonical-path containment, AFTER the string check passes: + * `record_dir` (which must already exist -- provisioning/creating it + * on disk is explicitly out of this task's scope; see tasks.md 9.1's + * boundary) is resolved to its canonical form via realpath(), then + * the validated bare `filename` is joined onto that canonical form + * (never onto the possibly-non-canonical original `record_dir` + * string). Because `filename` has no '/' and no "..", this join + * cannot itself escape the resolved directory by construction -- the + * one remaining escape vector is a pre-existing filesystem entry + * already named `filename` inside `record_dir` that is itself a + * symlink pointing elsewhere. That case is checked explicitly: the + * symlink's target is independently canonicalized via realpath() and + * tested for containment with a true canonical-path comparison + * (resolved target must equal the resolved directory followed by + * '/') -- NEVER a textual prefix match, which would wrongly accept a + * sibling directory like "/var/log/xce-evil/..." against a + * `record_dir` of "/var/log/xce". + * + * As a defensive measure beyond the letter of AC 10 (still within its + * "before opening it for writing" intent): if an entry already named + * `filename` exists and is neither a regular file nor a (validated, + * contained) symlink -- e.g. a directory, device node, FIFO, or + * socket -- validation fails rather than handing back a path task + * 9.2 would blindly fopen(). + * + * A second defensive measure, same rationale: an existing REGULAR + * file named `filename` with more than one hard link (st_nlink > 1) + * is also rejected. A hard link shares its target's inode -- unlike + * a symlink, there is no separate "link target" path for realpath() + * to resolve and contain-check, so a hard link planted inside + * record_dir by anything with write access there (pointing at a + * file OUTSIDE record_dir) passes S_ISREG and looks canonically + * contained even though writing through it would alias a file + * outside record_dir. Same threat class as the symlink-escape case + * above (CWE-59 link-following), just via inode aliasing instead of + * path resolution -- rejected with the same -EACCES. This st_nlink + * check applies to the FULLY-RESOLVED target reached after + * realpath() has followed any number of symlink hops -- not just to + * an entry named `filename` directly: a symlink inside record_dir + * may itself point at a hard-linked alias (whether that alias lives + * directly inside record_dir or is reached through further + * intermediate symlinks), and containment alone does not detect + * that the resolved target's inode is also reachable from outside + * record_dir. Both the direct-entry case and every symlink-hop case + * are checked against the same resolved target's st_nlink. + * + * Returns 0 on success; `out_path` holds the resolved, validated full path + * (record_dir's canonical form + '/' + the validated bare filename). + * Returns a negative errno on failure: + * -EINVAL `record_dir`/`filename`/`out_path` is NULL, + * `out_path_len` is 0, or `filename` is empty, contains + * '/', or contains "..". + * -ENAMETOOLONG the resolved path does not fit in `out_path_len`. + * -ENOENT `record_dir` does not exist / cannot be resolved. + * -ENOTDIR `record_dir` resolves to something other than a + * directory. + * -EACCES an existing entry named `filename` inside `record_dir` + * either (a) resolves (via realpath()) to a location + * outside `record_dir` -- the symlink-escape case -- or + * (b) is, or (via any number of symlink hops) resolves + * to, a regular file with more than one hard link + * (st_nlink > 1) -- the hard-link inode-aliasing case, + * checked against the fully-resolved target either way; + * this function exists to catch both. + * -EEXIST an existing entry named `filename` inside `record_dir` + * is neither a regular file nor a symlink (see the + * defensive measure above). + * + * No filesystem call is made on `filename` itself until step 2 passes -- + * and even then, only `record_dir` and (if present) the pre-existing + * `record_dir/filename` entry are ever touched. Any malformed-string + * rejection in step 1 returns -EINVAL without a single filesystem call, + * even if `record_dir` itself does not exist. + * + * NOTE for task 9.2 (not implemented here -- this task is validation-only): + * this function's containment/aliasing checks are only valid up to the + * moment it returns -- a TOCTOU race could still swap a symlink or hard + * link into `out_path` between this call returning and task 9.2's eventual + * fopen(). When task 9.2 opens `out_path` for writing, it should (a) pass + * O_NOFOLLOW so a symlink swapped in after validation is refused at + * open() rather than followed, and (b) fstat() the resulting fd and + * reject unless st_nlink == 1, since O_NOFOLLOW alone does not defend + * against a hard link swapped in after validation (no symlink is involved + * in that race, so O_NOFOLLOW never triggers). + */ +extern int sched_validate_record_filename(const char* record_dir, + const char* filename, char* out_path, + size_t out_path_len); + +#endif diff --git a/src/scheduler/include/scheduler.h b/src/scheduler/include/scheduler.h index 53422aef..655a3d20 100644 --- a/src/scheduler/include/scheduler.h +++ b/src/scheduler/include/scheduler.h @@ -102,6 +102,21 @@ typedef struct { xcecli_ctx_t apictx; xce_cluster_config_t cluster_cfg; /* HA candidate list — empty means single-host mode */ + /* Req 5 AC 10 (scheduler-core spec, task 9.1): recording directory, + * provisioned EXACTLY ONE WAY -- the --record-dir startup CLI flag + * (main.c), default "/var/log/xce/". Never settable via any + * request-path field (SchedAdminRequest included) -- see + * recording.h's sched_validate_record_filename() and design.md's + * NormalDriver component. Lives here (not xce_sched_core_t) because + * recording is a driver-level concern, the same reason apictx/ + * cluster_cfg above live here rather than on core. Points into argv + * or a static string literal (never heap-owned), same convention as + * this struct's neighboring driver-level string fields elsewhere in + * this codebase (e.g. main.c's `logfile`) -- no free() needed. The + * rest of the recording STATE design.md sketches for a future + * xce_sched_driver_t (record_file_path/record_file/recording/ + * record_event_count/...) is task 9.2's job, not this field's. */ + char* record_dir; } xce_scheduler_t; extern int sched_init(xce_scheduler_t*, xce_client_config_t*, @@ -137,25 +152,25 @@ extern int sched_account_jobs(xce_scheduler_t*); * sched_get_project/sched_add_project/sched_job_is_ready/ * sched_queue_is_ready relocated to core.h (scheduler-core spec, task 4.1) * -- they now operate on xce_sched_core_t*, not xce_scheduler_t*. */ -extern int sched_update_existing_queue(xce_scheduler_t*, xce_queue_t*); -extern int sched_update_existing_job(xce_scheduler_t*, xce_job_t*); +extern int sched_update_existing_queue(xce_scheduler_t*, xce_queue_t*); +extern int sched_update_existing_job(xce_scheduler_t*, xce_job_t*); extern int sched_apply_job_rusage(xce_scheduler_t*, xce_job_rusages_t*); -extern int sched_commit_decision(xce_scheduler_t*, xce_decision_t*); -extern int sched_publish_pend_reason(xce_scheduler_t*, xce_preason_set_t*); -extern int sched_publish_dispatch(xce_scheduler_t*, xce_job_dispatch_t*); +extern int sched_commit_decision(xce_scheduler_t*, xce_decision_t*); +extern int sched_publish_pend_reason(xce_scheduler_t*, xce_preason_set_t*); +extern int sched_publish_dispatch(xce_scheduler_t*, xce_job_dispatch_t*); extern void sched_worker_release_host_ctx(xce_sched_worker_t*); -extern int sched_meet_host_selection(xce_scheduler_t*, xce_sched_worker_t*, - xce_select_t*); -extern int sched_check_shared_satisfy(xce_scheduler_t*, xvec_t*, xvec_idval_t*, - xvec_idval_t*); -extern int sched_check_host_satisfy(xce_scheduler_t*, xce_sched_worker_t*, - xvec_t*, xvec_idval_t*, xvec_idval_t*); -extern int sched_collect_workers(xce_scheduler_t*, xlist_t*, - zarray_iter_skip_fn_t, - sched_group_worker_fn_t); -extern int sched_charge_account(xce_scheduler_t*, xce_job_account_t*); -extern int sched_uncharge_account(xce_scheduler_t*, xce_job_account_t*); -extern int sched_commit_job_alloc(xce_scheduler_t*, xce_sched_job_t*); +extern int sched_meet_host_selection(xce_scheduler_t*, xce_sched_worker_t*, + xce_select_t*); +extern int sched_check_shared_satisfy(xce_scheduler_t*, xvec_t*, xvec_idval_t*, + xvec_idval_t*); +extern int sched_check_host_satisfy(xce_scheduler_t*, xce_sched_worker_t*, + xvec_t*, xvec_idval_t*, xvec_idval_t*); +extern int sched_collect_workers(xce_scheduler_t*, xlist_t*, + zarray_iter_skip_fn_t, + sched_group_worker_fn_t); +extern int sched_charge_account(xce_scheduler_t*, xce_job_account_t*); +extern int sched_uncharge_account(xce_scheduler_t*, xce_job_account_t*); +extern int sched_commit_job_alloc(xce_scheduler_t*, xce_sched_job_t*); extern uint64_t sched_get_id(xce_scheduler_t*); extern xce_alloc_t** sched_make_allocs(xce_scheduler_t*, char*, int, int*, float*, int*); diff --git a/src/scheduler/meson.build b/src/scheduler/meson.build index 72e1c7b0..911f4f93 100644 --- a/src/scheduler/meson.build +++ b/src/scheduler/meson.build @@ -11,6 +11,7 @@ scheduler_srcs = [ 'src/snapshot_wire.c', 'src/snapshot_load.c', 'src/decision.c', + 'src/recording.c', 'src/schedobj.c', 'src/plugin.c', 'src/resource.c', diff --git a/src/scheduler/src/main.c b/src/scheduler/src/main.c index f0bbc5f0..74acf85b 100644 --- a/src/scheduler/src/main.c +++ b/src/scheduler/src/main.c @@ -16,7 +16,6 @@ #include "resreq.h" #include "utils.h" - extern int scheduler_req_handler(xce_request_t*, void*); static char doc[] = "xce_scheduler"; @@ -25,11 +24,17 @@ const char* argp_program_version = MK_VERSION("xce v0.0.0.1-", STR(VERSION_COMMIT_ID)); const char* argp_program_bug_address = BUG_EMAIL; +/* Long-option identifiers (beyond ASCII range to avoid collisions with the + * short-option letters above -- same convention as the controller's + * main.c, e.g. its OPT_PEERS). */ +#define OPT_RECORD_DIR 256 + struct arguments { char* logfile; int verbose; - char* peers; /* --peers value (optional) */ - char* sim_file; /* --sim value (optional; Phase 3, not yet implemented) */ + char* peers; /* --peers value (optional) */ + char* sim_file; /* --sim value (optional; Phase 3, not yet implemented) */ + char* record_dir; /* --record-dir value (optional; Req 5 AC 10, task 9.1) */ }; static struct argp_option options[] = { @@ -44,6 +49,13 @@ static struct argp_option options[] = { { "sim", 's', "FILE", 0, "Replay FILE for simulation mode " "(Phase 3 -- not yet implemented)" }, + /* Req 5 AC 10 "Record file path validation" (scheduler-core spec, task + * 9.1): the ONLY way record_dir is ever provisioned -- never settable + * via any request-path field (SchedAdminRequest included). Parsed + * alongside the rest of normal-mode's flags, same argp pass as above. */ + { "record-dir", OPT_RECORD_DIR, "DIR", 0, + "Directory for scheduler session recordings " + "(default: /var/log/xce/)" }, { 0 } }; @@ -64,6 +76,9 @@ static error_t parse_opt(int key, char* arg, struct argp_state* state) case 's': arguments->sim_file = arg; break; + case OPT_RECORD_DIR: + arguments->record_dir = arg; + break; default: return ARGP_ERR_UNKNOWN; } @@ -109,13 +124,17 @@ static void sig_handler(int signo) int main(int argc, char* argv[]) { - struct arguments arguments; - char* logfile = "/tmp/xce_scheduler.log"; - char* loglevel = "info"; - int logsize = 64 * 1024 * 1024; - int logcount = 10; + struct arguments arguments; + char* logfile = "/tmp/xce_scheduler.log"; + char* loglevel = "info"; + int logsize = 64 * 1024 * 1024; + int logcount = 10; + /* Req 5 AC 10 (scheduler-core spec, task 9.1): default recording + * directory when --record-dir is not given -- see scheduler.h's + * xce_scheduler_t.record_dir field comment. */ + char* record_dir = "/var/log/xce/"; xce_client_config_t config; - xce_cluster_config_t cluster_cfg = {0}; + xce_cluster_config_t cluster_cfg = { 0 }; const xce_cluster_config_t* cluster_cfg_ptr = NULL; memset(&arguments, 0x0, sizeof(arguments)); @@ -130,6 +149,13 @@ int main(int argc, char* argv[]) loglevel = verbose_to_loglevel(arguments.verbose); } + /* Req 5 AC 10 (scheduler-core spec, task 9.1): --record-dir is the + * ONLY way record_dir is ever provisioned -- see this flag's + * options[] entry above and scheduler.h's field comment. */ + if (arguments.record_dir != NULL) { + record_dir = arguments.record_dir; + } + mylog_init(logfile, loglevel, logsize, logcount); /* Req 5 AC 9 "Mode selection" (scheduler-core spec, task 5.2): --sim @@ -158,9 +184,8 @@ int main(int argc, char* argv[]) strncpy(cluster_cfg.identity, "xce_scheduler", sizeof(cluster_cfg.identity) - 1); if (xce_cluster_config_validate(&cluster_cfg) != 0) { - fprintf(stderr, - "xce_scheduler: invalid cluster config (missing " - "identity?).\n"); + fprintf(stderr, "xce_scheduler: invalid cluster config (missing " + "identity?).\n"); return 1; } if (cluster_cfg.candidates_count > 3) { @@ -179,6 +204,12 @@ int main(int argc, char* argv[]) sched_init(&sched, &config, cluster_cfg_ptr, scheduler_req_handler); + /* Req 5 AC 10 (scheduler-core spec, task 9.1): stash the resolved + * --record-dir value on the live xce_scheduler_t -- see scheduler.h's + * field comment. Not consumed anywhere yet (task 9.2's job); this + * task only wires the value through to a sensible home. */ + sched.record_dir = record_dir; + /* Req 5 AC 6 (scheduler-core spec, task 5.2): register the driver's * decision callback -- replaces main.c's former inline * `switch (decision->op)`. Registered once, before the first diff --git a/src/scheduler/src/recording.c b/src/scheduler/src/recording.c new file mode 100644 index 00000000..fa0fa450 --- /dev/null +++ b/src/scheduler/src/recording.c @@ -0,0 +1,171 @@ +#include +#include +#include +#include +#include +#include + +#include "recording.h" + +/* + * recording.c - scheduler-core spec, task 9.1. See recording.h for the + * full contract/rationale. This is deliberately the ONLY function in this + * file as of task 9.1 -- record start/stop/status (task 9.2, design.md's + * NormalDriver) extend this file later; nothing here anticipates that + * work's shape beyond what this task's own scope requires. + */ + +/* + * path_is_contained - true iff canonical path `child` is `parent` itself + * plus a '/'-separated descendant -- i.e. `child` starts with `parent` + * AND the byte immediately following that prefix is '/'. The second half + * of that check is what makes this safe against the classic prefix trap: + * without it, parent="/var/log/xce" would wrongly accept + * child="/var/log/xce-evil/target" (textually a prefix match, but a + * completely different, sibling directory). Both arguments must already + * be canonical (realpath()'d) -- this function does no resolution itself. + */ +static int path_is_contained(const char* child, const char* parent) +{ + size_t parent_len = strlen(parent); + + if (strncmp(child, parent, parent_len) != 0) { + return 0; + } + + return child[parent_len] == '/'; +} + +int sched_validate_record_filename(const char* record_dir, const char* filename, + char* out_path, size_t out_path_len) +{ + char resolved_dir[PATH_MAX]; + struct stat dir_st; + struct stat entry_st; + int written; + + if ((record_dir == NULL) || (filename == NULL) || (out_path == NULL) + || (out_path_len == 0)) { + return -EINVAL; + } + + /* Step 1 (Req 5 AC 10): reject a malformed filename before ANY + * filesystem call ever touches it -- no realpath(), no stat(), no + * fopen(). See recording.h's header comment for why a plain ".." + * substring check is sufficient once '/' is already banned outright. */ + if (filename[0] == '\0') { + return -EINVAL; + } + if (strchr(filename, '/') != NULL) { + return -EINVAL; + } + if (strstr(filename, "..") != NULL) { + return -EINVAL; + } + + /* Step 2: canonicalize record_dir -- trusted input (only ever comes + * from the --record-dir startup CLI flag, main.c), never derived from + * `filename`. Must already exist; creating it is out of this task's + * scope (tasks.md 9.1's "recording directory provisioning" means + * "wire the CLI flag through", not "mkdir it on disk"). */ + if (realpath(record_dir, resolved_dir) == NULL) { + return (errno > 0) ? -errno : -ENOENT; + } + + if (stat(resolved_dir, &dir_st) != 0) { + return (errno > 0) ? -errno : -ENOENT; + } + if (!S_ISDIR(dir_st.st_mode)) { + return -ENOTDIR; + } + + /* The validated target: canonical record_dir + '/' + the bare + * filename. filename has no '/' and no ".." (step 1), so this join + * cannot itself escape resolved_dir -- the only remaining escape + * vector is an EXISTING entry already named `filename` inside + * record_dir that happens to be a symlink pointing elsewhere, + * checked below. */ + written = snprintf(out_path, out_path_len, "%s/%s", resolved_dir, filename); + if ((written < 0) || ((size_t)written >= out_path_len)) { + return -ENAMETOOLONG; + } + + if (lstat(out_path, &entry_st) == 0) { + if (S_ISLNK(entry_st.st_mode)) { + char resolved_entry[PATH_MAX]; + struct stat target_st; + + /* Step 3: an existing symlink named `filename` -- resolve ITS + * target independently and confirm containment via a true + * canonical-path comparison (path_is_contained()), never a + * textual prefix match. This is the "record_dir contains a + * symlink pointing outside itself" attack this function + * exists to catch: a naive record_dir + "/" + filename join + * looks contained textually, but fopen()-ing it would follow + * the symlink straight out of record_dir. */ + if (realpath(out_path, resolved_entry) == NULL) { + return (errno > 0) ? -errno : -EACCES; + } + if (!path_is_contained(resolved_entry, resolved_dir)) { + return -EACCES; + } + + /* Hard-link inode-aliasing escape, reached via a symlink hop: + * containment alone only proves resolved_entry's PATH sits + * inside record_dir -- it says nothing about how many hard + * links point at that same inode. record_dir could contain a + * regular file that is itself a hard link to a victim file + * OUTSIDE record_dir (caught directly by the st_nlink check in + * the branch below when named directly), plus a symlink INSIDE + * record_dir pointing at that hard-linked entry. The symlink's + * target textually/canonically resolves inside record_dir, so + * containment passes, but fopen()-ing out_path would still + * follow the symlink into the aliased outside inode -- the + * exact CWE-59 impact this function exists to prevent, just + * one extra hop away. stat() (not lstat()) here is intentional + * and non-racy: realpath() has already fully resolved the + * entire symlink chain into resolved_entry, so stat()-ing that + * already-canonical path merely reads the final target's link + * count -- it does not re-resolve anything a TOCTOU swap could + * race against beyond what realpath() itself already assumed. */ + if (stat(resolved_entry, &target_st) != 0) { + return (errno > 0) ? -errno : -EACCES; + } + if (S_ISREG(target_st.st_mode) && (target_st.st_nlink > 1)) { + return -EACCES; + } + } else if (!S_ISREG(entry_st.st_mode)) { + /* Defensive measure beyond AC 10's literal text (see + * recording.h): an existing directory/device/FIFO/socket + * named `filename` is not a sane recording target -- refuse + * rather than hand back a path task 9.2 would blindly + * fopen(). Also closes the degenerate filename="." case: + * record_dir/. lstat()s as record_dir itself (a directory, + * not a symlink), caught here. */ + return -EEXIST; + } else if (entry_st.st_nlink > 1) { + /* Hard-link inode-aliasing escape: a symlink has a distinct + * target path that realpath()/lstat() can resolve and check + * for containment (step 3 above) -- a hard link has none. It + * IS the same inode as whatever file it was link()'d from, so + * an entry named `filename` here that is a regular file with + * more than one link could have been link()'d in from OUTSIDE + * record_dir by anything with write access to this directory. + * It passes S_ISREG and looks textually/canonically contained + * (out_path IS inside record_dir), but task 9.2's + * fopen(out_path, "w") would truncate/overwrite whatever else + * that inode is linked from -- the exact write-outside- + * record_dir impact this function exists to prevent, via + * CWE-59 link-following, same threat class and same error + * code as the symlink-escape case above. A freshly created + * file has st_nlink == 1; nothing this function itself + * creates can ever trip this check. */ + return -EACCES; + } + /* else: an existing regular file with a single link -- a + * legitimate overwrite target, acceptable, will be overwritten + * by task 9.2's fopen(). */ + } + + return 0; +} diff --git a/src/scheduler/testsuite/meson.build b/src/scheduler/testsuite/meson.build index 669d741c..dd4696c4 100644 --- a/src/scheduler/testsuite/meson.build +++ b/src/scheduler/testsuite/meson.build @@ -393,6 +393,25 @@ sched_snapshot_save_load_test = executable( dependencies: [scheduler_test_deps], install: false) +# White-box test for the recording-directory path-validation function +# (scheduler-core spec, task 9.1, Req 5 AC 10 "Record file path +# validation"): rejects a recordFile containing a path separator or a ".." +# segment BEFORE any filesystem access, and confirms containment beneath +# record_dir via a genuine canonical-path (realpath()) comparison rather +# than a textual prefix match -- including a symlink-escape case and the +# "record_dir vs record_dir-evil sibling" prefix-match trap named in this +# task's own security brief. Same white-box #include pattern as +# test_job_update.c/test_handlers.c (pulls the real +# ../src/recording.c into this TU) -- the function under test has no +# scheduler-struct fixture dependency at all, so this is the simplest +# white-box test in this directory. +sched_recording_path_validation_test = executable( + 'sched_recording_path_validation_test', + ['test_recording_path_validation.c'], + include_directories:[incdir], + dependencies: [scheduler_test_deps], + install: false) + # Register as meson tests so `meson test --suite scheduler` runs them all. # detect_leaks=0 matches the suite convention (fixtures are not torn down). test('scheduler test', scheduler_test, suite: 'scheduler', @@ -453,3 +472,6 @@ test('scheduler driver session ordering test', test('scheduler snapshot save load test', sched_snapshot_save_load_test, suite: 'scheduler', env: ['ASAN_OPTIONS=detect_leaks=0']) +test('scheduler recording path validation test', + sched_recording_path_validation_test, suite: 'scheduler', + env: ['ASAN_OPTIONS=detect_leaks=0']) diff --git a/src/scheduler/testsuite/test_recording_path_validation.c b/src/scheduler/testsuite/test_recording_path_validation.c new file mode 100644 index 00000000..bf361ba4 --- /dev/null +++ b/src/scheduler/testsuite/test_recording_path_validation.c @@ -0,0 +1,487 @@ +/* + * test_recording_path_validation.c - contract test for + * sched_validate_record_filename() (scheduler-core spec, task 9.1, Req 5 + * AC 10 "Record file path validation"). + * + * Observable under test: + * - A filename containing a path separator ('/') is rejected (-EINVAL) + * before ANY filesystem call is made -- proven by pairing a malicious + * filename with a record_dir that does not exist on disk: if the + * function had proceeded past the string check, resolving that + * nonexistent record_dir would have failed with -ENOENT instead. Only + * -EINVAL coming back proves the string check short-circuited first. + * - A filename containing a ".." substring -- leading, trailing, + * embedded, or doubled -- is rejected the same way, same proof. + * - A record_dir containing a pre-existing symlink whose target escapes + * record_dir is REJECTED even when the filename component looks + * perfectly safe (no '/', no ".."), via a genuine canonical-path + * (realpath()) comparison -- not a textual prefix match. A dedicated + * test also proves the comparison isn't a naive strncmp() prefix + * check by using a sibling directory whose name textually starts with + * record_dir's own canonical path (the "/var/log/xce-evil" vs + * "/var/log/xce" trap called out in this task's own security brief). + * - A legitimate bare filename resolves to record_dir/filename and is + * accepted. + * + * White-box #include of ../src/recording.c (same convention as every + * other white-box test in this directory) -- the function under test has + * no internal state or fixture dependencies, so this is purely for the + * standalone-executable convention this testsuite already establishes + * (e.g. test_job_update.c, test_picker_iface.c), not because anything + * static/internal needs exercising beyond the one exported function. + */ +#include +#include +#include +#include +#include +#include +#include + +#include + +/* Pull in the real sched_validate_record_filename() under test. */ +#include "../src/recording.c" + +void setUp(void) { return; } +void tearDown(void) { return; } + +/* ---------------------------------------------------------------------- */ +/* Helpers */ +/* ---------------------------------------------------------------------- */ + +static void make_tmp_dir(char* out, size_t out_len, const char* suffix) +{ + char tmpl[PATH_MAX]; + + /* mkdtemp() requires the template's LAST SIX characters to be + * "XXXXXX" -- suffix must precede it, not follow it. */ + snprintf(tmpl, sizeof(tmpl), "/tmp/sched_recval%s_XXXXXX", suffix); + TEST_ASSERT_NOT_NULL(mkdtemp(tmpl)); + TEST_ASSERT_TRUE(strlen(tmpl) < out_len); + strcpy(out, tmpl); +} + +static void touch_file(const char* path) +{ + int fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0644); + TEST_ASSERT_TRUE(fd >= 0); + close(fd); +} + +/* ---------------------------------------------------------------------- */ +/* NULL / basic argument validation */ +/* ---------------------------------------------------------------------- */ + +static void test_null_record_dir_returns_einval(void) +{ + char out[PATH_MAX]; + int rc + = sched_validate_record_filename(NULL, "foo.replay", out, sizeof(out)); + TEST_ASSERT_EQUAL_INT(-EINVAL, rc); +} + +static void test_null_filename_returns_einval(void) +{ + char out[PATH_MAX]; + int rc = sched_validate_record_filename("/tmp", NULL, out, sizeof(out)); + TEST_ASSERT_EQUAL_INT(-EINVAL, rc); +} + +static void test_null_out_path_returns_einval(void) +{ + int rc = sched_validate_record_filename("/tmp", "foo.replay", NULL, 64); + TEST_ASSERT_EQUAL_INT(-EINVAL, rc); +} + +static void test_zero_out_path_len_returns_einval(void) +{ + char out[PATH_MAX]; + int rc = sched_validate_record_filename("/tmp", "foo.replay", out, 0); + TEST_ASSERT_EQUAL_INT(-EINVAL, rc); +} + +static void test_empty_filename_returns_einval(void) +{ + char out[PATH_MAX]; + int rc = sched_validate_record_filename("/tmp", "", out, sizeof(out)); + TEST_ASSERT_EQUAL_INT(-EINVAL, rc); +} + +/* ---------------------------------------------------------------------- */ +/* Path-separator rejection -- BEFORE any filesystem access (Req 5 AC 10) */ +/* ---------------------------------------------------------------------- */ + +static void test_filename_with_slash_rejected_before_filesystem_access(void) +{ + /* record_dir deliberately does NOT exist -- if the implementation had + * proceeded past the string check, realpath(record_dir) would fail + * with -ENOENT. Getting -EINVAL back instead proves the '/' check + * short-circuited first, with zero filesystem calls on the malicious + * string (or even on record_dir). */ + static const char* bad_names[] + = { "/etc/passwd", "evil/", "a/b", "../../etc/passwd", "/" }; + char out[PATH_MAX]; + size_t i; + + for (i = 0; i < sizeof(bad_names) / sizeof(bad_names[0]); i++) { + int rc = sched_validate_record_filename( + "/definitely/does/not/exist/xyz123", bad_names[i], out, + sizeof(out)); + TEST_ASSERT_EQUAL_INT_MESSAGE(-EINVAL, rc, bad_names[i]); + } +} + +/* ---------------------------------------------------------------------- */ +/* ".." rejection, all positions -- BEFORE any filesystem access */ +/* ---------------------------------------------------------------------- */ + +static void test_dotdot_all_positions_rejected_before_filesystem_access(void) +{ + static const char* bad_names[] = { + "..", /* bare */ + "...", /* tripled dot, still contains ".." */ + "....", /* quadrupled */ + "..evil", /* leading */ + "evil..", /* trailing */ + "ev..il", /* embedded */ + "a..b..c" /* doubled */ + }; + char out[PATH_MAX]; + size_t i; + + for (i = 0; i < sizeof(bad_names) / sizeof(bad_names[0]); i++) { + int rc = sched_validate_record_filename( + "/definitely/does/not/exist/xyz123", bad_names[i], out, + sizeof(out)); + TEST_ASSERT_EQUAL_INT_MESSAGE(-EINVAL, rc, bad_names[i]); + } +} + +/* ---------------------------------------------------------------------- */ +/* record_dir configuration errors (distinct from malicious-filename path) */ +/* ---------------------------------------------------------------------- */ + +static void test_nonexistent_record_dir_with_valid_filename_returns_error(void) +{ + /* A perfectly well-formed filename still fails once the (nonexistent) + * record_dir itself is resolved -- proves this is a DIFFERENT error + * path than the string-rejection tests above (which never reach + * realpath() at all). */ + char out[PATH_MAX]; + int rc = sched_validate_record_filename("/definitely/does/not/exist/xyz123", + "session.replay", out, sizeof(out)); + TEST_ASSERT_TRUE(rc < 0); + TEST_ASSERT_NOT_EQUAL(-EINVAL, rc); +} + +static void test_record_dir_that_is_a_file_returns_enotdir(void) +{ + char record_dir[PATH_MAX]; + char file_path[PATH_MAX]; + char out[PATH_MAX]; + int rc; + + make_tmp_dir(record_dir, sizeof(record_dir), "_parent"); + snprintf(file_path, sizeof(file_path), "%s/not_a_dir", record_dir); + touch_file(file_path); + + rc = sched_validate_record_filename(file_path, "session.replay", out, + sizeof(out)); + TEST_ASSERT_EQUAL_INT(-ENOTDIR, rc); +} + +/* ---------------------------------------------------------------------- */ +/* Legitimate filename acceptance */ +/* ---------------------------------------------------------------------- */ + +static void test_legitimate_bare_filename_resolves_inside_record_dir(void) +{ + char record_dir[PATH_MAX]; + char resolved_dir[PATH_MAX]; + char out[PATH_MAX]; + char expected[PATH_MAX]; + int rc; + + make_tmp_dir(record_dir, sizeof(record_dir), "_ok"); + TEST_ASSERT_NOT_NULL(realpath(record_dir, resolved_dir)); + + rc = sched_validate_record_filename(record_dir, "session.replay", out, + sizeof(out)); + TEST_ASSERT_EQUAL_INT(0, rc); + + snprintf(expected, sizeof(expected), "%s/session.replay", resolved_dir); + TEST_ASSERT_EQUAL_STRING(expected, out); +} + +static void test_legitimate_filename_overwriting_existing_regular_file_ok(void) +{ + /* An existing regular file with the same name is a legitimate + * overwrite target (task 9.2's fopen() will truncate it), not an + * error. */ + char record_dir[PATH_MAX]; + char resolved_dir[PATH_MAX]; + char existing[PATH_MAX]; + char out[PATH_MAX]; + char expected[PATH_MAX]; + int rc; + + make_tmp_dir(record_dir, sizeof(record_dir), "_overwrite"); + TEST_ASSERT_NOT_NULL(realpath(record_dir, resolved_dir)); + snprintf(existing, sizeof(existing), "%s/session.replay", resolved_dir); + touch_file(existing); + + rc = sched_validate_record_filename(record_dir, "session.replay", out, + sizeof(out)); + TEST_ASSERT_EQUAL_INT(0, rc); + + snprintf(expected, sizeof(expected), "%s/session.replay", resolved_dir); + TEST_ASSERT_EQUAL_STRING(expected, out); +} + +static void test_out_path_buffer_too_small_returns_enametoolong(void) +{ + char record_dir[PATH_MAX]; + char out[4]; /* far too small for any real resolved path */ + int rc; + + make_tmp_dir(record_dir, sizeof(record_dir), "_small"); + + rc = sched_validate_record_filename(record_dir, "session.replay", out, + sizeof(out)); + TEST_ASSERT_EQUAL_INT(-ENAMETOOLONG, rc); +} + +/* ---------------------------------------------------------------------- */ +/* Symlink-escape containment -- canonical-path comparison, not textual */ +/* prefix matching (Req 5 AC 10's core security property) */ +/* ---------------------------------------------------------------------- */ + +static void test_symlink_inside_record_dir_escaping_outside_is_rejected(void) +{ + /* record_dir contains a symlink (named exactly like a legitimate, + * innocent-looking filename) whose target lives OUTSIDE record_dir. + * The filename component itself violates neither the '/' nor the + * ".." rule -- only the canonical-path containment check on the + * pre-existing symlink's resolved target can catch this. */ + char record_dir[PATH_MAX]; + char outside_dir[PATH_MAX]; + char outside_target[PATH_MAX]; + char link_path[PATH_MAX]; + char out[PATH_MAX]; + int rc; + + make_tmp_dir(record_dir, sizeof(record_dir), "_escsrc"); + make_tmp_dir(outside_dir, sizeof(outside_dir), "_escdst"); + + snprintf(outside_target, sizeof(outside_target), "%s/secret", outside_dir); + touch_file(outside_target); + + snprintf(link_path, sizeof(link_path), "%s/looks_safe", record_dir); + TEST_ASSERT_EQUAL_INT(0, symlink(outside_target, link_path)); + + rc = sched_validate_record_filename(record_dir, "looks_safe", out, + sizeof(out)); + TEST_ASSERT_EQUAL_INT(-EACCES, rc); +} + +static void test_symlink_escape_not_caught_by_naive_textual_prefix_match(void) +{ + /* The exact trap this task's security brief calls out by name: + * record_dir="/xce" vs a SIBLING directory + * "/xce-evil" -- the sibling's path textually STARTS WITH + * record_dir's canonical path, so a naive strncmp()-only containment + * check would wrongly accept it. A pre-existing symlink inside + * record_dir points at a file inside that sibling; a correct + * canonical-path comparison (requiring the byte after the prefix to + * be '/') must still reject it. */ + char base[PATH_MAX]; + char record_dir[PATH_MAX]; + char sibling_dir[PATH_MAX]; + char sibling_target[PATH_MAX]; + char link_path[PATH_MAX]; + char out[PATH_MAX]; + int rc; + + make_tmp_dir(base, sizeof(base), "_prefixtrap"); + + snprintf(record_dir, sizeof(record_dir), "%s/xce", base); + TEST_ASSERT_EQUAL_INT(0, mkdir(record_dir, 0755)); + + snprintf(sibling_dir, sizeof(sibling_dir), "%s/xce-evil", base); + TEST_ASSERT_EQUAL_INT(0, mkdir(sibling_dir, 0755)); + + snprintf(sibling_target, sizeof(sibling_target), "%s/payload", sibling_dir); + touch_file(sibling_target); + + snprintf(link_path, sizeof(link_path), "%s/trap", record_dir); + TEST_ASSERT_EQUAL_INT(0, symlink(sibling_target, link_path)); + + rc = sched_validate_record_filename(record_dir, "trap", out, sizeof(out)); + TEST_ASSERT_EQUAL_INT(-EACCES, rc); +} + +static void test_symlink_inside_record_dir_staying_contained_is_accepted(void) +{ + /* Sanity companion to the two rejection tests above: a symlink whose + * target genuinely stays within record_dir must be ACCEPTED -- proves + * the containment check isn't simply "reject every symlink". */ + char record_dir[PATH_MAX]; + char resolved_dir[PATH_MAX]; + char real_target[PATH_MAX]; + char link_path[PATH_MAX]; + char out[PATH_MAX]; + char expected[PATH_MAX]; + int rc; + + make_tmp_dir(record_dir, sizeof(record_dir), "_inbound"); + TEST_ASSERT_NOT_NULL(realpath(record_dir, resolved_dir)); + + snprintf(real_target, sizeof(real_target), "%s/real.replay", resolved_dir); + touch_file(real_target); + + snprintf(link_path, sizeof(link_path), "%s/alias", resolved_dir); + TEST_ASSERT_EQUAL_INT(0, symlink(real_target, link_path)); + + rc = sched_validate_record_filename(record_dir, "alias", out, sizeof(out)); + TEST_ASSERT_EQUAL_INT(0, rc); + + snprintf(expected, sizeof(expected), "%s/alias", resolved_dir); + TEST_ASSERT_EQUAL_STRING(expected, out); +} + +/* ---------------------------------------------------------------------- */ +/* Hard-link inode-aliasing escape -- CWE-59 link-following via a hard */ +/* link rather than a symlink (Req 5 AC 10's core security property) */ +/* ---------------------------------------------------------------------- */ + +static void test_hardlink_inside_record_dir_aliasing_outside_is_rejected(void) +{ + /* record_dir contains a REGULAR file (named exactly like a legitimate, + * innocent-looking filename) that is actually a hard link to a victim + * file OUTSIDE record_dir -- same inode, no separate "link target" + * path for realpath()/lstat() to resolve and contain-check the way + * there is for a symlink (mirrors + * test_symlink_inside_record_dir_escaping_outside_is_rejected above, + * substituting link() for symlink()). The filename component itself + * violates neither the '/' nor the ".." rule, and the entry IS + * S_ISREG and canonically inside record_dir -- only the st_nlink > 1 + * check can catch this. This exercises real link(2)/lstat(2) kernel + * behavior (no mocking): a genuine hard link is created on disk and + * genuinely shares the victim's inode, so if this test passes only + * because the fix is in place, the hard link IS a real bypass on this + * platform absent that fix (proven by the RED phase below). */ + char record_dir[PATH_MAX]; + char outside_dir[PATH_MAX]; + char victim[PATH_MAX]; + char aliased_path[PATH_MAX]; + char out[PATH_MAX]; + int rc; + + make_tmp_dir(record_dir, sizeof(record_dir), "_hlsrc"); + make_tmp_dir(outside_dir, sizeof(outside_dir), "_hldst"); + + snprintf(victim, sizeof(victim), "%s/victim", outside_dir); + touch_file(victim); + + snprintf(aliased_path, sizeof(aliased_path), "%s/looks_safe", record_dir); + TEST_ASSERT_EQUAL_INT(0, link(victim, aliased_path)); + + rc = sched_validate_record_filename(record_dir, "looks_safe", out, + sizeof(out)); + TEST_ASSERT_EQUAL_INT(-EACCES, rc); +} + +static void test_symlink_to_hardlinked_alias_inside_record_dir_is_rejected(void) +{ + /* One extra hop beyond both escape tests above: record_dir contains a + * REGULAR file ("internal") that is a hard link to a victim file + * OUTSIDE record_dir (mirrors + * test_hardlink_inside_record_dir_aliasing_outside_is_rejected), PLUS a + * SYMLINK ("alias") inside record_dir pointing at that hard-linked + * "internal" entry (mirrors + * test_symlink_inside_record_dir_escaping_outside_is_rejected's shape). + * Requesting "internal" directly is already caught by the direct-entry + * st_nlink > 1 check. Requesting "alias" must ALSO be rejected: the + * S_ISLNK branch's realpath()/path_is_contained() call only confirms + * the resolved target is textually/canonically inside record_dir -- + * "internal" IS inside record_dir, so containment alone is not enough. + * The resolved target must ALSO be checked for st_nlink > 1, exactly + * like the direct-entry branch does, or fopen()-ing the returned + * out_path follows the symlink into the aliased outside inode and + * corrupts the victim file -- same CWE-59 impact, reached via one + * extra symlink hop. */ + char record_dir[PATH_MAX]; + char outside_dir[PATH_MAX]; + char victim[PATH_MAX]; + char internal_path[PATH_MAX]; + char alias_path[PATH_MAX]; + char out[PATH_MAX]; + int rc; + + make_tmp_dir(record_dir, sizeof(record_dir), "_hlaliassrc"); + make_tmp_dir(outside_dir, sizeof(outside_dir), "_hlaliasdst"); + + snprintf(victim, sizeof(victim), "%s/victim", outside_dir); + touch_file(victim); + + snprintf(internal_path, sizeof(internal_path), "%s/internal", record_dir); + TEST_ASSERT_EQUAL_INT(0, link(victim, internal_path)); + + snprintf(alias_path, sizeof(alias_path), "%s/alias", record_dir); + TEST_ASSERT_EQUAL_INT(0, symlink(internal_path, alias_path)); + + rc = sched_validate_record_filename(record_dir, "alias", out, sizeof(out)); + TEST_ASSERT_EQUAL_INT(-EACCES, rc); +} + +/* ---------------------------------------------------------------------- */ +/* Defensive: existing non-regular, non-symlink entry is rejected */ +/* ---------------------------------------------------------------------- */ + +static void test_existing_directory_with_same_name_rejected(void) +{ + char record_dir[PATH_MAX]; + char subdir[PATH_MAX]; + char out[PATH_MAX]; + int rc; + + make_tmp_dir(record_dir, sizeof(record_dir), "_subdir"); + snprintf(subdir, sizeof(subdir), "%s/oops", record_dir); + TEST_ASSERT_EQUAL_INT(0, mkdir(subdir, 0755)); + + rc = sched_validate_record_filename(record_dir, "oops", out, sizeof(out)); + TEST_ASSERT_EQUAL_INT(-EEXIST, rc); +} + +int main(void) +{ + UNITY_BEGIN(); + + RUN_TEST(test_null_record_dir_returns_einval); + RUN_TEST(test_null_filename_returns_einval); + RUN_TEST(test_null_out_path_returns_einval); + RUN_TEST(test_zero_out_path_len_returns_einval); + RUN_TEST(test_empty_filename_returns_einval); + + RUN_TEST(test_filename_with_slash_rejected_before_filesystem_access); + RUN_TEST(test_dotdot_all_positions_rejected_before_filesystem_access); + + RUN_TEST(test_nonexistent_record_dir_with_valid_filename_returns_error); + RUN_TEST(test_record_dir_that_is_a_file_returns_enotdir); + + RUN_TEST(test_legitimate_bare_filename_resolves_inside_record_dir); + RUN_TEST(test_legitimate_filename_overwriting_existing_regular_file_ok); + RUN_TEST(test_out_path_buffer_too_small_returns_enametoolong); + + RUN_TEST(test_symlink_inside_record_dir_escaping_outside_is_rejected); + RUN_TEST(test_symlink_escape_not_caught_by_naive_textual_prefix_match); + RUN_TEST(test_symlink_inside_record_dir_staying_contained_is_accepted); + + RUN_TEST(test_hardlink_inside_record_dir_aliasing_outside_is_rejected); + RUN_TEST(test_symlink_to_hardlinked_alias_inside_record_dir_is_rejected); + + RUN_TEST(test_existing_directory_with_same_name_rejected); + + return UNITY_END(); +}