hs: mitigate TOC-TOU stat use by converting to fstat v1 #14672
Conversation
Addressing coverity warning to open and inspect file properties, instead of just "observing" the properties from the file. Ticket: 8243
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14672 +/- ##
==========================================
- Coverage 82.11% 82.11% -0.01%
==========================================
Files 1011 1011
Lines 262812 262820 +8
==========================================
- Hits 215812 215811 -1
- Misses 47000 47009 +9
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses a Coverity warning about Time-of-Check Time-of-Use (TOC-TOU) vulnerabilities in the Hyperscan cache pruning function. The change replaces stat() with fopen() followed by fstat(), ensuring the file properties being inspected belong to the actual opened file rather than potentially observing a different file that could be swapped in between the stat call and subsequent operations.
Changes:
- Replaces
stat()withfopen()+fstat()to mitigate TOC-TOU race conditions - Refactors early exit
continuestatements to use a cleanup label pattern - Adds proper file handle cleanup in loop iterations
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (namelen < 3 || strcmp(name + namelen - 3, ".hs") != 0) { | ||
| goto loop_cleanup; | ||
| } | ||
|
|
||
| if (PathMerge(path, ARRAY_SIZE(path), mpm_conf->cache_dir_path, name) != 0) { | ||
| goto loop_cleanup; | ||
| } |
There was a problem hiding this comment.
The loop_cleanup label is reached for several non-error cases (e.g., when a file doesn't match the .hs extension on line 371-372, or when prune conditions aren't met on line 393-395). In these cases, the file pointer will be NULL, which makes the check on line 411 correct. However, there's a logic issue: when file is NULL (not opened), you should use continue to proceed to the next iteration instead of goto loop_cleanup, because there's no file handle to clean up. The current code works but is less clear - you're jumping to cleanup even when there's nothing to clean up. Consider using continue directly for these non-error early exit cases and reserve goto loop_cleanup only for cases where a file has been successfully opened.
There was a problem hiding this comment.
I wanted to unify both paths, instead of mixing up continue and the loop_cleanup label.
|
Information: QA ran without warnings. Pipeline = 29260 |
| if (namelen < 3 || strcmp(name + namelen - 3, ".hs") != 0) | ||
| continue; | ||
| if (namelen < 3 || strcmp(name + namelen - 3, ".hs") != 0) { | ||
| goto loop_cleanup; |
There was a problem hiding this comment.
Why do we goto here ?
We did not fopen yet...
There was a problem hiding this comment.
I wanted to unify both paths, instead of mixing up continue and the loop_cleanup label.
| if (stat(path, &st) != 0 || !S_ISREG(st.st_mode)) | ||
| continue; | ||
| SCStat st; | ||
| if (SCFstatFn(fileno(file), &st) != 0 || !S_ISREG(st.st_mode)) { |
There was a problem hiding this comment.
What difference between stat and SCFstatFn ?
There was a problem hiding this comment.
fstat operates on an active file descriptor which should mean that you are actually "owning" the file, whereas stat just loads the file metadata without opening it.
Also, later I've found Suricata contains OS-agnostic (Linux/Windows support), so I used the SC version. This code, at the moment, runs only on Linux, though.
|
|
||
| if (PathMerge(path, ARRAY_SIZE(path), mpm_conf->cache_dir_path, name) != 0) | ||
| continue; | ||
| file = fopen(path, "r"); |
There was a problem hiding this comment.
Why do we need to open the file ?
Can you explain more the TOCTOU in the commit message ?
There was a problem hiding this comment.
So honestly, I don't think we particularly need to open the file as only 1 instance of Suricata should operate in the cache folder, meaning there should not be more than one thread pruning the folder and therefore no TOC-TOU problem should occur. Additionally, the worst that can happen is the deletion of the cache file that was recently used by a different Suricata instance.
But yeah, it doesn't hurt to actually "own" the file and to follow the best practices.
|
Continues in #14690 |
3 participants
Addressing the Coverity warning to open and inspect file properties, instead of just "observing" the properties from the file.
Ticket: https://redmine.openinfosecfoundation.org/issues/8243