diff options
| -rw-r--r-- | bin/main.ml | 99 | ||||
| -rw-r--r-- | test/cli.t | 48 |
2 files changed, 138 insertions, 9 deletions
diff --git a/bin/main.ml b/bin/main.ml index 4f1fbf2..6f3cbd5 100644 --- a/bin/main.ml +++ b/bin/main.ml @@ -561,6 +561,63 @@ let read_manifest out = | Error _ -> [] | Ok contents -> String.split_on_char '\n' contents |> List.filter (fun l -> l <> "") +(* Fix round 1 (coordinator review), CRITICAL: a manifest entry is + UNTRUSTED input the moment [--prune] reads it back. The manifest is a + plain-text file that lives INSIDE the very tree this feature exists to + have committed into a git repo -- an ordinary bad merge or a hand-edit is + enough to put an arbitrary path in it, no attacker required. Without a + check, an entry like "../outside/CANARY.txt" resolves, via + [Filename.concat out entry], to a path OUTSIDE [out], and the prune loop + below would [Sys.remove] it -- deleting a file [publish] never wrote, + breaking the "never deletes a file it does not own" contract outright. + + Two independent checks, deliberately, because either alone is easy to + regress later without anyone noticing in review: + + 1. STRUCTURAL ([manifest_entry_is_safe]) -- reject an entry that is + absolute, or that has a ".." path component anywhere. Split on '/' + and compare COMPONENTS, never a bare substring test: substring- + matching ".." would wrongly reject a legitimate name like + "foo..bar", which contains the two characters but has no ".." + component of its own. + 2. CONTAINMENT ([resolves_under]) -- even an entry that passes check 1 + is not trusted until the path it actually resolves to, symlinks + included, is verified to sit under [out]. [Unix.realpath] resolves + symlinks as well as "..", so this also catches an entry that a + symlink planted inside [out] could use to defeat check 1 alone. A + plain string-prefix compare is not enough by itself either: + "/tmp/pub1" is a byte-prefix of "/tmp/pub1-evil", a directory that is + not nested inside it at all, so [is_under] insists the character + right after the prefix is the path separator (or that the paths are + identical). *) +let manifest_entry_is_safe entry = + entry <> "" + && entry.[0] <> '/' + && not (List.mem ".." (String.split_on_char '/' entry)) + +let is_under ~root path = + let root = + if String.length root > 1 && root.[String.length root - 1] = '/' then + String.sub root 0 (String.length root - 1) + else root + in + String.equal path root + || (String.length path > String.length root + && String.sub path 0 (String.length root) = root + && path.[String.length root] = '/') + +(* [Unix.realpath] requires the path to exist, which is fine here: every + caller below checks [Sys.file_exists]/[Sys.readdir] first. Any failure + (missing path, dangling symlink, permission error) is treated as "not + contained" -- refuse to act rather than guess. *) +let resolves_under out p = + match Unix.realpath out with + | exception (Unix.Unix_error _ | Sys_error _) -> false + | out_real -> ( + match Unix.realpath p with + | exception (Unix.Unix_error _ | Sys_error _) -> false + | p_real -> is_under ~root:out_real p_real) + (* [--prune] deletes the FILES a stale manifest entry names, but that alone can leave their parent directories (ef/<year>/<mm>/, then ef/<year>/) empty behind them -- and an empty directory still makes `test -d @@ -568,9 +625,18 @@ let read_manifest out = an old year is gone. Walk upward from each deleted file's own directory, removing it while it is empty, stopping at (never including) [out] itself: [out] is the caller's own directory, never ours to remove, even - when it is empty. *) + when it is empty. The same containment discipline as the file deletions + above applies here too ([resolves_under]), not only structurally (this + function is only ever reached via a [p] the file-deletion path already + validated, but re-checking each directory step is the belt to that + entry's braces -- see the two-layer reasoning above). *) let rec prune_empty_dirs ~out dir = - if dir <> out && String.length dir > String.length out && Sys.file_exists dir then + if + dir <> out + && String.length dir > String.length out + && Sys.file_exists dir + && resolves_under out dir + then match Sys.readdir dir with | [||] -> (try Unix.rmdir dir with Unix.Unix_error _ -> ()); @@ -679,16 +745,31 @@ let publish_report ~from_y ~to_y ~out ~overlays ~dtstamp ~prune = emit "schema/day-v1.json" schema; emit "index.html" (index_html ~from_y ~to_y); let now = List.sort compare !written in + (* Every stale entry is validated TWICE before anything is removed -- + see [manifest_entry_is_safe]/[resolves_under]'s own comment above for + why both layers exist. A rejected entry is skipped and warned about on + stderr, never fatal: a corrupt or hand-mangled manifest must not make + `publish` itself unusable -- it completes, having refused to act on + the bad line. *) if prune then List.iter (fun old -> - if not (List.mem old now) then begin - let p = Filename.concat out old in - if Sys.file_exists p then begin - Sys.remove p; - prune_empty_dirs ~out (Filename.dirname p) - end - end) + if not (List.mem old now) then + if not (manifest_entry_is_safe old) then + Printf.eprintf + "colitur: refusing to prune manifest entry %S (absolute path or .. component) +" old + else begin + let p = Filename.concat out old in + if Sys.file_exists p then + if resolves_under out p then begin + Sys.remove p; + prune_empty_dirs ~out (Filename.dirname p) + end + else + Printf.eprintf "colitur: refusing to prune %s (resolves outside %s) +" p out + end) (read_manifest out); write_file (Filename.concat out manifest_name) (String.concat "\n" now ^ "\n"); Printf.printf "colitur: wrote %d files to %s\n" (List.length now) out @@ -1004,3 +1004,51 @@ ignored, the same discipline as everywhere else: $ colitur day 2027 --out /tmp/pub3 --prune colitur: --out/--prune have no effect on `day`; refusing rather than ignoring them [2] + +--prune's manifest-driven deletion is hardened against a manifest entry it +did not itself write (fix round 1, F1, CRITICAL): the manifest lives INSIDE +the tree publish writes into, so a bad merge or a hand-edit can put an +arbitrary path in it -- no attacker required. This is the exact CANARY +reproduction the finding was raised with: a ".." entry appended to the +manifest must never let --prune delete outside --out. + + $ rm -rf /tmp/pub-sec /tmp/pub-sec-outside + $ mkdir -p /tmp/pub-sec-outside + $ touch /tmp/pub-sec-outside/CANARY.txt + $ colitur publish --from 2027 --to 2027 --out /tmp/pub-sec >/dev/null + $ echo '../pub-sec-outside/CANARY.txt' >> /tmp/pub-sec/.colitur-manifest + $ colitur publish --from 2028 --to 2028 --out /tmp/pub-sec --prune >/dev/null + colitur: refusing to prune manifest entry "../pub-sec-outside/CANARY.txt" (absolute path or .. component) + $ test -f /tmp/pub-sec-outside/CANARY.txt && echo canary-survives + canary-survives + +The same run's own legitimate stale entries (2027's files, superseded by +2028) still prune normally -- the hardening does not disable pruning, only +unsafe entries: + + $ test -d /tmp/pub-sec/ef/2027 || echo 2027-pruned-normally + 2027-pruned-normally + +An absolute-path entry is refused the same way, not only a ".." one: + + $ echo '/tmp/pub-sec-outside/CANARY.txt' >> /tmp/pub-sec/.colitur-manifest + $ colitur publish --from 2028 --to 2028 --out /tmp/pub-sec --prune >/dev/null + colitur: refusing to prune manifest entry "/tmp/pub-sec-outside/CANARY.txt" (absolute path or .. component) + $ test -f /tmp/pub-sec-outside/CANARY.txt && echo canary-still-survives + canary-still-survives + +A legitimate filename that merely CONTAINS two dots -- but has no ".." path +COMPONENT -- is not caught by the same check, proving it is not +over-broad: it still prunes normally when stale. + + $ touch /tmp/pub-sec/ef/2027..old.json + $ echo 'ef/2027..old.json' >> /tmp/pub-sec/.colitur-manifest + $ colitur publish --from 2029 --to 2029 --out /tmp/pub-sec --prune >/dev/null + $ test -f /tmp/pub-sec/ef/2027..old.json || echo dotted-name-pruned + dotted-name-pruned + +...and that same run is an ordinary --prune cycle in every other respect -- +2028's own files, now stale relative to 2029, are gone too: + + $ test -d /tmp/pub-sec/ef/2028 || echo pruned-2028 + pruned-2028 |