Re: Draft criteria for passing+1 and pasing+2 - comments?


David A. Wheeler
 

From: liujin (K) [mailto:liujin1@huawei.com]
It is noticed code review suggestion and rules had set to "Probably not",  and it may not be a right choice.
Even today, buffer overflow, integer overflow, and improper validation of array index is still the most common C language code security issues.
These are certainly common security issues in C!

For small software, to do code review is very easy to operate, and it is the best way to find out most of the potential code secure fault in small software, e.g., the software less than 100 thousand lines of code. Code view convenient and cheap.
In passing+2 we have two proposed requirements: two_person_review (which I think is somewhat similar to your proposal) and security_review. Currently two_person_review requires that at least 50% of modified code be reviewed; we could make larger (even 100%).

Certainly code review can be very effective. I co-edited the IEEE book, "Software Inspectino: An Industry Best Practice", where I identified many good arguments for code review (especially a specific code review technique called "software inspections"). So there's plenty of evidence for its use.

However: It is *not* cheap. Yes, if you compute the number of defects found per hour of review, it can be very efficient. But not all defects are of equal importance, and *many* projects simply do not have the resources available to require code review no matter how low the cost/defect-found is.

Putting this at passing+2 enables projects that are not as resource-flush to still make forward progress.

For large software, code review has almost become the industry standard. For example, Google and Microsoft have established a clear code view system. This is because the code view is always more accurate and can make up for the lack of static code checking tools.
Both Google and Microsoft have literally billions of dollars to spend. Not everyone has their resources. Also, code review techniques are best *combined* with static & dynamic tools, since each have their strengths & weaknesses.

The code review suggestions as following:
1.The direct data or the indirect data from the untrusted sources which is used as the array index MUST be ensured within a legal range.
2.The direct data or the indirect data from the untrusted sources which is used as the buffer length for read/write MUST be ensured within a legal range.
3.The direct data or the indirect data from the untrusted sources which is used as the loop ending condiction MUST be avoided infinite loop or other logic mistake.
4.When copying from a string that is not a trusted source, it MUST ensure that there is enough space to hold the data and the end.
5.The integer values from untrusted sources MUST be avoided the integer overflow or wraparound.
6.Appropriate size limits SHOULD be used to allocate memory from an unreliable source, and MUST check the return value of the allocate function. 
First: We should avoid language-specific requirements; tech changes! If we use a list like this, it needs to be recast as additional requirements for memory-unsafe code (C, C++, and some other languages when memory safety is disabled). Almost all of these seem to be specific to memory-unsafe code.

#3 won't work. Many programs are specifically designed as infinite loops, that's the whole point. For example, many real-time systems, after boot, look like this:
while true {
read_input
do_stuff
}

One possibility is turning this into a set of minimum criteria for code reviews (in passing+2) when using a memory-unsafe language. Or maybe it's a new "code_review" or "code_review_guidelines" criterion.

The above 6 rules are very simple and very detail, but they can help us to avoid the most dangerous codes' mistakes. It may be the beginning that helps us improve the efficiency of the code review by specifying rules. We hope that through the contribution of the people, the extraction of limited code view rules can reduce most of our security coding problems.
--- David A. Wheeler

Join CII-badges@lists.coreinfrastructure.org to automatically receive all group messages.