[PATCH] D120369: [analyzer] Add more propagations to Taint analysis

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 added inline comments.


================
Comment at: clang/docs/analyzer/checkers.rst:2361
 Default propagations defined by ``GenericTaintChecker``:
-``atoi``, ``atol``, ``atoll``, ``fgetc``, ``fgetln``, ``fgets``, ``fscanf``, ``sscanf``, ``getc``, ``getc_unlocked``, ``getdelim``, ``getline``, ``getw``, ``pread``, ``read``, ``strchr``, ``strrchr``, ``tolower``, ``toupper``
+``atoi``, ``atol``, ``atoll``, ``basename``, ``dirname``, ``fgetc``, ``fgetln``, ``fgets``, ``fnmatch``, ``fread``, ``fscanf``, ``fscanf_s``, ``index``, ``inflate``, ``isalnum``, ``isalpha``, ``isascii``, ``isblank``, ``iscntrl``, ``isdigit``, ``isgraph``, ``islower``, ``isprint``, ``ispunct``, ``isspace``, ``isupper``, ``isxdigit``, ``memchr``, ``memrchr``, ``sscanf``, ``getc``, ``getc_unlocked``, ``getdelim``, ``getline``, ``getw``, ``memcmp``, ``memcpy``, ``memmem``, ``memmove``, ``mbtowc``, ``pread``, ``qsort``, ``qsort_r``, ``rawmemchr``, ``read``, ``readv``, ``recv``, ``recvfrom``, ``rindex``, ``strcasestr``, ``strchr``, ``strchrnul``,  ``strcasecmp``, ``strcmp``, ``strcspn``, ``strlen``, ``strncasecmp``, ``strncmp``, ``strndup``, ``strndupa``, ``strpbrk``, ``strrchr``, ``strsep``, ``strspn``, ``strstr``, ``strtol``, ``strtoll``, ``strtoul``, ``strtoull``, ``tolower``, ``toupper``, ``ttyname``, ``ttyname_r``, ``vfscanf``, ``vfscanf``, ``wctomb``, ``wcwidth``
 
----------------
steakhal wrote:
> 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.
Removed `vscanf` and `vfscanf` as modeling their taint is not straightforward and should be done in another checker.
The problem with those is that they do not use variadic arguments, but an abstraction of those implemented by the type `va_list`, which is used to support invocations, where the number of arguments is determined at runtime.
In addition to modeling individual v-variants we will also have to handle the creation of `va_list` objects, and `va_start` function calls and try to reason about which parameters will be tainted.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:561
+      {{"vscanf"}, TR::Prop({{0}}, {{}, 1})},
+      {{"vfscanf"}, TR::Prop({{0}}, {{}, 2})},
+
----------------
steakhal wrote:
> This function has exactly 3 arguments.
> I'm also puzzled how tainting `va_list` will work out. That should be modeled separately IMO.
> This comment applies to all of the similar `va_list` accepting functions, such as `vscanf`, `vfscanf`, and possibly others.
> 
> That being said, I think `vscanf` is more like `scanf`, so it should be modeled as a taint source instead of a propagator.
removed


================
Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:575
+      {{"fread"}, TR::Prop({{3}}, {{0, ReturnValueIndex}})},
+      {{"readv"}, TR::Prop({{0}}, {{1, ReturnValueIndex}})},
+      {{"recv"}, TR::Prop({{0}}, {{1, ReturnValueIndex}})},
----------------
steakhal wrote:
> I'm on board with marking read operations as props/sources.
> Let's look at the declaration: `ssize_t readv(int fd, const struct iovec *iov, int iovcnt);`
> I'm not sure how could the pointee of `iov` be modified by this call, as its `const`.
> Additionally, I doubt the effectiveness of the rule, since I don't think it would be too likely to have a path leading to a taint sink with an `iov` pointer. That being said, let it be, but don't expect much from this rule :D
On second thought, this seems pointless indeed, removed.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:582-583
+
+      {{"dirname"}, TR::Prop({{0}}, {{ReturnValueIndex}})},
+      {{"basename"}, TR::Prop({{0}}, {{ReturnValueIndex}})},
+      {{"fnmatch"}, TR::Prop({{0}}, {{ReturnValueIndex}})},
----------------
steakhal wrote:
> These should be sorted, isn't it?
swapped, thx


================
Comment at: clang/test/Analysis/taint-generic.c:399-400
+struct iovec {
+  void *iov_base; /* Starting address */
+  size_t iov_len; /* Number of bytes to transfer */
+};
----------------
steakhal wrote:
> Please use single-line comments.
> It makes debugging test files easier in some cases.
removed


================
Comment at: clang/test/Analysis/taint-generic.c:408
+  size_t read = readv(fd, iov, iovcnt);
+  // FIXME: should be able to assert that iov is also tainted
+  return 1 / read; // expected-warning {{Division by a tainted value, possibly zero}}
----------------
steakhal wrote:
> `clang_analyzer_isTainted(*iov)`
test case removed


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120369/new/

https://reviews.llvm.org/D120369