I finally got unlazy and got a local build w/ the UBISAN checking and your patch works!
env CC=clang LD=clang CFLAGS=-fsanitize=undefined ./config.nice Can you commit to trunk? On Sun, Aug 8, 2021 at 5:03 PM Christophe JAILLET <[email protected]> wrote: > > Hi, > > attached, my proposal. (compile tested only, but you'll get the idea) > It applies on top of current trunk (so after 1892038,1892063). > > I'm not sure that the "(check > APR_INT64_MAX || check < tout)" check in > your patches is enough to catch overflows. > > CJ > > Le 07/08/2021 à 20:33, Eric Covener a écrit : > > 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]
