[clang] [analyzer] TaintPropagation checker strlen() should not propagate (PR #66086)

[Daniel Krupp via cfe-commits]

dkrupp wrote:

If we remove the malloc(..) as the taint sink, we would lose some true positive findings where the size of the allocated
area is specified directly as a number by the attacker:
```
char *size=getenv("SIZE");
if (size){
    pathbuf=(char*) malloc(atoi(size)+1); // warn: denial of service attack!
...
}
```
The above example is prone to denial of service attack as the the attacker just specifies an arbitrarily large number to which a buffer will be allocated. The attacker needs much less resources to specify a large number than the recevier to allocate a large chuck of memory.

On the other hand when we have a code like this:
```
char *user_txt=getenv("SIZE");
if (user_txt){ 
    pathbuf=(char*) malloc(strlen(user_txt)+1); // No Warning as the malloc parameter comes from the size of an already allocated buffer
...
}
```
Here we should not warn as the size passed to malloc is the size of an already allocated buffer. So invested resources by the attacker to provide the large string and the server allocating another buffer to contain that string is symmetrical. So not prone to DoS attack.

A more sophisticated longer term solution could be that we add a flag to the taint info (or introduce a taint type) that the tainted value was originating from an existing buffer size and then specify the malloc sink so that it should not warn in that case. I know we cannot do this know, but the taint analysis could be extended into this direction.

Back to this solution.
Please note that this is only the default configuration of the checker.
The user could add the stren as a propagator into the taint config file.
If we decide to remove strlen() as a propagator (as is in this patch) we could highlight this in the documentation of the checker that the  user may want to add it back.

So for me either solution would work:
a) remove strlen() as a propagator and note it in the checker doc
b) remove malloc() as a sink and note it in the checker doc
c) don't do anything and live with the false positives

Which one would you prefer?

https://github.com/llvm/llvm-project/pull/66086