A Case Study - Selecting a Code Review Approach

This case study from company ABC will analyze and identify an approach to develop and maintain secure systems and applications, including selecting suitable static-analysis code scanning tools for application development. ABC positioned different approaches to prevent data theft (and the attack-paths to the data ? different types of apps, databases ...), data (protection (encryption, hashing, tokenizing) and C++ code scanning. The solution for ABC is based on the conclusion that every layer of defense is critical. A holistic and layered approach can provide the best level data security and the sooner sensitive data gets encrypted, the more secure the environment. ABC is planning an enterprise data protection approach and protects data across the information life cycle. ABC acknowledges that secure development will take a long time to implement, partly based on expensive and time-consuming manual code reviews. ABC is selecting a solution including code reviews and scanning of internal code non-web applications. ABC also identified a long term project that will include penetration testing and scanning and review of the web application code base. This article is based on a project case study in protecting an enterprise application that are not web related.

Coding standards help developers avoid introducing flaws that can lead to security vulnerabilities. Testing standards and best practices help to ensure that testing focuses on detecting potential security vulnerabilities rather than concentrating only on correct operation of software functions and features. "Fuzzing" supplies structured but invalid inputs to software application programming interfaces (APIs) and network interfaces so as to maximize the likelihood of detecting errors that may lead to software vulnerabilities.

Apply static-analysis code scanning tools and code reviews

Tools can detect some kinds of coding flaws that result in vulnerabilities, including buffer overruns, integer overruns, and uninitialized variables. Microsoft has made a major investment in the development of such tools (the two that have been in longest use are known as PREfix and PREfast) and continually enhances those tools as new kinds of coding flaws and software vulnerabilities are discovered. Code reviews supplement automated tools and tests by applying the efforts of trained developers to examine source code and detect and remove potential security vulnerabilities. They are a crucial step in the process of removing security vulnerabilities from software during the development process.

Separate code reviews as a way to enhance security

Both PCI DSS and SDL mention separate code reviews as a way to enhance security. In addition SDL mentions the use of static-analysis code scanning tools. Such tools often assists during code reviews, but may also be applied during normal development.

Static/dynamic analysis and other definitions

There are two principal types of program analysis. Static, or compile-time, analysis is aimed to investigate a program''s run-time properties without actually executing it. Normally this is performed by source code inspection, but binaries may also be used. Dynamic, or run-time, analysis is performed when observing the program at execution. Testing, debugging and performance monitoring are examples of dynamic analysis.

Example of a very simple static analysis

An example of a very simple static analysis would be searching for specific words like strcpy using a file-search utility like grep. The goal would be to identify where unsafe functions are used.

A search for strcpy will however also list values like strcpy_s. If strcpy is part of a comment, this will also be presented as a valid occurrence. Such output is called a false positive, i e something reported as a vulnerability though not. False positives are a big issue in static analysis since they give the user more data to evaluate than necessary. Each program construction reported as a vulnerability must be considered and reviewed.

Suppose strcpy is renamed to something else, for instance with a macro like ''#define mycopy strcpy''.

In this case a word search for strcpy won''t list any occurrence of mycopy, though strcpy really is used. This is called a false negative, i e a real vulnerability that hasn''t been presented.

An ideal analysis tool

An ideal analysis tool would have no false negatives and no false positives, only true vulnerabilities would be presented. That is however not realistic. Instead they are often somewhat related; a lower false positive rate means a higher false negative rate, and vice versa.

Free open source static-analysis tools

There are different free open source static-analysis tools, as desribed further below. These are only marginally better than the simple word-search as above. They search for specific unsafe calls like strcpy as listed in an internal database, and when found they present the position and a general comment about the problem. This handling gives a lot of false positives, but also a lot of false negatives since they only look for some calls. Such simple tools are of limited value.

More advanced tools

More advanced tools try to interpret the context of the word, based on the programming language used. This is called semantic analysis. The better this analysis is, the fewer false positives there will be. The free open source tools do perform some semantic analysis, meaning at least some false positives will be skipped.

In addition to look for certain language-specific words, advanced tools also look at the general program context. An intra-procedural analysis only looks at what happens within a specific function. This may be inaccurate, for instance when external entities like globals are used. An inter-procedural analysis tries to consider all parameters of the function, and the interaction of functions. This is much more complicated than intra-procedural analysis, considering different possible parameter values and paths for execution. Related to this is flow-sensitive and path-sensitive analysis, which try to consider the program flow and the different paths possible.

Inter-procedural analysis may in some cases not be impossible

Even if supported by the tool, inter-procedural analysis may in some cases not be possible. If there are third-party libraries for which source code isn''t available, or there are yet unimplemented functions, the tools can''t inspect what happens inside these calls. This may result in false negatives produced.

Tools often try to simplify analysis, to get better performance

Tools often try to simplify analysis, to get better performance. Doing such as inter-procedural and flow-sensitive analysis could consume considerable resources. A tool may for instance only consider min/max values when handling integer input. Such simplifications are also a source of false negatives. In general, a tool will never be totally accurate, but the better it performs different types of advanced analysis, the more vulnerabilities it will find.

Using static-analysis tools during development

There is a wide range of static-analysis tools, from simple syntax checkers to advanced tools performing semantic, inter-procedural and flow-sensitive analysis. The advanced tools are also getting more advanced for each version, applying new types of analysis and vulnerability detection rules.

Examples of what tools may look at are:

- resource leaks
- references to NULL pointers
- use of uninitialized data
- buffer array overruns
- unsafe use of signed values
- use of resources that have been freed
- concurrency handling

Without doubt a tool capable of such analysis would be valuable during development. A tool may however be used in different configurations. The questions are when, where and by whom should the tool be applied? There are different options:

1) When the code is written by the developer

