Posts Tagged 'Coding'

Do Not Detect Overflow With Overflow

Credits to a gweilo for the sharing below.

Integer overflow and underflow manifest themselves as vulnerabilities. Here is an overflow bug fired by Sir BugFinder. I assigned our fictional developer Sir FastFix ownership of the bug, and he jumped into the code straight.

First, look at this problematic pseudo-code snippet below :

SWORD param = 0;

while ( flag )
{
	param ++ ;
	//
	//manipulate the flag value...
	//
}

buffer = malloc(sizeof(BYTE) * param);
...

The param can increase definitely. No good. Sir FastFix quickly identifies the problem and sends me this code review below.

// sirfastfix: now uses unsigned.
UWORD param = 0;

while ( flag )
{
	// sirfastfix: code fix for overflow bug.
	if ( param > param + 1 )
	{
		TRACE_ERROR("Overflow occurred at param\n");
		return E_UNEXPECTED;
	}
	param ++ ;
	//
	//manipulate the flag value...
	//
}

buffer = malloc(sizeof(BYTE) * param);
...

Now, I have to review it. Let’s look at the changes.

  1. param is now checked with the condition (param > param + 1). Since it must be false, an overflow must have occurred if it is true. Intuitive.
  2. param is now unsigned using UWORD, and not signed SWORD. I find no reasons for negative buffers. A good move.

But, something smells stinky. Let’s think again.

  1. Why not use well-defined constants like MAX_INT, MAX_SHORT or MAX_LONG constants to check before incrementing param? Like MAX_INT – a < b ?
  2. Why the code to detect overflow is using yet another overflow to check?

Sir FastFix, I am not approving this code check-in. This fix is not going in anywhere into the source tree. Who knows what this overflow to check overflow can result in? Let’s write more solid and not college quality code, and not rushing to resolve the bug.