Re: svn commit: r1892038 - /httpd/httpd/trunk/server/util.c

[Christophe JAILLET]

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;
  }