C++: Improvements to the cpp/toctou-race-condition query#6335
C++: Improvements to the cpp/toctou-race-condition query#6335rdmarsh2 merged 10 commits intogithub:mainfrom
Conversation
|
Here are a couple of real world toctou CVEs:
The attacks often involve the program checking some security relevant property of the file (such as the user having permission to access it), then the attacker switches that file for a symlink to the real target, then the program operates on that target using its privilege but without having checked the property on it. |
|
Hmm, there are some regressions in the old internal tests. I'll make a PR to accept the changes, but I'm beginning to think there's a second case we really want to permit as well: any check, followed by a highly sensitive operation such as |
|
Pushed changes and internal PR. New diff query: https://lgtm.com/query/6665823716934953527/ . There aren't many changes, but we get back what I believe are good results in a few places such as stellar/stellar-core. |
rdmarsh2
left a comment
There was a problem hiding this comment.
Results and code LGTM other than the one nitpick.
rdmarsh2
left a comment
There was a problem hiding this comment.
LGTM, and I'm merging now to avoid submodule merge conflicts. Do we want to bump the precision to high as a separate PR?
I'll review the results again first, but yes. |
Improvements to the
cpp/toctou-race-conditionquery. The main change is disallowing results from file operations such asfopen,renameetc from counting as the "check", so now the check must be output from astat,accessor similar call, whilst the "use" can still be a wide range of file operations. Most of the results this removes are either clear FPs or of dubious interest, so this is a big improvement to precision.I've made a couple of other improvements that widen the results a little (not as much as the above narrows them), in hopefully uncontroversial ways.
statarguments so that it actually works in most cases._wfsopen.rename(some implementations will overwrite an existing file).LGTM diff query results here: https://lgtm.com/query/4593284742579958544/
I haven't upgraded the precision of this query to
highas yet, but I'm thinking about it. The main remaining class of FP / weak result IMO is wherestatis being used to check a file exists before doing something that would perhaps fail if it didn't; orstatis checking another not-so-security-relevant property. These are still race conditions, but not necessarily security issues, so I think I'd rather not report them but I don't have a strong opinion. On the flip side, this is pretty much what the examples on https://cwe.mitre.org/data/definitions/367.html do and what the SAMATE tests do, so perhaps I'm being too strict.