Support safe quarantine removal on FreeBSD - #1791
Conversation
The CLAM-2959 quarantine hardening requires an atomic way to prove that a pathname still identifies the source retained during scanning before removing it. FreeBSD lacks the no-replace rename primitive used by the generic POSIX capture path on supported releases, causing --move and --remove to fail with ENOTSUP. Detect funlinkat() and use it with the retained scan descriptor after securely traversing to the source parent. FreeBSD atomically checks that the basename still names the retained descriptor before unlinking it. Normalize EDEADLK to EAGAIN so replacement races follow the existing action-failure behavior. Resolve descriptor paths through F_KINFO so FreeBSD symlink submissions retain the real scanned source path. Other platform implementations and the conservative unsupported-POSIX fallback remain unchanged. Reported-by: Hiroki Imai from Ricerca Security, Inc. CLAM-2959
There was a problem hiding this comment.
🟡 Changes recommended
The new FreeBSD F_KINFO usage is not feature-guarded, which risks breaking compilation on FreeBSD toolchains that don’t provide F_KINFO/kinfo_file.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR restores secure quarantine --move/--remove behavior on FreeBSD by leveraging funlinkat() so pathname removal is atomically bound to the scanned file descriptor, eliminating TOCTOU replacement races while keeping existing behavior unchanged on other platforms.
Changes:
- Add CMake feature detection for
funlinkat()and exposeHAVE_FUNLINKATin generated config headers. - Implement FreeBSD file-descriptor-to-path resolution via
F_KINFOto support quarantine source-path handling. - Use
funlinkat(parent_fd, basename, scan_fd, 0)on FreeBSD (when available), mappingEDEADLKtoEAGAINto match existing replacement-race failure semantics.
File summaries
| File | Description |
|---|---|
| libclamav/others.h | Updates platform documentation for fd→path resolution to include FreeBSD. |
| libclamav/others_common.c | Adds FreeBSD F_KINFO-based filepath resolution for open file descriptors. |
| common/actions.c | Introduces FreeBSD funlinkat()-based safe unlink path for quarantine actions. |
| CMakeLists.txt | Detects funlinkat() availability during configuration. |
| clamav-config.h.cmake.in | Adds HAVE_FUNLINKAT to the generated config header template. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d98bafe5f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
FreeBSD F_KINFO resolves a vnode through the name cache and may return a different hard-link name from the path used to open the scanned file. Using that result for a path-based quarantine action could remove another link and leave the submitted path in place. Retain the already-resolved path supplied to path-based action sources and use descriptor path resolution only when no authoritative path is available. Guard F_KINFO use so FreeBSD releases without that command fall back cleanly instead of failing to compile. Add hard-link regression coverage for move and remove actions. CLAM-2959
There was a problem hiding this comment.
🟢 Ready to approve
The changes are well-scoped, guarded by feature detection, and include targeted regression tests that cover the restored FreeBSD-safe unlink behavior.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
On macOS, /tmp is reported as its canonical /private/tmp path. The quarantine directory replacement tests compared the complete clamscan success message against paths created from the noncanonical temporary directory. A successful copy or move could therefore be misclassified as an action failure. Match the stable copy and move action markers instead. Existing filesystem assertions continue to verify that the payload reached the pinned quarantine directory and was not redirected through a replaced path. CLAM-2959
Summary
Restore safe
--moveand--removequarantine actions on FreeBSD by usingfunlinkat()to atomically bind pathname removal to the file descriptor thatClamAV retained while scanning.
This change:
funlinkat()during CMake configuration;F_KINFO;funlinkat(parent_fd, basename, scan_fd, 0)to remove the source onlyif the pathname still identifies the scanned object; and
EDEADLKreplacement-race result toEAGAIN, matching theexisting quarantine action-failure behavior.
Linux, macOS, Windows, and the conservative fallback for other POSIX systems
are unchanged.
Problem
The CLAM-2959 quarantine hardening prevents
--moveand--removefrom actingon a different filesystem object if an attacker replaces the submitted path
after it is scanned. The generic POSIX implementation captures the source in a
private directory and requires an atomic no-replace rename to restore it after
a failed action.
Supported FreeBSD releases do not provide the no-replace primitive used by
that path, so the safe fallback intentionally returns
ENOTSUP. Consequently,--removeleaves the infected source in place. The--movehard-link pathremoves its quarantine link again, while the copy fallback can leave a copied
quarantine file but still reports failure and leaves the source in place.
--copyis not affected by this unlink limitation.FreeBSD 13 and newer provide
funlinkat(). The kernel atomically verifies thatthe basename still names the supplied open descriptor before unlinking it,
which directly provides the object-binding guarantee required by CLAM-2959
without weakening replacement-race handling.
Affected Versions
This FreeBSD regression is present in ClamAV 1.4.5, ClamAV 1.5.3, and the
1.6.0 development branch before this change. These versions contain the
CLAM-2959 quarantine hardening but lack a FreeBSD-safe unlink implementation.
Earlier versions do not have this specific regression because they predate
that hardening.
Background and the original FreeBSD analysis are recorded in
Cisco-Talos/clamav#1755.
Resulting Behavior
--moveand--removeremove an unchanged FreeBSD source successfully.quarantine action failure.
--copybehavior is unchanged.Validation
Local macOS:
cmake --build build --target clamscan -j12 ctest -V -R '^(libclamav|clamscan)$' git diff --checkFreeBSD 13.3 and 14.3 jails:
HAVE_FUNLINKAT=1;extended-attribute test skipped; and