Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 118 additions & 5 deletions src/main/ipc/malware-scanner.ipc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@ import { ipcMain, app, BrowserWindow } from 'electron'
import { execFile } from 'child_process'
import { promisify } from 'util'
import { readFile, readdir, writeFile, stat, mkdir, rename, rm } from 'fs/promises'
import { createReadStream } from 'fs'
import { join, basename, extname, resolve, normalize } from 'path'
import { randomUUID } from 'crypto'
import { randomUUID, createHash } from 'crypto'
import { IPC } from '../../shared/channels'
import type {
MalwareThreat,
Expand All @@ -12,13 +13,14 @@ import type {
MalwareActionResult,
QuarantinedItem,
QuarantineMeta,
MalwareAllowlistEntry,
YaraRulesInfo
} from '../../shared/types'
import type { WindowGetter } from './index'
import { getPlatform } from '../platform'
import { validateStringArray } from '../services/ipc-validation'
import { psUtf8, execNativeUtf8 } from '../services/exec-utf8'
import { getSettings } from '../services/settings-store'
import { getSettings, getMalwareAllowlist, addMalwareAllowlistEntry, removeMalwareAllowlistEntry } from '../services/settings-store'
import { isExcluded } from '../services/file-utils'
import { createYaraEngine, yaraMatchToThreatFields, type YaraEngine } from '../services/yara-engine'
import { getAllRulePaths, getCachedRulePaths, getRulesMetadata, startPeriodicRuleChecks, fetchAndCacheRules, RULES_ENDPOINT } from '../services/yara-rules-store'
Expand Down Expand Up @@ -1026,6 +1028,66 @@ function isPathInAllowedDirs(filePath: string, allowedDirs: string[]): boolean {
})
}

/** Stream a file's full SHA-256, used to match the false-positive allowlist. */
function hashFileSha256(filePath: string): Promise<string> {
return new Promise((resolve, reject) => {
const hash = createHash('sha256')
const stream = createReadStream(filePath, { highWaterMark: 65536 })
stream.on('data', (chunk) => hash.update(chunk))
stream.on('end', () => resolve(hash.digest('hex')))
stream.on('error', reject)
})
}

// Detection name for hidden Alternate Data Streams. Its `path` is the *base*
// file, so hashing that path would hash the file's main stream — not the ADS
// payload that was actually flagged. Such detections can't be allowlisted by
// content hash, so they're excluded from suppression (see #193 review).
const ADS_DETECTION_NAME = 'Heuristic.HiddenADS'

/** Whether a detection can be matched against the allowlist by file content
* hash. False for ADS detections (path points at the base file, not the
* stream) — those are never suppressed via the allowlist. */
function isHashAllowlistable(threat: MalwareThreat): boolean {
return threat.detectionName !== ADS_DETECTION_NAME
}

/**
* Drop any detection whose file content hash is on the user's false-positive
* allowlist. Only real files are hashed (non-file detections — registry, hosts,
* ADS — can't be allowlisted and are left untouched). Hashing failures are
* swallowed so an unreadable path never aborts a scan. Returns the surviving
* threats plus the count suppressed, so the caller can report it (no silent caps).
*/
async function filterAllowlistedThreats(threats: MalwareThreat[]): Promise<{ kept: MalwareThreat[]; suppressed: number }> {
const allowed = new Set(getMalwareAllowlist().map((e) => e.sha256))
if (allowed.size === 0) return { kept: threats, suppressed: 0 }
const kept: MalwareThreat[] = []
let suppressed = 0
for (const threat of threats) {
let isFile = false
if (isHashAllowlistable(threat)) {
try {
isFile = (await stat(threat.path)).isFile()
} catch {
isFile = false
}
}
if (isFile) {
try {
if (allowed.has(await hashFileSha256(threat.path))) {
suppressed++
continue
Comment on lines +1078 to +1080
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Do not allowlist ADS detections by base-file hash

On Windows, ADS detections are created with path: ads.path for the base file (Heuristic.HiddenADS), so stat(threat.path).isFile() is true and this hashes only the unnamed file stream. If a user has allowlisted that base file (or clicks “Not a virus” on an ADS row), future hidden alternate streams on the same file are suppressed even though the stream contents were never hashed or trusted. Exclude ADS detections from hash suppression or include the stream identity/content in the allowlist key.

Useful? React with 👍 / 👎.

}
} catch {
// Unreadable file — keep the detection rather than silently dropping it.
}
}
kept.push(threat)
}
return { kept, suppressed }
}

