Flawfinder static analysis

[Vincent43]

I run Flawfinder static analysis tool for C/C++ on firejail sources. Here's the output: flawfinder.txt. I'm lacking of C expertise so I don't know if there is something interesting there, probably a lot of noise. I hope you find something useful.

EDIT: You can see more about what CWE-* means here.

[SkewedZeppelin]

:+1: On the topic of similar tools we should look into them some more. I let afl run against firejail for a good two days a couple of months ago to no gain, however I'm pretty sure I didn't configure it very well.

[Fred-Barclay]

On that note we can possibly integrate flawfinder, at least, with our Travis CI using the docker image: https://github.com/fkromer/docker-flawfinder

[Vincent43]

In case of Travis CI integration we can silence false positives with /* Flawfinder: ignore */ comments i.e. strcpy(tmp, pScreenSize); /* Flawfinder: ignore */

[smitsohu]

I'm not sure if silencing a large number of warnings is the right approach.... if Travis CI integration is really desired, we should get rid of the warnings first. That would be a bigger effort, but doesn't mean it is not doable. It is only too much for one developer I guess... we would need a number of volunteers :smile:

Another question is if it is worth it. The vast majority of these warnings are certainly false positives, in the sense that there is no corresponding bug.

[Fred-Barclay]

I don't mind working on changing the code if someone is willing to review all 314 159 pull requests I'll probably end up generating. :laughing: I'm a python guy; I haven't caught the hang of C just yet and I need supervision!

Probably a lot are false positives but I'm not opposed to getting rid of them (if possible and within reason) anyways. :) Especially with firejail being setuid, we should probably take extra care and make sure our code is as tight as possible. I'm probably showcasing my C ignorance, but messsages like Does not check for buffer overflows (CWE-120) sound like something we should look into and change.

EDIT: also according to https://stackoverflow.com/a/1892556 some things we do (like using chmod instead of fchmod) present some risk since firejail has root permissions. It may not be easily exploitable for firejail (I have no idea), but it's worth handling IMHO.

[smitsohu]

If I'm allowed a wish we should indeed look a bit closer at the races, though again I would expect most of them to be non-issues (e.g. functions dealing only with root stuff). At potentially overflowing functions I wouldn't look first.... even if we got a bug somewhere, chances are that the compiler takes care of it already with FORTIFY_SOURCE=2

[smitsohu]

(if possible and within reason)

+1 from me. Flawfinder is obviously not especially smart as a tool, and following it without thinking doesn't make Firejail better necessarily.

Also if it turns out we are silencing hundreds of warnings because we consider them unfounded (and this is the most likely outcome), showcasing clean results then doesn't feel right. In fact I'm still not convinced of the Travis CI integration bit.

[Vincent43]

Here's slightly improved output (it shows offending line of code) - flawfinder -cQ firejail: flawfinder-firejail.txt

[Fred-Barclay]

Also if it turns out we are silencing hundreds of warnings because we consider them unfounded (and this is the most likely outcome), showcasing clean results then doesn't feel right. In fact I'm still not convinced of the Travis CI integration bit.

Yeah, that's just an idea for later (if at all). :smile:

[smitsohu]

I've spend some time with the provided list, I guess my comments above are not especially helpful. Please disregard them to the extent possible. I will continue looking at it in silence. Sorry for the distraction.

[matu3ba]

grep -o "\(CWE-[0-9]*\)" flawfinder-firejail.txt | sort -n -k1.5 | uniq > CWE-list.txt
returns the according CWEs as list. CWE-list.txt Does flawfinder support different formatted output (CWE as first index)? We could use the CWE as commits etc. Ideas?

[SkewedZeppelin]

@matu3ba I hope it doesn't, because I just made a script to reformat it and get the CWEs and their descriptions only.

Some seem to get a risk associated with them resulting in duplicates.

flawfinder-firejail-cwe_list.txt

[matu3ba]

@SkewedZeppelin Neat, since they are not alphanumerically ordered, heres the sorted form grep "\(CWE-[0-9]*\)" flawfinder-firejail-cwe_list.txt | sort -n -k1.5 > flawfinder-firejail-cwe_list2.txt There is still CWE-362: line twice which even uniq does not cut away. flawfinder-firejail-cwe_list2.txt