sandlock learn follow-up fixes - #165
Conversation
|
…pens / during startup which caused dedup to subsume all other observed paths into read = ["/"]
145d883 to
4920d68
Compare
congwang-mk
left a comment
There was a problem hiding this comment.
Inline comments for the issues found in review. The individual fixes (N1, N2, N4, A1, C1, D1) look solid and CI is green, but there are three blocking issues (protected-root regression, --merge data loss, unchecked mknod device nodes) plus three correctness issues below.
| /// Protected: no legitimate sandboxed workload needs a recursive grant here. | ||
| /// Write collapse to these is skipped + error. Read collapse never fires here. | ||
| const PROTECTED_PATHS: &[&[u8]] = &[b"/", b"/root"]; | ||
| const PROTECTED_PATHS: &[&[u8]] = &[b"/root"]; |
There was a problem hiding this comment.
Blocking: removing / from PROTECTED_PATHS regresses read-collapse safety.
The new root guard was only added to collapse_write_paths, but classify_path("/") now returns Normal, which changes two read paths:
collapse_by_threshold: if N or more reads share parent/, it now silently emitsread = ["/"], a whole-filesystem read grant including/rootand the credential dirs. Previously/wasProtected, so the individual paths were kept.- The
--collapse-prefixvetting inrunchecksclassify_path(prefix) != PathTier::Normal, so--collapse-prefix /no longer requires--force-sensitive-collapse.
Suggestion: keep / in PROTECTED_PATHS and special-case the warning text inside collapse_write_paths instead of declassifying root globally.
| profile_out.limits.open_files = Some((fds * 2).max(32) as u32); | ||
| // TODO: learn limits.open_files via supervisor once open_files enforcement is implemented. | ||
|
|
||
| // --merge: union observed profile into an existing one. |
There was a problem hiding this comment.
Blocking: --merge silently destroys existing profile content.
Only program, filesystem.read/write, network.allow, allow_bind, and limits are carried over. Everything else in the existing file is dropped and then overwritten in place: filesystem.deny, chroot, mount, on_exit/on_error, network.deny, deny_bind, port_remap, and the entire [config], [determinism], [http], [syscalls] sections. A hand-added deny rule is exactly the kind of thing an operator puts into a learned profile, and merge erases it.
Additionally, port_spec_to_u16 drops any PortSpec::Spec that is not a single number, so an existing allow_bind = ["9000-9005"] range vanishes from the merged output.
A union operation should preserve every field it does not merge (start from existing and union the observed fields into it) and keep non-numeric port specs verbatim.
| /// Handle mknodat: create a file-system node in the upper layer. | ||
| /// | ||
| /// Returns `Err(QuotaExceeded)` when the node would exceed `max_disk`. | ||
| pub fn handle_mknod(&mut self, path: &str, mode: u32, dev: u64) -> Result<bool, BranchError> { |
There was a problem hiding this comment.
Blocking (security): mode/dev pass through unchecked, allowing device-node creation when the supervisor runs as root.
mknodat is executed by the supervisor in the upper layer with the supervisor's privileges (COW writes never need the child's Landlock perms). When sandlock runs as root (critest, container hosts), a sandboxed process can do mknod("x", S_IFBLK, makedev(8,0)), then open the node through the COW open-on-behalf path and get raw access to the host disk: a sandbox escape. Unprivileged runs are safe only because the kernel refuses without CAP_MKNOD.
Suggestion: restrict the type bits to S_IFIFO | S_IFSOCK | S_IFREG (and treat 0 as S_IFREG), returning EPERM for S_IFCHR/S_IFBLK.
|
|
||
| /// Resolve symlinks to get the canonical path. Falls back to the original | ||
| /// if the path doesn't exist yet (e.g. COW-intercepted creates). | ||
| fn canonicalize_or_keep(p: PathBuf) -> PathBuf { |
There was a problem hiding this comment.
Correctness: full-path canonicalize resolves the wrong path for link-affecting syscalls.
unlinkat and renameat2 operate on the link itself, not its target. If the workload runs rm /tmp/link where /tmp/link -> /data/file, canonicalize yields /data/file, so learn records write on /data (overbroad, and wrong) instead of /tmp (needed), and the replayed run gets a denial.
Full-path canonicalize is only correct for syscalls that follow symlinks (open, truncate). For unlink/rename/the symlinkat linkpath, canonicalize the parent directory and rejoin the final component instead.
| self.connects.lock().unwrap().insert(format!("{proto}://{ip}:{port}")); | ||
| // Skip UDP connect to port 80: glibc getaddrinfo uses connect() on a | ||
| // temporary UDP socket to port 80 purely to probe routing, no data sent. | ||
| if proto == "udp" && port == 80 && event.syscall == "connect" { |
There was a problem hiding this comment.
Correctness: this filter does not match the PR description and is both over- and under-broad.
The PR text says unspecified-peer (0.0.0.0/::) probes are dropped, but there is no is_unspecified() check here; the code drops every UDP connect() to port 80 regardless of destination. That silently under-learns any real UDP:80 traffic (e.g. HTTP/3 on the alt port), while glibc address-sorting probes for a workload resolving a different service port would still get through, since only port 80 is matched.
At minimum, align the code with the description. A tighter heuristic would key on the destination address or on connect-without-subsequent-send rather than a hardcoded port.
| } | ||
|
|
||
| // Save exec+args for the denial hint before profile_program_spec is consumed below. | ||
| let profile_hint_cmd: Option<String> = profile_program_spec.as_ref().and_then(|spec| { |
There was a problem hiding this comment.
Correctness: the denial hint suggests the wrong command.
profile_hint_cmd comes from the profile's [program] section. If the user runs sandlock run -p prof.toml -- other-cmd and it fails, the hint tells them to re-learn with the profile's original command rather than the command that just failed. Prefer args.cmd when non-empty, falling back to the profile spec.
…ld and --collapse-prefix
…ink, rename, symlink)
a33d669 to
053d4b9
Compare
|
Thanks for the update, @ghazariann . |
Fixes issues identified in the review of PR #113. Additional gaps surfaced in PR #113 review thread and PR #155 discussion are also included.
bind()ports were never recorded in the profile. Added a"bind"arm inlearn.rsthat writesevent.portintoobserver.allow_bind.::1:80instead of `[::1]:80.udp://<ip>:80entries from glibc getaddrinfo address-sorting probes appeared in the profile. glibc callsconnect()on a temporary UDP socket to each resolved address to probe routing (no data is sent). Dropped as a heuristic; the correct fix (connect-without-send tracking) requires fd inSyscallEvent.sendmmsgdecoding stopped at the firstmmsghdrentry, under-learning multi-destination sends. Fixed by reading eachmmsghdrentry's destination address from the child's memory and emitting a separate observation event for each in a loop.execvewere inserted intopending_mapsunder their TID, not the process TGID. Fixed by looking up the thread group leader PID from/proc/<tid>/statusbefore inserting intopending_maps.mknodat/mknodwere absent from the COW dispatch table andpolicy_event_syscalls '. Added COW, policy-event, and learn observer coverage formknodat/mknod. AF_UNIXbind()was also skipped; the parent directory is now recorded forMAKE_SOCK` grant./proc-polling sampler could miss short-lived processes, readVmHWM(not the same metric the enforcer uses), and didn't see child processes. Addedpeak_mem_usedandpeak_proc_counttoResourceState. The supervisor already intercepts everymmap/brkand everyfork/clone, so peaks come for free.learnreads them viasandbox.resource_peaks()after the workload exits and drops the sampler loop entirely.canonicalize_or_keep()to all fs syscall paths before recording. The executed binary canonicalization was previously solved in 1719c6a by reading/proc/<pid>/exe