// Only scan files with these extensions for performance
function getScannableExtensions(): Set<string> {
const platformExts = getPlatform().malware.scannableExtensions()
Expand Down Expand Up @@ -1496,7 +1558,7 @@ export async function scanMalware(
path: ads.path,
fileName: basename(ads.path),
size: fileStat2?.size || 0,
detectionName: 'Heuristic.HiddenADS',
detectionName: ADS_DETECTION_NAME,
severity: 'high',
source: 'heuristic',
details: `Hidden Alternate Data Stream "${ads.stream}" — may conceal malware payload`,
Expand Down Expand Up @@ -1675,7 +1737,14 @@ export async function scanMalware(
updateCategory('defender', { status: 'skipped' })
}

completedSteps.push(`Scan complete — ${threats.length} total threat${threats.length !== 1 ? 's' : ''} detected`)
// Suppress detections the user has marked as false positives (matched by
// file content hash) before reporting results.
const { kept: visibleThreats, suppressed } = await filterAllowlistedThreats(threats)
if (suppressed > 0) {
completedSteps.push(`Ignored ${suppressed} detection${suppressed !== 1 ? 's' : ''} you previously marked safe`)
}

completedSteps.push(`Scan complete — ${visibleThreats.length} total threat${visibleThreats.length !== 1 ? 's' : ''} detected`)
sendProgress({
step: 'complete',
stepLabel: 'Scan complete',
Expand All @@ -1686,7 +1755,7 @@ export async function scanMalware(
})

return {
threats,
threats: visibleThreats,
filesScanned,
duration: Date.now() - startTime,
engines
Expand Down Expand Up @@ -1841,6 +1910,37 @@ export async function restoreMalware(quarantinedPath: string, originalPath: stri
}
}

/**
* Mark a file as a false positive: hash its current contents and add it to the
* allowlist so future scans suppress it. Returns the created entry, or null if
* the path is outside the allowed scan dirs, isn't a readable file, or hashing
* fails. `meta.path` is ignored — the explicit `path` argument is authoritative.
*/
export async function ignoreMalware(path: string, meta?: QuarantineMeta): Promise<MalwareAllowlistEntry | null> {
if (typeof path !== 'string' || !isPathInAllowedDirs(path, getScanPaths())) return null
// ADS detections key off the base file, whose hash doesn't represent the
// flagged stream — never allowlist them by content hash (see #193 review).
if (meta?.detectionName === ADS_DETECTION_NAME) return null
try {
if (!(await stat(path)).isFile()) return null
const sha256 = await hashFileSha256(path)
const detectionName = typeof meta?.detectionName === 'string' && meta.detectionName.length > 0
? meta.detectionName.slice(0, 500)
: undefined
const entry: MalwareAllowlistEntry = {
sha256,
path,
fileName: basename(path),
detectionName,
addedAt: Date.now()
}
await addMalwareAllowlistEntry(entry)
return entry
} catch {
return null
}
}

export function registerMalwareScannerIpc(getWindow: WindowGetter): void {
ipcMain.handle(IPC.MALWARE_SCAN, () => scanMalware((data) => {
const win = getWindow()
Expand Down Expand Up @@ -1868,6 +1968,19 @@ export function registerMalwareScannerIpc(getWindow: WindowGetter): void {

ipcMain.handle(IPC.MALWARE_QUARANTINE_LIST, () => listQuarantinedFiles())

ipcMain.handle(IPC.MALWARE_IGNORE, async (_event, path: string, meta?: QuarantineMeta) => {
const validMeta = meta && typeof meta === 'object' && typeof meta.path === 'string' ? meta : undefined
return ignoreMalware(path, validMeta)
})

ipcMain.handle(IPC.MALWARE_ALLOWLIST_LIST, () => getMalwareAllowlist())

ipcMain.handle(IPC.MALWARE_ALLOWLIST_REMOVE, async (_event, sha256: string) => {
if (typeof sha256 !== 'string' || !sha256) return false
await removeMalwareAllowlistEntry(sha256)
return true
})

ipcMain.handle(IPC.MALWARE_YARA_INFO, () => getYaraRulesInfo())

ipcMain.handle(IPC.MALWARE_YARA_UPDATE, async () => {
Expand Down
80 changes: 80 additions & 0 deletions src/main/ipc/malware-scanner.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -317,3 +317,83 @@ describe('PE heuristic scoring', () => {
})).toBe(false)
})
})

// ─── Allowlist suppression decision (replica) ────────────────────
// Mirrors filterAllowlistedThreats: a detection is suppressed only when it is
// hash-allowlistable (not ADS), a real file, and its content hash is on the
// allowlist. Non-files, ADS, and unreadable files are kept (never silently
// dropped).

const ADS_DETECTION_NAME = 'Heuristic.HiddenADS'

function isSuppressed(opts: { detectionName?: string; isFile: boolean; hash: string | null; allowed: Set<string> }): boolean {
if (opts.detectionName === ADS_DETECTION_NAME) return false // ADS keys off the base file, not the stream
if (!opts.isFile) return false // registry/hosts targets can't be allowlisted
if (opts.hash === null) return false // unreadable file — keep the detection
return opts.allowed.has(opts.hash)
}

describe('allowlist suppression decision', () => {
const allowed = new Set(['abc123'])

it('suppresses a file whose hash is allowlisted', () => {
expect(isSuppressed({ isFile: true, hash: 'abc123', allowed })).toBe(true)
})

it('keeps a file whose hash is not allowlisted', () => {
expect(isSuppressed({ isFile: true, hash: 'def456', allowed })).toBe(false)
})

it('keeps a non-file detection even if its path string somehow matches', () => {
expect(isSuppressed({ isFile: false, hash: 'abc123', allowed })).toBe(false)
})

it('keeps an unreadable file rather than dropping it', () => {
expect(isSuppressed({ isFile: true, hash: null, allowed })).toBe(false)
})

it('keeps an ADS detection even if the base file hash is allowlisted (#193)', () => {
expect(isSuppressed({ detectionName: ADS_DETECTION_NAME, isFile: true, hash: 'abc123', allowed })).toBe(false)
})

it('suppresses nothing when the allowlist is empty', () => {
expect(isSuppressed({ isFile: true, hash: 'abc123', allowed: new Set() })).toBe(false)
})
})

// ─── canAllowlistThreat (replica of MalwareScannerPage) ──────────
// The "Not a virus" action only appears for real-filesystem-path detections
// that aren't ADS.

function canAllowlistThreat(threat: { path: string; detectionName: string }): boolean {
if (threat.detectionName === ADS_DETECTION_NAME) return false
return /^([a-zA-Z]:[\\/]|\\\\|\/)/.test(threat.path)
}

describe('canAllowlistThreat', () => {
const det = 'Heuristic.Suspicious.PE'

it('accepts a Windows drive path', () => {
expect(canAllowlistThreat({ path: 'C:\\Games\\Guild Wars\\Gw.exe', detectionName: det })).toBe(true)
})

it('accepts a UNC path', () => {
expect(canAllowlistThreat({ path: '\\\\server\\share\\app.exe', detectionName: det })).toBe(true)
})

it('accepts a POSIX path', () => {
expect(canAllowlistThreat({ path: '/usr/local/bin/app', detectionName: det })).toBe(true)
})

it('rejects a registry key target', () => {
expect(canAllowlistThreat({ path: 'HKLM\\Software\\Microsoft\\Windows\\CurrentVersion\\Run', detectionName: det })).toBe(false)
})

it('rejects a scheduled-task path (single leading backslash, not UNC)', () => {
expect(canAllowlistThreat({ path: '\\Microsoft\\Windows\\SuspiciousTask', detectionName: det })).toBe(false)
})

it('rejects an ADS detection even with a valid file path (#193)', () => {
expect(canAllowlistThreat({ path: 'C:\\Users\\me\\notes.txt', detectionName: ADS_DETECTION_NAME })).toBe(false)
})
})
57 changes: 56 additions & 1 deletion src/main/services/settings-store.persistence.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ vi.mock('electron', () => ({
},
}))

import { setSettings, getSettings, flushSettings, updateRegistryIgnoredTweaks } from './settings-store'
import { setSettings, getSettings, flushSettings, updateRegistryIgnoredTweaks, getMalwareAllowlist, addMalwareAllowlistEntry, removeMalwareAllowlistEntry } from './settings-store'
import type { MalwareAllowlistEntry } from '../../shared/types'

describe('settings persistence — game mode toggle round-trip (issue #172)', () => {
afterAll(() => {
Expand Down Expand Up @@ -91,3 +92,57 @@ describe('settings persistence — game mode toggle round-trip (issue #172)', ()
expect(getSettings().registryIgnoredTweaks).toEqual([b])
})
})

describe('malware allowlist (false positives)', () => {
afterAll(() => {
if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true, force: true })
})

const entry = (sha256: string, over: Partial<MalwareAllowlistEntry> = {}): MalwareAllowlistEntry => ({
sha256,
path: `C:/Games/${sha256}.exe`,
fileName: `${sha256}.exe`,
detectionName: 'Heuristic.Suspicious.PE',
addedAt: 1,
...over,
})

it('defaults the allowlist to an empty array', () => {
expect(getMalwareAllowlist()).toEqual([])
})

it('adds an entry that survives a simulated restart', async () => {
addMalwareAllowlistEntry(entry('aaa'))
await flushSettings()
const list = getMalwareAllowlist()
expect(list).toHaveLength(1)
expect(list[0].sha256).toBe('aaa')
})

it('de-dupes by content hash, refreshing the existing entry', async () => {
addMalwareAllowlistEntry(entry('aaa', { path: 'C:/Moved/Gw.exe', fileName: 'Gw.exe' }))
await flushSettings()
const list = getMalwareAllowlist()
expect(list.filter((e) => e.sha256 === 'aaa')).toHaveLength(1)
expect(list.find((e) => e.sha256 === 'aaa')?.fileName).toBe('Gw.exe')
})

it('removes an entry by hash', async () => {
addMalwareAllowlistEntry(entry('bbb'))
await flushSettings()
expect(getMalwareAllowlist().some((e) => e.sha256 === 'bbb')).toBe(true)

removeMalwareAllowlistEntry('bbb')
await flushSettings()
expect(getMalwareAllowlist().some((e) => e.sha256 === 'bbb')).toBe(false)
})

it('merges concurrent adds without dropping earlier entries', async () => {
addMalwareAllowlistEntry(entry('c1'))
addMalwareAllowlistEntry(entry('c2'))
await flushSettings()
const hashes = getMalwareAllowlist().map((e) => e.sha256)
expect(hashes).toContain('c1')
expect(hashes).toContain('c2')
})
})
Loading
Loading