Let’s say that a developer has recently left the company and has taken up residence in a tent in a wilderness area. You’re the proud new owner of 250,000 lines of code, none of which you’ve seen before, and now your management wants a security code review conducted in the next month. It might seem like your worst nightmare, but there are ways to cope with the problem other than going to live in a tent yourself!

The first thing to do is to prioritize—some pieces of the code are more risky than others. Once you understand the basic application flow and how it works, refer to the threat model and a data flow diagram for the application. This will point you to the most security-critical portions of the application. Anything that handles user input, makes transitions between user contexts, or exposes interfaces to the network needs to be handled most carefully. Pay special attention to code that has a history of vulnerability.

Now that you’ve sorted your application into portions according to risk, apply commensurate effort to auditing each area. High-risk areas require a detailed, line-by-line review, optimally by using a formal approach. A less risky module might get by with a less detailed review, and the lowest risk areas might get examined only for use of dangerous function calls.

While you’re looking at the code, get a feel for the overall quality—some code just needs to be replaced. I once worked on a relatively simple function written by a very junior programmer who consumed such huge quantities of Coca-Cola that he seemed as if he’d bounce off the walls at any instant. The quality of the code was extremely poor, and even after review by several experienced programmers (and many bug fixes), it continued to generate a large number of bug reports. I’d personally tried to fix all the problems more than once, and there were still bugs crawling out of it. Finally, I stayed a couple of hours late and rewrote the function (and two others by the same author) from scratch. As far as I know, the new modules didn’t generate any bugs after the rewrite. It took me a fraction of the time to write solid functions than it had taken me to try to fix the bad code. Likewise, if you’re looking at a 1200-line function with huge, complex loops and you’d like some garlic toast to go with your spaghetti, maybe it needs to be flagged for replacement. During the Windows Security Push in early 2002, we found a couple of areas where we decided the best thing to do was to ask people to use different libraries and retire those that were deemed too difficult to repair. Again, it can sometimes take a lot less effort to replace bad code than it takes to fix it. The only caveat to replacing the code is that it will need much more testing; there could be areas where the previous maintainers learned lessons from the school of hard knocks.

An approach that one of the best code reviewers at Microsoft advocates is to take several passes through the code. First, you start with a high-level review. Understand the environment, and examine the data structures and initialization. Start to build a model of the code, and understand the linkages between functions. Any code that appears overly complex should be flagged for extra attention. Finally, establish your starting points to trace the code. The starting points are used to examine particular questions, such as "Can this password string ever overrun a buffer?" This allows you to focus your review on one problem at a time.

Once you’ve completed the preliminary groundwork, begin the investigation by checking all your starting points and iterate through these until you’re done. If one starting point starts to branch off too far, create a new starting point to follow—don’t lose focus on where you started. Now you’re ready to examine the code function by function. There are certain mistakes that most programmers make, and you might find patterns of mistakes by an individual programmer. Check unusual code paths most carefully, because these are almost always less well tested and you’re more likely to find security bugs in obscure corners.

One of the first things to try is to look for known unsafe functions; a good list of these is found in Chapter Appendix A. In particular, the string-handling functions need careful examination, even if the calls are from the safe libraries. Recall that the off-by-one example in Chapter 5, used strncpy, not strcpy. Examine each of these and ask whether the input pointers might be NULL, whether the input strings could be missing a terminating null character, and whether the caller might have gotten the length arguments wrong. Next, look for off-by-one errors; these are among the most common when people are attempting to use safe string handlers. If the classic counted string functions are used, look for a null termination immediately after the function—strncpy, strncat, and snprintf aren’t guaranteed to null-terminate. Likewise, look for truncation errors. The traditional "safe" functions can make it difficult to determine whether an input string was truncated.

Buffers of any type need to be checked carefully; bounds checking must be enforced on any access to an array. Exploits can be built out of overflows of any type, not just strings. Hopefully, the examples in Chapter 5 have shown that heap overflows are just as dangerous as stack-based buffer overruns. An additional problem with heaps—one that you won’t see with other types of overflows—is that freeing memory twice can lead to an exploitable condition. Under the right circumstances, freeing memory twice will lead to execution of arbitrary code, not just a program crash. Likewise, failure to free allocated memory can sometimes be used by an attacker in a denial of service attack. Use of _alloca needs to be checked carefully—if an attacker can cause you to allocate a very large buffer on the stack, your application could run out of stack space and crash. I would tend to discourage use of _alloca in general; using it in a recursive function is extremely dangerous.

If your application deals with mixed Unicode and ANSI character sets, be extremely careful when dealing with conversion functions. For example, WideCharToMultiByte is defined as follows:

int WideCharToMultiByte(
    UINT CodePage,            //code page
    DWORD dwFlags,            //performance and mapping flags
    LPCWSTR lpWideCharStr,    //wide-character string
    int cchWideChar,          //number of chars in string
    LPSTR lpMultiByteStr,     //buffer for new string
    int cbMultiByte,          //size of buffer
    LPCSTR lpDefaultChar,     //default for unmappable chars
    LPBOOL lpUsedDefaultChar  //set when default char used
);

