cipherdyne.org

03 July, 2013

A few months ago Coverity announced that they would grant open source projects access to their "Scan" static analysis tool for free. This was certainly a bold move, and one that has the potential to improve the security landscape as more projects take advantage of it. The first time I had encountered Coverity was several years ago when Linux kernel ChangeLog entries were referencing Coverity's Scan tool as having found various classes of bugs in the kernel sources. This impressed me because the Linux kernel source code is not the easiest code to grok, and at the same time it is extremely widely deployed and audited so bugs get reported and fixed. Andrew Morton, second in command of the kernel, has pegged the time required to become proficient as a kernel developer at about one year of full time dedicated effort for an experienced systems programmer. Hence, any automated static analysis infrastructure that is good enough to find kernel bugs and have it credited in the kernel ChangeLog is a strong vote of confidence to my mind. Here is a recent instance of Coverity finding a NULL pointer dereference bug in Infiniband driver code for example.

Even though Coverity started being used by the Linux kernel development community circa 2005, Coverity was expensive and therefore not generally available to open source projects that typically do not have access to significant resources. Today, that has all changed with Coverity's open source initiative, and the community is better off. Simultaneously, Coverity also gets a benefit in terms of free press as more open source projects start using Coverity Scan. As of this writing, over 300 projects have signed up including fwknop, and Coverity maintains a list of these here. OpenVPN and OpenSSL are participating, but it doesn't look like OpenSSH is yet and I (for one) hope they do eventually.

Here is an example of a bug that Coverity found in the fwknop client:



This is a classic memory leak bug in the fwknop client that is only triggered when an invalid port number is specified from the command line when sending SPA packet data through an HTTP proxy (this is not a widely used method for SPA packet transmission, but it is supported by the fwknop client along with other transmission modes). The spa_data_copy variable points to heap allocated memory that wasn't free()'d after the error condition was detected and before returning from the send_spa_packet_http() function call. Coverity was able to spot this bug through static analysis, and the diff output above shows the corresponding fix. In terms of other bugs found by Coverity, they have all been fixed in the current fwknop sources in preparation for the 2.5 release. A -pre release is available through the new Github releases feature, and will likely become 2.5 within a week: fwknop-2.5-pre3.tar.gz.

This same bug could also have been caught at run time with the excellent valgrind "memcheck" tool, but the invalid port number error would have had to be triggered in order for valgrind to detect the memory leak. While the fwknop test suite is fairly comprehensive and can automatically run every test underneath valgrind to help with the detection of memory leaks, not every error condition is triggered by the test suite and hence static analysis is quite important. At the same time, run time analysis is also important to pick up leaks and other problems that static analysis techniques may miss, so running valgrind will continue to be a critical test suite component.

Now, on the vulnerability research side, there are implications for vulnerability discovery. That is, source control changelogs that mention Coverity or other static analysis tools can help malicious actors find potentially exploitable code. Here is an example Github search that shows all issues where Coverity is mentioned. Is this a bad thing? Well, in a word I would say "no". The exploit community is constantly looking for ways to exploit software regardless of whether static analysis tools are used by project developers. The real problem is that bugs are frequently never discovered or fixed in the first place. Further, keeping bugs under wraps in a vein effort to hope they won't be found has never worked, and this is true regardless of whether a project is open source or not (binary patches released by Microsoft are reversed engineered to figure out how to exploit unpatched code for example). What we need is better, more security conscious software engineering - not imagined bug secrecy. Consistent, advertised usage of static analysis tools helps to drive this. In addition, static analysis tools should be seen as a piece of the bug hunting puzzle and be used in conjunction with other standard techniques such as dynamic analysis, unit tests, and good old fashioned code reviews.

Finally, in terms of Coverity's own competition such as Veracode, Klocwork, or Checkmarx, it seems to me that Coverity has made a shrewd move by offering code analysis to open source projects for free. They will continue to make important inroads into the OS community (which has become quite powerful over the past, say, 15 years and longer), and will most likely see a gain on the commercial side as a result.

The bottom line is that Coverity has an excellent static analysis product, and if you run or contribute to an open source project written in C/C++, you should be using Coverity Scan. It will likely find bugs that can certainly have security implications in your code. Even if Coverity doesn't turn up any bugs, additional verification is always a good thing - especially for security projects.