mod_dav: avoid signed overflow parsing the Timeout header

changes-entries/dav-lock-timeout-overflow.txt

@@ -0,0 +1,3 @@
+  *) mod_dav: Cap a LOCK request's Timeout at 2^32-1 seconds and ignore a
+     negative or malformed Second- value, rather than creating a lock which
+     has already expired.  [arshiya tabasum <arshi bugqore.com>]

modules/dav/main/util.c

@@ -549,6 +549,10 @@ DAV_DECLARE(time_t) dav_get_timeout(request_rec *r)
     return dav_get_timeout_string(r, timeout_const);
 }
 
+/* Largest value a signed time_t can hold, as an apr_int64_t. */
+#define DAV_TIME_T_MAX \
+    ((apr_int64_t)((((apr_uint64_t)1) << (sizeof(time_t) * 8 - 1)) - 1))
+
 /* dav_get_timeout_string:  From a Timeout header value, return a time_t
  *    when this lock is expected to expire.  Otherwise, return
  *    a time_t of DAV_TIMEOUT_INFINITE.
@@ -577,12 +581,27 @@ DAV_DECLARE(time_t) dav_get_timeout_string(request_rec *r,
         }
 
         if (!strncmp(val, "Second-", 7)) {
+            apr_int64_t secs;
+            char *end;
+
             val += 7;
-            /* ### We need to handle overflow better:
-             * ### timeout will be <= 2^32 - 1
+            secs = apr_strtoi64(val, &end, 10);
+            if (end == val || (*end && *end != ',') || secs < 0) {
+                /* not a valid Second-xxx value, try the next one */
+                continue;
+            }
+            /* RFC 4918: the timeout MUST NOT be greater than 2^32 - 1 */
+            if (secs > APR_UINT32_MAX) {
+                secs = APR_UINT32_MAX;
+            }
+            /* The absolute expiry must also fit in a (signed) time_t,
+             * which may be only 32 bits wide.
              */
-            expires = atol(val);
-            now     = time(NULL);
+            now = time(NULL);
+            if (secs > DAV_TIME_T_MAX - (apr_int64_t)now) {
+                secs = DAV_TIME_T_MAX - (apr_int64_t)now;
+            }
+            expires = (time_t)secs;
             return now + expires;
         }
     }

test/modules/dav/test_002_lock_timeout.py

@@ -0,0 +1,99 @@
+import glob
+import os
+import re
+import time
+import pytest
+
+from pyhttpd.conf import HttpdConf
+
+
+class TestLockTimeout:
+
+    LOCKINFO = ('<?xml version="1.0" encoding="utf-8"?>'
+                '<D:lockinfo xmlns:D="DAV:">'
+                '<D:lockscope><D:exclusive/></D:lockscope>'
+                '<D:locktype><D:write/></D:locktype>'
+                '</D:lockinfo>')
+
+    @pytest.fixture(autouse=True, scope='class')
+    def _class_scope(self, env):
+        dav_dir = os.path.join(env.gen_dir, 'dav-lock')
+        os.makedirs(dav_dir, exist_ok=True)
+        for name in ['a.txt', 'b.txt', 'c.txt', 'd.txt',
+                     'e.txt', 'f.txt', 'g.txt']:
+            with open(os.path.join(dav_dir, name), 'w') as fd:
+                fd.write('hello\n')
+        for fpath in glob.glob(os.path.join(env.gen_dir, 'davlock-timeout*')):
+            os.remove(fpath)
+        conf = HttpdConf(env, extras={
+            'base': f"""
+        DavLockDB "{env.gen_dir}/davlock-timeout"
+        """,
+            f"test1.{env.http_tld}": f"""
+        Alias /dav-lock "{dav_dir}"
+        <Directory "{dav_dir}">
+            Dav On
+            Require all granted
+        </Directory>
+        """,
+        })
+        conf.add_vhost_test1()
+        conf.install()
+        assert env.apache_restart() == 0
+
+    def _lock(self, env, name: str, timeout: str):
+        fpath = os.path.join(env.gen_dir, 'lockinfo.xml')
+        with open(fpath, 'w') as fd:
+            fd.write(self.LOCKINFO)
+        url = env.mkurl("https", "test1", f"/dav-lock/{name}")
+        return env.curl_raw(url, options=[
+            '-X', 'LOCK', '-H', f'Timeout: {timeout}',
+            '-H', 'Content-Type: text/xml', '--data-binary', f'@{fpath}'])
+
+    def _lock_refused(self, env, name: str):
+        # a second exclusive lock on a still locked resource is refused
+        r = self._lock(env, name, "Second-60")
+        env.httpd_error_log.ignore_recent(
+            matches=[r'.*Could not LOCK .* due to a failed precondition.*',
+                     r'.*Existing lock\(s\) on the requested resource.*'])
+        return r.response["status"] == 423
+
+    @staticmethod
+    def _timeout(r) -> str:
+        m = re.search(r'<D:timeout>([^<]*)</D:timeout>', r.stdout)
+        assert m, f'no timeout in LOCK response: {r.stdout}'
+        return m.group(1)
+
+    # an ordinary Second-xxx timeout is used as given, and the lock is held
+    @pytest.mark.parametrize("name, timeout, expect", [
+        ("a.txt", "Second-600", 600),
+        # too large to add to the current time: capped at the RFC 4918
+        # maximum of 2^32 - 1 rather than wrapping to an expired lock
+        ("b.txt", "Second-9223372036854775807", 4294967295),
+        # a malformed value is skipped in favour of the next TimeType
+        ("d.txt", "Second-abc, Second-60", 60),
+        # boundaries of the RFC 4918 maximum
+        ("e.txt", "Second-4294967295", 4294967295),
+        ("f.txt", "Second-4294967296", 4294967295),
+        # fits a signed 32-bit time_t, but now + timeout does not
+        ("g.txt", "Second-2147483647", 2147483647),
+    ])
+    def test_dav_002_01(self, env, name, timeout, expect):
+        # with a signed 32-bit time_t the expiry is further capped so
+        # that it still fits; accept either result
+        high = expect
+        low = min(expect, 2**31 - 1 - int(time.time())) - 10
+        r = self._lock(env, name, timeout)
+        assert r.response["status"] == 200
+        m = re.match(r'Second-(\d+)$', self._timeout(r))
+        assert m, self._timeout(r)
+        assert low <= int(m.group(1)) <= high
+        assert self._lock_refused(env, name)
+
+    # a negative timeout is not understood, so the lock is not
+    # created already expired
+    def test_dav_002_02(self, env):
+        r = self._lock(env, "c.txt", "Second--100")
+        assert r.response["status"] == 200
+        assert self._timeout(r) == "Infinite"
+        assert self._lock_refused(env, "c.txt")