The fourth parameter is the number of wide characters in the input string, but the size of the output buffer is the number of bytes. MultiByteToWideChar behaves similarly. Although this may seem unnecessarily confusing, remember that the output might be a multibyte character set, not ANSI. Another good example of an API set where buffers are sometimes defined by the number of bytes and sometimes by the number of wide characters is the C++ DCOM interface for administering IIS (Internet Information Services). If you look closely, the calls that require a number of bytes can return binary data, but it can be tricky. Another point to consider is that the author of the code (or even the documentation) might not have used Hungarian notation correctly—check the variable type as declared.

Another potential problem that’s worth mentioning is the use of TCHAR. A TCHAR is either a char or WCHAR type, depending on whether there’s a #define UNICODE for that source file. I’ve seen a number of bugs that resulted from not being certain whether a buffer was single-byte or double-byte. I prefer to always explicitly use the character type I want.

Integer overflows are one of my "favorite" problems. I gained a healthy respect for the limits of how a computer represents data while writing code to simulate airfoils. Large matrix manipulation using floating-point arithmetic will give you a number of lessons in the school of hard knocks. Most programmers deal only with integer types, and there are only two major classes of problems you might encounter. Let’s take a look at signed-unsigned mismatches. Consider the following code:

int Example(char* str, int size)
{
    char buf[80];

    if(size < sizeof(buf))
    {
        //Should be safe…
        strcpy(buf, str);
    }
}

Quick, what’s the problem here? If you didn’t spot it immediately, here it is: any native integer type is almost always signed. But sizeof returns a size_t type, which is unsigned. What if the caller managed to pass in negative size? Assuming that the compiler casts sizeof(buf) to a signed integer for you, the comparison will succeed and you’ll overflow your buffer. The solution is to always declare your integers as unsigned unless you explicitly require negative numbers. Most systems will treat an integer that isn’t explicitly declared as unsigned as signed. Fortunately, the compiler will report signed-unsigned mismatches unless the programmer has gone in and cast away the warnings. Examine string length comparisons very closely, and don’t ignore signed-unsigned mismatch warnings without careful examination. If the programmer has cast away warnings, examine these carefully—a security bug could be lurking!

Here’s another way to cause problems: adding one to MAX_INT. If you have code that adds some predetermined amount of storage for a trailing delimiter, make sure to do your size checking before you add to it, or alternately, explicitly check for the overflow with this:

if(result < original)
{
  //Error!
  return false;
}

This is actually a common problem when using GetTickCount to determine how long something has run. GetTickCount rolls over about every 40 days, so if you’re using this, make sure and catch this condition.

Integer overruns are another area where you can create some interesting bugs. Consider the following data type:

typedef struct _LSA_UNICODE_STRING {
  USHORT Length;
  USHORT MaximumLength;
  PWSTR Buffer;
} LSA_UNICODE_STRING;

In this case, the Length and MaximumLength members store the number of bytes that the buffer can contain, which would allow for 32,768 Unicode characters. Let’s look at a possible implementation of a function that takes in a WCHAR pointer and initializes one of these structures:

void InitLsaUnicodeString(const WCHAR* str, 
                     LSA_UNICODE_STR* pUnicodeStr)
{
    if(str == NULL)
    {
        pUnicodeStr->Buffer = NULL;
        pUnicodeStr->Length = 0;
        pUnicodeStr->MaximumLength = 0;
    }
    else
    {
        unsigned short len = 
                     (unsigned short)wcslen(str) * sizeof(WCHAR);

        pUnicodeStr->Buffer = str;
        pUnicodeStr->Length = len;
        pUnicodeStr->MaximumLength = len;
    }
}

Examine the code carefully; consider what happens if someone passes in a string that is 32,769 bytes. If a computer is nearby, pop up an instance of calc.exe and follow along. Let’s multiply that by 2. Now switch to hexadecimal display, and you’ll see that the length is 0x10002. Once we cast the result to an unsigned short, we now see that the Length field has just been set to 2! Now to complete the train wreck, imagine this LSA_UNICODE_STRING structure gets passed to another function that merely checks whether Length is less than the MaximumLength of the destination and calls wcscpy! Be extremely careful when truncating integers—here’s how the code could be improved:

unsigned long len = wcslen(str) * sizeof(WCHAR);

if(len > 0xffff)
{
    pUnicodeStr->Buffer = NULL;
    pUnicodeStr->Length = 0;
    pUnicodeStr->MaximumLength = 0;
}

Now let’s look at another way that we can bungle integers; integer multiplication can get a little tricky. Take a look at this example:

int AllocateStructs(void** ppMem, 
                    unsigned short StructSize, 
                    unsigned short Count)
{
    unsigned short bytes_req;
    bytes_req = StructSize * Count;

    *ppMem = malloc(bytes_req);

    if(*ppMem == NULL)
        return -1;
    else
        return 0;
}

