diff options
Diffstat (limited to 'bin')
| -rw-r--r-- | bin/main.ml | 99 |
1 files changed, 90 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 |