First option would be to run the tool when the code is being written. In this case it''s the developer that runs the tool, in the local IDE used. Later versions of commercial tools also support a mixed handling where there are local instances, but still some central repository for handling overall metrics, for instance to see if coding skill is evolving over time.

There are both advantages and disadvantages with the local approach:

Plus side:

- it''s easier and faster to handle a bug if caught directly when the code is written; the programmers know their own code best, and the code is in current focus
- handling a bug locally means it''s not propagated to the central repository, thereby affecting other developers
- running a tool and interpreting the output will educate the developers in secure coding. Tools have contextual help that explains a given vulnerability and how to handle it

Minus side:

- tools are often licensed per user, one tool instance per developer could mean a large total cost for the tool
- running a tool too early could mean an unnecessarily high number of false positives. Tools are less precise when the code is in an initial phase, and inter-procedural analysis doesn''t really apply when many code pieces are missing (later versions of commercial tools however claim to be better in this aspect)
- each developer must be trained in using the tool. Interpreting tool output is for senior developers, with appropriate security knowledge. Marking a valid bug as a false positive could mean the weakness is lost
- each developer workstation needs a local installation of additional software

2) At build time, when scheduled builds are performed

A second option would be use a tool integrated in a central build process. The scan is performed when the total application is built. This is an option often used with commercial tool offerings.

Plus side:

- a central configuration means a group of senior developers may evaluate tool output before it''s distributed to responsible developer, the analysis could be transparent to developers.
- providing error reports to responsible developers means education in secure coding still applies
- server installations minimizes the number of software deployments necessary
- reports are easily generated concerning overall secure coding metrics, and may be accessed by everyone
- tool is executed a bit later in the development process, not until the code has been checked into the build system. This will reduce false positives caused by premature code

Minus side:

- tool license cost may still be based on number of users, or it may be some general server license. The licensing cost could still be high
- bugs are not handled directly, but if the build is performed often the code is still current and not that hard to modify
- errors are propagated to the central repository, thereby affecting other developers until corrected
- is using a specific group of reviewers, they may become a constrained resource. They will however likely become tool experts after some time, speeding up the process

3) At certain code review checkpoints, by a security oriented code reviewer

A third option would be to use the tool as an assistant during code reviews, to be performed at certain project milestones like before product release.

Plus side:

- tool license cost should be smaller, since only a few security oriented users will have the tool. License could however also be based on code size and other aspects.
- tool is executed late in the development process, which will reduce false positives caused by premature code
- senior security oriented developers are evaluating output before it''s distributed to responsible developer, the analysis could be transparent to developers
- distributing error reports to the developer in charge means education in secure coding will still apply. Errors have been filtered by the code reviewer, though
- reports may be generated concerning overall secure coding metrics