As in the LSA_UNICODE_STRING example, it’s possible that the multiplication could result in an overflow, which would lead to our allocating a buffer that’s much too small for the job and the subsequent copy into the buffer would cause an overflow. In this example, declaring bytes_req as an unsigned integer would overcome the problem. Here’s a more robust way to deal with the general problem:

int AllocateStructs(void** ppMem, 
                    unsigned short StructSize, 
                    unsigned short Count)
{
    unsigned short bytes_req;

    if(StructSize == 0 || Count > 0xffff/StructSize)
    {
        assert(false);
        return -1;
    }

    bytes_req = StructSize * Count;

    *ppMem = malloc(bytes_req);

    if(*ppMem == NULL)
        return -1;
    else
        return 0;
}

If a program has custom memory allocation routines, it’s a fairly common error to not account for integer overflows, and a straightforward example like this one could be hidden within complicated code that makes sure you allocate only blocks of a certain size. Any time you see a multiplication operation conducted on an integer, ask yourself what happens if this causes the integer to wrap around.

Another interesting aspect of integer overflows is the fact that a pointer is just an unsigned integer containing a memory location. Pointer arithmetic is prone to exactly the same types of problems as we’ve outlined with other types of integer math. Any time someone is doing pointer arithmetic, check to be sure that there aren’t integer overflows. One thing to remember is that this is an area where the attackers are currently looking for problems. Simple string-based buffer overflows are getting more and more difficult to find in production code, and so the attackers are starting to look for more subtle types of errors.

void AllocMemory(size_t cbAllocSize) 
{
    //We don’t accommodate for trailing ’’
    cbAllocSize--;
    char *szData = malloc(cbAllocSize);
    ...
}

I shouldn’t have to repeat this as it should be common sense, but all function calls that return errors should be checked. If a function doesn’t return errors, it might be a good idea to test whether the operation really succeeded. A good example of this is checking a buffer after a strncpy to determine whether the string was truncated, as was detailed in Chapter 5. It is particularly critical to check the returns of critical security functions, such as impersonation functions like ImpersonateNamedPipeClient. Although it’s simple to check many functions for errors, some functions have trinary returns (three possible return values)—some of the sockets functions behave this way.

Consider the following code:

while(bytes = recv(sock, buf, len, 0))
    WriteFile(hFile, buf, bytes, &written, NULL);

What’s wrong with this picture? If you look at recv, you find that it typically returns 0 when there are no more bytes to read from a TCP connection. This assumes a graceful shutdown of the connection. If the connection aborts for some reason, bytes has just been set to -1 and WriteFile will attempt to write four gigabytes of memory into the file handle pointed to by hFile. Your application will throw an exception before that manages to happen, assuming you’re not running a 64-bit version of the operating system.

If you didn’t have enough problems already, there are a couple of functions where success just isn’t enough. Consider the AdjustTokenPrivileges function. The documentation helpfully states:

Now if all you wanted to do was adjust one privilege, you might think that the function would fail if it couldn’t adjust the only privilege it was asked to manipulate. Unfortunately, it will return TRUE, and you must call GetLastError to determine whether it actually adjusted the privilege properly. This is especially important when dropping privileges. The moral of the story is that if you’re not extremely familiar with an API call’s behavior, read the remarks section carefully—you might find some interesting bugs.

If you analyze most buffer overrun exploits, you’ll notice they involve overwriting a pointer to change the code execution flow. You should therefore double-check any code for buffer overruns if pointers are close by. This includes C++ classes with virtual methods, function pointers, linked lists, and so on. Of course, the easiest "pointer" to overwrite is a stack-based function return address.

Hopefully, we’ve hammered this point home in previous chapters, but there’s an interesting wrinkle that tends to bite people working with document types and network protocols. If you assume that the client (or the application that created the document) is benign (perhaps because it was created by your group), you might be leaving yourself open to attack. Here’s an example of the general problem—assume that you have a binary network protocol that sends data with the following structure:

struct blob
{
    DWORD Size;
    BYTE* Data;
};

Looks fairly simple, but there are a lot of possible problems here. An attacker could specify a size of up to 4 GB. If you allocate a buffer based on the Size member, be sure and check it for sanity. Second, an attacker could specify a size that is much smaller than the data. The client then starts reading data hoping to hit a delimiter (or simply the end of the data sent) and then overflows the buffer. This tends to be a bigger problem with network-supplied data than with documents, but documents can have problems also. The Size of a document could be larger than the data actually is—on a network this can lead to timeouts. Problems of this type have accounted for a variety of security bugs in Microsoft Office applications. The root of the problem was always that the document was assumed to be created by a benign client.

In this chapter, we’ve covered some areas that ought to be examined more closely when reviewing code for security bugs. You should consider using a more intensive, formal process for your riskiest code, and if you have to review a large application, use threat models and data flow diagrams to find the portions of the code that require the most attention. Integer overflows are an often-overlooked problem that the attackers consider to be a great new source of exploits—hopefully your code won’t give them any new attacks!