C++: Tests for cpp/toctou-race-condition #6150
Conversation
| { | ||
| if (!rename(path1, path2)) | ||
| { | ||
| remove(path1); // BAD |
There was a problem hiding this comment.
Do we actually want a result here? My understanding is that this pattern is a race, but not one that's exploitable for a privilege escalation - to put something at path1, it would either be freshly created or moved. In the latter case, the attacker would need to have permission to delete it
There was a problem hiding this comment.
Yes, I can't think of a way this could be exploited either, so it should probably not be within the scope of the query...
There was a problem hiding this comment.
I wonder if the only cases we're [strongly] interested in are where the first operation is stat or access (or one of the variants of those). i.e. not fopen, rename, chmod or any of the modifying operations we look for as the second operation.
It appears this approach is sufficient to catch all of the SAMATE cases at any rate. It would mean we don't get the original test cases for this query in test.cpp though (which are also rename -> remove variants), and the fopen ->chmod cases in test2.cpp.
| // ... | ||
| } | ||
|
|
||
| remove(path1); // GOOD (does not depend on the rename) |
There was a problem hiding this comment.
I don't think this is meaningfully different in terms of security than the previous two tests.
There was a problem hiding this comment.
I'm inclined to agree, apart from my reservations (see the chmod case) about whether we're looking at two operations that are intended to be associated or logically independent. I'm not sure how much merit my argument there really has.
| fclose(f); | ||
| } | ||
|
|
||
| chmod(path, 0); // GOOD (doesn't depend on the fopen) |
There was a problem hiding this comment.
This doesn't technically fall under CWE-367, since there isn't a control flow dependency on the call to fopen, but it is still a race condition with similar consequences. Do we have an alternate query that should handle these?
There was a problem hiding this comment.
In the case above I can imagine two different scenarios.
(1) the chmod is closely associated with the fopen, so as you say there's a race condition if another process accesses / modifies the file between the fopen and chmod. In addition there's likely a second bug in that if the fopen fails calling chmod on whatever happens to be there may be wrong.
(2) the fopen and chmod are in fact independent operations that just happen to be done sequentially in this function. The fact that chmod happens regardless of the result of fopen is (weak) evidence that the two are intended to be independent.
I'm not aware of an alternate query, and I would imagine it would end up with a high FP rate.
|
A comment from Slack that I should address: "The pattern that I have seen in the wild is access followed by open. I see that you already have a test for access followed by fopen. Can you add the Linux file descriptor version too?" |
…the base case of.
|
I've added a few more test cases and cleaned things up a bit. The ones marked |
|
I talked to @kevinbackhouse a few weeks ago, for the benefit of this thread he said:
If anyone has any ideas what we might do with that second point, please speak up. I do think that even an unprivileged process should ideally not be laced with race conditions on file names, but that issue is much less severe. |
|
To move things forward, I've also now updated the test case comments to reflect my best understanding now. There are now three classifications - I'm looking to get these changes merged soon so that I can get to work on the query itself. |
|
[the comment this was replying to has been deleted] Hi @bad-sarbot, would you like to try and explain your comment a bit better (or should I conclude that you're not human?) |
|
It also looks like there's an autoformat issue. I'm happy to merge once that's addressed. |
|
Fixes done. |
Add test cases to the
cpp/toctou-race-conditionquery, some of which are inspired by real-world code. Clarify some parts of the query (without changing behaviour).I have some ideas for minor improvements to the QL in this query, but the big question is which pairs of (check, use) should be flagged in the first place. At present the 'check' can be any operation understood by the query as long as it returns something, whereas the 'use' must be an operation that might change the file (except currently
fopen(..., "r")is included even though it couldn't modify the file). Some pointers for discussion:fopens, which I'm fairly sure is a false positive (i.e. safe). But what if one or bothfopenwas for writing, is that dangerous?