Why with so many Win32 programmers out there do we have a lack of Win32 secure programming checklists? Beats me, but it's time to fill the void. Here are 15 do's and don'ts for keeping your Win32 programming secure.
1. Beware of Dangerous Functions
Certain function calls are simply dangerous. Incorrect use of these functions may lead to buffer-overruns (see my previous "Best Defense" article, Testing for Buffer Overruns), which if you're lucky will crash only your application and if you're unlucky may cause code to be injected into your process and executed. Here are some of the most common functions to look out for:
- strcpy (and variants such as lstrcpy, wcscpy, etc.)
- strcat (and variants such as lstrcat, wcscat, etc.)
- memcpy (and variants such as CopyMemory, _memccpy, bcopy, etc.)
- gets (and to a lesser extent, fgets)
- sprintf (and variants such as swprintf, vsprintf, etc.)
- scanf (and variants such as sscanf, swscanf, fscanf, etc.)
Analyze all calls to these functions carefully to make sure they are checking the boundary conditions correctly. A number of programs have gone as far as banning the use of these functions altogether, classifying their use as code defects or bugs.
The problem with functions 1 and 2 is that they copy data until the functions hit a null character, leaving them vulnerable to attack. An attacker could leave off the null or place it in a strategic position beyond the end of the buffer. At the very least, you should replace calls to these functions with the 'n' versions. So, rather than strcpy() use strncpy(), and rather than strcat() use strncat(). For example:
#define MAX_BUFF 80
char szBuff[MAX_BUFF];
strcpy(szBuff,szArg);
becomes
#define MAX_BUFF 80
char szBuff[MAX_BUFF];
strncpy(szBuff,szArg,MAX_BUFF);
Worried about what this might do to performance? See my Note from the Trenches.
Function number 4, the gets() function, is so dangerous that you should remove it from your code immediately. It copies user input directly into a buffer once it reaches the end of a line. There is no way to tell the function how much it should copy. Recently, I did a design and code audit of a security product and found instances of gets() in test code (luckily, not the core product!). We fixed the code by replacing gets() with ReadConsole().
Watch Out for Those Stack-Based Buffers
Be especially careful when you call any of the dangerous functions to copy data into stack-based buffers. Generally speaking, executing malicious code is much easier on a system where the buffer is allocated on the stack than where memory is allocated on the heap. For example, the following C/C++ code allocates a 64-byte buffer on the stack:
void foo() {
char buff[64];
}
While this code allocates code on the heap:
void foo() {
char *buff = malloc(64);
}
Also, be careful with _alloca(). It looks and feels like malloc(), but it allocates memory on the stack. Two string classes in particular, the Microsoft Foundation Classes (MFC) CString class and the Standard Template Library (STL) string class, are a little safer because they allocate data on the heap.