From 77da3a781a5a210d5e8a94e793ffa0e115a3e158 Mon Sep 17 00:00:00 2001 From: Luther Monson Date: Fri, 10 Jul 2026 21:30:00 -0700 Subject: [PATCH 1/2] sec(windows): tighten per-job dir ACLs, fail-closed egress, restrict control-pipe/socket SDs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit UNVERIFIED — authored on macOS with no live Windows host. Every change below is cross-compile-clean (GOOS=windows GOARCH=amd64) and unit-tested for pure string/SDDL construction, but the runtime behaviour (icacls effects, HCN ACL enforcement, winio pipe SD, AF_UNIX ACL on Windows) MUST be validated on a real Windows runner before merge/deploy. WIN-1 (per-job runner dir ACLs): grant the Hyper-V VM group (S-1-5-83-0) — the same principal hcsshim's GrantVmGroupAccess uses — directly on each per-job dir instead of Everyone (S-1-1-0). Lock the runners parent to SYSTEM + Administrators + VM-group and break inheritance from world-traversable ProgramData. Closes cross-job / local-user reads of checkouts, artifacts, JIT config. WIN-4 (container egress firewall): make egress-ACL application FATAL. setup() now tears down the endpoint and returns an error on ACL failure, and Create() aborts the job on Setup error instead of warn-and-continue. buildEgressBlockPolicies fails closed on marshal error or an empty rule set. No more fail-open path to the host LAN / RFC1918 / link-local metadata. WIN-5 (containerd control pipe SD): pass an explicit protected DACL (D:P(A;;GA;;;SY)(A;;GA;;;BA)) to winio.ListenPipe instead of nil, restricting the SYSTEM-backed containerd control API to SYSTEM + Administrators. WIN-6 (control socket ACL): replace the no-op os.Chmod(0600) on Windows with a real NTFS ACL (SYSTEM + Administrators, inheritance broken) via a platform-split secureControlSocket helper; POSIX keeps chmod 0600. Deferred: WIN-2 (run service under NT SERVICE\ephemerd virtual account instead of LocalSystem) — larger operational change, needs live testing. --- pkg/containerd/listen_windows.go | 20 +++++- pkg/containerd/listen_windows_test.go | 45 +++++++++++++ pkg/networking/network_windows.go | 68 ++++++++++++++----- pkg/networking/network_windows_test.go | 55 +++++++++++++++ pkg/runtime/grant_windows.go | 85 ++++++++++++++++-------- pkg/runtime/grant_windows_test.go | 36 ++++++++++ pkg/runtime/runtime.go | 9 ++- pkg/scheduler/grpc.go | 13 +++- pkg/scheduler/socketperm_other.go | 19 ++++++ pkg/scheduler/socketperm_windows.go | 39 +++++++++++ pkg/scheduler/socketperm_windows_test.go | 24 +++++++ 11 files changed, 363 insertions(+), 50 deletions(-) create mode 100644 pkg/containerd/listen_windows_test.go create mode 100644 pkg/networking/network_windows_test.go create mode 100644 pkg/runtime/grant_windows_test.go create mode 100644 pkg/scheduler/socketperm_other.go create mode 100644 pkg/scheduler/socketperm_windows.go create mode 100644 pkg/scheduler/socketperm_windows_test.go diff --git a/pkg/containerd/listen_windows.go b/pkg/containerd/listen_windows.go index 0365d45a..e724c233 100644 --- a/pkg/containerd/listen_windows.go +++ b/pkg/containerd/listen_windows.go @@ -8,6 +8,24 @@ import ( "github.com/Microsoft/go-winio" ) +// pipeSecurityDescriptor restricts who may open the containerd control pipe +// (\\.\pipe\ephemerd-containerd). This pipe exposes the full, unauthenticated +// containerd control API (create/run/exec any container, mount any host path) +// backed by a SYSTEM-privileged process, so it must not be openable by ordinary +// local users. The SDDL below is a protected DACL granting GENERIC_ALL to only +// NT AUTHORITY\SYSTEM (SY) and BUILTIN\Administrators (BA): +// +// D:P DACL, Protected (no inherited ACEs) +// (A;;GA;;;SY) Allow GENERIC_ALL to SYSTEM +// (A;;GA;;;BA) Allow GENERIC_ALL to Administrators +// +// This mirrors the HvSocket default bind SD already used for the Linux VM in +// pkg/vm/linuxvm_windows.go, and replaces the previous nil PipeConfig which let +// go-winio apply its (historically permissive, version-dependent) default SD. +const pipeSecurityDescriptor = "D:P(A;;GA;;;SY)(A;;GA;;;BA)" + func listen(address string) (net.Listener, error) { - return winio.ListenPipe(address, nil) + return winio.ListenPipe(address, &winio.PipeConfig{ + SecurityDescriptor: pipeSecurityDescriptor, + }) } diff --git a/pkg/containerd/listen_windows_test.go b/pkg/containerd/listen_windows_test.go new file mode 100644 index 00000000..aef992f8 --- /dev/null +++ b/pkg/containerd/listen_windows_test.go @@ -0,0 +1,45 @@ +//go:build windows + +package containerd + +import ( + "strings" + "testing" + + "github.com/Microsoft/go-winio" +) + +// TestPipeSecurityDescriptorSDDL locks in the exact SDDL for the containerd +// control pipe and proves go-winio can parse it into a real security +// descriptor. A regression here (e.g. a widened ACE or a typo) would silently +// re-expose the SYSTEM-backed containerd control API to non-privileged local +// callers. +func TestPipeSecurityDescriptorSDDL(t *testing.T) { + const want = "D:P(A;;GA;;;SY)(A;;GA;;;BA)" + if pipeSecurityDescriptor != want { + t.Fatalf("pipeSecurityDescriptor = %q, want %q", pipeSecurityDescriptor, want) + } + + // Protected DACL: no inherited ACEs may weaken it. + if !strings.HasPrefix(pipeSecurityDescriptor, "D:P") { + t.Fatalf("DACL is not protected (missing D:P): %q", pipeSecurityDescriptor) + } + // Only SYSTEM (SY) and Administrators (BA) may be granted. + for _, sid := range []string{";SY)", ";BA)"} { + if !strings.Contains(pipeSecurityDescriptor, sid) { + t.Fatalf("SDDL %q missing expected grant to %q", pipeSecurityDescriptor, sid) + } + } + // No World/Everyone (WD) or Authenticated Users (AU) grant. + for _, bad := range []string{";WD)", ";AU)"} { + if strings.Contains(pipeSecurityDescriptor, bad) { + t.Fatalf("SDDL %q must not grant %q", pipeSecurityDescriptor, bad) + } + } + + // The SDDL must be convertible to a Windows security descriptor; an invalid + // string would fail here (and would fail winio.ListenPipe at runtime). + if _, err := winio.SddlToSecurityDescriptor(pipeSecurityDescriptor); err != nil { + t.Fatalf("SddlToSecurityDescriptor(%q): %v", pipeSecurityDescriptor, err) + } +} diff --git a/pkg/networking/network_windows.go b/pkg/networking/network_windows.go index 3fdd91e9..3b1677be 100644 --- a/pkg/networking/network_windows.go +++ b/pkg/networking/network_windows.go @@ -99,9 +99,18 @@ func (w *windowsNetworking) setup(ctx context.Context, id string, netns string) return nil, fmt.Errorf("creating HCN endpoint for %s: %w", id, err) } - // Apply ACL policies to block private network access + // Apply ACL policies to block private network access. This is the ONLY + // egress restriction on Windows (there is no global firewall backstop — + // installFirewallRules is a no-op), so a failure here means the container + // would otherwise run with unrestricted egress to the host LAN, other + // RFC1918 services, and link-local metadata endpoints. Fail CLOSED: tear + // down the endpoint we just created and refuse the job rather than start a + // container we cannot firewall. if err := w.applyACLPolicies(created); err != nil { - w.cfg.Log.Warn("failed to apply ACL policies", "id", id, "error", err) + if delErr := created.Delete(); delErr != nil { + w.cfg.Log.Warn("failed to delete endpoint after ACL failure", "id", id, "error", delErr) + } + return nil, fmt.Errorf("applying egress ACL policies for %s (refusing to start unfirewalled): %w", id, err) } // Create an HCN network namespace and attach the endpoint. @@ -151,18 +160,27 @@ func (w *windowsNetworking) teardown(ctx context.Context, id string, netns strin return nil } -// applyACLPolicies blocks container traffic to RFC 1918 and link-local ranges. -func (w *windowsNetworking) applyACLPolicies(endpoint *hcn.HostComputeEndpoint) error { - blocked := []string{ - "10.0.0.0/8", - "172.16.0.0/12", - "192.168.0.0/16", - "169.254.0.0/16", - } +// egressBlockedCIDRs are the RFC 1918 + link-local ranges a job container must +// not reach. 169.254.0.0/16 also covers cloud-metadata endpoints +// (169.254.169.254). This is the complete intended egress deny list; every +// entry (except the container's own DefaultSubnet) must become an enforced +// block rule or the container is under-firewalled. +var egressBlockedCIDRs = []string{ + "10.0.0.0/8", + "172.16.0.0/12", + "192.168.0.0/16", + "169.254.0.0/16", +} +// buildEgressBlockPolicies constructs the per-endpoint block ACLs from +// egressBlockedCIDRs. It fails closed: a marshal error on any rule, or an empty +// resulting set, is an error rather than a silently weaker rule set. Split out +// from applyACLPolicies so the (pure) rule construction is unit-testable +// without a live HCN endpoint. +func buildEgressBlockPolicies() ([]hcn.EndpointPolicy, error) { var policies []hcn.EndpointPolicy - for _, cidr := range blocked { + for _, cidr := range egressBlockedCIDRs { if cidr == DefaultSubnet { continue } @@ -178,7 +196,9 @@ func (w *windowsNetworking) applyACLPolicies(endpoint *hcn.HostComputeEndpoint) settings, err := json.Marshal(acl) if err != nil { - continue + // Fail closed: a rule we cannot serialize is a rule we cannot + // enforce. Do not skip it and continue with a weaker rule set. + return nil, fmt.Errorf("marshaling egress block ACL for %s: %w", cidr, err) } policies = append(policies, hcn.EndpointPolicy{ @@ -187,13 +207,27 @@ func (w *windowsNetworking) applyACLPolicies(endpoint *hcn.HostComputeEndpoint) }) } - if len(policies) > 0 { - return endpoint.ApplyPolicy(hcn.RequestTypeAdd, hcn.PolicyEndpointRequest{ - Policies: policies, - }) + if len(policies) == 0 { + // Nothing to block would mean no egress restriction at all — treat as + // an error so the caller refuses to start an unfirewalled container. + return nil, fmt.Errorf("no egress block ACLs constructed (would run unfirewalled)") } - return nil + return policies, nil +} + +// applyACLPolicies blocks container traffic to RFC 1918 and link-local ranges. +// The full rule set is built up front and applied atomically; any failure is +// returned so the caller (setup) can treat it as fatal for the job. +func (w *windowsNetworking) applyACLPolicies(endpoint *hcn.HostComputeEndpoint) error { + policies, err := buildEgressBlockPolicies() + if err != nil { + return err + } + + return endpoint.ApplyPolicy(hcn.RequestTypeAdd, hcn.PolicyEndpointRequest{ + Policies: policies, + }) } func (w *windowsNetworking) installFirewallRules() error { diff --git a/pkg/networking/network_windows_test.go b/pkg/networking/network_windows_test.go new file mode 100644 index 00000000..d6642fc2 --- /dev/null +++ b/pkg/networking/network_windows_test.go @@ -0,0 +1,55 @@ +//go:build windows + +package networking + +import ( + "encoding/json" + "testing" + + "github.com/Microsoft/hcsshim/hcn" +) + +// TestBuildEgressBlockPolicies verifies the fail-closed egress rule set (WIN-4). +// Every RFC1918 + link-local range must produce an outbound Block ACL; a +// partial or empty set would let a job reach the host LAN / other tenants / +// cloud metadata. +func TestBuildEgressBlockPolicies(t *testing.T) { + policies, err := buildEgressBlockPolicies() + if err != nil { + t.Fatalf("buildEgressBlockPolicies: %v", err) + } + + // Collect the CIDRs that actually became block rules. + got := map[string]bool{} + for _, p := range policies { + if p.Type != hcn.ACL { + t.Errorf("policy type = %v, want ACL", p.Type) + } + var acl hcn.AclPolicySetting + if err := json.Unmarshal(p.Settings, &acl); err != nil { + t.Fatalf("unmarshal ACL setting: %v", err) + } + if acl.Action != hcn.ActionTypeBlock { + t.Errorf("CIDR %s action = %v, want Block", acl.RemoteAddresses, acl.Action) + } + if acl.Direction != hcn.DirectionTypeOut { + t.Errorf("CIDR %s direction = %v, want Out", acl.RemoteAddresses, acl.Direction) + } + got[acl.RemoteAddresses] = true + } + + // Every configured range (minus the container's own subnet) must be blocked. + for _, cidr := range egressBlockedCIDRs { + if cidr == DefaultSubnet { + continue + } + if !got[cidr] { + t.Errorf("egress range %s was not turned into a block rule", cidr) + } + } + + // Link-local / metadata range must always be present. + if !got["169.254.0.0/16"] { + t.Error("link-local/metadata range 169.254.0.0/16 not blocked") + } +} diff --git a/pkg/runtime/grant_windows.go b/pkg/runtime/grant_windows.go index fcad7de3..4287d831 100644 --- a/pkg/runtime/grant_windows.go +++ b/pkg/runtime/grant_windows.go @@ -8,47 +8,78 @@ import ( "syscall" ) -// everyoneSID is the well-known SID for "Everyone" (S-1-1-0). We grant to -// Everyone because Hyper-V utility VMs (used for the isolated Windows -// containers we create via containerd's runhcs shim) open host paths through -// a virtual account that does NOT inherit membership in -// "NT VIRTUAL MACHINE\Virtual Machines" (S-1-5-83-0) on Windows 11 client -// SKUs — we verified this empirically by observing VSMB Modify failures with -// ACCESS_DENIED even after the VM group had full inherited rights. The -// grants are narrowed by path: the parent runners directory only needs -// traverse (RX, no inheritance to children), while each per-job directory -// gets Modify with inheritance. That preserves isolation between concurrent -// jobs — the utility VM for job A can traverse `runners` but cannot touch -// job B's subdirectory because only job A's dir has an ACE for it. -const everyoneSID = "S-1-1-0" +// Well-known SIDs used for the per-job runner-directory DACLs. +// +// - vmGroupSID (S-1-5-83-0) = "NT VIRTUAL MACHINE\Virtual Machines". Hyper-V +// utility VMs (used for the isolated Windows containers we create via +// containerd's runhcs shim) open host paths — the VSMB-shared per-job +// runner directory — through a per-VM virtual account whose token is a +// member of this group. This is the exact principal hcsshim itself grants +// when it prepares a host path for VSMB: see +// github.com/Microsoft/hcsshim/internal/security.GrantVmGroupAccess, which +// sets a DACL entry for sidVMGroup = "S-1-5-83-0" *directly* on the target +// path (it never relies on inheritance from the VM Machines group). +// - systemSID (S-1-5-18) = "NT AUTHORITY\SYSTEM" — the account the ephemerd +// daemon runs as; it must retain access to manage these dirs. +// - adminsSID (S-1-5-32-544) = "BUILTIN\Administrators". +// +// We previously granted "Everyone" (S-1-1-0) here on the theory that the VM +// group ACE "does not reliably inherit" on Windows 11 client SKUs. That +// conflated two things: the VM group ACE does not *inherit* from a parent, but +// hcsshim's approach never relies on inheritance — it grants the VM group SID +// directly on the specific path the utility VM opens. Granting Everyone was +// scoped by *path*, not *principal*, so any local user or sibling job that +// could reach the filesystem read every job's checkout, artifacts, and +// (transiently) its JIT runner config. Granting S-1-5-83-0 directly, per path, +// gives the utility VM exactly the access it needs while keeping other +// principals out. +const ( + vmGroupSID = "S-1-5-83-0" + systemSID = "S-1-5-18" + adminsSID = "S-1-5-32-544" +) -// grantHyperVTraverse grants Everyone read+execute (traverse) on a directory -// without inheritance. Use this on the runners parent directory so Hyper-V -// utility VMs can step into any per-job subdirectory whose DACL explicitly -// grants them Modify access. +// grantHyperVTraverse locks down the runners parent directory: it breaks +// inheritance from the world-traversable C:\ProgramData ACL, re-grants only +// SYSTEM + Administrators (so the daemon can still manage the tree), and grants +// the Hyper-V VM group traverse (read+execute) so utility VMs can step into a +// per-job subdirectory whose DACL explicitly grants them Modify. None of these +// ACEs are inheritable, so they do not propagate into per-job subdirectories. func grantHyperVTraverse(path string) error { - // RX = GENERIC_READ + GENERIC_EXECUTE (traverse). No (OI)(CI) flags, so - // the ACE applies only to the directory itself and does not leak into - // per-job subdirectories. - cmd := exec.Command("icacls", path, "/grant", "*"+everyoneSID+":(RX)", "/C", "/Q") + // /inheritance:r removes inherited ACEs (so ProgramData's world-readable + // ACL no longer applies to this tree). We then explicitly re-grant the + // principals that must retain access. RX = read+execute (traverse); no + // (OI)(CI) flags, so the ACEs apply only to this directory itself. + cmd := exec.Command("icacls", path, + "/inheritance:r", + "/grant", "*"+systemSID+":(RX)", + "/grant", "*"+adminsSID+":(RX)", + "/grant", "*"+vmGroupSID+":(RX)", + "/C", "/Q") cmd.SysProcAttr = &syscall.SysProcAttr{HideWindow: true} out, err := cmd.CombinedOutput() if err != nil { - return fmt.Errorf("icacls grant Everyone RX on %s: %w: %s", path, err, string(out)) + return fmt.Errorf("icacls lock-down + grant VM-group RX on %s: %w: %s", path, err, string(out)) } return nil } -// grantHyperVModify grants Everyone Modify on a per-job directory, with -// Object Inherit + Container Inherit so files the runner writes during the +// grantHyperVModify grants the Hyper-V VM group Modify on a per-job directory, +// with Object Inherit + Container Inherit so files the runner writes during the // job also get the ACE. Modify covers read+write+execute+delete but NOT -// changing the DACL itself. +// changing the DACL itself. Because the ACE is scoped to the VM group SID +// (S-1-5-83-0) rather than Everyone, other local users and other jobs' non-VM +// processes are excluded. The residual is that a *concurrent* job's utility VM +// (a distinct virtual account, but still a VM-group member) could in principle +// open this directory — the same trust boundary hcsshim itself uses for VSMB, +// and a strict improvement over Everyone, which also admitted every +// interactive/local/authenticated principal on the host. func grantHyperVModify(path string) error { - cmd := exec.Command("icacls", path, "/grant", "*"+everyoneSID+":(OI)(CI)M", "/C", "/Q") + cmd := exec.Command("icacls", path, "/grant", "*"+vmGroupSID+":(OI)(CI)M", "/C", "/Q") cmd.SysProcAttr = &syscall.SysProcAttr{HideWindow: true} out, err := cmd.CombinedOutput() if err != nil { - return fmt.Errorf("icacls grant Everyone M on %s: %w: %s", path, err, string(out)) + return fmt.Errorf("icacls grant VM-group M on %s: %w: %s", path, err, string(out)) } return nil } diff --git a/pkg/runtime/grant_windows_test.go b/pkg/runtime/grant_windows_test.go new file mode 100644 index 00000000..12bb7050 --- /dev/null +++ b/pkg/runtime/grant_windows_test.go @@ -0,0 +1,36 @@ +//go:build windows + +package runtime + +import "testing" + +// TestGrantSIDs locks in the well-known SIDs used for per-job runner-directory +// DACLs. WIN-1 was a regression where these grants used Everyone (S-1-1-0), +// exposing every job's checkout/artifacts/JIT config to all local principals. +// The fix is to grant the Hyper-V VM group directly (the same principal +// hcsshim's GrantVmGroupAccess uses), plus SYSTEM+Administrators on the parent. +func TestGrantSIDs(t *testing.T) { + cases := map[string]string{ + "vmGroupSID": vmGroupSID, + "systemSID": systemSID, + "adminsSID": adminsSID, + } + want := map[string]string{ + "vmGroupSID": "S-1-5-83-0", // NT VIRTUAL MACHINE\Virtual Machines + "systemSID": "S-1-5-18", // NT AUTHORITY\SYSTEM + "adminsSID": "S-1-5-32-544", // BUILTIN\Administrators + } + for name, got := range cases { + if got != want[name] { + t.Errorf("%s = %q, want %q", name, got, want[name]) + } + } + + // The over-broad Everyone SID must not have crept back into any grant + // constant used for per-job directories. + for name, got := range cases { + if got == "S-1-1-0" { + t.Errorf("%s grants Everyone (S-1-1-0); per-job dirs must be principal-scoped", name) + } + } +} diff --git a/pkg/runtime/runtime.go b/pkg/runtime/runtime.go index 51fdd60f..b14f4347 100644 --- a/pkg/runtime/runtime.go +++ b/pkg/runtime/runtime.go @@ -762,8 +762,13 @@ func (r *Runtime) Create(ctx context.Context, cfg CreateConfig) (*RunnerEnv, err if goruntime.GOOS == "windows" && r.cfg.Network != nil { result, err := r.cfg.Network.Setup(ctx, id, "") if err != nil { - r.cfg.Log.Warn("failed to setup Windows network endpoint", "id", id, "error", err) - } else if result != nil { + // Setup applies the per-endpoint egress ACLs (RFC1918 + link-local + // block) and returns an error if it could not — there is no global + // firewall backstop on Windows. Fail CLOSED: abort the job rather + // than start a container with an unfirewalled (or absent) endpoint. + return nil, fmt.Errorf("setting up Windows network endpoint for %s: %w", id, err) + } + if result != nil { windowsEndpointID = result.EndpointID windowsNetNS = result.NetNS opts = append(opts, withWindowsNetwork(windowsNetNS, windowsEndpointID)) diff --git a/pkg/scheduler/grpc.go b/pkg/scheduler/grpc.go index 22f2f91a..ad7aeaf9 100644 --- a/pkg/scheduler/grpc.go +++ b/pkg/scheduler/grpc.go @@ -39,9 +39,16 @@ func (s *Scheduler) startControlServer() (func(), error) { if err != nil { return nil, fmt.Errorf("listen on %s: %w", socketPath, err) } - if err := os.Chmod(socketPath, 0o600); err != nil { - _ = lis.Close() - return nil, fmt.Errorf("chmod %s: %w", socketPath, err) + // Restrict the control socket to privileged principals. The control API can + // KillJob (DoS) and stream GetJobLogs (may contain job secrets), so it must + // not be reachable by ordinary local users. On POSIX this is chmod 0600; on + // Windows POSIX mode bits are a near-no-op for an AF_UNIX socket file, so + // secureControlSocket applies a real NTFS ACL (SYSTEM + Administrators only). + if err := secureControlSocket(socketPath); err != nil { + if closeErr := lis.Close(); closeErr != nil { + s.cfg.Log.Warn("closing control socket listener after ACL failure", "error", closeErr) + } + return nil, fmt.Errorf("securing control socket %s: %w", socketPath, err) } srv := grpc.NewServer() diff --git a/pkg/scheduler/socketperm_other.go b/pkg/scheduler/socketperm_other.go new file mode 100644 index 00000000..fbb070b4 --- /dev/null +++ b/pkg/scheduler/socketperm_other.go @@ -0,0 +1,19 @@ +//go:build !windows + +package scheduler + +import ( + "fmt" + "os" +) + +// secureControlSocket restricts the control socket to its owner. On POSIX the +// AF_UNIX socket file's mode bits are enforced, so chmod 0600 (owner rw only) +// is sufficient: the daemon runs as root/the service user and only that user +// may connect. +func secureControlSocket(path string) error { + if err := os.Chmod(path, 0o600); err != nil { + return fmt.Errorf("chmod %s: %w", path, err) + } + return nil +} diff --git a/pkg/scheduler/socketperm_windows.go b/pkg/scheduler/socketperm_windows.go new file mode 100644 index 00000000..932f3bc3 --- /dev/null +++ b/pkg/scheduler/socketperm_windows.go @@ -0,0 +1,39 @@ +//go:build windows + +package scheduler + +import ( + "fmt" + "os/exec" + "syscall" +) + +// Well-known SIDs for the control-socket DACL. +const ( + systemSID = "S-1-5-18" // NT AUTHORITY\SYSTEM (the daemon's account) + adminsSID = "S-1-5-32-544" // BUILTIN\Administrators +) + +// secureControlSocket applies a real NTFS ACL to the AF_UNIX control socket +// file. On Windows, os.Chmod maps POSIX mode bits to at most a read-only +// attribute — it does NOT produce a restrictive DACL for a socket file, so +// access is otherwise governed by the inherited, world-traversable +// C:\ProgramData ACL. Any local user could then open the socket and KillJob or +// stream job logs. We instead break inheritance and grant full control to only +// SYSTEM (the daemon) and Administrators, mirroring the containerd control-pipe +// and HvSocket security descriptors used elsewhere in the daemon. +func secureControlSocket(path string) error { + // /inheritance:r drops inherited ACEs (removing ProgramData's permissive + // grants); the explicit grants restore access for the two principals that + // legitimately administer the daemon. + cmd := exec.Command("icacls", path, + "/inheritance:r", + "/grant", "*"+systemSID+":(F)", + "/grant", "*"+adminsSID+":(F)", + "/C", "/Q") + cmd.SysProcAttr = &syscall.SysProcAttr{HideWindow: true} + if out, err := cmd.CombinedOutput(); err != nil { + return fmt.Errorf("icacls lock-down control socket %s: %w: %s", path, err, string(out)) + } + return nil +} diff --git a/pkg/scheduler/socketperm_windows_test.go b/pkg/scheduler/socketperm_windows_test.go new file mode 100644 index 00000000..81a43df8 --- /dev/null +++ b/pkg/scheduler/socketperm_windows_test.go @@ -0,0 +1,24 @@ +//go:build windows + +package scheduler + +import "testing" + +// TestControlSocketSIDs locks in the principals granted on the Windows control +// socket. WIN-6: os.Chmod is a near-no-op for an AF_UNIX socket file on Windows, +// so the socket must instead be locked to SYSTEM (the daemon) and +// Administrators only — nothing broader — or a local user could KillJob and +// stream job-secret logs. +func TestControlSocketSIDs(t *testing.T) { + if systemSID != "S-1-5-18" { + t.Errorf("systemSID = %q, want S-1-5-18 (NT AUTHORITY\\SYSTEM)", systemSID) + } + if adminsSID != "S-1-5-32-544" { + t.Errorf("adminsSID = %q, want S-1-5-32-544 (BUILTIN\\Administrators)", adminsSID) + } + for name, got := range map[string]string{"systemSID": systemSID, "adminsSID": adminsSID} { + if got == "S-1-1-0" { + t.Errorf("%s grants Everyone (S-1-1-0); control socket must stay privileged-only", name) + } + } +} From d558987fea28877ee6e570dcf7aaac4379afea7b Mon Sep 17 00:00:00 2001 From: Luther Monson Date: Sun, 12 Jul 2026 14:23:48 -0700 Subject: [PATCH 2/2] fix(windows): keep SYSTEM/Admins writable on runners parent after lockdown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up to the WIN-1 hardening. grantHyperVTraverse ran `icacls /inheritance:r` on the runners parent and re-granted SYSTEM only (RX). But the daemon runs as SYSTEM and creates each per-job dir (`job-`) directly under that parent via copyDirForJob at Create(), and removes orphan `job-*` dirs there at startup. Creating a subdirectory needs write (FILE_ADD_SUBDIRECTORY) on the parent; breaking inheritance also dropped the inherited SYSTEM full-control ACE. Net: every Windows job needing a runner mount would fail to provision (fail-closed, but total). Grant SYSTEM + Administrators inheritable Full (OI)(CI)F on the parent so the daemon can create/clean per-job dirs; the VM group stays traverse-only (RX, non-inheritable) — each job dir still gets its own explicit VM-group Modify ACE. Extracted the icacls arg construction into pure helpers (traverseGrantArgs/modifyGrantArgs) and added tests that pin the policy (SYSTEM/Admins keep inheritable write; VM group traverse-only; no Everyone) so the RX regression can't recur without a live Windows host to catch it. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01ChVRC9ZrvQarrctDwSy1S5 --- pkg/runtime/grant_windows.go | 67 +++++++++++++++++++++-------- pkg/runtime/grant_windows_test.go | 70 ++++++++++++++++++++++++++++++- 2 files changed, 119 insertions(+), 18 deletions(-) diff --git a/pkg/runtime/grant_windows.go b/pkg/runtime/grant_windows.go index 4287d831..cc1fd1ae 100644 --- a/pkg/runtime/grant_windows.go +++ b/pkg/runtime/grant_windows.go @@ -40,30 +40,56 @@ const ( ) // grantHyperVTraverse locks down the runners parent directory: it breaks -// inheritance from the world-traversable C:\ProgramData ACL, re-grants only -// SYSTEM + Administrators (so the daemon can still manage the tree), and grants -// the Hyper-V VM group traverse (read+execute) so utility VMs can step into a -// per-job subdirectory whose DACL explicitly grants them Modify. None of these -// ACEs are inheritable, so they do not propagate into per-job subdirectories. +// inheritance from the world-traversable C:\ProgramData ACL, re-grants +// SYSTEM + Administrators FULL control (so the daemon can still create, clean, +// and manage per-job subdirectories), and grants the Hyper-V VM group +// traverse-only (read+execute) so utility VMs can step into a per-job +// subdirectory whose DACL explicitly grants them Modify. +// +// The principal grants differ deliberately: +// - SYSTEM/Administrators get (OI)(CI)F: the daemon runs as SYSTEM and MUST +// retain write on this directory — it creates each `job-` dir here at +// Create() time (copyDirForJob) and removes orphan `job-*` dirs here at +// startup. Granting only (RX) would break inheritance's SYSTEM full-control +// ACE and leave the daemon unable to add or delete subdirectories, failing +// every Windows job that needs a runner mount. The inheritable flags also +// ensure per-job dirs stay daemon-manageable (create/cleanup/RemoveAll). +// - The VM group gets (RX) with NO inherit flags: utility VMs only need to +// traverse the parent to reach their own job dir, and each job dir receives +// its own explicit VM-group Modify ACE in grantHyperVModify — we do not want +// a blanket inherited VM-group ACE on every child. func grantHyperVTraverse(path string) error { - // /inheritance:r removes inherited ACEs (so ProgramData's world-readable - // ACL no longer applies to this tree). We then explicitly re-grant the - // principals that must retain access. RX = read+execute (traverse); no - // (OI)(CI) flags, so the ACEs apply only to this directory itself. - cmd := exec.Command("icacls", path, - "/inheritance:r", - "/grant", "*"+systemSID+":(RX)", - "/grant", "*"+adminsSID+":(RX)", - "/grant", "*"+vmGroupSID+":(RX)", - "/C", "/Q") + cmd := exec.Command("icacls", traverseGrantArgs(path)...) cmd.SysProcAttr = &syscall.SysProcAttr{HideWindow: true} out, err := cmd.CombinedOutput() if err != nil { - return fmt.Errorf("icacls lock-down + grant VM-group RX on %s: %w: %s", path, err, string(out)) + return fmt.Errorf("icacls lock-down + grant SYSTEM/Admins F, VM-group RX on %s: %w: %s", path, err, string(out)) } return nil } +// traverseGrantArgs builds the icacls arguments for the runners parent +// lock-down. Split out so the grant POLICY (who gets what) is unit-testable +// without a live Windows host or actually invoking icacls. +// +// - /inheritance:r drops inherited ACEs (removing ProgramData's world-readable +// grant) — but that also drops the inherited SYSTEM/Admins full-control ACE, +// so we MUST re-grant them write or the daemon can no longer create/clean +// per-job dirs here. +// - SYSTEM + Administrators: (OI)(CI)F — full and inheritable. +// - VM group: (RX) only, non-inheritable — traverse to reach a job dir; each +// job dir gets its own explicit VM-group Modify ACE in grantHyperVModify. +func traverseGrantArgs(path string) []string { + return []string{ + path, + "/inheritance:r", + "/grant", "*" + systemSID + ":(OI)(CI)F", + "/grant", "*" + adminsSID + ":(OI)(CI)F", + "/grant", "*" + vmGroupSID + ":(RX)", + "/C", "/Q", + } +} + // grantHyperVModify grants the Hyper-V VM group Modify on a per-job directory, // with Object Inherit + Container Inherit so files the runner writes during the // job also get the ACE. Modify covers read+write+execute+delete but NOT @@ -75,7 +101,7 @@ func grantHyperVTraverse(path string) error { // and a strict improvement over Everyone, which also admitted every // interactive/local/authenticated principal on the host. func grantHyperVModify(path string) error { - cmd := exec.Command("icacls", path, "/grant", "*"+vmGroupSID+":(OI)(CI)M", "/C", "/Q") + cmd := exec.Command("icacls", modifyGrantArgs(path)...) cmd.SysProcAttr = &syscall.SysProcAttr{HideWindow: true} out, err := cmd.CombinedOutput() if err != nil { @@ -83,3 +109,10 @@ func grantHyperVModify(path string) error { } return nil } + +// modifyGrantArgs builds the icacls arguments granting the VM group Modify on a +// per-job directory, inheritable so files the runner writes also get the ACE. +// Split out for the same unit-testability reason as traverseGrantArgs. +func modifyGrantArgs(path string) []string { + return []string{path, "/grant", "*" + vmGroupSID + ":(OI)(CI)M", "/C", "/Q"} +} diff --git a/pkg/runtime/grant_windows_test.go b/pkg/runtime/grant_windows_test.go index 12bb7050..e77dc90a 100644 --- a/pkg/runtime/grant_windows_test.go +++ b/pkg/runtime/grant_windows_test.go @@ -2,7 +2,10 @@ package runtime -import "testing" +import ( + "strings" + "testing" +) // TestGrantSIDs locks in the well-known SIDs used for per-job runner-directory // DACLs. WIN-1 was a regression where these grants used Everyone (S-1-1-0), @@ -34,3 +37,68 @@ func TestGrantSIDs(t *testing.T) { } } } + +// argAfterGrant returns the ACL string granted to sid within an icacls arg +// slice, i.e. the token following the "/grant" that names sid. Returns "" if +// sid is not granted. +func argAfterGrant(args []string, sid string) string { + for i := 0; i+1 < len(args); i++ { + if args[i] == "/grant" && strings.HasPrefix(args[i+1], "*"+sid+":") { + return strings.TrimPrefix(args[i+1], "*"+sid+":") + } + } + return "" +} + +// TestTraverseGrantArgs pins the runners-parent lock-down policy. The daemon +// (SYSTEM) creates per-job dirs under this parent at Create() and removes orphan +// job-* dirs here at startup, so /inheritance:r (which drops the inherited +// SYSTEM full-control ACE) MUST be paired with an explicit write grant for +// SYSTEM — otherwise every Windows job that needs a runner mount fails to +// provision. Regression guard for exactly that: SYSTEM/Admins get inheritable +// Full, the VM group gets traverse-only. +func TestTraverseGrantArgs(t *testing.T) { + args := traverseGrantArgs(`C:\ProgramData\ephemerd\runners`) + + // Inheritance must be broken to shed the world-readable ProgramData ACL. + found := false + for _, a := range args { + if a == "/inheritance:r" { + found = true + } + } + if !found { + t.Error("traverseGrantArgs must include /inheritance:r to drop the inherited ProgramData ACL") + } + + // SYSTEM and Administrators must retain WRITE (Full), inheritable, or the + // daemon cannot create/clean per-job subdirectories after inheritance is + // broken. (RX) here would be the WIN-1 regression. + for _, sid := range []string{systemSID, adminsSID} { + acl := argAfterGrant(args, sid) + if acl != "(OI)(CI)F" { + t.Errorf("grant for %s = %q, want (OI)(CI)F (daemon must keep inheritable write on the parent)", sid, acl) + } + if acl == "(RX)" { + t.Errorf("grant for %s is read-only — daemon would fail to create per-job dirs", sid) + } + } + + // The VM group only needs traverse, and must NOT be inheritable (each job + // dir gets its own explicit Modify ACE instead). + if acl := argAfterGrant(args, vmGroupSID); acl != "(RX)" { + t.Errorf("grant for VM group = %q, want (RX) traverse-only", acl) + } +} + +// TestModifyGrantArgs pins that per-job dirs grant the VM group inheritable +// Modify and nothing broader (no Everyone). +func TestModifyGrantArgs(t *testing.T) { + args := modifyGrantArgs(`C:\ProgramData\ephemerd\runners\job-abc`) + if acl := argAfterGrant(args, vmGroupSID); acl != "(OI)(CI)M" { + t.Errorf("per-job grant for VM group = %q, want (OI)(CI)M", acl) + } + if acl := argAfterGrant(args, "S-1-1-0"); acl != "" { + t.Errorf("per-job dir must not grant Everyone (S-1-1-0), got %q", acl) + } +}