feat(scheduler-core): implement recording directory provisioning and path validation (task 9.1)
Adds --record-dir CLI flag and sched_validate_record_filename() with canonical-path containment (realpath()-based, not textual-prefix) plus separator/".." rejection before any filesystem access. Adversarial review found and this task fixed two CWE-59 inode-aliasing bypasses: a direct hard link inside record_dir aliasing an outside file, and a compound variant where a symlink pointed at an in-directory file itself hard-linked outside record_dir. Both closed via st_nlink checks on the direct entry and the fully-resolved symlink target respectively, each verified with a live RED->GREEN reproduction. TOCTOU residual risk documented and deferred to task 9.2 (O_NOFOLLOW + fstat() re-check). 18/18 recording subtests, 24/24 scheduler suite, clang-format clean.
This commit is contained in:
parent
31ef5d7093
commit
4e5547ee55
SSH key fingerprint:
SHA256:8Vf2pZ3h6rJa7EhGK+WaAQ9Mta/apZvteW6g9U/WKKk
8 changed files with 889 additions and 30 deletions
|
|
@ -216,12 +216,13 @@
|
||||||
|
|
||||||
- [ ] 9. Core: Recording Control
|
- [ ] 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
|
- 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
|
- 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
|
- 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_
|
- _Requirements: 5.10_
|
||||||
- _Depends: 8.1_
|
- _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
|
- [ ] 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
|
- On start: take a full snapshot, write the replay header followed by the initial snapshot entry, begin tracking recording state
|
||||||
|
|
|
||||||
131
src/scheduler/include/recording.h
Normal file
131
src/scheduler/include/recording.h
Normal file
|
|
@ -0,0 +1,131 @@
|
||||||
|
#if !defined(_RECORDING_H_)
|
||||||
|
|
||||||
|
#define _RECORDING_H_ 1
|
||||||
|
|
||||||
|
#include <stddef.h>
|
||||||
|
|
||||||
|
/*
|
||||||
|
* 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
|
||||||
|
|
@ -102,6 +102,21 @@ typedef struct {
|
||||||
xcecli_ctx_t apictx;
|
xcecli_ctx_t apictx;
|
||||||
xce_cluster_config_t
|
xce_cluster_config_t
|
||||||
cluster_cfg; /* HA candidate list — empty means single-host mode */
|
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;
|
} xce_scheduler_t;
|
||||||
|
|
||||||
extern int sched_init(xce_scheduler_t*, xce_client_config_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_get_project/sched_add_project/sched_job_is_ready/
|
||||||
* sched_queue_is_ready relocated to core.h (scheduler-core spec, task 4.1)
|
* 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*. */
|
* -- 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_queue(xce_scheduler_t*, xce_queue_t*);
|
||||||
extern int sched_update_existing_job(xce_scheduler_t*, xce_job_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_apply_job_rusage(xce_scheduler_t*, xce_job_rusages_t*);
|
||||||
extern int sched_commit_decision(xce_scheduler_t*, xce_decision_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_pend_reason(xce_scheduler_t*, xce_preason_set_t*);
|
||||||
extern int sched_publish_dispatch(xce_scheduler_t*, xce_job_dispatch_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 void sched_worker_release_host_ctx(xce_sched_worker_t*);
|
||||||
extern int sched_meet_host_selection(xce_scheduler_t*, xce_sched_worker_t*,
|
extern int sched_meet_host_selection(xce_scheduler_t*, xce_sched_worker_t*,
|
||||||
xce_select_t*);
|
xce_select_t*);
|
||||||
extern int sched_check_shared_satisfy(xce_scheduler_t*, xvec_t*, xvec_idval_t*,
|
extern int sched_check_shared_satisfy(xce_scheduler_t*, xvec_t*, xvec_idval_t*,
|
||||||
xvec_idval_t*);
|
xvec_idval_t*);
|
||||||
extern int sched_check_host_satisfy(xce_scheduler_t*, xce_sched_worker_t*,
|
extern int sched_check_host_satisfy(xce_scheduler_t*, xce_sched_worker_t*,
|
||||||
xvec_t*, xvec_idval_t*, xvec_idval_t*);
|
xvec_t*, xvec_idval_t*, xvec_idval_t*);
|
||||||
extern int sched_collect_workers(xce_scheduler_t*, xlist_t*,
|
extern int sched_collect_workers(xce_scheduler_t*, xlist_t*,
|
||||||
zarray_iter_skip_fn_t,
|
zarray_iter_skip_fn_t,
|
||||||
sched_group_worker_fn_t);
|
sched_group_worker_fn_t);
|
||||||
extern int sched_charge_account(xce_scheduler_t*, xce_job_account_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_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_commit_job_alloc(xce_scheduler_t*, xce_sched_job_t*);
|
||||||
extern uint64_t sched_get_id(xce_scheduler_t*);
|
extern uint64_t sched_get_id(xce_scheduler_t*);
|
||||||
extern xce_alloc_t** sched_make_allocs(xce_scheduler_t*, char*, int, int*,
|
extern xce_alloc_t** sched_make_allocs(xce_scheduler_t*, char*, int, int*,
|
||||||
float*, int*);
|
float*, int*);
|
||||||
|
|
|
||||||
|
|
@ -11,6 +11,7 @@ scheduler_srcs = [
|
||||||
'src/snapshot_wire.c',
|
'src/snapshot_wire.c',
|
||||||
'src/snapshot_load.c',
|
'src/snapshot_load.c',
|
||||||
'src/decision.c',
|
'src/decision.c',
|
||||||
|
'src/recording.c',
|
||||||
'src/schedobj.c',
|
'src/schedobj.c',
|
||||||
'src/plugin.c',
|
'src/plugin.c',
|
||||||
'src/resource.c',
|
'src/resource.c',
|
||||||
|
|
|
||||||
|
|
@ -16,7 +16,6 @@
|
||||||
#include "resreq.h"
|
#include "resreq.h"
|
||||||
#include "utils.h"
|
#include "utils.h"
|
||||||
|
|
||||||
|
|
||||||
extern int scheduler_req_handler(xce_request_t*, void*);
|
extern int scheduler_req_handler(xce_request_t*, void*);
|
||||||
|
|
||||||
static char doc[] = "xce_scheduler";
|
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));
|
= MK_VERSION("xce v0.0.0.1-", STR(VERSION_COMMIT_ID));
|
||||||
const char* argp_program_bug_address = BUG_EMAIL;
|
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 {
|
struct arguments {
|
||||||
char* logfile;
|
char* logfile;
|
||||||
int verbose;
|
int verbose;
|
||||||
char* peers; /* --peers value (optional) */
|
char* peers; /* --peers value (optional) */
|
||||||
char* sim_file; /* --sim value (optional; Phase 3, not yet implemented) */
|
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[] = {
|
static struct argp_option options[] = {
|
||||||
|
|
@ -44,6 +49,13 @@ static struct argp_option options[] = {
|
||||||
{ "sim", 's', "FILE", 0,
|
{ "sim", 's', "FILE", 0,
|
||||||
"Replay FILE for simulation mode "
|
"Replay FILE for simulation mode "
|
||||||
"(Phase 3 -- not yet implemented)" },
|
"(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 }
|
{ 0 }
|
||||||
};
|
};
|
||||||
|
|
||||||
|
|
@ -64,6 +76,9 @@ static error_t parse_opt(int key, char* arg, struct argp_state* state)
|
||||||
case 's':
|
case 's':
|
||||||
arguments->sim_file = arg;
|
arguments->sim_file = arg;
|
||||||
break;
|
break;
|
||||||
|
case OPT_RECORD_DIR:
|
||||||
|
arguments->record_dir = arg;
|
||||||
|
break;
|
||||||
default:
|
default:
|
||||||
return ARGP_ERR_UNKNOWN;
|
return ARGP_ERR_UNKNOWN;
|
||||||
}
|
}
|
||||||
|
|
@ -109,13 +124,17 @@ static void sig_handler(int signo)
|
||||||
|
|
||||||
int main(int argc, char* argv[])
|
int main(int argc, char* argv[])
|
||||||
{
|
{
|
||||||
struct arguments arguments;
|
struct arguments arguments;
|
||||||
char* logfile = "/tmp/xce_scheduler.log";
|
char* logfile = "/tmp/xce_scheduler.log";
|
||||||
char* loglevel = "info";
|
char* loglevel = "info";
|
||||||
int logsize = 64 * 1024 * 1024;
|
int logsize = 64 * 1024 * 1024;
|
||||||
int logcount = 10;
|
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_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;
|
const xce_cluster_config_t* cluster_cfg_ptr = NULL;
|
||||||
|
|
||||||
memset(&arguments, 0x0, sizeof(arguments));
|
memset(&arguments, 0x0, sizeof(arguments));
|
||||||
|
|
@ -130,6 +149,13 @@ int main(int argc, char* argv[])
|
||||||
loglevel = verbose_to_loglevel(arguments.verbose);
|
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);
|
mylog_init(logfile, loglevel, logsize, logcount);
|
||||||
|
|
||||||
/* Req 5 AC 9 "Mode selection" (scheduler-core spec, task 5.2): --sim
|
/* 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",
|
strncpy(cluster_cfg.identity, "xce_scheduler",
|
||||||
sizeof(cluster_cfg.identity) - 1);
|
sizeof(cluster_cfg.identity) - 1);
|
||||||
if (xce_cluster_config_validate(&cluster_cfg) != 0) {
|
if (xce_cluster_config_validate(&cluster_cfg) != 0) {
|
||||||
fprintf(stderr,
|
fprintf(stderr, "xce_scheduler: invalid cluster config (missing "
|
||||||
"xce_scheduler: invalid cluster config (missing "
|
"identity?).\n");
|
||||||
"identity?).\n");
|
|
||||||
return 1;
|
return 1;
|
||||||
}
|
}
|
||||||
if (cluster_cfg.candidates_count > 3) {
|
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);
|
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
|
/* Req 5 AC 6 (scheduler-core spec, task 5.2): register the driver's
|
||||||
* decision callback -- replaces main.c's former inline
|
* decision callback -- replaces main.c's former inline
|
||||||
* `switch (decision->op)`. Registered once, before the first
|
* `switch (decision->op)`. Registered once, before the first
|
||||||
|
|
|
||||||
171
src/scheduler/src/recording.c
Normal file
171
src/scheduler/src/recording.c
Normal file
|
|
@ -0,0 +1,171 @@
|
||||||
|
#include <errno.h>
|
||||||
|
#include <limits.h>
|
||||||
|
#include <stdio.h>
|
||||||
|
#include <stdlib.h>
|
||||||
|
#include <string.h>
|
||||||
|
#include <sys/stat.h>
|
||||||
|
|
||||||
|
#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;
|
||||||
|
}
|
||||||
|
|
@ -393,6 +393,25 @@ sched_snapshot_save_load_test = executable(
|
||||||
dependencies: [scheduler_test_deps],
|
dependencies: [scheduler_test_deps],
|
||||||
install: false)
|
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.
|
# Register as meson tests so `meson test --suite scheduler` runs them all.
|
||||||
# detect_leaks=0 matches the suite convention (fixtures are not torn down).
|
# detect_leaks=0 matches the suite convention (fixtures are not torn down).
|
||||||
test('scheduler test', scheduler_test, suite: 'scheduler',
|
test('scheduler test', scheduler_test, suite: 'scheduler',
|
||||||
|
|
@ -453,3 +472,6 @@ test('scheduler driver session ordering test',
|
||||||
test('scheduler snapshot save load test',
|
test('scheduler snapshot save load test',
|
||||||
sched_snapshot_save_load_test, suite: 'scheduler',
|
sched_snapshot_save_load_test, suite: 'scheduler',
|
||||||
env: ['ASAN_OPTIONS=detect_leaks=0'])
|
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'])
|
||||||
|
|
|
||||||
487
src/scheduler/testsuite/test_recording_path_validation.c
Normal file
487
src/scheduler/testsuite/test_recording_path_validation.c
Normal file
|
|
@ -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 <errno.h>
|
||||||
|
#include <fcntl.h>
|
||||||
|
#include <stdio.h>
|
||||||
|
#include <stdlib.h>
|
||||||
|
#include <string.h>
|
||||||
|
#include <sys/stat.h>
|
||||||
|
#include <unistd.h>
|
||||||
|
|
||||||
|
#include <unity/unity.h>
|
||||||
|
|
||||||
|
/* 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="<tmp>/xce" vs a SIBLING directory
|
||||||
|
* "<tmp>/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();
|
||||||
|
}
|
||||||
Loading…
Add table
Add a link
Reference in a new issue