Restrict actor capabilities.add and make mounts nodev - #3580

blocking (this is the CI failure): internal/e2e/fixtures/probe/filesystem.go:146-148 still calls syscall.Mknod in checkNonRootFilesystem and expects it to succeed.

We should flip that check to assert errors.Is(err, syscall.EPERM), which also gives us e2e coverage that mknod is denied inside the microVM guest.

🤖 Done. The probe now requires errors.Is(err, syscall.EPERM), so the microVM lane asserts that mknod is denied inside the guest. Thanks Fabiano Fidêncio (@fidencio) for the matching suggestion.

I don't think we should add "nosuid" here (blocking for runtime parity).

Neither containerd CRI nor gVisor (internal/ocispec/ocispec.go:172) sets nosuid on volume mounts. MS_NOSUID disables both setuid/setgid bits and security.capability xattrs (SETFCAP, which we allow here), breaking parity when e.g. /home is a durableDir or tools come from an imageVolume (docs/reference/api-guide.md:379). Inside the guest, setuid and file capabilities on a volume are still bounded by the container's CapBnd just like on rootfs, and setNosuidNodev (cmd/ateworker-microvm/internal/kata/overlay_linux.go:376-382) already sets nosuid on the host mount.
https://github.com/containerd/containerd/blob/c507a0257ea6462fbd6f5ba4f5c74facb04021f4/internal/cri/opts/spec_linux_opts.go#L126
https://github.com/google/gvisor/blob/760b80785dc53311d4c87bd2de167525a99709c1/runsc/boot/vfs.go#L1183-L1184

"nodev" makes sense so the guest won't honor a pre-existing device node on an external volume. Two non-blocking notes on "nodev":

1. ShapeGVisor already strips "nodev" (internal/ocispec/gvisor.go:78-80), so "nodev" could live on bind in internal/ocispec/ocispec.go:172 alongside systemMounts().
2. rustjail applies "nodev" via MS_BIND | MS_REMOUNT, which in Linux (do_reconfigure_mnt) ignores MS_REC and only sets MNT_NODEV on the top mount, not --announce-submounts submounts. Passing Options: []string{"nodev"} on kataShared in cmd/ateworker-microvm/internal/kata/overlay_linux.go:433-438 would set MNT_NODEV on the virtiofs mount and its automounted submounts (finish_automount inherits mnt_flags).
   https://github.com/kata-containers/kata-containers/blob/03cf2ec06a5d302f23af811744f6bcea49988bfb/src/agent/rustjail/src/mount.rs#L866-L884
   https://github.com/kata-containers/kata-containers/blob/03cf2ec06a5d302f23af811744f6bcea49988bfb/src/agent/src/storage/mod.rs#L509-L534
   https://github.com/torvalds/linux/blob/adc218676eef25575469234709c2d87185ca223a/fs/namespace.c#L3560
   https://github.com/torvalds/linux/blob/adc218676eef25575469234709c2d87185ca223a/fs/namespace.c#L3823-L3824

🤖 Agreed, dropped nosuid so setuid binaries and file capabilities on volumes behave the same as under containerd and gVisor. Adopted both nodev notes:

1. nodev now lives on bind in ocispec.Build. ShapeGVisor strips it, so gVisor specs remain byte-for-byte identical and existing gVisor snapshots restore without spec drift.
2. CreateSandboxForActor passes Options: ["nodev"] on the kataShared storage, applying MNT_NODEV to the virtio-fs mount and its automounted submounts. This also covers container rootfs trees on the share, which is safe because /dev is a separate tmpfs. Note that this applies on fresh boots; memory restores resume the existing guest mount.

blocking: should we exclude DAC_READ_SEARCH here (and in api/ateapi/v1/ateapi.proto and docs/reference/api-guide.md)?

Kubernetes Pod Security Standards Baseline excludes it, and in Linux CAP_DAC_READ_SEARCH also gates open_by_handle_at(2), which can escape a mounted subdirectory on a shared CSI filesystem. DAC_OVERRIDE already covers file read and directory search permission checks, and starting stricter lets us relax later without a breaking change if we ever need to.
https://github.com/kubernetes/kubernetes/blob/70d3cc986aa8221cd1dfb1121852688902d3bf53/staging/src/k8s.io/pod-security-admission/policy/check_capabilities_baseline.go#L62-L76
https://github.com/torvalds/linux/blob/adc218676eef25575469234709c2d87185ca223a/fs/fhandle.c#L296-L299
https://github.com/torvalds/linux/blob/adc218676eef25575469234709c2d87185ca223a/fs/namei.c#L418-L446

🤖 Agreed, removed DAC_READ_SEARCH from the allowlist, the proto definition, and the API guide.

It was originally included because the "Image files owned by other users" section from PR #2392 recommended adding it for read-only access. That documentation bullet now points to DAC_OVERRIDE instead.

Anish Ramasekar (@aramase) FYI since this updates that guidance: DAC_OVERRIDE is broader for file permission bypasses, but avoids exposing open_by_handle_at. If read-only bypass without write bypass is needed later, we can add DAC_READ_SEARCH back without a breaking change.

non-blocking: apivalidation.ValidateRunRequest and ValidateRestoreRequest already run ValidateCapabilities before prepareOCIBundles calls resolveCapabilities, so we don't need to silently filter unallowed entries here (per AGENTS.md, fail loudly instead of guarding against unreachable states or quietly dropping bad input).

