sec(windows): tighten per-job dir ACLs, fail-closed egress, restrict control-pipe/socket SDs

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,
+	})
 }

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)
+	}
+}

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 {

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")
+	}
+}

pkg/runtime/grant_windows.go

@@ -8,47 +8,111 @@ 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
+// 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-<id>` 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 {
-	// 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")
+	cmd := exec.Command("icacls", traverseGrantArgs(path)...)
 	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 SYSTEM/Admins F, 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
+// 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
-// 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", modifyGrantArgs(path)...)
 	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
 }
+
+// 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"}
+}