diff --git a/CHANGELOG.md b/CHANGELOG.md index e7343506..709c77dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,23 @@ ## 0.4.6 - Unreleased +### Security and Correctness + +- Report Windows owner SIDs and whether the owner is the current user, + LocalSystem, or built-in Administrators so credential-bearing executable + checks can reject foreign-owned paths even when their visible DACL is + read-only. Secure reads enforce the result and remote filesystems fail + closed. +- Read Windows owner and DACL data through the underlying .NET security + descriptor APIs so verification does not depend on PowerShell security-module + autoloading. +- Invoke `icacls.exe` with its supported path-only inspection syntax and use + the live Windows user/domain environment for named ACE classification, so + ACL verification works on supported Windows hosts instead of failing on the + invalid `/sid` argument. +- Normalize trailing Windows install-root separators with a bounded linear + scan so library-provided environment maps cannot trigger regex backtracking. + ### Docs and Tooling - Add the public repository governance baseline, pinned CodeQL analysis, diff --git a/docs/permissions.md b/docs/permissions.md index 753645f9..2eaf4640 100644 --- a/docs/permissions.md +++ b/docs/permissions.md @@ -38,7 +38,7 @@ isWorldReadable(bits); isGroupReadable(bits); ``` -`inspectPathPermissions()` follows symlink targets for the effective mode but tells you whether the original path was a symlink. On POSIX it reports owner/group/world bits. On Windows it delegates to the ACL helpers below. +`inspectPathPermissions()` follows symlink targets for the effective mode but tells you whether the original path was a symlink. On POSIX it reports owner/group/world bits. On Windows it delegates to the ACL helpers below and also reports `ownerSid` plus `ownerTrusted` when ownership can be verified. `ownerTrusted` is true only for a local volume owned by the current user, LocalSystem, or built-in Administrators; remote filesystems fail closed. Secure reads and callers that protect credential-bearing execution require `ownerTrusted === true`. ## Advanced Windows ACL helpers @@ -82,6 +82,9 @@ type PermissionCheck = { groupWritable: boolean; worldReadable: boolean; groupReadable: boolean; + ownerSid?: string; + ownerTrusted?: boolean; + ownerError?: string; aclSummary?: string; error?: string; }; diff --git a/src/local-file-access.ts b/src/local-file-access.ts index e3b32d9e..9ac4b9fa 100644 --- a/src/local-file-access.ts +++ b/src/local-file-access.ts @@ -21,7 +21,16 @@ export function isWindowsNetworkPath( return false; } const normalized = filePath.replace(/\//g, "\\"); - return normalized.startsWith("\\\\?\\UNC\\") || normalized.startsWith("\\\\"); + const extendedDrive = + normalized.length >= 7 && + normalized.startsWith("\\\\?\\") && + /^[a-z]$/i.test(normalized[4] ?? "") && + normalized[5] === ":" && + normalized[6] === "\\"; + if (extendedDrive) { + return false; + } + return normalized.startsWith("\\\\"); } export function isWindowsDriveLetterPath( diff --git a/src/permissions.ts b/src/permissions.ts index b5616667..ee6893b9 100644 --- a/src/permissions.ts +++ b/src/permissions.ts @@ -5,6 +5,12 @@ import path from "node:path"; import { promisify } from "node:util"; import { normalizeLowercaseStringOrEmpty } from "./string-coerce.js"; +import { resolveWindowsSystemCommand } from "./windows-command.js"; +import { + inspectWindowsOwner, + resolveWindowsCurrentUserSid, + resolveWindowsPrincipalSids, +} from "./windows-owner.js"; const execFileAsync = promisify(execFile); @@ -24,6 +30,12 @@ export type PermissionCheck = { groupWritable: boolean; worldReadable: boolean; groupReadable: boolean; + /** Canonical Windows owner SID when the owner query succeeds. */ + ownerSid?: string; + /** Whether the Windows owner is the current user, LocalSystem, or Administrators. */ + ownerTrusted?: boolean; + /** Owner-query failure detail when Windows ownership could not be verified. */ + ownerError?: string; aclSummary?: string; error?: string; }; @@ -46,6 +58,8 @@ export type SafeStatResult = { export type WindowsAclEntry = { principal: string; + /** Canonical principal SID when resolved from Windows. */ + sid?: string; rights: string[]; rawRights: string; canRead: boolean; @@ -172,7 +186,23 @@ export async function inspectPathPermissions( const bits = modeBits(effectiveMode); const platform = opts?.platform ?? process.platform; if (platform === "win32") { - const acl = await inspectWindowsAcl(targetPath, { env: opts?.env, exec: opts?.exec }); + const owner = await inspectWindowsOwner({ + targetPath, + env: opts?.env, + exec: opts?.exec ?? defaultPermissionExec, + }); + const acl = await inspectWindowsAcl(targetPath, { + env: opts?.env, + exec: opts?.exec, + currentUserSid: owner.currentUserSid, + principalSids: owner.principalSids, + principalTranslationFailed: owner.principalTranslationFailed, + }); + const ownerFields = { + ...(owner.sid ? { ownerSid: owner.sid } : {}), + ...(owner.trusted !== undefined ? { ownerTrusted: owner.trusted } : {}), + ...(owner.error ? { ownerError: owner.error } : {}), + }; if (!acl.ok) { return { ok: true, @@ -185,6 +215,7 @@ export async function inspectPathPermissions( groupWritable: false, worldReadable: false, groupReadable: false, + ...ownerFields, error: acl.error, }; } @@ -199,6 +230,7 @@ export async function inspectPathPermissions( groupWritable: acl.untrustedGroup.some((entry) => entry.canWrite), worldReadable: acl.untrustedWorld.some((entry) => entry.canRead), groupReadable: acl.untrustedGroup.some((entry) => entry.canRead), + ...ownerFields, aclSummary: formatWindowsAclSummary(acl), }; } @@ -298,42 +330,6 @@ function buildTrustedPrincipals(env?: NodeJS.ProcessEnv): Set { return trusted; } -function getEnvValueCaseInsensitive(env: NodeJS.ProcessEnv, name: string): string | undefined { - const direct = env[name]; - if (direct !== undefined) { - return direct; - } - const lower = name.toLowerCase(); - for (const [key, value] of Object.entries(env)) { - if (key.toLowerCase() === lower) { - return value; - } - } - return undefined; -} - -function normalizeWindowsInstallRoot(value: string | undefined): string | null { - const trimmed = value?.trim(); - if (!trimmed || !path.win32.isAbsolute(trimmed)) { - return null; - } - return trimmed.replace(/[\\/]+$/, ""); -} - -function resolveWindowsSystemRoot(env?: NodeJS.ProcessEnv): string { - const source = env ?? process.env; - return ( - normalizeWindowsInstallRoot(getEnvValueCaseInsensitive(source, "SystemRoot")) ?? - normalizeWindowsInstallRoot(getEnvValueCaseInsensitive(source, "WINDIR")) ?? - "C:\\Windows" - ); -} - -function resolveWindowsSystemCommand(command: string, env?: NodeJS.ProcessEnv): string { - const root = resolveWindowsSystemRoot(env); - return path.win32.join(root, "System32", command); -} - function classifyPrincipal( principal: string, trustedPrincipals: Set, @@ -401,7 +397,14 @@ function parseAceEntry(entry: string): WindowsAclEntry | null { if (tokens.some((token) => token.toUpperCase() === "DENY")) return null; const rights = tokens.filter((token) => !INHERIT_FLAGS.has(token.toUpperCase())); if (rights.length === 0) return null; - return { principal, rights, rawRights, ...rightsFromTokens(rights) }; + const normalizedPrincipal = normalizeSid(principal); + return { + principal, + ...(SID_RE.test(normalizedPrincipal) ? { sid: normalizedPrincipal } : {}), + rights, + rawRights, + ...rightsFromTokens(rights), + }; } export function parseIcaclsOutput(output: string, targetPath: string): WindowsAclEntry[] { @@ -440,7 +443,7 @@ export function summarizeWindowsAcl( const untrustedWorld: WindowsAclEntry[] = []; const untrustedGroup: WindowsAclEntry[] = []; for (const entry of entries) { - const classification = classifyPrincipal(entry.principal, trustedPrincipals); + const classification = classifyPrincipal(entry.sid ?? entry.principal, trustedPrincipals); if (classification === "trusted") trusted.push(entry); else if (classification === "world") untrustedWorld.push(entry); else untrustedGroup.push(entry); @@ -448,44 +451,51 @@ export function summarizeWindowsAcl( return { trusted, untrustedWorld, untrustedGroup }; } -async function resolveCurrentUserSid( - exec: PermissionExec, - env?: NodeJS.ProcessEnv, -): Promise { - try { - const { stdout, stderr } = await exec(resolveWindowsSystemCommand("whoami.exe", env), [ - "/user", - "/fo", - "csv", - "/nh", - ]); - const match = `${stdout}\n${stderr}`.match(/\*?S-\d+-\d+(?:-\d+)+/i); - return match ? normalizeSid(match[0]) : null; - } catch { - return null; - } -} - export async function inspectWindowsAcl( targetPath: string, - opts?: { env?: NodeJS.ProcessEnv; exec?: PermissionExec }, + opts?: { + env?: NodeJS.ProcessEnv; + exec?: PermissionExec; + currentUserSid?: string; + principalSids?: Record; + principalTranslationFailed?: boolean; + }, ): Promise { const exec = opts?.exec ?? defaultPermissionExec; try { + if (opts?.principalTranslationFailed) { + throw new Error("Windows ACL principal SID translation failed"); + } const { stdout, stderr } = await exec(resolveWindowsSystemCommand("icacls.exe", opts?.env), [ targetPath, - "/sid", ]); - const entries = parseIcaclsOutput(`${stdout}\n${stderr}`.trim(), targetPath); - let effectiveEnv = opts?.env; + let entries = parseIcaclsOutput(`${stdout}\n${stderr}`.trim(), targetPath); + const unresolvedPrincipals = entries + .filter((entry) => !entry.sid) + .map((entry) => entry.principal); + const principalSids = await resolveWindowsPrincipalSids({ + principals: unresolvedPrincipals, + known: opts?.principalSids, + env: opts?.env, + exec, + }); + entries = entries.map((entry) => { + const sid = entry.sid ?? principalSids[entry.principal.toLowerCase()]; + if (!sid) { + throw new Error(`Windows ACL principal SID could not be verified: ${entry.principal}`); + } + return { ...entry, sid }; + }); + let currentUserSid = normalizeSid(opts?.currentUserSid ?? ""); + let effectiveEnv = currentUserSid ? { USERSID: currentUserSid } : undefined; let { trusted, untrustedWorld, untrustedGroup } = summarizeWindowsAcl(entries, effectiveEnv); const needsUserSidResolution = - !effectiveEnv?.USERSID && - untrustedGroup.some((entry) => SID_RE.test(normalize(entry.principal))); + !currentUserSid && untrustedGroup.some((entry) => entry.sid && !TRUSTED_SIDS.has(entry.sid)); if (needsUserSidResolution) { - const currentUserSid = await resolveCurrentUserSid(exec, effectiveEnv); + currentUserSid = + (await resolveWindowsCurrentUserSid({ exec, env: opts?.env })) ?? ""; if (currentUserSid) { - effectiveEnv = { ...effectiveEnv, USERSID: currentUserSid }; + effectiveEnv = { USERSID: currentUserSid }; ({ trusted, untrustedWorld, untrustedGroup } = summarizeWindowsAcl(entries, effectiveEnv)); } } diff --git a/src/secure-file.ts b/src/secure-file.ts index d1747f4d..8be0834f 100644 --- a/src/secure-file.ts +++ b/src/secure-file.ts @@ -186,6 +186,12 @@ async function assertSecurePermissions( `${label(options)} ACL verification unavailable on Windows for ${realPath}.`, ); } + if (platform === "win32" && permissions.ownerTrusted !== true) { + throw new FsSafeError( + permissions.ownerTrusted === false ? "not-owned" : "permission-unverified", + `${label(options)} owner could not be trusted on Windows: ${realPath}`, + ); + } const writableByOthers = permissions.worldWritable || permissions.groupWritable; const readableByOthers = permissions.worldReadable || permissions.groupReadable; if (writableByOthers || (!options.permissions?.allowReadableByOthers && readableByOthers)) { diff --git a/src/windows-command.ts b/src/windows-command.ts new file mode 100644 index 00000000..b027ca5c --- /dev/null +++ b/src/windows-command.ts @@ -0,0 +1,46 @@ +import path from "node:path"; + +function getEnvValueCaseInsensitive( + env: NodeJS.ProcessEnv, + name: string, +): string | undefined { + const direct = env[name]; + if (direct !== undefined) { + return direct; + } + const lower = name.toLowerCase(); + for (const [key, value] of Object.entries(env)) { + if (key.toLowerCase() === lower) { + return value; + } + } + return undefined; +} + +function normalizeWindowsInstallRoot(value: string | undefined): string | null { + const trimmed = value?.trim(); + if (!trimmed || !path.win32.isAbsolute(trimmed)) { + return null; + } + let end = trimmed.length; + while (end > 0 && (trimmed[end - 1] === "\\" || trimmed[end - 1] === "/")) { + end -= 1; + } + return trimmed.slice(0, end); +} + +function resolveWindowsSystemRoot(env?: NodeJS.ProcessEnv): string { + const source = env ?? process.env; + return ( + normalizeWindowsInstallRoot(getEnvValueCaseInsensitive(source, "SystemRoot")) ?? + normalizeWindowsInstallRoot(getEnvValueCaseInsensitive(source, "WINDIR")) ?? + "C:\\Windows" + ); +} + +export function resolveWindowsSystemCommand( + command: string, + env?: NodeJS.ProcessEnv, +): string { + return path.win32.join(resolveWindowsSystemRoot(env), "System32", command); +} diff --git a/src/windows-owner.ts b/src/windows-owner.ts new file mode 100644 index 00000000..b56ba674 --- /dev/null +++ b/src/windows-owner.ts @@ -0,0 +1,171 @@ +import { resolveWindowsSystemCommand } from "./windows-command.js"; + +export type WindowsOwnerExec = ( + command: string, + args: string[], +) => Promise<{ stdout: string; stderr: string }>; + +export type WindowsOwnerSummary = { + sid?: string; + currentUserSid?: string; + principalSids?: Record; + principalTranslationFailed?: boolean; + remote?: boolean; + trusted?: boolean; + error?: string; +}; + +const SID_RE = /^\*?s-\d+-\d+(-\d+)+$/i; +const TRUSTED_OWNER_SIDS = new Set(["s-1-5-18", "s-1-5-32-544"]); + +function normalizeSid(value: string): string { + const normalized = value.trim().toLowerCase(); + return normalized.startsWith("*") ? normalized.slice(1) : normalized; +} + +function encodePowerShellCommand(source: string): string { + return Buffer.from(source, "utf16le").toString("base64"); +} + +function windowsOwnerQueryCommand(targetPath: string): string { + const encodedPath = Buffer.from(targetPath, "utf8").toString("base64"); + return [ + "$ErrorActionPreference='Stop'", + `$p=[Text.Encoding]::UTF8.GetString([Convert]::FromBase64String('${encodedPath}'))`, + "$sections=[System.Security.AccessControl.AccessControlSections]::Access -bor [System.Security.AccessControl.AccessControlSections]::Owner", + "$acl=if([IO.Directory]::Exists($p)){[IO.Directory]::GetAccessControl($p,$sections)}else{[IO.File]::GetAccessControl($p,$sections)}", + "$ownerSid=$acl.GetOwner([System.Security.Principal.SecurityIdentifier]).Value", + "$currentSid=[System.Security.Principal.WindowsIdentity]::GetCurrent().User.Value", + "$root=[IO.Path]::GetPathRoot($p)", + "$extendedDrive=$p.Length -ge 7 -and $p.StartsWith('\\\\?\\') -and [char]::IsLetter($p[4]) -and $p[5] -eq ':' -and $p[6] -eq '\\'", + "$driveRoot=if($extendedDrive){$p.Substring(4,3)}else{$root}", + "$namespacePath=$p.StartsWith('\\\\')", + "$remote=($namespacePath -and -not $extendedDrive) -or ([IO.DriveInfo]::new($driveRoot).DriveType -eq [IO.DriveType]::Network)", + "$rules=$acl.GetAccessRules($true,$true,[System.Security.Principal.SecurityIdentifier])", + "$principalSids=@($rules|ForEach-Object {$identity=$_.IdentityReference;$sid=$identity.Value;@{name=$sid;sid=$sid};try{@{name=$identity.Translate([System.Security.Principal.NTAccount]).Value;sid=$sid}}catch{}})", + "@{ownerSid=$ownerSid;currentUserSid=$currentSid;principalSids=$principalSids;principalTranslationFailed=$false;remote=$remote}|ConvertTo-Json -Depth 4 -Compress", + ].join(";"); +} + +function windowsPrincipalQueryCommand(principals: string[]): string { + const encodedPrincipals = Buffer.from(JSON.stringify(principals), "utf8").toString("base64"); + return [ + "$ErrorActionPreference='Stop'", + `$names=[Text.Encoding]::UTF8.GetString([Convert]::FromBase64String('${encodedPrincipals}'))|ConvertFrom-Json`, + "$rows=@($names|ForEach-Object {@{name=$_;sid=(New-Object System.Security.Principal.NTAccount($_)).Translate([System.Security.Principal.SecurityIdentifier]).Value}})", + "ConvertTo-Json -InputObject $rows -Compress", + ].join(";"); +} + +function parsePrincipalSidRows(value: unknown): Record { + const rows = Array.isArray(value) ? value : value ? [value] : []; + const result: Record = {}; + for (const row of rows) { + if (!row || typeof row !== "object") { + continue; + } + const name = "name" in row && typeof row.name === "string" ? row.name.trim() : ""; + const sid = "sid" in row && typeof row.sid === "string" ? normalizeSid(row.sid) : ""; + if (name && SID_RE.test(sid)) { + result[name.toLowerCase()] = sid; + } + } + return result; +} + +export async function resolveWindowsPrincipalSids(params: { + principals: string[]; + known?: Record; + env?: NodeJS.ProcessEnv; + exec: WindowsOwnerExec; +}): Promise> { + const principals = [...new Set(params.principals.map((value) => value.trim()).filter(Boolean))]; + const known = Object.fromEntries( + Object.entries(params.known ?? {}).map(([name, sid]) => [name.toLowerCase(), normalizeSid(sid)]), + ); + const unresolved = principals.filter((principal) => !known[principal.toLowerCase()]); + if (unresolved.length === 0) { + return known; + } + const command = resolveWindowsSystemCommand( + String.raw`WindowsPowerShell\v1.0\powershell.exe`, + params.env, + ); + const { stdout } = await params.exec(command, [ + "-NoLogo", + "-NoProfile", + "-NonInteractive", + "-EncodedCommand", + encodePowerShellCommand(windowsPrincipalQueryCommand(unresolved)), + ]); + const resolved = { ...known, ...parsePrincipalSidRows(JSON.parse(stdout.trim())) }; + if (principals.some((principal) => !resolved[principal.toLowerCase()])) { + throw new Error("Windows ACL principal translation returned incomplete SID data"); + } + return resolved; +} + +export async function resolveWindowsCurrentUserSid(params: { + env?: NodeJS.ProcessEnv; + exec: WindowsOwnerExec; +}): Promise { + try { + const { stdout, stderr } = await params.exec( + resolveWindowsSystemCommand("whoami.exe", params.env), + ["/user", "/fo", "csv", "/nh"], + ); + const match = `${stdout}\n${stderr}`.match(/\*?S-\d+-\d+(?:-\d+)+/i); + return match ? normalizeSid(match[0]) : null; + } catch { + return null; + } +} + +export async function inspectWindowsOwner(params: { + targetPath: string; + env?: NodeJS.ProcessEnv; + exec: WindowsOwnerExec; +}): Promise { + try { + const command = resolveWindowsSystemCommand( + String.raw`WindowsPowerShell\v1.0\powershell.exe`, + params.env, + ); + const { stdout } = await params.exec(command, [ + "-NoLogo", + "-NoProfile", + "-NonInteractive", + "-EncodedCommand", + encodePowerShellCommand(windowsOwnerQueryCommand(params.targetPath)), + ]); + const parsed = JSON.parse(stdout.trim()) as { + ownerSid?: unknown; + currentUserSid?: unknown; + principalSids?: unknown; + principalTranslationFailed?: unknown; + remote?: unknown; + }; + const ownerSid = + typeof parsed.ownerSid === "string" && SID_RE.test(parsed.ownerSid) + ? normalizeSid(parsed.ownerSid) + : undefined; + const currentUserSid = + typeof parsed.currentUserSid === "string" && SID_RE.test(parsed.currentUserSid) + ? normalizeSid(parsed.currentUserSid) + : undefined; + if (!ownerSid || !currentUserSid) { + return { error: "Windows owner query returned invalid SID data" }; + } + const remote = parsed.remote === true; + return { + sid: ownerSid, + currentUserSid, + principalSids: parsePrincipalSidRows(parsed.principalSids), + principalTranslationFailed: parsed.principalTranslationFailed === true, + remote, + trusted: !remote && (ownerSid === currentUserSid || TRUSTED_OWNER_SIDS.has(ownerSid)), + }; + } catch (err) { + return { error: String(err) }; + } +} diff --git a/test/new-primitives.test.ts b/test/new-primitives.test.ts index 04c6cd59..9b70c2ff 100644 --- a/test/new-primitives.test.ts +++ b/test/new-primitives.test.ts @@ -1,7 +1,9 @@ +import { execFile } from "node:child_process"; import fs from "node:fs/promises"; import syncFs from "node:fs"; import os from "node:os"; import path from "node:path"; +import { promisify } from "node:util"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { appendRegularFile, @@ -49,6 +51,24 @@ import { } from "../src/walk.js"; let root: string; +const execFileAsync = promisify(execFile); + +async function secureWindowsTestFile(filePath: string): Promise { + const username = os.userInfo().username; + const commandEnv = { SystemRoot: process.env.SystemRoot }; + const userInfo = () => ({ username }); + const principal = resolveWindowsUserPrincipal(commandEnv, userInfo); + const reset = createIcaclsResetCommand(filePath, { + isDir: false, + env: commandEnv, + userInfo, + }); + if (!principal || !reset) { + throw new Error("Windows test principal could not be resolved"); + } + await execFileAsync(reset.command, [filePath, "/setowner", principal], { windowsHide: true }); + await execFileAsync(reset.command, reset.args, { windowsHide: true }); +} beforeEach(async () => { root = await fs.mkdtemp(path.join(os.tmpdir(), "fs-safe-new-")); @@ -195,21 +215,60 @@ describe("secure file reads", () => { }); it.runIf(process.platform === "win32")( - "fails closed on windows when ACL inspection is unavailable", + "reads from a validated Windows ACL and owner", async () => { - // See src/secure-file.ts:177 — readSecureFile throws permission-unverified - // on Windows because ACL inspection has no portable equivalent. const filePath = path.join(root, "secret.json"); await fs.writeFile(filePath, '{"token":"ok"}', { mode: 0o600 }); + await secureWindowsTestFile(filePath); - await expect( - readSecureFile({ - filePath, - label: "test secret", - io: { maxBytes: 1024 }, - }), - ).rejects.toMatchObject({ code: "permission-unverified" }); + const permissions = await inspectPathPermissions(filePath); + expect(permissions, permissions.ownerError ?? JSON.stringify(permissions)).toMatchObject({ + source: "windows-acl", + ownerTrusted: true, + }); + + const result = await readSecureFile({ + filePath, + label: "test secret", + io: { maxBytes: 1024 }, + }); + + expect(result.buffer.toString("utf8")).toBe('{"token":"ok"}'); + expect(result.permissions).toMatchObject({ + source: "windows-acl", + ownerTrusted: true, + }); + }, + 15_000, + ); + + it.runIf(process.platform === "win32")( + "treats an extended-length local Windows path as local", + async () => { + const filePath = path.join(root, "extended-secret.json"); + await fs.writeFile(filePath, '{"token":"ok"}', { mode: 0o600 }); + await secureWindowsTestFile(filePath); + const extendedPath = `\\\\?\\${path.resolve(filePath)}`; + + const permissions = await inspectPathPermissions(extendedPath); + expect(permissions, permissions.ownerError ?? JSON.stringify(permissions)).toMatchObject({ + source: "windows-acl", + ownerTrusted: true, + }); + + const result = await readSecureFile({ + filePath: extendedPath, + label: "extended-path secret", + io: { maxBytes: 1024 }, + }); + + expect(result.buffer.toString("utf8")).toBe('{"token":"ok"}'); + expect(result.permissions).toMatchObject({ + source: "windows-acl", + ownerTrusted: true, + }); }, + 15_000, ); it("rejects symlinks and files outside trusted dirs", async () => { @@ -287,10 +346,16 @@ describe("secure file reads", () => { it("uses Windows ACL permission checks for secure reads when requested", async () => { const filePath = path.join(root, "windows-secret.txt"); await fs.writeFile(filePath, "secret", { mode: 0o600 }); - const exec = vi.fn().mockResolvedValue({ - stdout: "*S-1-5-18:(F)\n", - stderr: "", - }); + const exec = vi + .fn() + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + ownerSid: "S-1-5-21-42", + currentUserSid: "S-1-5-21-42", + }), + stderr: "", + }) + .mockResolvedValueOnce({ stdout: "*S-1-5-18:(F)\n", stderr: "" }); const result = await readSecureFile({ filePath, @@ -300,10 +365,17 @@ describe("secure file reads", () => { expect(result.buffer.toString("utf8")).toBe("secret"); expect(result.permissions?.source).toBe("windows-acl"); - const unsafeExec = vi.fn().mockResolvedValue({ - stdout: "Everyone:(R)\n", - stderr: "", - }); + const unsafeExec = vi + .fn() + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + ownerSid: "S-1-5-21-42", + currentUserSid: "S-1-5-21-42", + principalSids: [{ name: "Everyone", sid: "S-1-1-0" }], + }), + stderr: "", + }) + .mockResolvedValueOnce({ stdout: "Everyone:(R)\n", stderr: "" }); await expect( readSecureFile({ filePath, @@ -311,6 +383,24 @@ describe("secure file reads", () => { }), ).rejects.toMatchObject({ code: "insecure-permissions" }); + const foreignOwnerExec = vi + .fn() + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + ownerSid: "S-1-5-21-999", + currentUserSid: "S-1-5-21-42", + }), + stderr: "", + }) + .mockResolvedValueOnce({ stdout: "*S-1-5-21-999:(R)\n", stderr: "" }); + await expect( + readSecureFile({ + filePath, + inject: { platform: "win32", exec: foreignOwnerExec }, + permissions: { allowReadableByOthers: true }, + }), + ).rejects.toMatchObject({ code: "not-owned" }); + const failedExec = vi.fn().mockRejectedValue(new Error("icacls failed")); await expect( readSecureFile({ @@ -344,7 +434,6 @@ describe("secure file reads", () => { expect(result.ok).toBe(true); expect(exec).toHaveBeenCalledWith("D:\\Windows\\System32\\icacls.exe", [ String.raw`C:\Users\me\secret.txt`, - "/sid", ]); const fallbackExec = vi.fn().mockResolvedValue({ @@ -357,7 +446,6 @@ describe("secure file reads", () => { }); expect(fallbackExec).toHaveBeenCalledWith("E:\\Windows\\System32\\icacls.exe", [ String.raw`C:\Users\me\secret.txt`, - "/sid", ]); const command = createIcaclsResetCommand(String.raw`C:\Users\me\secret.txt`, { @@ -366,6 +454,19 @@ describe("secure file reads", () => { userInfo: () => ({ username: "me" }), }); expect(command?.command).toBe("C:\\Windows\\System32\\icacls.exe"); + + const trailingSeparatorsExec = vi.fn().mockResolvedValue({ + stdout: String.raw`C:\Users\me\secret.txt *S-1-5-18:(F)`, + stderr: "", + }); + await inspectWindowsAcl(String.raw`C:\Users\me\secret.txt`, { + exec: trailingSeparatorsExec, + env: { SystemRoot: `D:\\Windows${"/".repeat(10_000)}` }, + }); + expect(trailingSeparatorsExec).toHaveBeenCalledWith( + "D:\\Windows\\System32\\icacls.exe", + [String.raw`C:\Users\me\secret.txt`], + ); }); it("covers permission formatting and ACL classification helpers", async () => { @@ -473,6 +574,186 @@ describe("secure file reads", () => { expect(result.groupWritable).toBe(false); }); + it("reports a foreign Windows owner even when the visible ACL is read-only", async () => { + const target = path.join(root, "windows-foreign-owner.txt"); + await fs.writeFile(target, "secret", { mode: 0o600 }); + const exec = vi.fn(async (command: string) => { + if (command.toLowerCase().endsWith("powershell.exe")) { + return { + stdout: JSON.stringify({ + owner: "DOMAIN\\attacker", + ownerSid: "S-1-5-21-999", + currentUserSid: "S-1-5-21-42", + }), + stderr: "", + }; + } + return { + stdout: `${target} *S-1-5-21-42:(RX)\n*S-1-5-18:(F)\n*S-1-5-32-544:(F)\n`, + stderr: "", + }; + }); + + const result = await inspectPathPermissions(target, { + platform: "win32", + env: { SystemRoot: "C:\\Windows" }, + exec, + }); + + expect(result).toMatchObject({ + source: "windows-acl", + groupWritable: false, + worldWritable: false, + ownerSid: "s-1-5-21-999", + ownerTrusted: false, + }); + }); + + it.each(["S-1-5-21-42", "S-1-5-18", "S-1-5-32-544"])( + "trusts the supported Windows owner SID %s", + async (ownerSid) => { + const target = path.join(root, `windows-trusted-owner-${ownerSid}.txt`); + await fs.writeFile(target, "secret", { mode: 0o600 }); + const exec = vi.fn(async (command: string) => { + if (command.toLowerCase().endsWith("powershell.exe")) { + return { + stdout: JSON.stringify({ + owner: ownerSid, + ownerSid, + currentUserSid: "S-1-5-21-42", + }), + stderr: "", + }; + } + return { + stdout: `${target} *S-1-5-21-42:(RX)\n*S-1-5-18:(F)\n*S-1-5-32-544:(F)\n`, + stderr: "", + }; + }); + + const result = await inspectPathPermissions(target, { + platform: "win32", + env: { SystemRoot: "C:\\Windows" }, + exec, + }); + + expect(result.ownerSid).toBe(ownerSid.toLowerCase()); + expect(result.ownerTrusted).toBe(true); + }, + ); + + it("queries the canonical Windows owner SID without a friendly-name round trip", async () => { + const target = path.join(root, "windows-canonical-owner.txt"); + await fs.writeFile(target, "secret", { mode: 0o600 }); + const exec = vi.fn(async (command: string, _args: string[]) => { + if (command.toLowerCase().endsWith("powershell.exe")) { + return { + stdout: JSON.stringify({ + ownerSid: "S-1-5-21-42", + currentUserSid: "S-1-5-21-42", + }), + stderr: "", + }; + } + return { stdout: `${target} *S-1-5-21-42:(F)\n`, stderr: "" }; + }); + + await inspectPathPermissions(target, { + platform: "win32", + env: { SystemRoot: "C:\\Windows" }, + exec, + }); + + const ownerArgs = exec.mock.calls[0]?.[1]; + const ownerQuery = Buffer.from(ownerArgs?.[4] ?? "", "base64").toString("utf16le"); + expect(ownerQuery).toContain( + "$acl.GetOwner([System.Security.Principal.SecurityIdentifier]).Value", + ); + expect(ownerQuery).toContain("[IO.File]::GetAccessControl($p,$sections)"); + expect(ownerQuery).toContain( + "$acl.GetAccessRules($true,$true,[System.Security.Principal.SecurityIdentifier])", + ); + expect(ownerQuery).not.toContain("Get-Acl"); + expect(ownerQuery).not.toContain("$acl.Owner"); + }); + + it("leaves Windows ownership unverified when the owner query fails", async () => { + const target = path.join(root, "windows-owner-query-failure.txt"); + await fs.writeFile(target, "secret", { mode: 0o600 }); + const exec = vi.fn(async (command: string) => { + if (command.toLowerCase().endsWith("powershell.exe")) { + throw new Error("owner lookup failed"); + } + return { + stdout: `${target} *S-1-5-21-42:(RX)\n`, + stderr: "", + }; + }); + + const result = await inspectPathPermissions(target, { + platform: "win32", + env: { SystemRoot: "C:\\Windows" }, + exec, + }); + + expect(result.source).toBe("windows-acl"); + expect(result.ownerSid).toBeUndefined(); + expect(result.ownerTrusted).toBeUndefined(); + expect(result.ownerError).toContain("owner lookup failed"); + }); + + it("fails Windows ACL verification closed when a principal SID cannot be translated", async () => { + const target = path.join(root, "windows-untranslated-principal.txt"); + await fs.writeFile(target, "secret", { mode: 0o600 }); + const exec = vi.fn(async () => ({ + stdout: JSON.stringify({ + ownerSid: "S-1-5-21-42", + currentUserSid: "S-1-5-21-42", + principalTranslationFailed: true, + }), + stderr: "", + })); + + const result = await inspectPathPermissions(target, { + platform: "win32", + env: { SystemRoot: "C:\\Windows" }, + exec, + }); + + expect(result).toMatchObject({ + source: "unknown", + ownerTrusted: true, + error: expect.stringContaining("principal SID translation failed"), + }); + expect(exec).toHaveBeenCalledTimes(1); + }); + + it("does not trust well-known local owners on remote Windows filesystems", async () => { + const target = path.join(root, "windows-remote-owner.txt"); + await fs.writeFile(target, "secret", { mode: 0o600 }); + const exec = vi.fn(async (command: string) => { + if (command.toLowerCase().endsWith("powershell.exe")) { + return { + stdout: JSON.stringify({ + ownerSid: "S-1-5-32-544", + currentUserSid: "S-1-5-21-42", + remote: true, + }), + stderr: "", + }; + } + return { stdout: `${target} *S-1-5-32-544:(F)\n`, stderr: "" }; + }); + + const result = await inspectPathPermissions(target, { + platform: "win32", + env: { SystemRoot: "C:\\Windows" }, + exec, + }); + + expect(result.ownerTrusted).toBe(false); + }); + it("resolves the current user SID when ACL output only contains an unknown SID", async () => { const target = String.raw`C:\Secrets\token.txt`; const exec = vi @@ -481,6 +762,10 @@ describe("secure file reads", () => { stdout: `${target} *S-1-5-21-42:(F)\nEveryone:(R)\n`, stderr: "", }) + .mockResolvedValueOnce({ + stdout: JSON.stringify([{ name: "Everyone", sid: "S-1-1-0" }]), + stderr: "", + }) .mockResolvedValueOnce({ stdout: '"USER","SID"\n"DOMAIN\\me","S-1-5-21-42"\n', stderr: "", @@ -489,7 +774,41 @@ describe("secure file reads", () => { const result = await inspectWindowsAcl(target, { exec, env: { SystemRoot: "C:\\Windows" } }); expect(result.ok).toBe(true); expect(result.trusted.some((entry) => entry.principal === "*S-1-5-21-42")).toBe(true); - expect(exec).toHaveBeenCalledTimes(2); + expect(exec).toHaveBeenCalledTimes(3); + }); + + it("classifies friendly ACL names by authoritative SID instead of spoofed environment names", async () => { + const target = String.raw`C:\Secrets\token.txt`; + const exec = vi + .fn() + .mockResolvedValueOnce({ + stdout: `${target} DOMAIN\\attacker:(F)\n`, + stderr: "", + }) + .mockResolvedValueOnce({ + stdout: JSON.stringify([{ name: "DOMAIN\\attacker", sid: "S-1-5-21-999" }]), + stderr: "", + }) + .mockResolvedValueOnce({ + stdout: '"USER","SID"\n"DOMAIN\\me","S-1-5-21-42"\n', + stderr: "", + }); + + const result = await inspectWindowsAcl(target, { + exec, + env: { + SystemRoot: "C:\\Windows", + USERDOMAIN: "DOMAIN", + USERNAME: "attacker", + USERSID: "S-1-5-21-999", + }, + }); + + expect(result.ok).toBe(true); + expect(result.trusted).toEqual([]); + expect(result.untrustedGroup).toMatchObject([ + { principal: "DOMAIN\\attacker", sid: "s-1-5-21-999", canWrite: true }, + ]); }); }); diff --git a/test/sidecar-lock-ownership-token.test.ts b/test/sidecar-lock-ownership-token.test.ts index 02a01693..edfeaa0b 100644 --- a/test/sidecar-lock-ownership-token.test.ts +++ b/test/sidecar-lock-ownership-token.test.ts @@ -238,7 +238,8 @@ describe("sidecar lock ownership tokens", () => { await fsp.writeFile(lockPath, raw, "utf8"); const stat = await fsp.lstat(lockPath); const driftedStat = Object.assign(Object.create(Object.getPrototypeOf(stat)), stat, { - ino: typeof stat.ino === "bigint" ? stat.ino + 1n : stat.ino + 1, + // Windows file indexes can be above Number.MAX_SAFE_INTEGER; use a visible delta. + ino: typeof stat.ino === "bigint" ? stat.ino + 1024n : stat.ino + 1024, }); expect( diff --git a/test/windows-path.test.ts b/test/windows-path.test.ts new file mode 100644 index 00000000..bd432d39 --- /dev/null +++ b/test/windows-path.test.ts @@ -0,0 +1,15 @@ +import { describe, expect, it } from "vitest"; +import { isWindowsNetworkPath } from "../src/local-file-access.js"; + +describe("Windows path classification", () => { + it("distinguishes extended local paths from UNC paths", () => { + expect(isWindowsNetworkPath("\\\\server\\share\\token", "win32")).toBe(true); + expect(isWindowsNetworkPath("\\\\?\\UNC\\server\\share\\token", "win32")).toBe(true); + expect(isWindowsNetworkPath("\\\\?\\C:\\secrets\\token", "win32")).toBe(false); + expect( + isWindowsNetworkPath("\\\\?\\GLOBALROOT\\Device\\Mup\\server\\share\\token", "win32"), + ).toBe(true); + expect(isWindowsNetworkPath("\\\\?\\Volume{abc}\\secrets\\token", "win32")).toBe(true); + expect(isWindowsNetworkPath("\\\\.\\pipe\\service", "win32")).toBe(true); + }); +});