diff --git a/src/main/ipc/firewall-audit.ipc.scan.test.ts b/src/main/ipc/firewall-audit.ipc.scan.test.ts new file mode 100644 index 00000000..0482d4e5 --- /dev/null +++ b/src/main/ipc/firewall-audit.ipc.scan.test.ts @@ -0,0 +1,137 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' + +// ── Mocks ── + +vi.mock('electron', () => ({ + ipcMain: { handle: vi.fn() }, +})) + +const mockSpawnTrackedLines = vi.fn() +vi.mock('../services/exec-utf8', () => ({ + psUtf8: (cmd: string) => cmd, + spawnTrackedLines: (...args: unknown[]) => mockSpawnTrackedLines(...args), +})) + +import { scanFirewallRules } from './firewall-audit.ipc' +import type { FirewallScanProgress } from '../../shared/types' + +const originalPlatform = process.platform + +function setPlatform(p: string): void { + Object.defineProperty(process, 'platform', { value: p, configurable: true }) +} + +/** + * Stand in for the PowerShell run: replay `lines` through the onLine callback, + * then resolve with the given process outcome. + */ +function stubScan( + lines: string[], + outcome: { stderr?: string; code?: number | null; timedOut?: boolean } = {} +): void { + mockSpawnTrackedLines.mockImplementation( + async (_file: string, _args: string[], onLine: (line: string) => void) => { + for (const l of lines) onLine(l) + return { stderr: outcome.stderr ?? '', code: outcome.code ?? 0, timedOut: outcome.timedOut ?? false } + } + ) +} + +// A third-party rule with a program that no longer exists → "stale". +const STALE_RULE = + 'RULE|MyApp-In|My App (In)|Lets my app listen||Private|TCP|8080|LocalSubnet|C:\\Apps\\gone.exe|C:\\Apps\\gone.exe|False||False|False|true' +// A Microsoft rule pointing at a real system binary → built-in, no findings. +const BUILTIN_RULE = + 'RULE|CoreNet-In|Core Networking (In)|@FirewallAPI.dll,-25000|@FirewallAPI.dll,-25000|Any|Any|Any|Any|%SystemRoot%\\system32\\svchost.exe|C:\\Windows\\system32\\svchost.exe|True|signed|True|False|true' + +beforeEach(() => { + mockSpawnTrackedLines.mockReset() + setPlatform('win32') +}) + +afterEach(() => { + setPlatform(originalPlatform) +}) + +describe('scanFirewallRules', () => { + it('returns an empty result off Windows without spawning anything', async () => { + setPlatform('linux') + const result = await scanFirewallRules() + expect(result.rules).toEqual([]) + expect(result.totalCount).toBe(0) + expect(mockSpawnTrackedLines).not.toHaveBeenCalled() + }) + + it('parses streamed rule lines and derives the issue counts', async () => { + stubScan(['TOTAL|2', STALE_RULE, BUILTIN_RULE]) + const result = await scanFirewallRules() + + expect(result.rules).toHaveLength(2) + expect(result.totalCount).toBe(2) + expect(result.staleCount).toBe(1) + expect(result.rules[0].name).toBe('MyApp-In') + expect(result.rules[0].issues).toContain('stale') + expect(result.rules[1].builtin).toBe(true) + expect(result.truncated).toBe(false) + }) + + it('reports progress from PROG markers as they stream in, not after the fact', async () => { + const seen: FirewallScanProgress[] = [] + // Record how many rules had been parsed when each progress event fired — + // buffering the whole run would make every event arrive at the end. + const parsedAt: number[] = [] + mockSpawnTrackedLines.mockImplementation( + async (_f: string, _a: string[], onLine: (line: string) => void) => { + onLine('TOTAL|2') + onLine(STALE_RULE) + onLine('PROG|1|2|My App (In)') + onLine(BUILTIN_RULE) + onLine('PROG|2|2|Core Networking (In)') + return { stderr: '', code: 0, timedOut: false } + } + ) + + await scanFirewallRules((p) => { + seen.push(p) + parsedAt.push(p.current) + }) + + const classifying = seen.filter((p) => p.phase === 'classifying') + expect(classifying).toHaveLength(2) + expect(classifying[0]).toMatchObject({ current: 1, total: 2, currentRule: 'My App (In)' }) + expect(classifying[1]).toMatchObject({ current: 2, total: 2, currentRule: 'Core Networking (In)' }) + expect(parsedAt).toEqual([0, 1, 2]) + }) + + it('surfaces stderr rather than the echoed script when the run fails outright', async () => { + stubScan([], { stderr: 'Get-NetFirewallRule : Access is denied.', code: 1 }) + + await expect(scanFirewallRules()).rejects.toThrow(/Access is denied/) + await expect(scanFirewallRules()).rejects.not.toThrow(/Get-NetFirewallApplicationFilter/) + }) + + it('falls back to the exit code when a failed run wrote nothing to stderr', async () => { + stubScan([], { stderr: '', code: 1 }) + await expect(scanFirewallRules()).rejects.toThrow(/exited with code 1/) + }) + + it('reports a timeout in plain language when nothing was returned', async () => { + stubScan([], { code: null, timedOut: true }) + await expect(scanFirewallRules()).rejects.toThrow(/timed out/i) + }) + + it('keeps rules streamed before a timeout and marks the result truncated', async () => { + stubScan(['TOTAL|2', STALE_RULE], { code: null, timedOut: true }) + const result = await scanFirewallRules() + + expect(result.rules).toHaveLength(1) + expect(result.totalCount).toBe(2) + expect(result.truncated).toBe(true) + }) + + it('marks the result truncated when fewer rules arrived than were enumerated', async () => { + stubScan(['TOTAL|5', STALE_RULE]) + const result = await scanFirewallRules() + expect(result.truncated).toBe(true) + }) +}) diff --git a/src/main/ipc/firewall-audit.ipc.ts b/src/main/ipc/firewall-audit.ipc.ts index fdbcdadf..c742611c 100644 --- a/src/main/ipc/firewall-audit.ipc.ts +++ b/src/main/ipc/firewall-audit.ipc.ts @@ -14,7 +14,7 @@ import type { FirewallRiskLevel, FirewallAction, } from '../../shared/types' -import { psUtf8 } from '../services/exec-utf8' +import { psUtf8, spawnTrackedLines } from '../services/exec-utf8' const execFileAsync = promisify(execFile) @@ -23,6 +23,31 @@ function psArgs(script: string): string[] { } const PS_OPTS = { timeout: 120_000, maxBuffer: 50 * 1024 * 1024, windowsHide: true } +// The scan streams its results, so a generous ceiling costs nothing on a +// healthy system and only bounds the pathological case (a machine where the +// bulk filter enumeration is denied and every rule falls back to a ~240 ms +// association query). +const SCAN_TIMEOUT_MS = 300_000 + +/** + * Build an actionable error for a failed PowerShell run. + * + * Node rejects with "Command failed: ", which for + * these scripts means kilobytes of echoed PowerShell with the actual reason + * nowhere in it (issue #246). Prefer stderr, fall back to the exit code, and + * never include the script itself. + */ +function powerShellError(context: string, code: number | null, stderr: string): Error { + const detail = stderr + .split('\n') + .map((l) => l.trim()) + .filter(Boolean) + .slice(0, 5) + .join(' ') + if (detail) return new Error(`${context}: ${detail}`) + return new Error(`${context}: PowerShell exited with code ${code ?? 'unknown'}`) +} + // User-defined firewall rule names can contain spaces, parentheses, and // other printable characters (e.g. "Microsoft Edge (mDNS-In)"). We // interpolate names into single-quoted PowerShell strings with `'` doubled, @@ -278,6 +303,25 @@ export async function scanFirewallRules( $total = $rules.Count Write-Output "TOTAL|$total" $i = 0 + # Resolving a rule's filters through the CIM association ("$r | Get-NetFirewallPortFilter") + # costs ~80 ms per call, and we need three per rule. Past ~500 inbound rules + # that alone exceeds the caller's timeout, and because $ErrorActionPreference + # is SilentlyContinue the kill produced no stderr at all — the UI showed only + # the echoed script (issue #246). Enumerate each filter class once instead and + # join on InstanceID, which is the rule's Name. -All can fail outright or come + # back partial when the process lacks rights on a policy store, so any rule + # missing from an index still falls back to its own association query. + $appIdx = @{} + $portIdx = @{} + $addrIdx = @{} + foreach ($f in @(Get-NetFirewallApplicationFilter -All)) { if ($f.InstanceID) { $appIdx[[string]$f.InstanceID] = $f } } + foreach ($f in @(Get-NetFirewallPortFilter -All)) { if ($f.InstanceID) { $portIdx[[string]$f.InstanceID] = $f } } + foreach ($f in @(Get-NetFirewallAddressFilter -All)) { if ($f.InstanceID) { $addrIdx[[string]$f.InstanceID] = $f } } + # Authenticode verification does certificate revocation lookups that can block + # on the network, and multiple rules routinely point at the same binary, so + # memoize the verdict per path. Hashtable keys are case-insensitive, which is + # what we want for Windows paths. + $sigCache = @{} # Append the directory separator so StartsWith matches on a directory boundary. # Without this, "C:\WindowsTemp\evil.exe" would match "C:\Windows" and be wrongly # treated as system-signed, suppressing the unsigned-binary finding. @@ -290,10 +334,14 @@ export async function scanFirewallRules( if ($pfx86) { $pfx86 = $pfx86.TrimEnd($sep) + $sep } foreach ($r in $rules) { $i++ + $key = if ($r.InstanceID) { [string]$r.InstanceID } else { [string]$r.Name } + $app = $appIdx[$key] + $port = $portIdx[$key] + $addr = $addrIdx[$key] try { - $app = $r | Get-NetFirewallApplicationFilter - $port = $r | Get-NetFirewallPortFilter - $addr = $r | Get-NetFirewallAddressFilter + if (-not $app) { $app = $r | Get-NetFirewallApplicationFilter } + if (-not $port) { $port = $r | Get-NetFirewallPortFilter } + if (-not $addr) { $addr = $r | Get-NetFirewallAddressFilter } } catch { continue } $programRaw = if ($app -and $app.Program) { [string]$app.Program } else { '' } @@ -325,6 +373,8 @@ export async function scanFirewallRules( if (-not $isSystemPath -and $pfx86 -and $programResolved.StartsWith($pfx86, 'OrdinalIgnoreCase')) { $isSystemPath = $true } if ($isSystemPath) { $signed = 'signed' + } elseif ($sigCache.ContainsKey($programResolved)) { + $signed = $sigCache[$programResolved] } else { try { $sig = Get-AuthenticodeSignature -LiteralPath $programResolved @@ -332,6 +382,7 @@ export async function scanFirewallRules( elseif ($sig.Status -eq 'NotSigned') { $signed = 'unsigned' } else { $signed = 'unknown' } } catch { $signed = 'unknown' } + $sigCache[$programResolved] = $signed } } } @@ -351,30 +402,49 @@ export async function scanFirewallRules( } ` - const { stdout } = await execFileAsync('powershell', psArgs(script), PS_OPTS) - const rules: FirewallRule[] = [] let total = 0 - for (const rawLine of stdout.split('\n')) { - const line = rawLine.trim() - if (!line) continue - if (line.startsWith('TOTAL|')) { - const n = parseInt(line.split('|')[1], 10) - if (!Number.isNaN(n)) total = n - continue - } - if (line.startsWith('PROG|')) { - const parts = line.split('|') - const cur = parseInt(parts[1], 10) - const tot = parseInt(parts[2], 10) - const ruleName = parts[3] ?? '' - if (!Number.isNaN(cur) && !Number.isNaN(tot)) { - onProgress?.({ phase: 'classifying', current: cur, total: tot, currentRule: ruleName }) + + // Consume stdout line by line rather than buffering it: the PROG markers the + // script emits only drive the progress bar if they arrive while the scan is + // still running, and anything already parsed survives a timeout. + const { stderr, code, timedOut } = await spawnTrackedLines( + 'powershell', + psArgs(script), + (rawLine) => { + const line = rawLine.trim() + if (!line) return + if (line.startsWith('TOTAL|')) { + const n = parseInt(line.split('|')[1], 10) + if (!Number.isNaN(n)) total = n + return } - continue + if (line.startsWith('PROG|')) { + const parts = line.split('|') + const cur = parseInt(parts[1], 10) + const tot = parseInt(parts[2], 10) + const ruleName = parts[3] ?? '' + if (!Number.isNaN(cur) && !Number.isNaN(tot)) { + onProgress?.({ phase: 'classifying', current: cur, total: tot, currentRule: ruleName }) + } + return + } + const parsed = parseRuleLine(line) + if (parsed) rules.push(parsed) + }, + { timeout: SCAN_TIMEOUT_MS } + ) + + // Only treat the run as a failure when it produced nothing usable — a scan + // that streamed rules before being cut short is reported as truncated. + if (rules.length === 0) { + if (timedOut) { + throw new Error( + `Firewall scan timed out after ${Math.round(SCAN_TIMEOUT_MS / 1000)}s without returning any rules. ` + + 'Check that the Windows Defender Firewall (MpsSvc) and Base Filtering Engine (BFE) services are running.' + ) } - const parsed = parseRuleLine(line) - if (parsed) rules.push(parsed) + if (code !== 0) throw powerShellError('Firewall scan failed', code, stderr) } const staleCount = rules.filter((r) => r.issues.includes('stale')).length @@ -387,6 +457,7 @@ export async function scanFirewallRules( staleCount, unsignedCount, broadScopeCount, + truncated: timedOut || (total > 0 && rules.length < total), } } @@ -453,11 +524,17 @@ try { } } catch (err) { failed = changes.length - succeeded - errors.push({ - name: '', - displayName: '', - reason: err instanceof Error ? err.message : 'PowerShell execution failed', - }) + // Same trap as the scan: the raw rejection message is the whole echoed + // script. Use the stderr/exit code Node attaches to it instead. + const e = err as NodeJS.ErrnoException & { stderr?: string; code?: number | string; killed?: boolean } + const reason = e?.killed + ? 'PowerShell timed out while applying firewall changes' + : powerShellError( + 'Failed to apply firewall changes', + typeof e?.code === 'number' ? e.code : null, + e?.stderr ?? '' + ).message + errors.push({ name: '', displayName: '', reason }) } return { succeeded, failed, errors } diff --git a/src/main/services/exec-utf8.test.ts b/src/main/services/exec-utf8.test.ts new file mode 100644 index 00000000..a99198da --- /dev/null +++ b/src/main/services/exec-utf8.test.ts @@ -0,0 +1,97 @@ +import { describe, it, expect } from 'vitest' +import { spawnTrackedLines } from './exec-utf8' + +// Drive a real child process — the point of spawnTrackedLines is its handling +// of stdout chunking, exit codes, timeouts and aborts, none of which a mocked +// ChildProcess would exercise faithfully. +const NODE = process.execPath + +function nodeEval(source: string): string[] { + return ['-e', source] +} + +describe('spawnTrackedLines', () => { + it('delivers one callback per stdout line, without trailing newlines', async () => { + const lines: string[] = [] + const { code, timedOut } = await spawnTrackedLines( + NODE, + nodeEval('process.stdout.write("a\\nb\\nc\\n")'), + (l) => lines.push(l), + { timeout: 30_000 } + ) + expect(lines).toEqual(['a', 'b', 'c']) + expect(code).toBe(0) + expect(timedOut).toBe(false) + }) + + it('reassembles lines split across chunk boundaries', async () => { + const lines: string[] = [] + await spawnTrackedLines( + NODE, + nodeEval( + 'process.stdout.write("RUL"); setTimeout(() => process.stdout.write("E|one\\nRULE|tw"), 20); setTimeout(() => process.stdout.write("o\\n"), 40)' + ), + (l) => lines.push(l), + { timeout: 30_000 } + ) + expect(lines).toEqual(['RULE|one', 'RULE|two']) + }) + + it('emits a final unterminated line when the process exits', async () => { + const lines: string[] = [] + await spawnTrackedLines(NODE, nodeEval('process.stdout.write("no-newline")'), (l) => lines.push(l), { + timeout: 30_000, + }) + expect(lines).toEqual(['no-newline']) + }) + + it('resolves with the exit code instead of throwing, so partial output stays usable', async () => { + const lines: string[] = [] + const { code, stderr } = await spawnTrackedLines( + NODE, + nodeEval('process.stdout.write("kept\\n"); process.stderr.write("boom\\n"); process.exit(3)'), + (l) => lines.push(l), + { timeout: 30_000 } + ) + expect(lines).toEqual(['kept']) + expect(code).toBe(3) + expect(stderr).toContain('boom') + }) + + it('reports a timeout via the flag and keeps lines emitted before the kill', async () => { + const lines: string[] = [] + const { timedOut } = await spawnTrackedLines( + NODE, + nodeEval('process.stdout.write("early\\n"); setTimeout(() => {}, 60_000)'), + (l) => lines.push(l), + { timeout: 400 } + ) + expect(lines).toEqual(['early']) + expect(timedOut).toBe(true) + }) + + it('rejects when the signal aborts mid-run', async () => { + const controller = new AbortController() + setTimeout(() => controller.abort(), 100) + await expect( + spawnTrackedLines(NODE, nodeEval('setTimeout(() => {}, 60_000)'), () => {}, { + timeout: 30_000, + signal: controller.signal, + }) + ).rejects.toThrow('Operation cancelled') + }) + + it('rejects immediately when the signal is already aborted', async () => { + await expect( + spawnTrackedLines(NODE, nodeEval('process.stdout.write("x")'), () => {}, { + signal: AbortSignal.abort(), + }) + ).rejects.toThrow('Operation cancelled') + }) + + it('rejects when the executable does not exist', async () => { + await expect( + spawnTrackedLines('kudu-no-such-binary-xyz', [], () => {}, { timeout: 5_000 }) + ).rejects.toThrow() + }) +}) diff --git a/src/main/services/exec-utf8.ts b/src/main/services/exec-utf8.ts index eb9c8f11..a3c1f076 100644 --- a/src/main/services/exec-utf8.ts +++ b/src/main/services/exec-utf8.ts @@ -10,7 +10,7 @@ * - Native tools: run via cmd /c with chcp 65001 (UTF-8 code page) */ -import { execFile, type ExecFileOptions, type ChildProcess } from 'child_process' +import { execFile, spawn, type ExecFileOptions, type ChildProcess } from 'child_process' import { promisify } from 'util' const execFileAsync = promisify(execFile) @@ -101,6 +101,82 @@ export async function execTracked( } } +/** Cap the partial-line buffer so a child that never emits \n can't exhaust memory. */ +const MAX_LINE_BUFFER = 1024 * 1024 +/** Cap retained stderr — we only ever surface the first few lines. */ +const MAX_STDERR = 64 * 1024 + +/** + * Spawn a process and hand each complete stdout line to `onLine` as it arrives. + * + * `execTracked` buffers all output and rejects on timeout, throwing away + * everything the child already produced. Use this instead for long-running + * scans that stream progress markers: the caller sees lines in real time and + * keeps whatever was emitted before a timeout, which is reported through the + * resolved `timedOut` flag rather than as an exception. The child is tracked + * like every other spawned process, so `killAllChildren()` on app exit and + * abort signals still kill the whole tree. + * + * Rejects only when the process could not be spawned or the operation was + * aborted — a non-zero exit is reported via the resolved `code` so callers can + * decide whether partial output is still usable. + */ +export function spawnTrackedLines( + file: string, + args: string[], + onLine: (line: string) => void, + opts?: { timeout?: number; signal?: AbortSignal; windowsHide?: boolean } +): Promise<{ stderr: string; code: number | null; timedOut: boolean }> { + return new Promise((resolve, reject) => { + if (opts?.signal?.aborted) { + reject(new Error('Operation cancelled')) + return + } + + const child = spawn(file, args, { windowsHide: opts?.windowsHide ?? true }) + child.stdout?.setEncoding('utf-8') + child.stderr?.setEncoding('utf-8') + + let pending = '' + let stderr = '' + let timedOut = false + let settled = false + + child.stdout?.on('data', (chunk: string) => { + pending += chunk + let nl = pending.indexOf('\n') + while (nl !== -1) { + onLine(pending.slice(0, nl)) + pending = pending.slice(nl + 1) + nl = pending.indexOf('\n') + } + if (pending.length > MAX_LINE_BUFFER) pending = '' + }) + + child.stderr?.on('data', (chunk: string) => { + if (stderr.length < MAX_STDERR) stderr += chunk + }) + + // trackChild fires onKill for both the timeout and an abort; the abort case + // is disambiguated below by checking the signal itself. + const cleanup = trackChild(child, opts?.timeout ?? 15_000, opts?.signal, () => { timedOut = true }) + + const settle = (fn: () => void): void => { + if (settled) return + settled = true + cleanup() + fn() + } + + child.on('error', (err) => settle(() => reject(err))) + child.on('close', (code) => settle(() => { + if (pending) onLine(pending) + if (opts?.signal?.aborted) reject(new Error('Operation cancelled')) + else resolve({ stderr, code, timedOut }) + })) + }) +} + /** * Wrap a spawned child process with timeout and abort-signal handling * that kills the entire process tree (not just the root) on Windows. diff --git a/src/renderer/src/pages/FirewallAuditPage.tsx b/src/renderer/src/pages/FirewallAuditPage.tsx index 13fd57db..9c3f0c00 100644 --- a/src/renderer/src/pages/FirewallAuditPage.tsx +++ b/src/renderer/src/pages/FirewallAuditPage.tsx @@ -57,6 +57,7 @@ export function FirewallAuditPage() { const applyResult = useFirewallStore((s) => s.applyResult) const error = useFirewallStore((s) => s.error) const hasScanned = useFirewallStore((s) => s.hasScanned) + const truncated = useFirewallStore((s) => s.truncated) const searchQuery = useFirewallStore((s) => s.searchQuery) const riskFilter = useFirewallStore((s) => s.riskFilter) const programFilter = useFirewallStore((s) => s.programFilter) @@ -79,11 +80,13 @@ export function FirewallAuditPage() { store.setApplyResult(null) store.setError(null) store.setScanProgress(null) + store.setTruncated(false) try { const result = await window.kudu.firewallScan() const s = useFirewallStore.getState() s.setRules(result.rules) + s.setTruncated(!!result.truncated) s.setHasScanned(true) } catch (err) { toast.error('Firewall scan failed') @@ -272,6 +275,20 @@ export function FirewallAuditPage() { /> )} + {/* A partial rule set is still worth acting on, but it must not read as a + complete audit — rules Windows never returned aren't "clean". */} + {truncated && !scanning && ( +
+ + + This scan finished early, so the list below is incomplete. Rescan to audit every rule. + +
+ )} + {scanning && scanProgress && (
void setError: (error: string | null) => void setHasScanned: (hasScanned: boolean) => void + setTruncated: (truncated: boolean) => void setSearchQuery: (query: string) => void setRiskFilter: (filter: RiskFilter) => void @@ -51,6 +54,7 @@ export const useFirewallStore = create((set) => ({ applyResult: null, error: null, hasScanned: false, + truncated: false, searchQuery: '', riskFilter: 'all', @@ -66,6 +70,7 @@ export const useFirewallStore = create((set) => ({ setApplyResult: (applyResult) => set({ applyResult }), setError: (error) => set({ error }), setHasScanned: (hasScanned) => set({ hasScanned }), + setTruncated: (truncated) => set({ truncated }), setSearchQuery: (searchQuery) => set({ searchQuery }), setRiskFilter: (riskFilter) => set({ riskFilter }), @@ -97,6 +102,7 @@ export const useFirewallStore = create((set) => ({ applyResult: null, error: null, hasScanned: false, + truncated: false, searchQuery: '', riskFilter: 'all', programFilter: 'all', diff --git a/src/shared/types.ts b/src/shared/types.ts index aca50f9a..6a704f83 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -871,6 +871,10 @@ export interface FirewallScanResult { staleCount: number unsignedCount: number broadScopeCount: number + // Set when the scan was cut short (timeout) or returned fewer rules than it + // enumerated. The findings shown are real but incomplete, so the UI must not + // present them as a full audit. + truncated?: boolean } export interface FirewallApplyResult {