Minus side:

- bugs are not handled directly, but rather late in the development process. Fixing an error will take longer time and be more costly, code may not even be current for developer when bug is presented
- errors are propagated to the central repository, thereby affecting other developers until corrected
- the security reviewer may not know the code, which could slow down the interpretation of the tool output
- the group of reviewers will likely become a constrained resource, possibly handling different projects. They will however become tool experts after some time, speeding up the process

All these three cases could apply for ABC development

The first case seems like an attractive way to go; the errors are handled directly by the developer and won''t affect others, the developers will become more skilled as time goes by. But based on the cost involved to pursue such a configuration, it''s absolutely necessary to first verify how good the tool is in the ABC environment. A similar high cost would also be true for the second configuration.

A specific platform library, where system calls are hidden

ABC has a specific platform library, where system calls are hidden. There are different types of wrapper functions and classes used throughout the code. Vital parts like encryption and SSL are implemented in external libraries. All of this means inter-procedural analysis will be important for a ABC analysis tool. Until a tool is tested in the ABC environment, it''s not possible to say how good it will be, and it shouldn''t be used on a large scale.

Since there are much code already developed in ABC

Since there are much code already developed in ABC, and all this code is about to have a code review, the third option could be a good starter. This will give an indication of general code quality, and should minimize the initial cost for the tool. If the tool used is found to be very helpful, and the tool budget allows, the tool may later be propagated into the whole developer community either as option one or two.

ABC is a highly security oriented organization
Another consideration is that ABC is a highly security oriented organization. A higher degree of false positives could be accepted for a ABC scan, since this normally also means a higher percentage of errors identified. But this also means there will be more time spent with the tool output, each error reported must be evaluated. This is a big issue in a time-constrained environment, which is a reality for many developers. If using the first configuration, an option would be to restrict the vulnerability rule set for local development, and then have a more thorough rule set for a central build or security review.

Tool selection criteria

A static analysis tool would certainly be valuable during development, no matter where it''s applied in the development chain. But all tools will of course not be equally good. There are different things to consider when selecting a static analysis tool, especially for a commercial tool. Some considerations will depend on how the tool is going to be used.

1) Multiple language support

The tool must support the programming languages used for development. This is a requirement no matter where the tool is applied.

The main languages used for ABC development are C/C++ and Java; support for these is a basic tool requirement. But ABC also has some code built with for instance C#, PL/SQL and T-SQL. Support for these additional languages would be a further advantage, though not a requirement.

2) Capability in finding vulnerabilities

The principal task for the tool is to find code vulnerabilities. Strong capability in this area is another major requirement. This is a requirement no matter where the tool is applied.

This ability is two-fold; there should be a high rate of true errors identified compared to the total number of errors, but there should also be a low rate of false positives. These are often a bit contradictory; a lower false positive rate often means a higher rate of missed errors.

Being a security oriented organization, a higher degree of false positives could be accepted for a ABC scan if this means a lower false negative rate. The target for a security oriented organization must be to have the smallest amount of bugs possible, even if this means time for analysis will be extended.

3) Integration into development environment

If the tool is to be used as part of normal development operations, it''s important that the tool integrates smoothly into the development environment used, for instance Visual Studio. If necessary to run a separate tool, it will likely be less often used than if closely integrated, and additional time must be spent on handling two different output lists.

If used in a central build environment, the tool must of course integrate into what''s used there. Since ABC is a multi-platform product, there must be support for at least one UNIX version and Windows.

4) Management of true errors and false positives

Tools normally provide some explanation why an error has been reported. The more specific this explanation is, the easier and faster it will be to evaluate if the reported item really is an error, or if it''s a false positive. Good explanations are also important for educational purposes.

When an error has been fixed it shouldn''t be listed any more. It should also be easy to mark en error as a false positive when this has been decided, and this mark should be consistent (saved in-between invocations). Otherwise it will be necessary to mark it as a false positive for each execution, and much time will be spent on the same errors.

Related to this is the possibility to have different types of reports generated, for instance providing trends in number of errors reported. This may be useful for education and management.

