Read real Windows ACLs in ai_tools fsutil.Stat

.github/workflows/test-go-windows.yml

@@ -60,6 +60,7 @@ jobs:
             "./orbit/pkg/bitlocker/..."
             "./orbit/pkg/keystore/..."
             "./orbit/pkg/platform/..."
+            "./orbit/pkg/table/ai_tools/..."
             "./orbit/pkg/table/bitlocker_key_protectors/..."
             "./orbit/pkg/table/cis_audit/..."
             "./orbit/pkg/table/windowsupdatetable/..."

orbit/pkg/table/ai_tools/internal/fsutil/fsutil.go

@@ -1,6 +1,7 @@
-// Package fsutil holds small, dependency-free filesystem helpers shared across
-// collectors: content hashing (a diffable integrity fingerprint) and POSIX
-// permission inspection (used to flag world-readable secret-bearing files).
+// Package fsutil holds small filesystem helpers shared across collectors:
+// content hashing (a diffable integrity fingerprint) and permission inspection
+// (used to flag world-readable secret-bearing files and world-writable
+// instruction files) from POSIX mode bits or a Windows DACL.
 //
 // These never execute a discovered file — they only stat and read it — so they
 // preserve the extension's no-exec security posture.
@@ -12,7 +13,6 @@ import (
 	"io"
 	"os"
 	"path/filepath"
-	"runtime"
 	"strings"
 	"syscall"
 )
@@ -116,30 +116,23 @@ func SHA256Bytes(b []byte) string {
 	return hex.EncodeToString(sum[:])
 }
 
-// Perm describes the POSIX permission posture of a file. Known is false on
-// platforms where Unix mode bits are not meaningful (Windows), so callers don't
-// emit false "world-readable" signals there.
+// Perm describes how widely a file is readable or writable. "World" means the
+// POSIX group/other bits on macOS and Linux, and a DACL grant to a well-known
+// everyone-style SID on Windows. Known is false when the posture could not be
+// determined — an unreadable path, or a Windows security descriptor we could not
+// read — so callers don't emit a risk signal they haven't actually established.
+// A DACL that is read but contains an ACE type we don't decode still reports
+// Known: true; skipping such an ACE can only lose a signal, never invent one.
 type Perm struct {
-	WorldReadable bool // group OR other has read
-	WorldWritable bool // group OR other has write
+	WorldReadable bool
+	WorldWritable bool
 	Known         bool
 }
 
-// Stat returns the permission posture of path. On Windows, Known is false.
+// Stat returns the permission posture of path. The per-platform reader lives in
+// perm_unix.go and perm_windows.go.
 func Stat(path string) Perm {
-	if runtime.GOOS == "windows" {
-		return Perm{}
-	}
-	fi, err := os.Lstat(path)
-	if err != nil {
-		return Perm{}
-	}
-	m := fi.Mode().Perm()
-	return Perm{
-		WorldReadable: m&0o044 != 0,
-		WorldWritable: m&0o022 != 0,
-		Known:         true,
-	}
+	return statPerm(path)
 }
 
 // Exists reports whether path is an existing regular file. It uses Lstat and

orbit/pkg/table/ai_tools/internal/fsutil/perm_acl.go

