Repository navigation
mod_md: Fix a one-second race in md_asn1_time_get(). - #838
Open
arturobernalg wants to merge 1 commit into
Open
arturobernalg wants to merge 1 commit into
arturobernalg wants to merge 1 commit into
Conversation
* modules/md/md_crypt.c (md_asn1_time_get): For OpenSSL >= 1.1.1, convert the certificate's ASN1_TIME directly via ASN1_TIME_to_tm() instead of reconstructing it as apr_time_now() + ASN1_TIME_diff(). The latter samples the current time twice (once explicitly, once inside ASN1_TIME_diff()); if the wall clock ticks over a second between the two reads, the result is off by one second. This is a real, reproducible race, not a theoretical one: forcing the two reads to straddle a second boundary reproduces it in 29-39 out of 40 trials. It intermittently broke the exact-timestamp assertions in test/modules/md/test_920_status.py::test_md_920_002. * changes-entries/md-asn1-time-race.txt: Add entry.
Member
Author
|
The two CI failures on this PR (UBSan: test_proxy_beacon.py, "mod_http2 w/nghttp2": test_107_frame_lengths.py) are unrelated to this change - they also fail to reproduce against the md_crypt.c fix locally and look like pre-existing CI flakiness. I investigated both and filed them separately rather than bundling a fix into this PR:
|
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
1 participant
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.
Found while chasing an intermittent CI failure on an unrelated PR 836: test_md_920_002 occasionally fails an exact-timestamp assertion by exactly one second.
Root cause: md_asn1_time_get() in modules/md/md_crypt.c reconstructs a certificate's notBefore/notAfter as apr_time_now() plus ASN1_TIME_diff(). ASN1_TIME_diff() samples the current time itself, separately from the earlier apr_time_now() call, so if the wall clock ticks over a second in between, the result is off by one second.
Proved it with a small harness against the real fixture cert (test/modules/md/data/test_920/002.pubcert), deliberately forcing the two time reads to straddle a second boundary: the existing code was wrong in 29-39 out of 40 trials. The fix (direct ASN1_TIME_to_tm conversion, used when available) was wrong in 0 out of 40 trials under the identical forced race, and its output matches openssl x509 -dates exactly.
Fix: for OpenSSL 1.1.1 and newer (where ASN1_TIME_to_tm is available, excluding LibreSSL), convert the ASN1_TIME directly instead of going through now-plus-diff. Older OpenSSL/LibreSSL keep their existing conversion, unchanged.
Built and verified: mod_md.so compiles clean with this change. I do not have the pebble ACME test server available locally to run test_920_status.py itself, so CI is the first real run of the existing integration test against this fix.