fix: eliminate TOCTOU race in scan-status update - #511
Open
cschuerings wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When a file is replaced while a scan is already in flight, the scan completion
WHEREclause usedhash IS NULL OR hash = <old-hash>to find the row to stamp. Under concurrent uploads this is a TOCTOU race: the replacement upload starts a new scan that immediately setshash = NULLand begins scanning the new file. The old scan's completion query then matches the now-NULL hash and stamps its verdict — which belongs to the replaced file — onto the row for the new file.The result is that a clean new file can be marked Infected (or vice versa) purely due to ordering of DB writes.
Fix
Introduce a
scanTokenUUID column on theAttachmenttype.updateStatusgenerates a fresh token when transitioning toScanning, stores it in the row, and returns it to_scanAttachmentsFile. The scan-completionWHEREclause then matches onscanToken = <token>instead ofhash IS NULL. If a concurrent upload has already started a new scan (overwritingscanToken), the stale completion query matches zero rows and the wrong verdict is safely discarded.The
hash IS NULLdisjunct is removed entirely from the completion path.Out of Scope
Crash-recovery scenarios (attachment stuck in
Scanningafter a process crash between the Scanning and Clean writes) are a separate concern not addressed here.