@@ -0,0 +1,124 @@
+package fsutil
+
+// This file holds the Windows DACL decision logic, deliberately split from the
+// Win32 calls in perm_windows.go so it can be tested on any platform. Only
+// perm_windows.go uses it; the tests exercise it everywhere.
+
+// Well-known SIDs whose membership is effectively "any user who can log in to
+// this machine". A grant to one of them is the Windows analogue of the POSIX
+// group/other permission bits that drive Perm on macOS and Linux.
+//
+// Only these two universal SIDs count. Local groups — BUILTIN\Users
+// (S-1-5-32-545) above all — are excluded, so this is narrower than the POSIX
+// side, which counts the group bit as well: a grant to a group whose membership
+// varies per machine isn't the same claim as "anyone who can log in". Widening
+// it is a product decision about what the risk flag means, not an
+// implementation detail.
+const (
+	sidEveryone           = "S-1-1-0"
+	sidAuthenticatedUsers = "S-1-5-11"
+)
+
+func isWorldSID(sid string) bool {
+	return sid == sidEveryone || sid == sidAuthenticatedUsers
+}
+
+// Windows file access-mask bits (winnt.h). Declared here rather than taken from
+// golang.org/x/sys/windows so this file stays buildable on every platform.
+const (
+	fileReadData   = 0x00000001 // FILE_READ_DATA
+	fileWriteData  = 0x00000002 // FILE_WRITE_DATA
+	fileAppendData = 0x00000004 // FILE_APPEND_DATA
+	stdDelete      = 0x00010000 // DELETE
+	stdWriteDAC    = 0x00040000 // WRITE_DAC
+	stdWriteOwner  = 0x00080000 // WRITE_OWNER
+	genericAll     = 0x10000000 // GENERIC_ALL
+	genericExecute = 0x20000000 // GENERIC_EXECUTE
+	genericWrite   = 0x40000000 // GENERIC_WRITE
+	genericRead    = 0x80000000 // GENERIC_READ
+
+	// The file object's GENERIC_MAPPING: what each generic right stands for on a
+	// file. Note FILE_GENERIC_WRITE carries none of DELETE, WRITE_DAC or
+	// WRITE_OWNER.
+	fileAllAccess      = 0x001F01FF // FILE_ALL_ACCESS — GENERIC_ALL, `icacls /grant <x>:F`
+	fileGenericRead    = 0x00120089 // FILE_GENERIC_READ — GENERIC_READ, `icacls /grant <x>:R`
+	fileGenericWrite   = 0x00120116 // FILE_GENERIC_WRITE — GENERIC_WRITE, `icacls /grant <x>:W`
+	fileGenericExecute = 0x001200A0 // FILE_GENERIC_EXECUTE — GENERIC_EXECUTE
+)
+
+// writeMask is every bit that lets the holder change the file's contents, or
+// escalate to being able to. DELETE allows replacing the file wholesale, and
+// WRITE_DAC/WRITE_OWNER allow granting yourself the rest — all three are as good
+// as write for an attacker editing an agent instruction file.
+//
+// Both masks name specific rights only: mapGenericRights has already translated
+// the generic aliases away by the time a mask is evaluated.
+const writeMask = fileWriteData | fileAppendData | stdDelete | stdWriteDAC | stdWriteOwner
+
+const readMask = fileReadData
+
+// mapGenericRights rewrites an ACE mask's generic bits into the specific file
+// rights they stand for, and clears them — the same translation MapGenericMask
+// performs, and what the object manager is supposed to have done before a
+// descriptor reaches an object. It has to be done here because generic bits do
+// reach real file DACLs verbatim (icacls renders them GR/GW/GE/GA, and SDDL
+// strings write them as-is), and precedence has to be decided in one vocabulary:
+// otherwise a deny naming GENERIC_ALL and an allow naming FILE_ALL_ACCESS look
+// like disjoint sets of rights and neither settles the other.
+func mapGenericRights(m uint32) uint32 {
+	if m&genericRead != 0 {
+		m |= fileGenericRead
+	}
+	if m&genericWrite != 0 {
+		m |= fileGenericWrite
+	}
+	if m&genericExecute != 0 {
+		m |= fileGenericExecute
+	}
+	if m&genericAll != 0 {
+		m |= fileAllAccess
+	}
+	return m &^ (genericRead | genericWrite | genericExecute | genericAll)
+}
+
+// aceEntry is one DACL entry reduced to the fields the world-permission decision
+// needs.
+type aceEntry struct {
+	SID         string
+	Allow       bool   // ACCESS_ALLOWED_ACE_TYPE; false means ACCESS_DENIED_ACE_TYPE
+	InheritOnly bool   // INHERIT_ONLY_ACE — applies to children, not this object
+	Mask        uint32 // ACCESS_MASK
+}
+
+// worldPermFromACEs evaluates a DACL for read/write access granted to a world
+// SID, mirroring the Windows access check: ACEs are walked in order and each
+// individual right is settled by the first ACE that mentions it, so an earlier
+// deny beats a later allow (the canonical DACL ordering) but only for the bits
+// it actually names.
+//
+// Resolving per bit rather than per ACE matters. A DACL of "deny Everyone the
+// data-write rights, allow Everyone:(F)" — icacls /deny <sid>:(WD,AD) /grant
+// <sid>:(F) — still leaves Everyone holding DELETE, WRITE_DAC and WRITE_OWNER,
+// so the file is fully tamperable. Letting the deny settle write wholesale would
+// report it as safe, which is a two-command way to hide a tampered file.
+func worldPermFromACEs(aces []aceEntry) Perm {
+	var allowed, decided uint32
+	for _, a := range aces {
+		if a.InheritOnly || !isWorldSID(a.SID) {
+			continue
+		}
+		fresh := mapGenericRights(a.Mask) & ^decided // rights no earlier ACE has settled
+		if fresh == 0 {
+			continue
+		}
+		if a.Allow {
+			allowed |= fresh
+		}
+		decided |= fresh
+	}
+	return Perm{
+		WorldReadable: allowed&readMask != 0,
+		WorldWritable: allowed&writeMask != 0,
+		Known:         true,
+	}
+}

