lei.chen/myxce
1
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

feat(scheduler-core): implement recording directory provisioning and path validation (task 9.1)

Browse source 
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:
Rongsong Shen 2026-08-05 05:39:22 +00:00
parent 31ef5d7093
commit 4e5547ee55
Signed by: rshen
SSH key fingerprint: SHA256:8Vf2pZ3h6rJa7EhGK+WaAQ9Mta/apZvteW6g9U/WKKk
8 changed files with 889 additions and 30 deletions
Download patch file Download diff file Expand all files Collapse all files

131
src/scheduler/include/recording.h Normal file
View 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

49
src/scheduler/include/scheduler.h
View file

@ -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*);