5) Management of rule set

Tools are often based on some rule set, where different errors are grouped. There will always be false positives produced. It''s important that it''s possible to tweak the rule set, to adjust the amount of errors and/or false positives reported. A recommendation is to start with a small subset of rules in the beginning, to not get overwhelmed by tool output, and then step-by-step extend the rule set used. In the end a security oriented organization like ABC must be using an extensive list of rules.

Related to this is the complexity to add internal rules, to extend the default rule set. This is for advances users, but may be necessary in some situations, like when using external libraries or to catch certain error types. Writing an extension could mean writing a separate C/C++ library, or using some internal tool language format.

6) Price

Assuming the tool budget isn''t unlimited, price may be an important parameter for a tool. If using one copy of the tool per developer, cost may easily be very high since the tools are often licensed per user.

License cost may often be selected either as an annual fee, or a perpetual one with some maintenance cost.

Considered paths to go
Concerning tool selection, there are three paths to go from here:

? Use free tools only
? Select a commercial tool based on trials with all four possible vendors
? Select a single commercial tool, and either buy a single license or perform trial

1) Use free tools only

Using a free tool for C/C++ like Flawfinder doesn''t seem to be an option. Especially since ABC has a platform library, which is the only place for unsafe calls as for instance listed in Flawfinder. Free tools could possibly be used as education sources, learning the unsafe functions if not known already. The GPL license type must also be considered.

Using Microsoft''s PREfast should be added to normal development process. All existing ABC C/C++ code should be scanned with PREfast, and before new code is being checked in, it should have been scanned with PREfast (since compilation time will be longer when using PREfast, it probably shouldn''t always be used). Code scanning with PREfast will however be restricted to the Windows environment, some UNIX specific parts in ABC won''t be handled.

Looking at Java, the FindBugs tool should be a good choice. It has a LGPL license, and is even used in the Fortify commercial tool. All existing ABC Java code should be scanned with FindBugs, and before new Java code is being checked in, it should have been scanned with FindBugs.

2) Select a commercial tool based on trials with all four possible vendors

Using a commercial tool is recommended. A commercial tool will be more advanced concerning inter-procedural analysis than PREfast, and is expected to find more vulnerabilities. The C/C++ code is likely where most bugs will be found in ABC, being less secure programming languages than for instance Java.

The choice of a commercial tool is however not that clear. Based on the public tests available, there doesn''t seem to be any major differences concerning bug-finding capability. Different ranking is rather based on tool management.

A general recommendation is to test the tool in the own environment, and most vendors support trials. An environmental test is maybe even more important for ABC, with its platform library and different types of wrapper functions/classes. Strong ability in inter-procedural analysis is important.

3) Select a single commercial tool, and either buy a single license or perform trial

If decided not worth to spend too much time on tool evaluation, one commercial tool vendor could be chosen. A feeling is that all products will provide good assistance. A tool may certainly save time, since more bugs can be handled in an early phase. When evaluating the tool cost, a consideration must be how much time a tool will save, and the earnings from that. As stated in [20]:

"...An industry rule of thumb is that a bug which costs $1 to fix on the programmer''s desktop costs $100 to fix once it is incorporated into a build, and thousands of dollars if it is identified only after the software has been deployed in the field..."

The decision falls back to how many unknown bugs the tool will find, and how early these bugs are identified thanks to the tool.

Conclusion

An effective code-scanning tool would definitely be useful in ABC development. Being a security oriented organization, it''s very important to minimize the number of bugs. The use of code scanning tools is also mandated by Microsoft''s SDL. No matter what tool used, this should be accompanied with code reviews, appropriate testing including such as fuzzy testing, code standards that are followed, and proper education. No matter what tool configuration selected, manual code reviews, education, coding standards and proper testing must also be applied.

References and Suggested Reading

[1] Information Supplement: Requirement 6.6 Code Reviews and Application Firewalls Clarified, Feb 2008,
https://www.pcisecuritystandards.org/pdfs/infosupp_6_6_applicationfirewalls_codereviews.pdf

[2] The Future Of Application And Database Security, December 2007, http://securosis.com/