orbit/pkg/table/ai_tools/internal/fsutil/perm_acl_test.go

@@ -0,0 +1,157 @@
+package fsutil
+
+import "testing"
+
+// A local (non-well-known) account SID: a grant to it is not a world grant.
+const sidLocalUser = "S-1-5-21-1111111111-2222222222-3333333333-1001"
+
+func TestWorldPermFromACEs(t *testing.T) {
+	allow := func(sid string, mask uint32) aceEntry {
+		return aceEntry{SID: sid, Allow: true, Mask: mask}
+	}
+	deny := func(sid string, mask uint32) aceEntry {
+		return aceEntry{SID: sid, Mask: mask}
+	}
+
+	cases := []struct {
+		name      string
+		aces      []aceEntry
+		wantRead  bool
+		wantWrite bool
+	}{
+		{
+			name: "empty DACL grants nobody anything",
+			aces: nil,
+		},
+		{
+			// icacls <file> /grant Everyone:F — the reproduction from the bug report.
+			name:      "Everyone full control",
+			aces:      []aceEntry{allow(sidEveryone, fileAllAccess)},
+			wantRead:  true,
+			wantWrite: true,
+		},
+		{
+			name:     "Everyone read-only",
+			aces:     []aceEntry{allow(sidEveryone, fileGenericRead)},
+			wantRead: true,
+		},
+		{
+			name:      "Authenticated Users write",
+			aces:      []aceEntry{allow(sidAuthenticatedUsers, fileWriteData)},
+			wantWrite: true,
+		},
+		{
+			name:      "GENERIC_ALL counts as both read and write",
+			aces:      []aceEntry{allow(sidEveryone, genericAll)},
+			wantRead:  true,
+			wantWrite: true,
+		},
+		{
+			// Being able to rewrite the DACL is being able to grant yourself write.
+			name:      "WRITE_DAC alone counts as write",
+			aces:      []aceEntry{allow(sidEveryone, stdWriteDAC)},
+			wantWrite: true,
+		},
+		{
+			name:      "DELETE alone counts as write",
+			aces:      []aceEntry{allow(sidEveryone, stdDelete)},
+			wantWrite: true,
+		},
+		{
+			// Canonical DACL order puts deny first; it must win over the later allow
+			// for the rights it names — here only FILE_READ_DATA, icacls (RD).
+			name: "deny read ahead of allow full leaves write only",
+			aces: []aceEntry{
+				deny(sidEveryone, fileReadData),
+				allow(sidEveryone, fileAllAccess),
+			},
+			wantWrite: true,
+		},
+		{
+			// icacls /deny <sid>:(WD,AD) /grant <sid>:(F). The deny names only the
+			// two data-write rights, so Everyone keeps DELETE and WRITE_DAC from the
+			// allow and can still replace the file or re-ACL it into writability.
+			// Letting the deny settle write wholesale would report this as safe.
+			name: "partial write deny leaves the escalation rights a later allow grants",
+			aces: []aceEntry{
+				deny(sidEveryone, fileWriteData|fileAppendData),
+				allow(sidEveryone, fileAllAccess),
+			},
+			wantRead:  true,
+			wantWrite: true,
+		},
+		{
+			// icacls /deny Everyone:(F) — a deny that does name every right.
+			name: "deny full ahead of allow full grants nothing",
+			aces: []aceEntry{
+				deny(sidEveryone, fileAllAccess),
+				allow(sidEveryone, fileAllAccess),
+			},
+		},
+		{
+			// A deny and an allow can name the same rights in different
+			// vocabularies. GENERIC_ALL stands for FILE_ALL_ACCESS, so this deny
+			// settles every right the following allow asks for.
+			name: "generic deny ahead of specific allow grants nothing",
+			aces: []aceEntry{
+				deny(sidEveryone, genericAll),
+				allow(sidEveryone, fileAllAccess),
+			},
+		},
+		{
+			// The same in reverse: the deny names specific rights and the allow uses
+			// the generic alias for them, so it adds nothing.
+			name: "specific deny ahead of generic allow grants nothing",
+			aces: []aceEntry{
+				deny(sidEveryone, fileAllAccess),
+				allow(sidEveryone, genericAll),
+			},
+		},
+		{
+			// GENERIC_WRITE maps to FILE_GENERIC_WRITE, which carries none of DELETE,
+			// WRITE_DAC or WRITE_OWNER — so like the (WD,AD) case above, the allow's
+			// escalation rights survive the deny.
+			name: "generic write deny leaves the escalation rights a later allow grants",
+			aces: []aceEntry{
+				deny(sidEveryone, genericWrite),
+				allow(sidEveryone, fileAllAccess),
+			},
+			wantRead:  true,
+			wantWrite: true,
+		},
+		{
+			name: "inherit-only ACE does not apply to the object itself",
+			aces: []aceEntry{
+				{SID: sidEveryone, Allow: true, InheritOnly: true, Mask: fileAllAccess},
+			},
+		},
+		{
+			name: "grant to a specific local account is not a world grant",
+			aces: []aceEntry{allow(sidLocalUser, fileAllAccess)},
+		},
+		{
+			// A normal user-profile file: SYSTEM, Administrators, and the owner.
+			name: "typical user profile ACL is not world-accessible",
+			aces: []aceEntry{
+				allow("S-1-5-18", fileAllAccess),     // NT AUTHORITY\SYSTEM
+				allow("S-1-5-32-544", fileAllAccess), // BUILTIN\Administrators
+				allow(sidLocalUser, fileAllAccess),
+			},
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			got := worldPermFromACEs(c.aces)
+			if !got.Known {
+				t.Error("Known=false; a DACL we successfully read is a known posture")
+			}
+			if got.WorldReadable != c.wantRead {
+				t.Errorf("WorldReadable=%v want %v", got.WorldReadable, c.wantRead)
+			}
+			if got.WorldWritable != c.wantWrite {
+				t.Errorf("WorldWritable=%v want %v", got.WorldWritable, c.wantWrite)
+			}
+		})
+	}
+}

