Buffer overruns - keeping the inside in
Ah, another “Patch Tuesday” or “Update Tuesday” as we are supposed to call it. Patches have traditionally been replacements for only part of files and we typically replace multiple files.
So, last blog, I wittered on about why buffer overflows were a bad thing. Of course, you already knew that they were a bad thing but you might not have been that familiar with legacy exploit techniques. The new techniques are a bit different but they still require overwrites of the stack or heap and avoidance is the same either way.
So, let us consider the example that we had before:
MyFunc(int iStream, char* buffer)
{
char szLocalBuffer[100];
strcpy(szLocalBuffer, buffer);
…
}
This code is clearly an accident waiting to happen since we are doing an unbounded copy. One obvious improvement would be to use strncpy which only copies a certain number of characters. Here is one version – it is polymorphic but this is the simplest.
char *strncpy( char *strDest, const char *strSource, size_t count );
size_t is an unsigned integer which doesn’t really matter to us since we are going to have a constant in there.
Now, the more eagle eyed among you may have spotted that MSDN says that you should use strncpy_s instead of strncpy but there is a lot of legacy code and legacy coders out there so let us pass that by for a moment.
So, the code becomes:
strncpy(szLocalBuffer, buffer, 100);
Much better. Safe? No. What if I pass in a string with no null at the end or a null after the 100th position? Well, then Buffer has no null at the end. That could cause later code to walk off the end of the buffer. It might AV. It might alter memory including the return address. Best to make sure that there is a null on the end. I would make sure that there was one – and there are a number of ways of doing so. However, this highlights a potential problem. I said that the string should not be more than 100 characters. I didn’t say if that included the null or if the string could contain nulls. Sloppy specification can lead to all manner of errors including security holes.
Let us imagine that we have specified that the string must be no longer than 100 ANSI characters and null terminated taking the total length to 101 – before we didn’t specify if we were talking about 1 or 2 byte characters and that matters rather a lot.
So, next try.
MyFunc(int iStream, char* buffer)
{
char szLocalBuffer[101] = { 0 }; // Set the buf to all nulls
strncpy(szLocalBuffer, buffer, 100);
…
}
The buffer may contain nonsense but it is nonsense in the buffer and not sprawled over the stack. However, the 100 character limit is a bit arbitrary. Typically you would want a variable length string but to be sensible, we should limit the buffer a bit. If we allow a buffer of 1 GB then it won’t be hard to take down the process. 32K (including the null) is plenty. Assume that we are talking 32 bit here. So, Take 3.
MyFunc(int iStream, char* buffer, int iBufLen)
{
if (iBuflen> 32767)
{
// handle error somehow
…
}
// let us assume that we have allocated 32K byte on the heap.
// It won’t fragment memory badly and we won’t keep it for that long
szLocalBuffer[iBuflen]=0; // null at the end whatever
strncpy(szLocalBuffer, buffer, iBuflen);
…
}
That looks pretty safe, doesn’t it? Exercise for the reader. What happens when I tell the routine that my string is 0xFFFFFFFF bytes long?
Answers next post unless someone beats me to it.