False positive errno test after call to strtol()

[Mike Rumph <mi...@oracle.com>]

The man page for strtol() indicate that the function can set errno to 
ERANGE (EINVAL is also possible for some environments).
But for the errno check to be valid errno should be set to 0 before the 
function call.
- http://linux.die.net/man/3/strtol

I've reviewed all cases of calls to strtol() in httpd and APR code.
In some cases no validation is performed after the call.
In most cases endptr (the second parameter) is checked against the 
beginning and/or ending of the string which does not guarantee against 
numeric overflow.
In some cases errno is checked for ERANGE.

I've attached a patch for the simplest case, where errno is checked but 
was not set to 0 before the call.

I will consider working up a more extensive patch, if it is desired.

BTW, this discussion is not purely theoretical.
Erroneous "Invalid ThreadStackSize value: " messages have been witnessed 
in HP-UX environments.

Thanks,

Mike Rumph

Re: False positive errno test after call to strtol()

[Mike Rumph <mi...@oracle.com>]

Thanks Jeff,

Here is an example with no validation:
modules/aaa/mod_auth_digest.c (lines 980 - 982):
============================================
     if (resp->opaque) {
         resp->opaque_num = (unsigned long) strtol(resp->opaque, NULL, 16);
     }

Here is an example with limited validation:
server/log.c (lines 1590 - 1600):
===========================
     /* If we fill the buffer, we're probably reading a corrupt pid file.
      * To be nice, let's also ensure the first char is a digit. */
     if (bytes_read == 0 || bytes_read == BUFFER_SIZE - 1 || 
!apr_isdigit(*buf)) {
         return APR_EGENERAL;
     }

     buf[bytes_read] = '\0';
     *mypid = strtol(buf, &endptr, 10);


Here is a typical example of endptr validation:
modules/proxy/mod_proxy_connect.c (lines 91 - 108):
===============================================
     const char *p = arg;

     if (!apr_isdigit(arg[0]))
         return "AllowCONNECT: port numbers must be numeric";

     first = strtol(p, &endptr, 10);
     if (*endptr == '-') {
         p = endptr + 1;
         last = strtol(p, &endptr, 10);
     }
     else {
         last = first;
     }

     if (endptr == p || *endptr != '\0')  {
         return apr_psprintf(parms->temp_pool,
                             "Cannot parse '%s' as port number", p);
     }

(There is no ERANGE checking for numeric overflow.)
(Also none of the calls to strtol() in Apache httpd and APR check for 
EINVAL.)

If I were to work on a more extensive patch, I would consider each case 
to see if further validation would be warranted.

Take care,

Mike


On 11/15/2013 9:38 AM, Jeff Trawick wrote:
> On Thu, Nov 14, 2013 at 4:11 PM, Mike Rumph <mike.rumph@oracle.com 
> <ma...@oracle.com>> wrote:
>
>     The man page for strtol() indicate that the function can set errno
>     to ERANGE (EINVAL is also possible for some environments).
>     But for the errno check to be valid errno should be set to 0
>     before the function call.
>     - http://linux.die.net/man/3/strtol
>
>     I've reviewed all cases of calls to strtol() in httpd and APR code.
>     In some cases no validation is performed after the call.
>     In most cases endptr (the second parameter) is checked against the
>     beginning and/or ending of the string which does not guarantee
>     against numeric overflow.
>     In some cases errno is checked for ERANGE.
>
>     I've attached a patch for the simplest case, where errno is
>     checked but was not set to 0 before the call.
>
>
> committed to trunk as r1542338; I'll propose for backport to the 2.4.x 
> branch
>
>
>     I will consider working up a more extensive patch, if it is desired.
>
>
> I suggest posting a couple of examples of what you found first.
>
>
>     BTW, this discussion is not purely theoretical.
>     Erroneous "Invalid ThreadStackSize value: " messages have been
>     witnessed in HP-UX environments.
>
>     Thanks,
>
>     Mike Rumph
>
>
>
>
> -- 
> Born in Roswell... married an alien...
> http://emptyhammock.com/

Re: False positive errno test after call to strtol()

[Jeff Trawick <tr...@gmail.com>]

On Thu, Nov 14, 2013 at 4:11 PM, Mike Rumph <mi...@oracle.com> wrote:

> The man page for strtol() indicate that the function can set errno to
> ERANGE (EINVAL is also possible for some environments).
> But for the errno check to be valid errno should be set to 0 before the
> function call.
> - http://linux.die.net/man/3/strtol
>
> I've reviewed all cases of calls to strtol() in httpd and APR code.
> In some cases no validation is performed after the call.
> In most cases endptr (the second parameter) is checked against the
> beginning and/or ending of the string which does not guarantee against
> numeric overflow.
> In some cases errno is checked for ERANGE.
>
> I've attached a patch for the simplest case, where errno is checked but
> was not set to 0 before the call.
>

committed to trunk as r1542338; I'll propose for backport to the 2.4.x
branch


>
> I will consider working up a more extensive patch, if it is desired.
>

I suggest posting a couple of examples of what you found first.


>
> BTW, this discussion is not purely theoretical.
> Erroneous "Invalid ThreadStackSize value: " messages have been witnessed
> in HP-UX environments.
>
> Thanks,
>
> Mike Rumph
>



-- 
Born in Roswell... married an alien...
http://emptyhammock.com/