orbit/pkg/table/ai_tools/internal/fsutil/perm_acl_windows.go

@@ -0,0 +1,36 @@
+//go:build windows
+
+package fsutil
+
+import "golang.org/x/sys/windows"
+
+// The access-mask constants in perm_acl.go are hand-declared so that file builds
+// on every platform, which leaves nine literal hex values one typo away from
+// being silently wrong: TestWorldPermFromACEs uses those same constants on both
+// sides of its assertions, so a bad value is self-consistent and invisible
+// there, and the icacls test would only notice a mask that was broken outright.
+//
+// Pin them to x/sys's definitions here, where those are in scope. Each line is
+// zero while the two agree; any difference makes it a negative constant, which
+// does not fit in uint, so the package fails to compile with "constant -N
+// overflows uint" pointing at the offending line.
+//
+// fileAllAccess is absent below because x/sys declares no FILE_ALL_ACCESS.
+// TestStatPermWindowsACL covers it against a DACL that icacls really wrote.
+const (
+	_ uint = -(fileReadData ^ windows.FILE_READ_DATA)
+	_ uint = -(fileWriteData ^ windows.FILE_WRITE_DATA)
+	_ uint = -(fileAppendData ^ windows.FILE_APPEND_DATA)
+	_ uint = -(stdDelete ^ windows.DELETE)
+	_ uint = -(stdWriteDAC ^ windows.WRITE_DAC)
+	_ uint = -(stdWriteOwner ^ windows.WRITE_OWNER)
+	_ uint = -(genericAll ^ windows.GENERIC_ALL)
+	_ uint = -(genericExecute ^ windows.GENERIC_EXECUTE)
+	_ uint = -(genericWrite ^ windows.GENERIC_WRITE)
+	_ uint = -(genericRead ^ windows.GENERIC_READ)
+
+	// The generic mapping mapGenericRights applies.
+	_ uint = -(fileGenericRead ^ windows.FILE_GENERIC_READ)
+	_ uint = -(fileGenericWrite ^ windows.FILE_GENERIC_WRITE)
+	_ uint = -(fileGenericExecute ^ windows.FILE_GENERIC_EXECUTE)
+)