Potential integer overflow in common.c and ids_config.c #557

Hi,

My static analysis tool, Svace, detected a potential integer overflow in the time_string_to_seconds() function in common.c and in the block_address_time_parser() function in ids_config.c/

Potential problem

The value of i is obtained from the untrusted time_string argument using strtol():

i = strtol(time_string, &end, 10);

Although strtol() detects overflow during the string-to-integer conversion, the resulting value is subsequently multiplied by a time unit without checking whether the multiplication itself can overflow:

case 'm':
i *= MINUTES;
break;

case 'h':
i *= HOURS;
break;

case 'd':
i *= DAYS;
break;

case 'M':
i *= MONTHS;
break;

For sufficiently large or small input values, these operations may exceed the range of the long type and result in signed integer overflow or underflow.

The value is checked later by the caller, for example:

if (i > 40 * DAYS) {
...
}

However, this check is performed after time_string_to_seconds() has already performed the multiplication, so it cannot prevent an overflow occurring inside the function.

Possible solution

The value should be checked against the limits of long before performing the multiplication:

case 'm':
if (i > LONG_MAX / MINUTES || i < LONG_MIN / MINUTES)
return -1;
i *= MINUTES;
break;

case 'h':
if (i > LONG_MAX / HOURS || i < LONG_MIN / HOURS)
return -1;
i *= HOURS;
break;

case 'd':
if (i > LONG_MAX / DAYS || i < LONG_MIN / DAYS)
return -1;
i *= DAYS;
break;

case 'M':
if (i > LONG_MAX / MONTHS || i < LONG_MIN / MONTHS)
return -1;
i *= MONTHS;
break;

This ensures that the multiplication is only performed when the resulting value can be represented by long.

Author T. Mishin