From 7a961fb9e6ffde8d9ffdcf87e9459f7c46233c85 Mon Sep 17 00:00:00 2001 From: arshiya tabasum Date: Fri, 16 Oct 2026 13:41:22 +0530 Subject: [PATCH 1/2] mod_dav: avoid signed overflow parsing the LOCK Timeout header dav_get_timeout_string() added an unbounded atol() of the Second- value to the current time, so a huge value overflowed time_t (undefined behaviour) and in practice wrapped to a lock which had already expired; a negative or malformed value likewise expired the lock immediately. Parse the seconds strictly, skip a value which is not understood, and cap it at the 2^32-1 maximum allowed by RFC 4918. --- changes-entries/dav-lock-timeout-overflow.txt | 3 + modules/dav/main/util.c | 17 +++- test/modules/dav/test_002_lock_timeout.py | 88 +++++++++++++++++++ 3 files changed, 104 insertions(+), 4 deletions(-) create mode 100644 changes-entries/dav-lock-timeout-overflow.txt create mode 100644 test/modules/dav/test_002_lock_timeout.py diff --git a/changes-entries/dav-lock-timeout-overflow.txt b/changes-entries/dav-lock-timeout-overflow.txt new file mode 100644 index 00000000000..3e2bb080461 --- /dev/null +++ b/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 ] diff --git a/modules/dav/main/util.c b/modules/dav/main/util.c index beacb9613ec..55251c8e93b 100644 --- a/modules/dav/main/util.c +++ b/modules/dav/main/util.c @@ -577,11 +577,20 @@ 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 - */ - expires = atol(val); + 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; + } + expires = (time_t)secs; now = time(NULL); return now + expires; } diff --git a/test/modules/dav/test_002_lock_timeout.py b/test/modules/dav/test_002_lock_timeout.py new file mode 100644 index 00000000000..d7e95b09508 --- /dev/null +++ b/test/modules/dav/test_002_lock_timeout.py @@ -0,0 +1,88 @@ +import glob +import os +import re +import pytest + +from pyhttpd.conf import HttpdConf + + +class TestLockTimeout: + + 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']: + 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}" + + Dav On + Require all granted + + """, + }) + 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'([^<]*)', 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, low, high", [ + ("a.txt", "Second-600", 590, 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", 4294967285, 4294967295), + # a malformed value is skipped in favour of the next TimeType + ("d.txt", "Second-abc, Second-60", 50, 60), + ]) + def test_dav_002_01(self, env, name, timeout, low, high): + 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") From 41975db10bc2dd1824f43a6de7223778e216da67 Mon Sep 17 00:00:00 2001 From: arshiya tabasum Date: Sun, 18 Oct 2026 00:45:25 +0530 Subject: [PATCH 2/2] mod_dav: also clamp the LOCK expiry to the range of time_t Capping the Timeout at the RFC 4918 maximum of 2^32-1 is not enough when time_t is a signed 32-bit type: now + timeout can still overflow. Clamp the seconds so that the absolute expiry fits in time_t, and add boundary tests for 2^31-1, 2^32-1 and 2^32. --- modules/dav/main/util.c | 12 +++++++++++- test/modules/dav/test_002_lock_timeout.py | 23 +++++++++++++++++------ 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/modules/dav/main/util.c b/modules/dav/main/util.c index 55251c8e93b..3493cd54f32 100644 --- a/modules/dav/main/util.c +++ b/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. @@ -590,8 +594,14 @@ DAV_DECLARE(time_t) dav_get_timeout_string(request_rec *r, 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. + */ + 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; - now = time(NULL); return now + expires; } } diff --git a/test/modules/dav/test_002_lock_timeout.py b/test/modules/dav/test_002_lock_timeout.py index d7e95b09508..852ef326f42 100644 --- a/test/modules/dav/test_002_lock_timeout.py +++ b/test/modules/dav/test_002_lock_timeout.py @@ -1,6 +1,7 @@ import glob import os import re +import time import pytest from pyhttpd.conf import HttpdConf @@ -18,7 +19,8 @@ class TestLockTimeout: 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']: + 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*')): @@ -63,15 +65,24 @@ def _timeout(r) -> str: return m.group(1) # an ordinary Second-xxx timeout is used as given, and the lock is held - @pytest.mark.parametrize("name, timeout, low, high", [ - ("a.txt", "Second-600", 590, 600), + @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", 4294967285, 4294967295), + ("b.txt", "Second-9223372036854775807", 4294967295), # a malformed value is skipped in favour of the next TimeType - ("d.txt", "Second-abc, Second-60", 50, 60), + ("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, low, high): + 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))