Re: svn commit: r1892038 - /httpd/httpd/trunk/server/util.c
On Tue, Aug 10, 2021 at 2:52 PM Christophe JAILLET wrote: > > Le 08/08/2021 à 23:28, Eric Covener a écrit : > > 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? > > Now tested on my side as well and pushed as r1892185. > > CJ ty, just proposed.
Re: svn commit: r1892038 - /httpd/httpd/trunk/server/util.c
Le 08/08/2021 à 23:28, Eric Covener a écrit : 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? Now tested on my side as well and pushed as r1892185. CJ On Sun, Aug 8, 2021 at 5:03 PM Christophe JAILLET 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
Re: svn commit: r1892038 - /httpd/httpd/trunk/server/util.c
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 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 wrote: > >> > >> Seems like the fuzzer got farther but is still failing > >> > >> > >> util.c:2713:26: runtime error: signed integer overflow: > >> * 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 wrote: > >>> > >>> On Sat, Aug 7, 2021 at 3:00 AM Christophe JAILLET > >>> wrote: > > Hi, > > Le 06/08/2021 à 15:10, cove...@apache.org 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 > >> cove...@gmail.com > > > > > > > -- Eric Covener cove...@gmail.com
Re: svn commit: r1892038 - /httpd/httpd/trunk/server/util.c
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 wrote: Seems like the fuzzer got farther but is still failing util.c:2713:26: runtime error: signed integer overflow: * 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 wrote: On Sat, Aug 7, 2021 at 3:00 AM Christophe JAILLET wrote: Hi, Le 06/08/2021 à 15:10, cove...@apache.org 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 cove...@gmail.com Index: server/util.c === --- server/util.c (révision 1892087) +++ server/util.c (copie de travail) @@ -2668,6 +2669,7 @@ AP_DECLARE(char *) ap_append_pid(apr_pool_t *p, co * in timeout_parameter. * @return Status value indicating whether the parsing was successful or not. */ +#define CHECK_OVERFLOW(a, b) if (a > b) return APR_ERANGE AP_DECLARE(apr_status_t) ap_timeout_parameter_parse( const char *timeout_parameter, apr_interval_time_t *timeout, @@ -2697,11 +2699,13 @@ AP_DECLARE(apr_status_t) ap_timeout_parameter_pars /* Time is in seconds */ case 's': case 'S': +CHECK_OVERFLOW(tout, apr_time_sec(APR_INT64_MAX)); check = apr_time_from_sec(tout); break; +/* Time is in hours */ case 'h': case 'H': -/* Time is in hours */ +CHECK_OVERFLOW(tout, apr_time_sec(APR_INT64_MAX / 3600)); check = apr_time_from_sec(tout * 3600); break; case 'm': @@ -2710,11 +2714,13 @@ AP_DECLARE(apr_status_t) ap_timeout_parameter_pars /* Time is in milliseconds */ case 's': case 'S': -check = tout * 1000; +CHECK_OVERFLOW(tout, apr_time_as_msec(APR_INT64_MAX)); +check = apr_time_from_msec(tout); break; /* Time is in minutes */ case 'i': case 'I': +CHECK_OVERFLOW(tout, apr_time_sec(APR_INT64_MAX / 60)); check = apr_time_from_s
Re: svn commit: r1892038 - /httpd/httpd/trunk/server/util.c
Maybe making the constants unsigned? On Sat, Aug 7, 2021 at 2:14 PM Eric Covener wrote: > > Seems like the fuzzer got farther but is still failing > > > util.c:2713:26: runtime error: signed integer overflow: > * 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 wrote: > > > > On Sat, Aug 7, 2021 at 3:00 AM Christophe JAILLET > > wrote: > > > > > > Hi, > > > > > > Le 06/08/2021 à 15:10, cove...@apache.org 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 > cove...@gmail.com -- Eric Covener cove...@gmail.com
Re: svn commit: r1892038 - /httpd/httpd/trunk/server/util.c
Seems like the fuzzer got farther but is still failing util.c:2713:26: runtime error: signed integer overflow: * 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 wrote: > > On Sat, Aug 7, 2021 at 3:00 AM Christophe JAILLET > wrote: > > > > Hi, > > > > Le 06/08/2021 à 15:10, cove...@apache.org 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 cove...@gmail.com
Re: svn commit: r1892038 - /httpd/httpd/trunk/server/util.c
On Sat, Aug 7, 2021 at 3:00 AM Christophe JAILLET wrote: > > Hi, > > Le 06/08/2021 à 15:10, cove...@apache.org 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.
Re: svn commit: r1892038 - /httpd/httpd/trunk/server/util.c
Hi, Le 06/08/2021 à 15:10, cove...@apache.org 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). If paranoiac, and want to prevent overflow or wrap around after the multiplication, I think that we should compute the maximum acceptable value pour each case ('s', 'h', ...) and test against it. CJ +return APR_ERANGE; +} +*timeout = (apr_interval_time_t) check; return APR_SUCCESS; }