From 8227a8e31d37cf673d4c9de2065f85b884965cdd Mon Sep 17 00:00:00 2001 From: Christian Schuerings Date: Wed, 29 Jul 2026 14:48:04 +0200 Subject: [PATCH] fix: eliminate TOCTOU race in scan-status update via per-scan token --- CHANGELOG.md | 6 + db/index.cds | 1 + srv/malware-scanner/malwareScanner-mocked.js | 7 + srv/malware-scanner/malwareScanner.js | 114 ++++++----- tests/unit/toctou-scan-status.test.js | 191 +++++++++++++++++++ 5 files changed, 267 insertions(+), 52 deletions(-) create mode 100644 tests/unit/toctou-scan-status.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index aa2786a55..d50bb5d00 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file. This project adheres to [Semantic Versioning](http://semver.org/). The format is based on [Keep a Changelog](http://keepachangelog.com/). +## [Unreleased] + +### Fixed + +- TOCTOU race in scan-status update: a per-scan UUID token (`scanToken`) is now generated at scan-start and required in the completion `WHERE` clause, replacing the `hash IS NULL` disjunct that could match a concurrently-started scan and stamp the wrong verdict onto a replaced file. A new `scanToken` column has been added to the `Attachment` type. + ## Version 4.0.0 - 2026-08-03 **BREAKING CHANGE: The attachments plugin comes now without hyperscaler dependencies, please make sure to install them accordingly!** diff --git a/db/index.cds b/db/index.cds index 3afa9cea4..0fee6b57e 100644 --- a/db/index.cds +++ b/db/index.cds @@ -19,6 +19,7 @@ context sap.attachments { hash : String @UI.Hidden @Core.Computed; status : String default 'Unscanned' @title: '{i18n>ScanStatus}' @readonly; lastScan : Timestamp @title: '{i18n>LastScan}' @Core.Computed @readonly; + scanToken : UUID @UI.Hidden @Core.Computed; } aspect MediaData : Attachment { diff --git a/srv/malware-scanner/malwareScanner-mocked.js b/srv/malware-scanner/malwareScanner-mocked.js index ab7c58905..70943d403 100644 --- a/srv/malware-scanner/malwareScanner-mocked.js +++ b/srv/malware-scanner/malwareScanner-mocked.js @@ -10,6 +10,13 @@ class MockedMalwareScanner extends require("./malwareScanner") { async scanFile(req) { const { file } = req.data + // Widen the race window in tests. Set SCAN_DELAY_MS to a positive number of + // milliseconds to make the mock scanner pause before returning its verdict. + // This makes concurrent-upload TOCTOU scenarios reproducible without timing luck. + if (process.env.SCAN_DELAY_MS) { + await new Promise((r) => setTimeout(r, Number(process.env.SCAN_DELAY_MS))) + } + LOG.info(`Setting scan status to Clean (development mode)!`) let fileSize = 0 diff --git a/srv/malware-scanner/malwareScanner.js b/srv/malware-scanner/malwareScanner.js index da21cbb2d..371ee9066 100644 --- a/srv/malware-scanner/malwareScanner.js +++ b/srv/malware-scanner/malwareScanner.js @@ -74,7 +74,7 @@ class MalwareScanner extends cds.ApplicationService { return } - await this.updateStatus( + const scanToken = await this.updateStatus( _target, keys, "Scanning", @@ -122,48 +122,26 @@ class MalwareScanner extends cds.ApplicationService { LOG.debug(`Malware scan completed for ${target}, ${keys} - file is clean`) } - // Assign hash as another condition to ensure the correct file is marked as fine + // Use the per-scan token as the WHERE condition so that only the scan whose + // token is still in the DB row wins. If a concurrent upload has already started + // a new scan (overwriting scanToken), this WHERE silently matches zero rows and + // the stale verdict is safely discarded — eliminating the TOCTOU race. + const tokenField = prefix ? `${prefix}_scanToken` : "scanToken" await this.updateStatus( _target, - prefix - ? [ - "(", - { ref: [`${prefix}_hash`] }, - "=", - { val: hash }, - "or", - { ref: [`${prefix}_hash`] }, - "is", - "null", - ")", - "and", - { - xpr: Object.keys(keys).reduce((acc, key) => { - if (acc.length) acc.push("and") - acc.push({ ref: [key] }, "=", { val: keys[key] }) - return acc - }, []), - }, - ] - : [ - "(", - { ref: ["hash"] }, - "=", - { val: hash }, - "or", - { ref: ["hash"] }, - "is", - "null", - ")", - "and", - { - xpr: Object.keys(keys).reduce((acc, key) => { - if (acc.length) acc.push("and") - acc.push({ ref: [key] }, "=", { val: keys[key] }) - return acc - }, []), - }, - ], + [ + { ref: [tokenField] }, + "=", + { val: scanToken }, + "and", + { + xpr: Object.keys(keys).reduce((acc, key) => { + if (acc.length) acc.push("and") + acc.push({ ref: [key] }, "=", { val: keys[key] }) + return acc + }, []), + }, + ], status, hash, ...(prefix ? [prefix] : []), @@ -250,29 +228,61 @@ class MalwareScanner extends cds.ApplicationService { /** * Updates the scan status, lastScan timestamp, and optionally the hash on an attachment entity. - * When status is "Scanning", clears the hash to ensure the subsequent clean/infected update matches. * For draft-enabled entities, updates both the active and draft tables. + * + * When `status` is "Scanning": + * - Generates a fresh UUID token, stores it in `scanToken` (or `_scanToken`), and + * returns it to the caller so it can be used as the WHERE condition on scan completion. + * - Clears `hash` for inline/prefix attachments (preserves existing behaviour). + * - Does NOT set `lastScan` (scan has not completed yet). + * - Returns the generated token string. + * + * When `status` is "Clean", "Infected", or "Failed": + * - Sets `lastScan` to now and, when `hash` is provided, writes it. + * - Returns `undefined`. + * + * NOTE: The rescan path (generic-handlers.js) discards this token — it fires a fresh + * ScanAttachmentsFile event immediately, which overwrites scanToken before completion runs. + * * @param {import('@sap/cds').entity} target - The entity definition to update * @param {object|Array} where - WHERE condition: plain keys object or CQL expression array * @param {string} status - The new scan status ("Scanning", "Clean", "Infected", "Failed") * @param {string} [hash] - The SHA-256 hash of the scanned file; omitted when setting to "Scanning" * @param {string} [prefix] - Field prefix for inline attachments (e.g. "myAttachment") + * @returns {Promise} The generated scanToken when status is "Scanning"; otherwise undefined */ async updateStatus(target, where, status, hash, prefix) { const statusField = prefix ? `${prefix}_status` : "status" const lastScanField = prefix ? `${prefix}_lastScan` : "lastScan" const hashField = prefix ? `${prefix}_hash` : "hash" - - const updateObject = { [statusField]: status } - if (status !== "Scanning") { - updateObject[lastScanField] = new Date() - } else if (prefix) { - // Clear hash for inline attachments so the subsequent clean/infected update - // always matches via IS NULL when the hash changes between scans (e.g. re-upload or re-scan) - updateObject[hashField] = null + const tokenField = prefix ? `${prefix}_scanToken` : "scanToken" + + if (status === "Scanning") { + const scanToken = cds.utils.uuid() + const updateObject = { + [statusField]: "Scanning", + [tokenField]: scanToken, + ...(prefix ? { [hashField]: null } : {}), + } + if (target.drafts) { + await Promise.all([ + UPDATE.entity(target).where(where).set(updateObject), + UPDATE.entity(target.drafts).where(where).set(updateObject), + ]) + } else { + await UPDATE.entity(target).where(where).set(updateObject) + } + LOG.info( + `Updated scan status to Scanning for ${target.name}, ${JSON.stringify(where)}`, + ) + return scanToken } - if (hash) { - updateObject[hashField] = hash + + // Non-Scanning status update (Clean / Infected / Failed) + const updateObject = { + [statusField]: status, + [lastScanField]: new Date(), + ...(hash ? { [hashField]: hash } : {}), } if (target.drafts) { await Promise.all([ diff --git a/tests/unit/toctou-scan-status.test.js b/tests/unit/toctou-scan-status.test.js new file mode 100644 index 000000000..7d8370b36 --- /dev/null +++ b/tests/unit/toctou-scan-status.test.js @@ -0,0 +1,191 @@ +"use strict" +require("../../lib/csn-runtime-extension") +const cds = require("@sap/cds") +const crypto = require("crypto") +const path = require("path") +const app = path.resolve(__dirname, "../incidents-app") +cds.test(app) + +const MalwareScanner = require("../../srv/malware-scanner/malwareScanner") + +// --------------------------------------------------------------------------- +// Unit-level TOCTOU test: exercises updateStatus + _scanAttachmentsFile +// directly against the real SQLite DB, bypassing the HTTP layer. +// --------------------------------------------------------------------------- + +let scanner + +beforeEach(() => { + jest.clearAllMocks() + cds.env.requires.attachments = { scan: true } + cds.env.requires.malwareScanner = { + credentials: { uri: "host", certificate: "C", key: "K" }, + } + jest.spyOn(cds, "context", "get").mockReturnValue({ model: cds.model }) + scanner = new MalwareScanner() + scanner.retryConfig = { + enabled: false, + maxAttempts: 1, + initialDelay: 0, + maxDelay: 0, + } +}) + +describe("TOCTOU race in scan-status update", () => { + const target = "ProcessorService.Incidents.attachments" + + function hashOf(buf) { + return crypto.createHash("sha256").update(buf).digest("hex") + } + + it("baseline: updateStatus(Scanning) then updateStatus(Clean) leaves correct hash", async () => { + const _target = cds.model.definitions[target] + const keys = { up__ID: cds.utils.uuid(), ID: cds.utils.uuid() } + await INSERT.into(_target).entries({ + ...keys, + status: "Unscanned", + filename: "baseline.txt", + }) + + const hashA = hashOf(Buffer.from("FILE-A")) + + const scanToken = await scanner.updateStatus(_target, keys, "Scanning") + expect(typeof scanToken).toBe("string") + expect(scanToken).toHaveLength(36) + + // Verify row shows Scanning and has scanToken set + const scanning = await SELECT.one.from(_target).where(keys) + expect(scanning.status).toBe("Scanning") + expect(scanning.scanToken).toBe(scanToken) + + await scanner.updateStatus(_target, keys, "Clean", hashA) + + const row = await SELECT.one.from(_target).where(keys) + expect(row.status).toBe("Clean") + expect(row.hash).toBe(hashA) + }) + + it("TOCTOU race: stale scan-1 verdict must not overwrite scan-2 result", async () => { + const _target = cds.model.definitions[target] + const keys = { up__ID: cds.utils.uuid(), ID: cds.utils.uuid() } + await INSERT.into(_target).entries({ + ...keys, + status: "Unscanned", + filename: "race.txt", + }) + + const hashA = hashOf(Buffer.from("FILE-A-CONTENT")) + const hashB = hashOf(Buffer.from("FILE-B-CONTENT")) + + // scan-1 starts: sets scanToken-1 + const tokenScan1 = await scanner.updateStatus(_target, keys, "Scanning") + + // scan-2 starts: overwrites scanToken with tokenScan2 + const tokenScan2 = await scanner.updateStatus(_target, keys, "Scanning") + + expect(tokenScan1).not.toBe(tokenScan2) + + // scan-2 completes first with tokenField WHERE + const tokenField = "scanToken" + await scanner.updateStatus( + _target, + [ + { ref: [tokenField] }, + "=", + { val: tokenScan2 }, + "and", + { + xpr: Object.keys(keys).reduce((acc, key) => { + if (acc.length) acc.push("and") + acc.push({ ref: [key] }, "=", { val: keys[key] }) + return acc + }, []), + }, + ], + "Clean", + hashB, + ) + + const afterScan2 = await SELECT.one.from(_target).where(keys) + expect(afterScan2.status).toBe("Clean") + expect(afterScan2.hash).toBe(hashB) + + // Now scan-1 completes with its (now-stale) token — must NOT overwrite + await scanner.updateStatus( + _target, + [ + { ref: [tokenField] }, + "=", + { val: tokenScan1 }, + "and", + { + xpr: Object.keys(keys).reduce((acc, key) => { + if (acc.length) acc.push("and") + acc.push({ ref: [key] }, "=", { val: keys[key] }) + return acc + }, []), + }, + ], + "Clean", + hashA, + ) + + const afterScan1LateArrival = await SELECT.one.from(_target).where(keys) + // Bug (IS NULL): would have matched, overwriting hashB with hashA + // Fix (token): tokenScan1 no longer matches → row unchanged + expect(afterScan1LateArrival.hash).toBe(hashB) + expect(afterScan1LateArrival.status).toBe("Clean") + }) + + it("_scanAttachmentsFile: scan completion uses token so stale scan cannot overwrite", async () => { + const _target = cds.model.definitions[target] + const keys = { up__ID: cds.utils.uuid(), ID: cds.utils.uuid() } + await INSERT.into(_target).entries({ + ...keys, + status: "Unscanned", + filename: "token-test.txt", + }) + + const expectedHash = hashOf(Buffer.from("CONTENT-FOR-TOKEN-TEST")) + + // Simulate the attachment service getting the file + const { Readable } = require("stream") + const attachmentsSvc = { + get: jest.fn().mockResolvedValue(Readable.from([])), + emit: jest.fn().mockResolvedValue(undefined), + } + cds.connect.to = jest.fn().mockResolvedValue(attachmentsSvc) + + // Mock scan (called as this.scan(stream) by _scanWithRetry) to return a fixed hash + scanner.scan = jest + .fn() + .mockResolvedValue({ isMalware: false, hash: expectedHash }) + + // Intercept updateStatus to capture the token returned from the Scanning call + let capturedToken = null + const originalUpdateStatus = scanner.updateStatus.bind(scanner) + let callCount = 0 + scanner.updateStatus = jest.fn(async (...args) => { + callCount++ + const result = await originalUpdateStatus(...args) + if (callCount === 1) { + // First call = "Scanning": capture the returned token + capturedToken = result + } + return result + }) + + await scanner._scanAttachmentsFile({ data: { target, keys } }) + + const row = await SELECT.one.from(_target).where(keys) + expect(row.status).toBe("Clean") + expect(row.hash).toBe(expectedHash) + + // The Scanning call must have returned a token + expect(typeof capturedToken).toBe("string") + expect(capturedToken).toHaveLength(36) + + // Token is not cleared on completion — the DB retains which scan last wrote the verdict. + expect(row.scanToken).toBeTruthy() + }) +})