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..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. @@ -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; } } 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..852ef326f42 --- /dev/null +++ b/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 = ('' + '' + '' + '' + '') + + @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}" + + 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, 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")