Maybe making the constants unsigned?

On Sat, Aug 7, 2021 at 2:14 PM Eric Covener <[email protected]> wrote:
>
> Seems like the fuzzer got farther but is still failing
>
>
> util.c:2713:26: runtime error: signed integer overflow:
> 9999999999999999 * 1000 cannot be represented in type 'long'
> #0 0x4be247 in ap_timeout_parameter_parse /src/httpd/server/util.c:2713:26
>
>     case 'M':
>         switch (*(++time_str)) {
>         /* Time is in milliseconds */
>         case 's':
>         case 'S':
>             check = tout * 1000;
>             break;
>
>
>
> Does this require a cast?
>
> On Sat, Aug 7, 2021 at 6:45 AM Eric Covener <[email protected]> wrote:
> >
> > On Sat, Aug 7, 2021 at 3:00 AM Christophe JAILLET
> > <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > Le 06/08/2021 à 15:10, [email protected] a écrit :
> > > > Author: covener
> > > > Date: Fri Aug  6 13:10:45 2021
> > > > New Revision: 1892038
> > > >
> > > > URL: http://svn.apache.org/viewvc?rev=1892038&view=rev
> > > > Log:
> > > > fix int overflow in ap_timeout_parameter_parse
> > > >
> > > > signed integer overflow in ap_timeout_parameter_parse under fuzzing
> > > >
> > > > Modified:
> > > >      httpd/httpd/trunk/server/util.c
> > > >
> > > > Modified: httpd/httpd/trunk/server/util.c
> > > > URL: 
> > > > http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util.c?rev=1892038&r1=1892037&r2=1892038&view=diff
> > > > ==============================================================================
> > > > --- httpd/httpd/trunk/server/util.c (original)
> > > > +++ httpd/httpd/trunk/server/util.c Fri Aug  6 13:10:45 2021
> > > > @@ -2676,6 +2676,7 @@ AP_DECLARE(apr_status_t) ap_timeout_para
> > > >       char *endp;
> > > >       const char *time_str;
> > > >       apr_int64_t tout;
> > > > +    apr_uint64_t check;
> > > >
> > > >       tout = apr_strtoi64(timeout_parameter, &endp, 10);
> > > >       if (errno) {
> > > > @@ -2688,16 +2689,20 @@ AP_DECLARE(apr_status_t) ap_timeout_para
> > > >           time_str = endp;
> > > >       }
> > > >
> > > > +    if (tout < 0) {
> > > > +        return APR_ERANGE;
> > > > +    }
> > > > +
> > > >       switch (*time_str) {
> > > >           /* Time is in seconds */
> > > >       case 's':
> > > >       case 'S':
> > > > -        *timeout = (apr_interval_time_t) apr_time_from_sec(tout);
> > > > +        check = apr_time_from_sec(tout);
> > > >           break;
> > > >       case 'h':
> > > >       case 'H':
> > > >           /* Time is in hours */
> > > > -        *timeout = (apr_interval_time_t) apr_time_from_sec(tout * 
> > > > 3600);
> > > > +        check = apr_time_from_sec(tout * 3600);
> > > >           break;
> > > >       case 'm':
> > > >       case 'M':
> > > > @@ -2705,12 +2710,12 @@ AP_DECLARE(apr_status_t) ap_timeout_para
> > > >           /* Time is in milliseconds */
> > > >           case 's':
> > > >           case 'S':
> > > > -            *timeout = (apr_interval_time_t) tout * 1000;
> > > > +            check = tout * 1000;
> > > >               break;
> > > >           /* Time is in minutes */
> > > >           case 'i':
> > > >           case 'I':
> > > > -            *timeout = (apr_interval_time_t) apr_time_from_sec(tout * 
> > > > 60);
> > > > +            check = apr_time_from_sec(tout * 60);
> > > >               break;
> > > >           default:
> > > >               return APR_EGENERAL;
> > > > @@ -2719,6 +2724,10 @@ AP_DECLARE(apr_status_t) ap_timeout_para
> > > >       default:
> > > >           return APR_EGENERAL;
> > > >       }
> > > > +    if (check > APR_INT64_MAX || check < 0) {
> > >
> > > why "|| check < 0"?
> > > 'check' is unsigned (i.e. apr_uint64_t).
> > >
> >
> > ty, changed in 1892063.
>
>
>
> --
> Eric Covener
> [email protected]



-- 
Eric Covener
[email protected]

Reply via email to