Spotting the Sin During Code Review

C/C++ developers need to pay the most attention to integer overflows. Now that many developers are better about checking sizes when directly manipulating memory, the next line of attack is on the math you use to check what youre doing. C# and Java are next . You may not have the issue of direct memory manipulation, but the language lets you make nearly as many mistakes as C/C++ allows.

One comment that applies to all languages is to check input before you manipulate it! A very serious problem in Microsofts IIS 4.0 and 5.0 web server happened because the programmer added 1, and then checked for an overly large size afterwardwith the types he was using, 64K1 + 1 equals zero! There is a link to the bulletin in the Other Resources section.

C/C++

The first step is to find memory allocations . The most dangerous of these are where youre allocating an amount you calculated. The first step is to ensure that you have no potential integer overflows in your function. Next, go look at the functions you called to determine your inputs. The author of this chapter has seen code that looked similar to this:

 THING* AllocThings(int a, int b, int c, int d) {  int bufsize;  THING* ptr;  bufsize = IntegerOverflowsRUs(a, b, c, d);  ptr = (THING*)malloc(bufsize);  return ptr; } 

The problem is masked inside the function used to calculate the buffer size, and made worse by cryptic, non-descriptive variable names (and signed integers). If you have time to be thorough, investigate your called functions until you get to low-level run-time or system calls. Lastly, go investigate where the data came from: how do you know the function arguments havent been tampered with? Are the arguments under your control, or the control of a potential attacker?

According to the creators of the Perl language, the first great virtue of a programmer is laziness ! Lets do things the easy wayall these integers are hard enoughthe compiler can help us. Turn up the warning level to /W4 (Visual C++) or -Wall or -Wsign-compare (gcc), and youll find potential integer problems popping up all over the place. Pay close attention to integer- related warnings, especially signed-unsigned mismatches and truncation issues.

In Visual C++, the most important warnings to watch for are C4018, C4389, and C4244.

In gcc, watch for warning: comparison between signed and unsigned integer expressions warnings.

Be wary of using #pragmas to ignore warnings; alarm bells should go off if you see something like this in your code:

 #pragma warning(disable : 4244) 

The next thing to look for are places where youve tried to ensure writes into buffers (stack and heap buffers) are safe by bounding to the destination buffer size; and make sure the math is correct. Heres an example of the math going wrong:

 int ConcatBuffers(char *buf1, char *buf2,   size_t len1, size_t len2){  char buf[0xFF];  if((len1 + len2) > 0xFF) return -1;  memcpy(buf, buf1, len1);   memcpy(buf + len1, buf2, len2);  // do stuff with buf  return 0; } 

In this code, the two incoming buffer sizes are checked to make sure they are not bigger than the size of the destination buffer. The problem is if len1 is 0x103, and len2 is 0xfffffffc, and you add them together, they wrap around on a 32-bit CPU to 255 (0xff), so the data squeaks by the sanity check. Then the calls to mempcy attempt to copy about 4GB of junk to a 255 byte buffer!

Someone may have been trying to make those pesky warnings go away by casting one type to another. As you now know, these are perilous and ought to be carefully checked. Look at every cast, and make sure its safe. See the earlier section Casting Operations on C/C++ casting and conversion.

Heres another example to watch for:

 int read(char*buf, size_t count) {  // Do something with memory }  ...  while (true) {  BYTE buf[1024];  int skip = count - cbBytesRead;  if (skip > sizeof(buf))  skip = sizeof(buf);  if (read(buf, skip))   cbBytesRead += skip;  else  break;  ... 

This code compares the value of skip with 1024 and if its less, copies skip bytes to buf. The problem is if skip calculates out to a negative number (say, 2), that number is always smaller than 1024 and so the read() function copies 2 bytes, which, when expressed as an unsigned integer (size_t), is almost 4GB. So read() copies 4GB into a 1K buffer. Oops!

Another overlooked example is calling the C++ new operator. There is an implicit multiply:

 Foo *p = new Foo(N); 

If N is controlled by the bad guy, they could overflow operator new, because N * sizeof(Foo) might overflow.

C#

Although C# doesnt typically involve direct memory access, it can sometimes call into system APIs by declaring an unsafe section and compiling with the /unsafe flag. Any calculations used when calling into system APIs need to be checked. Speaking of checked, it is a great keyword or better yet compiler switch to use. Turn it on, and pay close attention when you end up in the exception handler. Conversely, use the unchecked keyword sparingly, and only after giving the problem some thought.

Pay close attention to any code which catches integer exceptionsif its done improperly, just swallowing an exception may lead to exploitable conditions.

In short, any C# code compiled with /unsafe should have all integer arithmetic reviewed (see the preceding C/C++ section for ideas) to make sure its safe.

Java

Java also doesnt allow direct memory access, and isnt quite as dangerous as C/C++. But you should still be wary: like C/C++, the language itself has no defense against integer overflows, and you can easily make logic errors. See the Redemption Steps section for programmatic solutions.

Visual Basic and Visual Basic .NET

Visual Basic has managed to turn integer overflows into a denial of service problemmuch the same situation as using the checked keyword in C#. A key indication of problems shows up when the programmer is using the error handling mechanism to ignore errors due to mishandling integers. Ensure the error handling is correct. The following in Visual Basic (not Visual Basic .NET) is a warning that the developer is lazy and does not want to handle any exception raised by the program at run time. Not good.

 On Error Continue 

Perl

Perl is cool, but floating point math is a little strange . Most of the time, it will do the right thing, but Perl is different in many ways, so be careful. This is especially true when calling into modules that may be thin wrappers over system calls.