[3] Multi-layer system for privacy enforcement and monitoring of suspicious data access behavior, February, 2006, United States Patent Application 20060259950

[4] OWASP 2007 item is A8 ? INSECURE CRYPTOGRAPHIC STORAGE, PROTECTION section and in the VERIFYING SECURITY section, http://www.owasp.org

[5] NIST ( http://csrc.nist.gov/CryptoToolkit/modes/ ). In Special Publication 800-38A

[6] http://usa.visa.com/merchants/risk_management/cisp_merchants.html

[7] Data type preserving encryption, November 2000, United States Patent 7,418,098

[8] NIST ? Application Vulnerability Scanners
https://samate.nist.gov/index.php/Web_Application_Vulnerability_Scanners

[9]OWASP Tools
http://www.owasp.org/index.php/Phoenix/Tools

[10] The code review project at OWASP
http://www.owasp.org/index.php/OWASP_Code_Review_Project

[11] How To: Perform a Security Code Review for Managed Code (Baseline Activity),
http://msdn.microsoft.com/en-us/library/ms998364.aspx

[12] Web Application Security Consortium, http://www.webappsec.org/

[13] IT Security is a news and information publication, bhttp://www.itsecurity.com/meet-experts/expert-biography-ulf-mattson-100206/

[14] Protegrity''s Defiance Threat Management System, http://www.protegrity.com

[15] Protecting the enterprise data flow, http://www.ulfmattsson.com

[16] The PCI Knowledge Base is a Research Community,
http://www.knowpci.com/index.php?option=com_adsmanager&page=show_ad&adid=25&catid=1&Itemid=97

[17] Data Security for PCI and Beyond , http://papers.ssrn.com/sol3/papers.cfm?abstract_id=974957

[18] Payment Card Data - Know Your Defense Options,
http://papers.ssrn.com/sol3/papers.cfm?abstract_id=1126002

[19] A Multi Layered Approach to Prevent Data Leakage, Help Net Security (HNS), November 2007
http://www.net-security.org/article.php?id=1092

[20] Software that makes software better, Economist, Mar 2008
http://www.economist.com/research/articlesBySubject/PrinterFriendly.cfm?story_id=10789417

[21] Payment Card Industry (PCI) Data Security Standard v1.1, Sep 2006
https://www.pcisecuritystandards.org/

[22] Payment Card Industry (PCI) Data Security Standard v1.1 - Security Audit Procedures, Sep 2006
https://www.pcisecuritystandards.org/

[23] Trustworthy Computing Security Development Lifecycle, Microsoft, Mar 2005
http://msdn2.microsoft.com/en-us/library/ms995349.aspx

[24] Code Scanners, False Sense of Security?, Network Computing Report, Apr 2007
http://www.networkcomputing.com/channels/security/199000936
http://www.networkcomputing.com/channels/security/198900460
http://www.fortify.com/products/sca/

[25] Fortify SCA 5.0 Extends App Protection, Redmond Developer News, Nov 2007 http://reddevnews.com/news/devnews/article.aspx?editorialsid=855

[26] Fortify Software Extends Leadership in Detecting..., Fortify press release, May 2007 http://www.fortify.com/news-events/releases/2007/2007-05-14.jsp

[27] Source-Code Assessment Tools Kill Bugs Dead, Secure Enterprise, Dec 2005
http://www.klocwork.com/company/releases/12_05_05.asp
http://www.neohapsis.com/publications/articles.html

[28] Coverity and Klocwork code analyzers drill deeper, InfoWorld, Jan 2006
http://www.infoworld.com/article/06/01/26/73919_05FEcode_1.html

[29] DHS Funds Open-Source Security Project, eWeek, Jan 2006
http://www.eweek.com/c/a/Security/DHS-Funds-OpenSource-Security-Project/

[30] Scan Project Rung 1 status, Coverity, Mar 2008
http://scan.coverity.com/rung1.html

[31] Closing Security Holes with Application Scanners, Enterprise Systems, Jul 2007 http://esj.com/news/article.aspx?EditorialsID=2714

[32] Klocwork Inc, The Wall Street Transcript, Nov 2007
http://www.nohau.se/page.asp?page=artall

[33] Imperva, http://www.imperva.com/