Add more functions as taint propators to GenericTaintChecker.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Huh, this was a long one. 😅 🚀
clang/docs/analyzer/checkers.rst | ||
---|---|---|
2361 | I cannot see the corresponding propagation rule. vfscanf occurs two times. vscanf is not mentioned here; and there are probably a couple others like this. | |
clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp | ||
586 | This function has exactly 3 arguments. That being said, I think vscanf is more like scanf, so it should be modeled as a taint source instead of a propagator. | |
598 | I'm on board with marking read operations as props/sources. | |
600 | It's sad that we don't model recvmsg, but it would be quite difficult to model. I can see why you left that out. | |
602–603 | I'm not sure how could these be used as gadgets, but let it be. | |
605–606 | These should be sorted, isn't it? | |
607 |
From this man entry I would think that we should propagate from the second argument. | |
619 | One could say that if memmem was called with a tainted needle and actually finds something in the haystack, that would mean essentially that the value pointed by the return value has the same content as the needle. | |
629 | ||
638–639 | IMO these should propagate from {0,1}, similarly how the strncmp does propagate from its last argument. | |
clang/test/Analysis/taint-generic.c | ||
491–497 | This test case definitely needs to be reworked.
| |
511–514 | For this type of things I can recommend using the clang_analyzer_isTainted(T) debug.ExprInspection introspection function. I've seen in some other test cases that you used an extra top-level fn parameter for the same. | |
518–519 | Please use single-line comments. | |
527 | clang_analyzer_isTainted(*iov) | |
1024 | You should group these all into a single test case. | |
1046–1047 |
| |
1058–1061 | Just use the cmp_less instead. |
- remove vscanf and co.
- use debug.ExprInspection for test cases
- fix semantic issues for modeled functions
clang/docs/analyzer/checkers.rst | ||
---|---|---|
2361 | Removed vscanf and vfscanf as modeling their taint is not straightforward and should be done in another checker. | |
clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp | ||
586 | removed | |
598 | On second thought, this seems pointless indeed, removed. | |
605–606 | swapped, thx | |
clang/test/Analysis/taint-generic.c | ||
518–519 | removed | |
527 | test case removed |
Looks good to me. Check the docs if they are still in sync.
Also, postpone this for two days to let others block this if they have objections.
Other than that, land it.
I cannot see the corresponding propagation rule.
That being said, it would be handy to mention that this is for zlib decompression and this should be probably a taint source anyway.
vfscanf occurs two times.
vscanf is not mentioned here; and there are probably a couple others like this.