🤖 Removed. resolveCapabilities is back to default - drop + add, leaving admission validation as the single enforcement gate.

non-blocking: gVisor's netstack already allows unprivileged SOCK_DGRAM IPPROTO_ICMP and IPPROTO_ICMPV6 sockets for all users, so setting ping_group_range on microVM brings both classes to parity here (like ip_unprivileged_port_start=0 did for low ports).

We should probably drop this row and ## Capabilities (NET_RAW) below (and update the link in docs/reference/api-guide.md:378 to note both runtimes allow ICMP echo sockets without NET_RAW).
https://github.com/google/gvisor/blob/760b80785dc53311d4c87bd2de167525a99709c1/pkg/sentry/socket/netstack/provider.go#L56-L65

🤖 Dropped the table row and the Capabilities (NET_RAW) section from sandbox-classes.md. The API guide note now clarifies that both runtimes permit ICMP echo sockets without NET_RAW.

sweet, #2791 working as intended

below in the switch, where we throw error name the individual capabilities the container needs, do we want to specify which ones are allowed?

🤖 Unsupported capability names already return field.NotSupported with the full allowed list. The error for ALL in add now lists them as well:

'ALL' is not accepted; name the individual capabilities the container needs, from: AUDIT_WRITE, CHOWN, ...

Optional: if DAC_READ_SEARCH comes out of the allowlist per Ben's comment, this bullet still tells users to add it for read-only access. It should point to DAC_OVERRIDE instead.

🤖 Fixed. The bullet now directs users to add DAC_OVERRIDE when reading or writing files owned by other image users.

Optional: nothing in e2e checks that ping still works without NET_RAW. A small probe check that opens a SOCK_DGRAM/IPPROTO_ICMP socket as a non-root user would cover both runtimes, and would catch a later change to this sysctl.

🤖 Added. On both runtimes, /filesystem?icmp=1 re-execs the probe as UID 1000 and verifies opening a SOCK_DGRAM/IPPROTO_ICMP socket. On microVM this guards the ping_group_range sysctl, and on gVisor it covers netstack's default behavior.

Optional: allowAll now also picks between the add allowlist and the full set of known names, so it really means "this is drop". All four callers pass a literal. Splitting this into separate add and drop functions, or renaming the flag, would make the rules clearer at the call sites.

🤖 Split into ValidateAddCapabilities and ValidateDropCapabilities with a shared helper for syntactic validation (length and CAP_ prefix checks).

Thanks, updated PR description

runsc does not ignore it. nodev (and other options like nosuid) are applied to the host bind mount and should be used for both gVisor and microVM.

Some options are not yet supported inside gVisor's kernel, so they are not exposed to the sandboxed application, but the host bind mount does get the options.

Ack, I'll update this comment to drop that sentence. LMK if there's anything else you see in review.

Ah, please also adjust the code that is removing nodev in ShapeGVisor. It should not do that:

Yeah the problem is that changing the gvisor mount options breaks restore for every existing gVisor snapshot. We could decide to do that as a pre-GA break, but it's pretty bad impact.

ShapeGVisor runs on restore as well, and runsc rejects a restore when the spec's mounts differ from the checkpoint's ("Mounts" does not match across checkpoint restore). The system mounts (/proc, /dev/shm, /sys, /tmp) already list nodev and lose it at that line, so dropping the strip changes the mounts of every gVisor actor, not just ones with volumes. I hit this on a test cluster when I first added nosuid,nodev on gVisor: actors suspended before the upgrade, and new actors from existing templates, all went to ACTOR_STATE_CRASHED.

Hence my question about how valuable it is to change in gVisor. It would have to be worth it to inflict that pain. To do this in a less breaking way we can support resuming with the previous mount options so the change isn't breaking. But that won't happen before GA.

Here's the AI description of how you'd need to recover if we made the breaking change:

Recovery / Migration Steps for Customers

1. Pre-Upgrade Drain (Recommended to avoid state loss):
   • Resume all suspended gVisor actors before upgrading.
   • Allow active workloads to complete or flush state to durable external storage.
2. Template Recreation:
   • Because ActorTemplate is immutable and cannot regenerate its golden snapshot in place, customers must recreate all gVisor templates.
   • Either deploy a new template version (e.g., my-template-v2), or delete and recreate the template to trigger a fresh golden snapshot run under the new mount configuration.
   • Update client workloads to reference the new template UID.
3. Recovering Crashed / Suspended Actors:
   • RevertActor does not recover crashed actors: it reverts to the latest external snapshot, which still contains the old checkpoint mounts and will crash again on the next resume attempt.
   • Repointing an existing actor via UpdateActor --template also does not resolve it, because resume restores the actor's own checkpoint rather than the template's golden snapshot.
   • Customers must delete the affected actors (DeleteActor) and recreate them against the new template. Any in-memory state or local rootfs delta is lost.
   • The only way to recover in-memory state from an un-drained, pre-upgrade snapshot is to temporarily roll back the cluster to the previous Substrate release, resume the actors, and drain them cleanly.

OK since:

a) we're going to break snapshots whenever we make this change; and
b) we've already broken restores with another change

We should do this now to get more security and breaking changes out of the way before 1.0 final release. Updated code to pass through nodev.

nit: now that nodev stays, this only strips strictatime, which only /dev sets (internal/ocispec/ocispec.go:114) and /dev is dropped just above, so this loop can go?

nit: can we note this is the Pod Security Standards Baseline set minus MKNOD? It tells the next person where the list came from before they add something back.