diff --git a/.github/workflows/test-go-windows.yml b/.github/workflows/test-go-windows.yml index aaf791b335d..a320846484a 100644 --- a/.github/workflows/test-go-windows.yml +++ b/.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/..." diff --git a/orbit/pkg/table/ai_tools/internal/fsutil/fsutil.go b/orbit/pkg/table/ai_tools/internal/fsutil/fsutil.go index d6571ce4386..a3a065a3dc2 100644 --- a/orbit/pkg/table/ai_tools/internal/fsutil/fsutil.go +++ b/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 diff --git a/orbit/pkg/table/ai_tools/internal/fsutil/perm_acl.go b/orbit/pkg/table/ai_tools/internal/fsutil/perm_acl.go new file mode 100644 index 00000000000..eb23bfad3f4 --- /dev/null +++ b/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 :F` + fileGenericRead = 0x00120089 // FILE_GENERIC_READ — GENERIC_READ, `icacls /grant :R` + fileGenericWrite = 0x00120116 // FILE_GENERIC_WRITE — GENERIC_WRITE, `icacls /grant :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 :(WD,AD) /grant +// :(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, + } +} diff --git a/orbit/pkg/table/ai_tools/internal/fsutil/perm_acl_test.go b/orbit/pkg/table/ai_tools/internal/fsutil/perm_acl_test.go new file mode 100644 index 00000000000..a2e4b4d3cb3 --- /dev/null +++ b/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 /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 :(WD,AD) /grant :(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) + } + }) + } +} diff --git a/orbit/pkg/table/ai_tools/internal/fsutil/perm_acl_windows.go b/orbit/pkg/table/ai_tools/internal/fsutil/perm_acl_windows.go new file mode 100644 index 00000000000..82e1bd27ed9 --- /dev/null +++ b/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) +) diff --git a/orbit/pkg/table/ai_tools/internal/fsutil/perm_unix.go b/orbit/pkg/table/ai_tools/internal/fsutil/perm_unix.go new file mode 100644 index 00000000000..ee771866400 --- /dev/null +++ b/orbit/pkg/table/ai_tools/internal/fsutil/perm_unix.go @@ -0,0 +1,21 @@ +//go:build !windows + +package fsutil + +import "os" + +// statPerm reads path's POSIX mode bits. Lstat, not Stat: the scanner runs as +// root over user-writable homes, and a symlink's target permissions say nothing +// about who can tamper with the path we actually reported. +func statPerm(path string) 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, + } +} diff --git a/orbit/pkg/table/ai_tools/internal/fsutil/perm_windows.go b/orbit/pkg/table/ai_tools/internal/fsutil/perm_windows.go new file mode 100644 index 00000000000..f9661b806d5 --- /dev/null +++ b/orbit/pkg/table/ai_tools/internal/fsutil/perm_windows.go @@ -0,0 +1,81 @@ +//go:build windows + +package fsutil + +import ( + "unsafe" + + "golang.org/x/sys/windows" +) + +// maxACEs bounds the DACL walk, so an implausibly long ACL can't turn one row of +// a table query into an unbounded loop of syscalls. No real file DACL is close. +const maxACEs = 4096 + +// statPerm reads path's DACL and reports whether it grants read or write to a +// well-known world SID — the Windows analogue of the POSIX other/group bits read +// on macOS and Linux. +// +// Every failure path reports an unknown posture rather than a clean one, so a +// file we couldn't inspect is never mistaken for a file we cleared. Reading the +// DACL needs an open handle, which POSIX mode bits don't, so a file held +// exclusively by another process reports unknown where Unix would still answer. +func statPerm(path string) Perm { + // Go through OpenRegular to inherit its guards: this runs as SYSTEM over + // user-writable paths, so it must refuse reparse points and non-regular files + // rather than be redirected onto another object. GetNamedSecurityInfo would + // skip the open, but it resolves reparse points, which is exactly what those + // guards exist to prevent. O_RDONLY maps to GENERIC_READ, which includes the + // READ_CONTROL right GetSecurityInfo needs. + f, err := OpenRegular(path) + if err != nil { + return Perm{} + } + defer func() { _ = f.Close() }() + + sd, err := windows.GetSecurityInfo(windows.Handle(f.Fd()), windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + return Perm{} + } + dacl, _, err := sd.DACL() + if err != nil { + return Perm{} + } + if dacl == nil { + // A NULL DACL is not an absent one: it grants full access to everyone. + return Perm{WorldReadable: true, WorldWritable: true, Known: true} + } + + count := int(dacl.AceCount) + if count > maxACEs { + // Truncating would drop the tail of the list, which under canonical + // ordering is where the allow ACEs live — a silent false negative. Report + // a DACL this size as a posture we did not establish. + return Perm{} + } + aces := make([]aceEntry, 0, count) + for i := range count { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(dacl, uint32(i), &ace); err != nil { + return Perm{} + } + // Only the two basic ACE types carry a plain SID at SidStart; object and + // callback ACE types lay out their trailing data differently, so reading a + // SID from them would be reading the wrong bytes. Neither type appears on + // an ordinary file DACL, and skipping one only loses a signal. + if ace.Header.AceType != windows.ACCESS_ALLOWED_ACE_TYPE && ace.Header.AceType != windows.ACCESS_DENIED_ACE_TYPE { + continue + } + // SidStart is the first DWORD of the variable-length SID that the ACE + // struct is only the fixed-size header of, so the SID is addressed + // through it rather than copied out. + sid := (*windows.SID)(unsafe.Pointer(&ace.SidStart)) //nolint:gosec // G103: reading the variable-length SID that trails the fixed ACE header is the documented Win32 layout + aces = append(aces, aceEntry{ + SID: sid.String(), + Allow: ace.Header.AceType == windows.ACCESS_ALLOWED_ACE_TYPE, + InheritOnly: ace.Header.AceFlags&windows.INHERIT_ONLY_ACE != 0, + Mask: uint32(ace.Mask), + }) + } + return worldPermFromACEs(aces) +} diff --git a/orbit/pkg/table/ai_tools/internal/fsutil/perm_windows_test.go b/orbit/pkg/table/ai_tools/internal/fsutil/perm_windows_test.go new file mode 100644 index 00000000000..8ae4b5cfef9 --- /dev/null +++ b/orbit/pkg/table/ai_tools/internal/fsutil/perm_windows_test.go @@ -0,0 +1,104 @@ +//go:build windows + +package fsutil + +import ( + "os" + "os/exec" + "os/user" + "path/filepath" + "testing" + + "golang.org/x/sys/windows" +) + +// TestWorldSIDConstants pins the SID strings in perm_acl.go to what Windows +// itself reports for the well-known SIDs they name. They have the same blind +// spot the mask constants do — TestWorldPermFromACEs compares them against +// themselves, so a typo is self-consistent there and would simply stop matching +// any real ACE, silently disabling the whole check. The masks are pinned at +// compile time in perm_acl_windows.go; these need a live SID to compare against. +func TestWorldSIDConstants(t *testing.T) { + for _, c := range []struct { + label string + typ windows.WELL_KNOWN_SID_TYPE + want string + }{ + {"Everyone", windows.WinWorldSid, sidEveryone}, + {"Authenticated Users", windows.WinAuthenticatedUserSid, sidAuthenticatedUsers}, + } { + sid, err := windows.CreateWellKnownSid(c.typ) + if err != nil { + t.Fatalf("CreateWellKnownSid(%s): %v", c.label, err) + } + got := sid.String() + if got != c.want { + t.Errorf("%s: Windows reports %s, our constant is %s", c.label, got, c.want) + } + if !isWorldSID(got) { + t.Errorf("%s (%s) not treated as a world SID", c.label, got) + } + } +} + +// TestStatPermWindowsACL drives the real DACL reader end to end with icacls, the +// same tool the bug report used to reproduce. worldPermFromACEs covers the +// decision rules; this covers the Win32 plumbing that feeds it. +func TestStatPermWindowsACL(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "GEMINI.md") + if err := os.WriteFile(path, []byte("# instructions\n"), 0o600); err != nil { + t.Fatal(err) + } + + // Replace the inherited ACL with a single ACE granting only the current user, + // so the baseline is the same whatever the runner's profile ACL looks like. + // SIDs are used throughout rather than names like "Everyone", which are + // localized on non-English Windows images. + u, err := user.Current() + if err != nil { + t.Fatal(err) + } + icacls(t, path, "/inheritance:r", "/grant:r", "*"+u.Uid+":(F)") + if p := Stat(path); !p.Known || p.WorldReadable || p.WorldWritable { + t.Errorf("owner-only ACL: %+v want known, not world readable/writable", p) + } + + icacls(t, path, "/grant", "*"+sidEveryone+":(R)") + if p := Stat(path); !p.Known || !p.WorldReadable || p.WorldWritable { + t.Errorf("Everyone:(R): %+v want known, world readable, not world writable", p) + } + + icacls(t, path, "/grant", "*"+sidEveryone+":(F)") + if p := Stat(path); !p.Known || !p.WorldReadable || !p.WorldWritable { + t.Errorf("Everyone:(F): %+v want known, world readable and writable", p) + } + + // Denying just the two data-write rights (icacls names them WD and AD) leaves + // the DELETE and WRITE_DAC from the grant above, so Everyone can still replace + // the file or re-ACL it — it stays world writable. icacls canonicalizes the + // DACL, putting this deny ahead of the allow, which is the ordering that would + // mask the risk if a deny settled write wholesale. + // + // The rights are named individually on purpose: icacls (W) expands to + // FILE_GENERIC_WRITE, which carries READ_CONTROL, and denying that to Everyone + // would stop the scanner from opening the file at all. + icacls(t, path, "/deny", "*"+sidEveryone+":(WD,AD)") + if p := Stat(path); !p.Known || !p.WorldWritable { + t.Errorf("Everyone:(F) with (WD,AD) denied: %+v want known and still world writable", p) + } +} + +func TestStatPermWindowsMissingFile(t *testing.T) { + if p := Stat(filepath.Join(t.TempDir(), "absent")); p.Known { + t.Errorf("%+v want Known=false for a path we cannot open", p) + } +} + +func icacls(t *testing.T, path string, args ...string) { + t.Helper() + out, err := exec.Command("icacls", append([]string{path}, args...)...).CombinedOutput() + if err != nil { + t.Fatalf("icacls %v: %v\n%s", args, err, out) + } +} diff --git a/orbit/pkg/table/ai_tools/internal/instructions/instructions_test.go b/orbit/pkg/table/ai_tools/internal/instructions/instructions_test.go index 635c1f46e4f..b184b09eab8 100644 --- a/orbit/pkg/table/ai_tools/internal/instructions/instructions_test.go +++ b/orbit/pkg/table/ai_tools/internal/instructions/instructions_test.go @@ -3,6 +3,7 @@ package instructions import ( "os" "path/filepath" + "runtime" "strings" "testing" @@ -87,6 +88,12 @@ func TestHiddenUnicode(t *testing.T) { } func TestWorldWritableFlag(t *testing.T) { + // os.Chmod on Windows only toggles the read-only attribute; it cannot produce + // a world-writable DACL, which is what fsutil.Stat reads there. The Windows + // side of the flag is covered by fsutil's icacls-driven TestStatPermWindowsACL. + if runtime.GOOS == "windows" { + t.Skip("POSIX mode bits not meaningful on Windows") + } home := t.TempDir() p := filepath.Join(home, "CLAUDE.md") write(t, p, "rules", 0o666) diff --git a/orbit/pkg/table/ai_tools/internal/mcp/risk_test.go b/orbit/pkg/table/ai_tools/internal/mcp/risk_test.go index c3ca9c38776..5069488f629 100644 --- a/orbit/pkg/table/ai_tools/internal/mcp/risk_test.go +++ b/orbit/pkg/table/ai_tools/internal/mcp/risk_test.go @@ -3,6 +3,7 @@ package mcp import ( "os" "path/filepath" + "runtime" "strings" "testing" @@ -75,11 +76,18 @@ func TestEnrichRiskFlags(t *testing.T) { } fs := by["fs"] - for _, want := range []string{"remote_fetch_exec", "unpinned_dependency", "plaintext_secret", "mcp_fs_write", "world_readable_config"} { + for _, want := range []string{"remote_fetch_exec", "unpinned_dependency", "plaintext_secret", "mcp_fs_write"} { if !strings.Contains(fs.RiskFlags, want) { t.Errorf("fs.RiskFlags=%q missing %q", fs.RiskFlags, want) } } + // world_readable_config comes from the 0644 mode above, which Windows ignores: + // there fsutil.Stat reads the file's DACL, and a temp file under the user + // profile grants no world SID. fsutil's TestStatPermWindowsACL covers that + // side; here the flag is only asserted where the mode bits mean something. + if runtime.GOOS != "windows" && !strings.Contains(fs.RiskFlags, "world_readable_config") { + t.Errorf("fs.RiskFlags=%q missing world_readable_config", fs.RiskFlags) + } if fs.SHA256 == "" { t.Error("fs.SHA256 should be set (config hash)") }