Repository navigation
mod_md: Fix ARI renewal time selection and RFC 3339 offset parsing. - #969
Open
arturobernalg wants to merge 1 commit into
Open
arturobernalg wants to merge 1 commit into
arturobernalg wants to merge 1 commit into
Conversation
acme_get_ari() picked a random time within the CA's suggested ARI renewal window using an unsigned byte c (0-255), computed as (end - start) * (c - 128) / 256. Subtracting 128 turns half of the possible values negative, placing the selected time before the window's start, and the positive half only reaches up to 256/512 = 50% of the window, never close to its end. Use c directly (0-255) instead, so the offset is always in [0, window) as intended. A window where start == end was also accepted (only start > end was rejected), picking exactly 'start' every time rather than being treated as invalid; reject it too. md_time_parse_rfc3339() parsed a +HH:MM/-HH:MM timezone offset by first computing the hour component into tm_gmtoff, then overwriting it (should have added to it) with the minutes component. Any offset with a non-zero hour was silently reduced to just its minutes (e.g. +02:30 was read as +00:30; -05:00 was read as no offset at all), shifting the parsed start/end times by up to several hours. Verified for real: a standalone test linked directly against the real md_time.c reproduces both exact cases from the commit above (+02:30 misread as 11:30 UTC instead of 09:30 UTC; -05:00 misread as no offset at all instead of 17:00 UTC) against the pre-fix source, and all pass against the fix. The ARI offset formula was checked exhaustively (not sampled) over all 256 byte values: the old formula puts exactly 128/256 before the window start and never exceeds 49.6% of the window; the new one never goes negative and reaches up to 99.6% of the window. PR 70309
5 tasks done
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.
acme_get_ari() picked a random time within the CA's suggested ARI renewal window using an unsigned byte
c(0-255), computed as(end - start) * (c - 128) / 256. Subtracting 128 turns half of the possible values negative, placing the selected time before the window's start, and the positive half only reaches up to 50% of the window, never close to its end. Usecdirectly (0-255) instead, so the offset is always in[0, window)as intended.A window where
start == endwas also accepted (onlystart > endwas rejected), silently picking exactlystartevery time rather than being treated as invalid; reject it too.md_time_parse_rfc3339()parsed a"+HH:MM"/"-HH:MM"timezone offset by first computing the hour component intotm_gmtoff, then overwriting it (should have added to it) with the minutes component. Any offset with a non-zero hour was silently reduced to just its minutes (e.g."+02:30"was read as"+00:30";"-05:00"was read as no offset at all), shifting the parsed start/end times by up to several hours.Verified for real, not just by inspection:
md_time.c(not a reimplementation) reproduces both exact cases against the pre-fix source --"+02:30"misread as 11:30 UTC instead of 09:30 UTC;"-05:00"misread as no offset at all instead of 17:00 UTC -- and all cases pass against the fix.modules/md/state for a consistent compile) with no errors or warnings.BZ: https://bz.apache.org/bugzilla/show_bug.cgi?id=70309
This PR does not report or address a security vulnerability
(see
SECURITY.mdandhttps://www.apache.org/security/#reporting-a-vulnerability).
Code follows the httpd style guide.
New log messages (level debug or higher) use an empty
APLOGNO()tag;numbers are assigned by a committer at merge time (see
docs/log-message-tags/README).Not applicable -- no new log messages.
If the change is user-visible, a
changes-entries/*.txtfile is included,following the template in
README.CHANGES(not needed otherwise).Test cases based on pyhttpd are included where appropriate, in the
test/modules/xxxdirectory matching themodules/xxxmodule source,or
test/modules/corefor core server changes. On Unix, verify thesepass locally with e.g.
make check-pytest PYTEST_DIRS=test/modules/xxx.Not included:
acme_get_ari()isstaticand its ARI window logic would need a live ACME test CA (e.g. Pebble) driven through/set-renewal-info/, matching the reporter's own validation method, to exercise end-to-end -- more involved than this fix's size warrants on its own. Verified instead with the standalone, exhaustive/real-source tests described above, which directly exercise the exact faulty expressions.