Repository navigation
mod_dav: Fix dav_get_timeout_string() overflowing an attacker-control… - #920
Open
arturobernalg wants to merge 1 commit into
Open
arturobernalg wants to merge 1 commit into
arturobernalg wants to merge 1 commit into
Conversation
…led Timeout value. dav_get_timeout_string() parses a "Timeout: Second-<N>" 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 "<D:timeout>Second-0</D:timeout>" before the fix (atol() saturates to LONG_MAX, and now + LONG_MAX overflows time_t to a large negative value), and "<D:timeout>Second-4294967295</D:timeout>" 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. PR 65788
arturobernalg
force-pushed
the
fix/httpd-65788-dav-timeout-overflow
branch
from
November 1, 2026 18:36
c3ed751 to
2b29637
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
dav_get_timeout_string() (modules/dav/main/util.c) 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 "<D:timeout>Second-0</D:timeout>" before the fix (atol() saturates to LONG_MAX on this platform, and now + LONG_MAX overflows time_t to a large negative value), and "<D:timeout>Second-4294967295</D:timeout>" after it -- the RFC 4918 TimeOut maximum, matching the overflow handling the existing code comment already called for ("We need to handle overflow better: timeout will be <= 2^32 - 1") 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") also clamps to 0, matching what atol() already silently did for that input on trunk -- not Infinite, which would grant more than before instead of less (verified: trunk's actual current behavior for "Second-notanumber" is "Second-0", not "Infinite" as an earlier version of this PR incorrectly claimed).
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.
Originally reported as BZ 65788 by Eric Covener, citing a finding from GHSL team member @antonio-morales.
PR 65788