hs: suppress TOCTOU stat use

To explain a bit more the TOCTOU issue found, we can consider
a case where Suricata starts to prune, yet externally somebody also
starts erasing cache files.
Right after Suricata checks the file age with the stat function,
somebody may delete or update the file of our interest.

Suricata aging decision doesn't reflect the actual state of the file.
This commit additionally adds a check for noent failure of the unlink operation
(considered as a success). The code can still delete a file that is recently
updated but was considered stale.

In the documentation-following deployments this should not happen anyway as
one cache folder should only be used by a single Suricata instance (and within
Suricata instance only one thread handles cache eviction).
Additionally, the `stat` and `unlink` command are immediatelly followed, making
this scenario extra unlikely.

Additional comment in the code explains problems of using fstat and potential
issues on Windows.

Ticket: 8243
pull/14706/head
Lukas Sismis 3 months ago committed by Lukas Sismis
parent 7627756360
commit 0fe0390a2f

@ -374,7 +374,15 @@ int SCHSCachePruneEvaluate(MpmConfig *mpm_conf, HashTable *inuse_caches)
continue;
struct stat st;
if (stat(path, &st) != 0 || !S_ISREG(st.st_mode))
/* TOCTOU: race window between stat and unlink is acceptable here.
* On Linux somebody can still modify (use the cache file) between the
* fstat and unlink, on Windows (HS not supported there but still relevant)
* TOCTOU happens when closing the file descriptor and unlinking the file.
* Cache mechanism is best-effort and e.g. not pruning or pruning an extra
* cache file is not problematic.
* Stat is used here to ease file handling as fstat doesn't bring any benefit */
/* coverity[toctou] */
if (SCStatFn(path, &st) != 0 || !S_ISREG(st.st_mode))
continue;
considered++;
@ -388,7 +396,9 @@ int SCHSCachePruneEvaluate(MpmConfig *mpm_conf, HashTable *inuse_caches)
if (cache_inuse != NULL)
continue; // in use
if (unlink(path) == 0) {
/* coverity[toctou] */
int ret = unlink(path);
if (ret == 0 || (ret == -1 && errno == ENOENT)) {
removed++;
SCLogDebug("File %s removed because of %s%s%s", path, prune_by_age ? "age" : "",
prune_by_age && prune_by_version ? " and " : "",

Loading…
Cancel
Save