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]
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_sec(tout * 60);
break;
default:
@@ -2724,12 +2730,11 @@ AP_DECLARE(apr_status_t) ap_timeout_parameter_pars
default:
return APR_EGENERAL;
}
- if (check > APR_INT64_MAX || check < tout) {
- return APR_ERANGE;
- }
- *timeout = (apr_interval_time_t) check;
+
+ *timeout = (apr_interval_time_t)check;
return APR_SUCCESS;
}
+#undef CHECK_OVERFLOW
AP_DECLARE(int) ap_parse_strict_length(apr_off_t *len, const char *str)
{
