From b26dec783da466684a3945c0ee69fcfd04bb3440 Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Tue, 3 Nov 2026 12:21:48 +0100 Subject: [PATCH] mod_dav: Fix dav_get_timeout_string() overflowing an attacker-controlled Timeout value. dav_get_timeout_string() parses a "Timeout: Second-" header with atol(), which has no overflow detection. A long enough digit string overflows it, and the later "now + expires" addition overflows time_t too, so the lock ends up stored with a nonsensical (already-expired) timeout instead of the large-but-finite one the client asked for -- a LOCK request that looks successful (200, a valid Lock-Token) silently grants a lock that is already expired. Confirmed with a real build: a LOCK request with "Timeout: Second-99999999999999999999999999999999" got back "Second-0" before the fix (atol() saturates to LONG_MAX, and now + LONG_MAX overflows time_t to a large negative value), and "Second-4294967295" after it -- the RFC 4918 TimeOut maximum, matching the overflow handling the existing code comment already called for but never implemented. Fix: parse with apr_strtoi64() (overflow-checked via errno), and clamp to 2^32 - 1 on overflow or on an in-range value above that maximum. A negative value (e.g. "Second--5") clamps to 0. A value with no digits at all (e.g. "Second-notanumber") clamps to 0 too, matching what atol() already silently did for that input -- not Infinite, which would grant more than before instead of less. The RFC 4918 maximum does not always fit a time_t: on platforms where time_t is 32 bits (still the glibc default for 32-bit Linux without _TIME_BITS=64, and standard on other 32-bit/legacy targets), casting 2^32 - 1 to time_t wraps to -1, and "now - 1" reintroduces the exact bug this change fixes, just through the clamp instead of atol(). Added DAV_TIME_T_MAX, computed from sizeof(time_t) rather than assumed, and cap "now + sec" against it regardless of the platform's time_t width. Verified under both a 64-bit and a 32-bit (-m32) build: the 64-bit behavior is unchanged, and the 32-bit cast no longer wraps negative. Per review, the current-time source now uses apr_time_t/apr_time_now() for the internal computation instead of time(NULL), narrowing to time_t only at the return (locks are still serialized as time_t on disk, unchanged). Re-verified all four cases (overflow, in-range, non-numeric, absent header) against a real build after this change. PR 65788 --- changes-entries/65788.txt | 4 + modules/dav/main/util.c | 55 ++++++++++-- .../dav/test_004_lock_timeout_overflow.py | 86 +++++++++++++++++++ 3 files changed, 139 insertions(+), 6 deletions(-) create mode 100644 changes-entries/65788.txt create mode 100644 test/modules/dav/test_004_lock_timeout_overflow.py diff --git a/changes-entries/65788.txt b/changes-entries/65788.txt new file mode 100644 index 00000000000..e9190c7b5cf --- /dev/null +++ b/changes-entries/65788.txt @@ -0,0 +1,4 @@ + *) mod_dav: Fix handling of invalid or overflowing "Timeout:" + request headers, which could silently grant a lock with an + already-expired timeout instead of the RFC 4918 maximum. + PR 65788 [Arturo Bernal] diff --git a/modules/dav/main/util.c b/modules/dav/main/util.c index 2a5b8b91b24..4c7e3e44a99 100644 --- a/modules/dav/main/util.c +++ b/modules/dav/main/util.c @@ -21,6 +21,7 @@ #include "apr_strings.h" #include "apr_lib.h" +#include "apr_time.h" #define APR_WANT_STRFUNC #include "apr_want.h" @@ -531,6 +532,10 @@ DAV_DECLARE(void) dav_xmlns_generate(dav_xmlns_info *xi, ** */ +/* largest value this platform's (signed) time_t can hold */ +#define DAV_TIME_T_MAX \ + ((time_t) (((apr_uint64_t) 1 << (sizeof(time_t) * 8 - 1)) - 1)) + /* dav_get_timeout: If the Timeout: header exists, return a time_t * when this lock is expected to expire. Otherwise, return * a time_t of DAV_TIMEOUT_INFINITE. @@ -560,7 +565,7 @@ DAV_DECLARE(time_t) dav_get_timeout(request_rec *r) DAV_DECLARE(time_t) dav_get_timeout_string(request_rec *r, const char *timeout_const) { - time_t now, expires = DAV_TIMEOUT_INFINITE; + apr_time_t now, expires = DAV_TIMEOUT_INFINITE; const char *timeout = apr_pstrdup(r->pool, timeout_const), *val; @@ -577,13 +582,51 @@ DAV_DECLARE(time_t) dav_get_timeout_string(request_rec *r, } if (!strncmp(val, "Second-", 7)) { + char *endp; + apr_int64_t sec; + val += 7; - /* ### We need to handle overflow better: - * ### timeout will be <= 2^32 - 1 + + /* atol() has no overflow detection: a long enough digit + * string overflows it, and "now + expires" below can then + * overflow time_t too, yielding a nonsensical (e.g. + * negative, already-expired) stored timeout instead of + * the large-but-finite one the client asked for. Use an + * overflow-checked parse, and clamp to the RFC 4918 + * TimeOut maximum of 2^32 - 1 instead of propagating the + * overflow. + */ + errno = 0; + sec = apr_strtoi64(val, &endp, 10); + if (endp == val) { + /* not a number at all -- same as atol(), treat as 0 + * (an immediately-expired lock), not as Infinite + */ + sec = 0; + } + else if (errno == ERANGE || sec > APR_UINT32_MAX) { + sec = APR_UINT32_MAX; + } + else if (sec < 0) { + sec = 0; + } + + now = apr_time_sec(apr_time_now()); + + /* The RFC 4918 maximum of 2^32 - 1 does not necessarily + * fit a time_t on platforms where time_t is 32 bits. + * Cap "sec" so "now + sec" cannot itself overflow time_t + * -- undefined behavior for a signed type, and in + * practice a silent wraparound to an already-expired + * value, i.e. exactly the bug this change fixes, + * reintroduced through the cast below. */ - expires = atol(val); - now = time(NULL); - return now + expires; + if (sec > (apr_int64_t) (DAV_TIME_T_MAX - now)) { + sec = (apr_int64_t) (DAV_TIME_T_MAX - now); + } + + expires = sec; + return (time_t) (now + expires); } } diff --git a/test/modules/dav/test_004_lock_timeout_overflow.py b/test/modules/dav/test_004_lock_timeout_overflow.py new file mode 100644 index 00000000000..600a44b3d31 --- /dev/null +++ b/test/modules/dav/test_004_lock_timeout_overflow.py @@ -0,0 +1,86 @@ +import os +import re + +import pytest + +from pyhttpd.conf import HttpdConf + +LOCK_BODY = ( + '' + '' + '' + '' + '' +) + + +# BZ 65788: dav_get_timeout_string() in modules/dav/main/util.c parsed a +# "Timeout: Second-" header with atol(), which has no overflow +# detection. A large enough digit string overflows it, and the later +# "now + expires" addition overflows time_t too, so the lock ends up +# stored with a nonsensical (already-expired) timeout instead of the +# large-but-finite one the client asked for -- a LOCK request that looks +# successful (200, a valid Lock-Token) silently grants a lock that is +# already expired. +class TestLockTimeoutOverflow: + + @pytest.fixture(autouse=True, scope='class') + def _class_scope(self, env): + dav_dir = os.path.join(env.gen_dir, 'dav-timeout-overflow') + os.makedirs(dav_dir, exist_ok=True) + for name in ('file1.txt', 'file2.txt', 'file3.txt'): + with open(os.path.join(dav_dir, name), 'w') as fd: + fd.write('hello\n') + + conf = HttpdConf(env, extras={ + 'base': f""" + DavLockDB "{env.gen_dir}/davlock-timeout-overflow" + """, + f"test1.{env.http_tld}": f""" + Alias /dav-timeout-overflow "{dav_dir}" + + Dav On + Require all granted + + """, + }) + conf.add_vhost_test1() + conf.install() + assert env.apache_restart() == 0 + + def _lock(self, env, path, timeout_hdr): + url = env.mkurl("https", "test1", path) + return env.curl_raw(url, options=[ + '-X', 'LOCK', '-H', f'Timeout: {timeout_hdr}', + '-H', 'Content-Type: text/xml', '--data-binary', LOCK_BODY]) + + # an overflowing Second- value must not silently collapse to an + # already-expired (or otherwise nonsensical) timeout; it is clamped + # to the RFC 4918 TimeOut maximum of 2^32 - 1 instead + def test_dav_004_01(self, env): + r = self._lock(env, "/dav-timeout-overflow/file1.txt", + "Second-99999999999999999999999999999999") + assert r.response["status"] == 200, r.response + m = re.search(r'([^<]*)', r.stdout) + assert m, f"no in response: {r.stdout}" + assert m.group(1) == "Second-4294967295", \ + f"timeout: {m.group(1)}" + + # an ordinary, in-range timeout request is unaffected + def test_dav_004_02(self, env): + r = self._lock(env, "/dav-timeout-overflow/file2.txt", "Second-600") + assert r.response["status"] == 200, r.response + m = re.search(r'([^<]*)', r.stdout) + assert m, f"no in response: {r.stdout}" + assert m.group(1) == "Second-600", f"timeout: {m.group(1)}" + + # a Second- value with no digits at all parses the same way atol() + # already did (0, not an error), same as before this fix: an + # immediately-expired lock, not Infinite + def test_dav_004_03(self, env): + r = self._lock(env, "/dav-timeout-overflow/file3.txt", + "Second-notanumber") + assert r.response["status"] == 200, r.response + m = re.search(r'([^<]*)', r.stdout) + assert m, f"no in response: {r.stdout}" + assert m.group(1) == "Second-0", f"timeout: {m.group(1)}"