false positive: Uncontrolled data used in OS command

[ktsaou]

Hi,

We just started using https://github.com/firehol/netdata in LGTM. Thank you!

We found that LGTM reports cpp/command-line-injection false positives.

Here is a screenshot:

But the code is the other way around: We use fgets() after we run the command, to read the output of the program we execute, like this:

https://lgtm.com/projects/g/firehol/netdata/snapshot/2a7cf3528a14cd50a69af4d75e1441a4b035d231/files/src/cgroup-network.c?#xb0514f82e375bcb6:1

[jbj]

Thanks for the report. It looks like the our analysis is getting confused by the reuse of buffer for two different purposes in one function. When a tainted value is assigned by reference, taint will flow from the argument to all other uses of the same variable. We're working on new taint-tracking libraries that should not be fooled by such code, but I can't say when we'll start using them in exactly that query.

As a mitigation, I recommend using two different buffers for the two different purposes. A good optimizing compiler should allocate them on top of each other because their uses don't overlap.

If you are ever concerned that someone can be tricked into running your program with a cgroup name containing a single quote, then perhaps you'll want to avoid composing a shell command in the buffer at all.

[ktsaou]

Thank you!

If you are ever concerned that someone can be tricked into running your program with a cgroup name containing a single quote, then perhaps you'll want to avoid composing a shell command in the buffer at all.

Yes, this is already taken care of: https://github.com/firehol/netdata/blob/49dea6ba3f842cd0ee4765dd1f2ecda1d294193a/src/cgroup-network.c#L512-L555

[jbj]

The internal Jira ticket for the taint-tracking library replacement is CPP-462.