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)}"