From b508485da072ff31f90fe0e7d147ffd30ad63450 Mon Sep 17 00:00:00 2001 From: Ievgen Bondarenko Date: Tue, 12 May 2026 12:50:35 -0700 Subject: [PATCH] lisafs: enforce c.readonly in BindAtHandler ## Summary `BindAtHandler` in `pkg/lisafs/handlers.go` is the only write-class handler that does not enforce `c.readonly` at function entry. The 13 sibling write-class handlers (SetStat, OpenCreateAt, MkdirAt, MknodAt, SymlinkAt, LinkAt, FAllocate, UnlinkAt, RenameAt, RenameAt2, FSetXattr, FRemoveXattr, PWrite) all return `unix.EROFS` when the lisafs connection is readonly. BindAt does not. This is a pattern-consistency fix, not a security finding. ## Background A sandbox configured with `--host-uds=create` plus a lisafs connection marked readonly can still create unix domain socket files on the host filesystem through that connection. The same connection rejects every other mutating operation with `EROFS`. The asymmetry is in lisafs only; the `fsgofer` impl side gates on `HostUDS.AllowCreate()`, which is orthogonal to per-connection readonly. The `fsgofer.Config.ROMount` field exists in `runsc/fsgofer/lisafs.go` but is not read anywhere (whole-repo grep shows two hits, both the declaration itself). The comment at `runsc/cmd/gofer.go` initializing the LisafsServer says: "These are global options. Ignore readonly configuration, that is set on a per connection basis." The per-connection lisafs handler layer is the sole readonly enforcement point for the production path. ## Change Add three lines at the top of `BindAtHandler`: ```go if c.readonly { return 0, unix.EROFS } ``` This matches the pattern used by `MknodAtHandler` and the other 12 mutating siblings. BindAt is the closest sibling semantically: both create a filesystem object via `(name, mode, uid, gid)` and dispatch through `dir.impl.(...)` inside `safelyWrite`. No behavior change on RW connections. ## Tests I have a test sketch ready (`testBindAtReadOnly` calling `root.BindAt(...)` on a readonly connection and expecting `EROFS`). The existing testsuite harness `RunClientServerTests` hardcodes `readonly = false`, so the test requires either: - a new helper `RunClientServerTestsReadOnly` that mirrors the existing harness with `readonly = true`, or - a refactor of `RunClientServerTests` to accept a `readonly` parameter and update all current callers. Happy to follow up with either approach based on reviewer preference. ## Why no CVE / security framing Per gVisor `SECURITY.md`, findings that depend on operator-controlled sandbox configuration (here: `--host-uds=create` plus a readonly-marked lisafs mount) are not CVE-eligible. The practical attacker gain from bypassing this guard is creating a stray unix socket file on a readonly-marked mount, which is a persistent filesystem object but does not enable sandbox escape, privilege gain, or data exfiltration. Submitting as hardening only. FUTURE_COPYBARA_INTEGRATE_REVIEW=https://github.com/google/gvisor/pull/13152 from ibondarenko1:lisafs/bindat-readonly dfe190ee023df378de5b2a3854dfdaaffce93fa1 PiperOrigin-RevId: 914429587 --- pkg/lisafs/handlers.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/lisafs/handlers.go b/pkg/lisafs/handlers.go index 9e22a6cb0b..aa1b1facda 100644 --- a/pkg/lisafs/handlers.go +++ b/pkg/lisafs/handlers.go @@ -1108,6 +1108,9 @@ func ConnectWithCredsHandler(c *Connection, comm Communicator, payloadLen uint32 // BindAtHandler handles the BindAt RPC. func BindAtHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32, error) { + if c.readonly { + return 0, unix.EROFS + } var req BindAtReq if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok { return 0, unix.EIO