An unrecognized BranchAction discriminant silently commits the COW branch

[dzerik]

sandlock_sandbox_builder_on_exit and ..._on_error map an unrecognized discriminant to BranchAction::Commit, so a binding that passes a bad value gets the change set merged into the workdir instead of an error.

crates/sandlock-ffi/src/lib.rs:228-232:

let action = match action {
    1 => BranchAction::Abort,
    2 => BranchAction::Keep,
    _ => BranchAction::Commit,
};

Commit is the most destructive of the three: Abort and Keep both leave the workdir untouched, Commit writes to it.

Reproduction

Verified on main (0.8.5, x86-64), driving the C ABI directly so the SDK's enum validation is out of the way — which is exactly the position a hand-written binding is in.

import ctypes, os, shutil, tempfile

lib = ctypes.CDLL("target/release/libsandlock_ffi.so")
P = ctypes.c_void_p

def bfn(name, *extra):
    f = getattr(lib, name); f.restype = P; f.argtypes = [P] + list(extra); return f

b_new     = lib.sandlock_sandbox_builder_new; b_new.restype = P; b_new.argtypes = []
b_read    = bfn("sandlock_sandbox_builder_fs_read", ctypes.c_char_p)
b_workdir = bfn("sandlock_sandbox_builder_workdir", ctypes.c_char_p)
b_storage = bfn("sandlock_sandbox_builder_fs_storage", ctypes.c_char_p)
b_on_exit = bfn("sandlock_sandbox_builder_on_exit", ctypes.c_uint8)

lib.sandlock_sandbox_build.restype = P
lib.sandlock_sandbox_build.argtypes = [P, ctypes.POINTER(ctypes.c_int),
                                       ctypes.POINTER(ctypes.c_char_p)]
lib.sandlock_run.restype = P
lib.sandlock_run.argtypes = [P, ctypes.c_char_p,
                             ctypes.POINTER(ctypes.c_char_p), ctypes.c_uint]

def trial(action_value, label):
    wd, st = tempfile.mkdtemp(), tempfile.mkdtemp()
    b = b_new()
    for p in (b"/usr", b"/lib", b"/lib64", b"/bin"):
        if os.path.exists(p.decode()):
            b = b_read(b, p)
    b = b_workdir(b, wd.encode())
    b = b_storage(b, st.encode())
    b = b_on_exit(b, action_value)

    err, msg = ctypes.c_int(0), ctypes.c_char_p()
    policy = lib.sandlock_sandbox_build(b, ctypes.byref(err), ctypes.byref(msg))
    assert policy, (err.value, msg.value)

    cmd = [b"/bin/sh", b"-c", b"echo written-by-guest > guest.txt"]
    arr = (ctypes.c_char_p * len(cmd))(*cmd)
    lib.sandlock_run(policy, None, arr, len(cmd))

    landed = os.path.exists(os.path.join(wd, "guest.txt"))
    print(f"{label:32} guest changes in workdir: {'YES (committed)' if landed else 'no'}")
    shutil.rmtree(wd, ignore_errors=True); shutil.rmtree(st, ignore_errors=True)

for v, l in [(0, "0 = Commit"), (1, "1 = Abort"), (2, "2 = Keep")]:
    trial(v, l)
for v in (7, 99, 255):
    trial(v, f"{v} = not a valid discriminant")

Output:

0 = Commit                       guest changes in workdir: YES (committed)
1 = Abort                        guest changes in workdir: no
2 = Keep                         guest changes in workdir: no

7 = not a valid discriminant     guest changes in workdir: YES (committed)
99 = not a valid discriminant    guest changes in workdir: YES (committed)
255 = not a valid discriminant   guest changes in workdir: YES (committed)

Every garbage value lands on the one action that modifies the workdir, and sandlock_sandbox_build returns success — the caller has no way to notice.

Why this reads as a missed spot rather than a new class

The same class was already fixed in this file by #92. Two hundred lines below, crates/sandlock-ffi/src/lib.rs:2152 does the opposite:

_ => Verdict::Deny,

with policy_ret_unrecognized_values_fail_closed() at :2893 pinning it. The BranchAction conversion was simply not part of that sweep.

Why the tests do not catch it

No coverage at all: on_exit, on_error and BranchAction appear nowhere under crates/sandlock-ffi/tests/. The core-level branch-action tests construct the enum directly and never cross the ABI, so the conversion is untested from either side.

Suggested fix

Reject the unknown value instead of choosing for the caller. These setters return the builder and have no error channel, so the cheapest fail-closed option is to map an unrecognized discriminant to Abort — the action that cannot destroy anything — and say so in the doc comment. If you would rather have a hard error, the setters need the err / err_msg treatment sandlock_sandbox_build already uses (crates/sandlock-ffi/src/lib.rs:903), which is a wider change.

A regression test shaped like policy_ret_unrecognized_values_fail_closed would fit next to it.

Happy to send a patch once you pick one.

[congwang-mk]

Confirmed, and thanks for the precise framing: every garbage value landing on the one action that writes to the workdir, with sandlock_sandbox_build reporting success, is exactly the failure mode the #92 sweep existed to kill.

Picking between your two options: neither, quite. I want the error latched in the builder and surfaced at build(), which is close to your "wider change" but cheaper than it sounds.

Reasoning:

• The ffi: fail closed on unrecognized policy_fn return values #92 precedent does not transfer cleanly here. policy_ret_to_verdict runs at runtime inside a callback with no error path at all, so coercing to Deny was the only option. The builder is different: it already has a deferred error channel (sandlock_sandbox_build's err/err_msg), and a bad discriminant is a static bug in the binding, not a runtime condition. Static bugs deserve loud failures.
• Mapping to Abort is host-safe but silently throws away the guest's work, which is still the wrong behaviour, just wearing a safer costume. The caller asked for something; nobody finds out they did not get it.
• The mechanism costs less than the full err/err_msg treatment because nothing about the ABI changes. The builder pointer is opaque, so a pending-error field checked in build() (setter sees garbage, records "on_exit: unrecognized branch action 7", build() returns the existing -1 + message) needs no new signatures and no header change.

While auditing this I found the file actually has three conventions for garbage input, which strengthens the case for one mechanism:

• unknown policy verdict: coerce to Deny (fail closed, documented, tested)
• unknown protection in sandlock_sandbox_builder_disable: silently ignore the call
• unknown branch action: coerce to Commit (fail open, this issue)

plus 29 setter sites that coerce invalid UTF-8 to "" via to_str().unwrap_or(""). The poison field gives all of these a uniform home. Scope for this issue: just on_exit/on_error plus the mechanism and a regression test shaped like policy_ret_unrecognized_values_fail_closed (both setters, a few bad discriminants, assert build() fails with a message naming the setter). Sweeping the UTF-8 and disable sites into it can be a follow-up.

A patch along those lines is very welcome. We are pre-1.0, so no compatibility shims for anyone who was relying on the coercion: if a binding passes 7 today, it should start failing at build, that is the point.