From da55aa24531367d75113ea3d3550f4009a48755b Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Sun, 18 Oct 2026 12:15:53 +0100 Subject: [PATCH 01/10] mod_auth_digest: Bind the nonce lifetime kind into the nonce hash: * modules/aaa/mod_auth_digest.c (gen_nonce_hash): Hash in whether the nonce time is a timestamp or a one-time ordinal, so that a nonce minted under one lifetime does not verify under the other. * test/modules/aaa/test_014_mixed_lifetime.py: New test suite. Co-Authored-By: Claude Fable 5.1 --- modules/aaa/mod_auth_digest.c | 13 +++- test/modules/aaa/test_014_mixed_lifetime.py | 83 +++++++++++++++++++++ 2 files changed, 94 insertions(+), 2 deletions(-) create mode 100644 test/modules/aaa/test_014_mixed_lifetime.py diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 3c8b1a9b0d5..09ba7c95d91 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -1123,6 +1123,14 @@ static void gen_nonce_hash(apr_pool_t *p, char hash[NONCE_HASH_LEN+1], const digest_config_rec *conf, const char *realm) { + /* The time field of a nonce is a timestamp, or with one-time nonces + * an ordinal from otn_counter; hashing in which it is keeps a nonce + * minted as one from verifying as the other, which a realm with both + * lifetimes configured would otherwise allow: a timestamp read as an + * ordinal is far beyond any the counter will reach, and once recorded + * as a client's last nonce used, every genuine one-time nonce after + * it is stale. */ + char kind = (conf->nonce_lifetime == 0) ? 'o' : 't'; #if DIGEST_SIPHASH_NONCE unsigned char mac[APR_SIPHASH_DSIZE]; const char *msg; @@ -1134,8 +1142,8 @@ static void gen_nonce_hash(apr_pool_t *p, char hash[NONCE_HASH_LEN+1], * realm ending in what another realm's timestamp begins with would * hash the same. An absent opaque is the empty string, which is what * the challenge side passes for it too. */ - msg = apr_psprintf(p, "%" APR_SIZE_T_FMT ":%s:%s:%s", - (apr_size_t)strlen(realm), realm, timestr, + msg = apr_psprintf(p, "%" APR_SIZE_T_FMT ":%s:%s:%c:%s", + (apr_size_t)strlen(realm), realm, timestr, kind, opaque ? opaque : ""); apr_siphash24_auth(mac, msg, strlen(msg), secret); @@ -1150,6 +1158,7 @@ static void gen_nonce_hash(apr_pool_t *p, char hash[NONCE_HASH_LEN+1], apr_sha1_update_binary(&ctx, (const unsigned char *) realm, strlen(realm)); apr_sha1_update_binary(&ctx, (const unsigned char *) timestr, strlen(timestr)); + apr_sha1_update_binary(&ctx, (const unsigned char *) &kind, 1); if (opaque) { apr_sha1_update_binary(&ctx, (const unsigned char *) opaque, strlen(opaque)); diff --git a/test/modules/aaa/test_014_mixed_lifetime.py b/test/modules/aaa/test_014_mixed_lifetime.py new file mode 100644 index 00000000000..d102e292ce4 --- /dev/null +++ b/test/modules/aaa/test_014_mixed_lifetime.py @@ -0,0 +1,83 @@ +"""A realm with both a one-time-nonce location (AuthDigestNonceLifetime 0) +and a timed one, which nothing stops a configuration from having: there is +no directory merge for the directive, so every starts from the +defaults. + +The time field of a nonce is a timestamp at the timed location and a small +ordinal at the one-time one, and a one-time client records the last nonce +it used by that field. So a *timed* nonce, verified at the one-time +location, used to be recorded as the client's last ordinal -- some 1.7e15, +which the counter will never reach -- after which every genuine one-time +nonce that client was handed was "already used", and the client locked out +until its entry was garbage collected. + +The nonce is bound to the opaque, so the timed nonce has to have been +minted for the one-time client's opaque; that just takes quoting the opaque +at the timed location, which issues one when it tracks clients +(AuthDigestNcCheck here). A browser does exactly this by itself: both +locations are one protection space, so it carries its current nonce and +opaque from one to the other. No attacker is needed to hit this, though +anybody with an account on the realm could do it to another client's +opaque. + +A nonce now verifies only for the kind of lifetime it was minted under. +""" + +from . import digest_client as dc +from .env import AAATestEnv + +NONCE_HASH_INVALID = "AH01776" + +ONETIME = "onetime" +TIMED = "nccheck" + + +class TestMixedLifetime: + + def url(self, env, location): + return env.mkurl("http", "aaa", f"/digest/{location}/secret.txt") + + def challenge(self, env, location): + r = env.curl_get(self.url(env, location)) + assert r.response["status"] == 401 + return dc.DigestChallenge.parse(r.response["header"]["www-authenticate"]) + + def header(self, location, challenge): + return dc.build_authorization( + AAATestEnv.DIGEST_USER, challenge, AAATestEnv.DIGEST_PASSWORD, + method="GET", uri=f"/digest/{location}/secret.txt") + + def send(self, env, location, auth): + r = env.curl_get(self.url(env, location), + options=["-H", f"Authorization: {auth}"]) + return r + + def test_digest_140_timed_nonce_does_not_poison_onetime_client(self, env): + # The client is established at the one-time location... + onetime = self.challenge(env, ONETIME) + assert onetime.opaque is not None + + # ...and roams to the timed one with that nonce and opaque. The + # nonce is refused (it is not a timed nonce) and the challenge + # carries a timed nonce minted for the same opaque. + r = self.send(env, TIMED, self.header(TIMED, onetime)) + assert r.response["status"] == 401 + timed = dc.DigestChallenge.parse(r.response["header"]["www-authenticate"]) + assert timed.stale is True + assert timed.opaque == onetime.opaque + + # Back at the one-time location with the timed nonce: this must + # not verify, since that would record its timestamp as the + # client's last one-time ordinal. + r = self.send(env, ONETIME, self.header(ONETIME, timed)) + assert r.response["status"] == 401, \ + "a timed nonce was accepted at the one-time location" + fresh = dc.DigestChallenge.parse(r.response["header"]["www-authenticate"]) + assert fresh.stale is True + assert fresh.opaque == onetime.opaque + + # The client follows the challenge and is not locked out. + r = self.send(env, ONETIME, self.header(ONETIME, fresh)) + env.httpd_error_log.ignore_recent(lognos=[NONCE_HASH_INVALID]) + assert r.response["status"] == 200, \ + "the client's one-time nonce state was poisoned" From 01aa31a0ce69250736635d0f78dac6f3e50e47d7 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Sun, 18 Oct 2026 13:26:44 +0100 Subject: [PATCH 02/10] mod_auth_digest: Bind a client entry to the user who first used it: * modules/aaa/mod_auth_digest.c (client_entry): Add owner field. (DEF_SHMEM_SIZE): Update the entry count for it. (nonce_state): Add NONCE_NOT_OWNER. (owner_hash): New, a keyed hash of the server name, realm and username. (client_update_nonce): Record the owner on first use; return NONCE_NOT_OWNER for a request from another user, leaving the entry alone. Drop const from r. (check_and_record_nonce): Challenge NONCE_NOT_OWNER as stale, with a new opaque, clearing the quoted one from the record. * test/modules/aaa/test_015_cross_client.py: New test suite. * test/modules/aaa/conftest.py: Add a second vhost sharing the realm with its own password file, and a tracked section with a domain. * test/modules/aaa/env.py (AAATestEnv): Write that file. * test/modules/aaa/htdocs/digest/nccheck-domain/secret.txt: New file. Co-Authored-By: Claude Fable 5.1 Co-Authored-By: Claude Mythos 5.1 --- modules/aaa/mod_auth_digest.c | 69 ++++++++- test/modules/aaa/conftest.py | 21 +++ test/modules/aaa/env.py | 11 ++ .../htdocs/digest/nccheck-domain/secret.txt | 1 + test/modules/aaa/test_015_cross_client.py | 141 ++++++++++++++++++ 5 files changed, 239 insertions(+), 4 deletions(-) create mode 100644 test/modules/aaa/htdocs/digest/nccheck-domain/secret.txt create mode 100644 test/modules/aaa/test_015_cross_client.py diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 09ba7c95d91..991d95a6412 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -154,6 +154,9 @@ typedef struct hash_entry { * for last_nonce_time */ apr_time_t last_nonce_time; /* nonce of the last request * accepted for this client */ + apr_uint64_t owner; /* owner_hash() of the user + * who first used this entry, + * zero until then */ } client_entry; /* The client table, in the shared memory segment. Once initialize_tables() @@ -179,7 +182,8 @@ enum hdr_sts { NO_HEADER, NOT_DIGEST, INVALID, VALID }; enum nonce_state { NONCE_ACCEPTED, /* recorded as the latest used by this client */ NONCE_STALE, /* already used, or the client is unknown */ - NONCE_BAD_COUNT /* nonce-count did not increase: possible replay */ + NONCE_BAD_COUNT, /* nonce-count did not increase: possible replay */ + NONCE_NOT_OWNER /* the entry quoted belongs to another user */ }; /* struct to hold a parsed Authorization header */ @@ -220,7 +224,7 @@ static apr_global_mutex_t *client_lock = NULL; static const char *client_mutex_type = "authdigest-client"; static const char *client_shm_filename; -#define DEF_SHMEM_SIZE 8192L /* ~ 140 entries */ +#define DEF_SHMEM_SIZE 8192L /* ~ 120 entries */ #define HASH_DEPTH 5 /* Buckets for a given segment size, so that the default and * AuthDigestShmemSize cannot disagree. */ @@ -724,6 +728,38 @@ static void client_note_renewed(void) apr_global_mutex_unlock(client_lock); } +/* Identifies the authenticated user of r, within the origin server and + * realm, for binding a client entry to. A keyed hash, so that it is + * fixed-size in shared memory and a username colliding with another's + * cannot be found. Never zero. + * + * AuthDigestDomain is deliberately not part of this: a client keeps one + * nonce and opaque per server and realm whatever domains it was given, so + * sections of one realm which differ in their domain would make every + * crossing between them a stale challenge and a fresh entry. */ +static apr_uint64_t owner_hash(request_rec *r) +{ + const char *realm = ap_auth_name(r); + const char *msg = apr_psprintf(r->pool, "%s:%" APR_SIZE_T_FMT ":%s:%s", + r->server->server_hostname, + (apr_size_t)strlen(realm), realm, r->user); + apr_uint64_t owner; +#if DIGEST_SIPHASH_NONCE + owner = apr_siphash24(msg, strlen(msg), secret); +#else + unsigned char sha1[APR_SHA1_DIGESTSIZE]; + apr_sha1_ctx_t ctx; + + apr_sha1_init(&ctx); + apr_sha1_update_binary(&ctx, secret, SECRET_LEN); + apr_sha1_update_binary(&ctx, (const unsigned char *) msg, strlen(msg)); + apr_sha1_final(sha1, &ctx); + memcpy(&owner, sha1, sizeof(owner)); +#endif + /* zero is reserved for an entry nobody has used yet */ + return owner ? owner : 1; +} + /* Check the nonce generated at nonce_time, and the nonce-count nc sent * with it, against the state tracked for the client identified by key, and * record them if acceptable. @@ -743,8 +779,13 @@ static void client_note_renewed(void) * Must only be called for a request which is fully verified - both the * response digest and the nonce - so that a request which fails to * authenticate cannot alter the state tracked for the client whose opaque - * it quotes. */ -static enum nonce_state client_update_nonce(const request_rec *r, + * it quotes. + * + * The opaque is public, so a verified request from anybody with an account + * on the realm could otherwise do so too. An entry is bound to the user + * who first used it, and a request from another user quoting it is + * NONCE_NOT_OWNER, leaving the entry alone. */ +static enum nonce_state client_update_nonce(request_rec *r, client_id_t key, const digest_config_rec *conf, apr_time_t nonce_time, @@ -755,6 +796,7 @@ static enum nonce_state client_update_nonce(const request_rec *r, unsigned long tracked = 0; enum nonce_state state; int known; + apr_uint64_t owner = owner_hash(r); apr_global_mutex_lock(client_lock); client = find_client(key); @@ -762,6 +804,9 @@ static enum nonce_state client_update_nonce(const request_rec *r, if (!known) { state = NONCE_STALE; } + else if (client->owner && client->owner != owner) { + state = NONCE_NOT_OWNER; + } else { tracked = client->nonce_count; if (conf->nonce_lifetime == 0) { @@ -780,6 +825,7 @@ static enum nonce_state client_update_nonce(const request_rec *r, if (state == NONCE_ACCEPTED) { client->last_nonce_time = nonce_time; client->nonce_count = nc; + client->owner = owner; } } apr_global_mutex_unlock(client_lock); @@ -789,6 +835,12 @@ static enum nonce_state client_update_nonce(const request_rec *r, "client %" APR_UINT64_T_FMT " is no longer known - " "sending new nonce", key); } + else if (state == NONCE_NOT_OWNER) { + ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO() + "user %s: client %" APR_UINT64_T_FMT " belongs to " + "another user - sending new nonce and opaque", + r->user, key); + } else if (state == NONCE_STALE) { ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(01779) "user %s: one-time-nonce %s already used - sending " @@ -1404,6 +1456,15 @@ static int check_and_record_nonce(request_rec *r, digest_header_rec *resp, * the nonce from this challenge */ return note_digest_auth_failure(r, conf, resp, 1); + case NONCE_NOT_OWNER: + /* likewise, but with an entry of its own rather than the one it + * quoted: dropping the opaque has the challenge issue a new one. + * Both forms go, since the record is shared with any later + * request in this tree. */ + resp->opaque = NULL; + resp->opaque_num = 0; + return note_digest_auth_failure(r, conf, resp, 1); + default: return note_digest_auth_failure(r, conf, resp, 0); } diff --git a/test/modules/aaa/conftest.py b/test/modules/aaa/conftest.py index 8c943a6685a..ce095eae802 100644 --- a/test/modules/aaa/conftest.py +++ b/test/modules/aaa/conftest.py @@ -90,6 +90,14 @@ def env(pytestconfig) -> AAATestEnv: f'AuthUserFile "{pwfile}"', 'AuthDigestDomain "/digest/domain/" "https://mirror.example.org/other/"', ])) + # Tracks clients like "nccheck", but with a domain; a client crossing + # between the two is still the same user (test_015). + conf.add(_digest_dir(docs, "nccheck-domain", [ + 'AuthDigestProvider file', + f'AuthUserFile "{pwfile}"', + 'AuthDigestNcCheck On', + 'AuthDigestDomain "/digest/nccheck-domain/"', + ])) conf.add(_digest_dir(docs, "noprovider", [ # AuthDigestProvider intentionally omitted: falls back to "file". f'AuthUserFile "{pwfile}"', @@ -114,6 +122,19 @@ def env(pytestconfig) -> AAATestEnv: ' Require valid-user', '', ]) + # A second server with the same realm but its own accounts, on its own + # port so that the main server keeps the rest. The main server's + # sections apply here too; this one overrides the password + # file for the nccheck location. + conf.add(f'Listen {env.http_port2}') + conf.start_vhost(domains=[f"aaa2.{env.http_tld}"], port=env.http_port2, + doc_root=docs) + conf.add(_digest_dir(docs, "nccheck", [ + 'AuthDigestProvider file', + f'AuthUserFile "{env.digest_pwfile2}"', + 'AuthDigestNcCheck On', + ])) + conf.end_vhost() conf.install() assert env.apache_restart() == 0 return env diff --git a/test/modules/aaa/env.py b/test/modules/aaa/env.py index 342afe0c96a..87fcb667762 100644 --- a/test/modules/aaa/env.py +++ b/test/modules/aaa/env.py @@ -26,11 +26,15 @@ class AAATestEnv(HttpdTestEnv): DIGEST_PASSWORD = "digestpass2617" DIGEST_USER2 = "otheruser" DIGEST_PASSWORD2 = "otherpass2617" + # DIGEST_USER's password in the second file, for a vhost with its own + # accounts under the same realm. + DIGEST_PASSWORD_VHOST2 = "vhost2pass2617" def __init__(self, pytestconfig=None): super().__init__(pytestconfig=pytestconfig) self.add_httpd_log_modules(["auth_digest", "authn_file", "authz_core"]) self._digest_pwfile = os.path.join(self.server_dir, "digest.passwd") + self._digest_pwfile2 = os.path.join(self.server_dir, "digest2.passwd") def setup_httpd(self, setup: HttpdTestSetup = None): super().setup_httpd(setup=AAATestSetup(env=self)) @@ -46,11 +50,18 @@ def ha1(user, password): f"{ha1(self.DIGEST_USER, self.DIGEST_PASSWORD)}\n") fd.write(f"{self.DIGEST_USER2}:{self.REALM}:" f"{ha1(self.DIGEST_USER2, self.DIGEST_PASSWORD2)}\n") + with open(self._digest_pwfile2, 'w') as fd: + fd.write(f"{self.DIGEST_USER}:{self.REALM}:" + f"{ha1(self.DIGEST_USER, self.DIGEST_PASSWORD_VHOST2)}\n") @property def digest_pwfile(self) -> str: return self._digest_pwfile + @property + def digest_pwfile2(self) -> str: + return self._digest_pwfile2 + def configtest(self, directory_lines: List[str], extra_top_lines: Optional[List[str]] = None ) -> ExecResult: """Run `httpd -t` against a minimal, standalone config built from the diff --git a/test/modules/aaa/htdocs/digest/nccheck-domain/secret.txt b/test/modules/aaa/htdocs/digest/nccheck-domain/secret.txt new file mode 100644 index 00000000000..3d2d3a289f7 --- /dev/null +++ b/test/modules/aaa/htdocs/digest/nccheck-domain/secret.txt @@ -0,0 +1 @@ +digest-nccheck-domain-secret diff --git a/test/modules/aaa/test_015_cross_client.py b/test/modules/aaa/test_015_cross_client.py new file mode 100644 index 00000000000..a20f9dec7e6 --- /dev/null +++ b/test/modules/aaa/test_015_cross_client.py @@ -0,0 +1,141 @@ +"""The per-client nonce state is updated only by requests which verify, so +nothing unauthenticated can move it (test_007, test_008). It is still keyed +by the opaque alone, and the opaque is public: in the clear in every +challenge and every request, and not covered by the response digest. So +anybody with an account on the realm can quote another client's opaque, +authenticate as themselves, and have that client's state updated with +their nonce-count or nonce. + +For the victim this is a 401 on its next request -- a nonce-count that is +suddenly behind, or a one-time nonce that has suddenly been used -- with a +"possible replay attack" logged against it. Not a bypass, since the +attacker only ever authenticates as the account they hold, but a denial +the victim cannot defend against, and a false replay alarm. + +An entry is now bound to the user who first used it, and a request from +anybody else quoting it is challenged afresh with an opaque of its own. + +"User" here means within the origin server and realm: the same username +under the same realm on another server, with its own accounts, is somebody +else. The nonce is bound to the realm but not to the server, so it +verifies there. AuthDigestDomain is not part of it: a client keeps one +nonce and opaque per server and realm whatever domains it was given, so +it is the same user in every section of the realm, domain or no domain. +""" + +from . import digest_client as dc +from .env import AAATestEnv + +NC_FAILED = "AH01774" + + +class TestCrossClient: + + def url(self, env, location): + return env.mkurl("http", "aaa", f"/digest/{location}/secret.txt") + + def challenge(self, env, location): + r = env.curl_get(self.url(env, location)) + assert r.response["status"] == 401 + challenge = dc.DigestChallenge.parse(r.response["header"]["www-authenticate"]) + assert challenge.opaque is not None + return challenge + + def header(self, location, challenge, user=AAATestEnv.DIGEST_USER, + password=AAATestEnv.DIGEST_PASSWORD, nc="00000001", + cnonce="victim-cnonce"): + return dc.build_authorization( + user, challenge, password, method="GET", + uri=f"/digest/{location}/secret.txt", nc=nc, cnonce=cnonce) + + def intruder(self, location, challenge, nc="00000001"): + """A correct Authorization header for the *other* account, quoting + the victim's nonce and opaque -- both of which it has seen.""" + return self.header(location, challenge, user=AAATestEnv.DIGEST_USER2, + password=AAATestEnv.DIGEST_PASSWORD2, nc=nc, + cnonce="intruder-cnonce") + + def send(self, env, location, auth): + return env.curl_get(self.url(env, location), + options=["-H", f"Authorization: {auth}"]) + + def test_digest_150_other_user_cannot_move_nonce_count(self, env): + location = "nccheck" + victim = self.challenge(env, location) + r = self.send(env, location, self.header(location, victim, nc="00000001")) + assert r.response["status"] == 200 + + # The other account quotes the victim's opaque with a nonce-count + # well ahead of the victim's. It is not this client's entry, so it + # is challenged with one of its own rather than served. + r = self.send(env, location, self.intruder(location, victim, nc="00000005")) + assert r.response["status"] == 401 + intruder = dc.DigestChallenge.parse(r.response["header"]["www-authenticate"]) + assert intruder.stale is True + assert intruder.opaque != victim.opaque + + # The victim's count was left alone. + r = self.send(env, location, self.header(location, victim, nc="00000002")) + assert r.response["status"] == 200, \ + "another user's request moved the victim's nonce-count on" + + def test_digest_151_other_user_cannot_use_up_onetime_nonce(self, env): + location = "onetime" + victim = self.challenge(env, location) + r = self.send(env, location, self.header(location, victim)) + assert r.response["status"] == 200 + ai = dc.parse_params(r.response["header"]["authentication-info"]) + victim.nonce = ai["nextnonce"] + + # The nextnonce went to the victim in the clear; the other account + # presents it, with the victim's opaque, before the victim does. + r = self.send(env, location, self.intruder(location, victim)) + assert r.response["status"] == 401 + intruder = dc.DigestChallenge.parse(r.response["header"]["www-authenticate"]) + assert intruder.stale is True + assert intruder.opaque != victim.opaque + + # The victim's one-time nonce is still unused. + r = self.send(env, location, self.header(location, victim)) + assert r.response["status"] == 200, \ + "another user's request used up the victim's one-time nonce" + + def test_digest_152_same_name_on_other_server_cannot_move_nonce_count(self, env): + location = "nccheck" + victim = self.challenge(env, location) + r = self.send(env, location, self.header(location, victim, nc="00000001")) + assert r.response["status"] == 200 + + # The second server has a "digestuser" of its own, with a different + # password, under the same realm. Its holder quotes the victim's + # nonce and opaque there. + url2 = f"http://aaa2.{env.http_tld}:{env.http_port2}/digest/{location}/secret.txt" + auth = self.header(location, victim, password=AAATestEnv.DIGEST_PASSWORD_VHOST2, + nc="00000005", cnonce="intruder-cnonce") + r = env.curl_get(url2, options=["-H", f"Authorization: {auth}"]) + assert r.response["status"] == 401 + intruder = dc.DigestChallenge.parse(r.response["header"]["www-authenticate"]) + assert intruder.stale is True + assert intruder.opaque != victim.opaque + + r = self.send(env, location, self.header(location, victim, nc="00000002")) + assert r.response["status"] == 200, \ + "a same-named user on another server moved the victim's nonce-count on" + + def test_digest_153_same_user_across_domains_keeps_entry(self, env): + # A client established at a section without AuthDigestDomain + # carries its nonce and opaque to a section of the same realm which + # has one. It is the same user, so the entry is kept, not + # challenged as somebody else's; and it is kept on the way back. + client = self.challenge(env, "nccheck") + r = self.send(env, "nccheck", self.header("nccheck", client, nc="00000001")) + assert r.response["status"] == 200 + + r = self.send(env, "nccheck-domain", + self.header("nccheck-domain", client, nc="00000002")) + assert r.response["status"] == 200, \ + "the same user was challenged as another on crossing into a domain" + + r = self.send(env, "nccheck", self.header("nccheck", client, nc="00000003")) + assert r.response["status"] == 200, \ + "the same user was challenged as another on crossing back" From abf9219aa6c427583bfcad38f68bdf01bc946327 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Sun, 18 Oct 2026 15:11:14 +0100 Subject: [PATCH 03/10] mod_auth_digest: Don't tear down shared state when initialization fails: * modules/aaa/mod_auth_digest.c (cleanup_tables, log_error_and_cleanup): Removed. (init_failed): New, replacing them; logs the failure and returns !OK. (initialize_tables): Use it. (initialize_child): Log a failure and continue with the inherited handles. Co-Authored-By: Claude Fable 5.1 --- modules/aaa/mod_auth_digest.c | 66 +++++++++++------------------------ 1 file changed, 20 insertions(+), 46 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 991d95a6412..177872c8133 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -238,39 +238,12 @@ static unsigned long num_buckets = DEF_NUM_BUCKETS; module AP_MODULE_DECLARE_DATA auth_digest_module; -/* Initialization. */ -static apr_status_t cleanup_tables(void *not_used) +/* Logs a failure to set up the client table, returning the hook result. */ +static int init_failed(server_rec *s, apr_status_t rv, const char *msg) { - ap_log_error(APLOG_MARK, APLOG_INFO, 0, NULL, APLOGNO(01756) - "cleaning up shared memory"); - - if (client_rmm) { - apr_rmm_destroy(client_rmm); - client_rmm = NULL; - } - - if (client_shm) { - apr_shm_destroy(client_shm); - client_shm = NULL; - } - - if (client_lock) { - apr_global_mutex_destroy(client_lock); - client_lock = NULL; - } - - client_list = NULL; - - return APR_SUCCESS; -} - -static void log_error_and_cleanup(char *msg, apr_status_t sts, server_rec *s) -{ - ap_log_error(APLOG_MARK, APLOG_ERR, sts, s, APLOGNO(01760) - "%s - all nonce-count checking and one-time nonces " - "disabled", msg); - - cleanup_tables(NULL); + ap_log_error(APLOG_MARK, APLOG_ERR, rv, s, APLOGNO(01760) + "%s - cannot track Digest clients", msg); + return !OK; } /* RMM helper functions that behave like single-step malloc/free. */ @@ -323,11 +296,10 @@ static int initialize_tables(server_rec *s, apr_pool_t *ctx) } if (APR_SUCCESS != sts) { - ap_log_error(APLOG_MARK, APLOG_ERR, sts, s, APLOGNO(01762) - "Failed to create shared memory segment on file %s", - client_shm_filename); - log_error_and_cleanup("failed to initialize shm", sts, s); - return HTTP_INTERNAL_SERVER_ERROR; + return init_failed(s, sts, apr_psprintf(ctx, "failed to create " + "shared memory segment on " + "file %s", + client_shm_filename)); } sts = apr_rmm_init(&client_rmm, @@ -335,15 +307,13 @@ static int initialize_tables(server_rec *s, apr_pool_t *ctx) apr_shm_baseaddr_get(client_shm), shmem_size, ctx); if (sts != APR_SUCCESS) { - log_error_and_cleanup("failed to initialize rmm", sts, s); - return !OK; + return init_failed(s, sts, "failed to initialize rmm"); } client_list = rmm_malloc(client_rmm, sizeof(*client_list) + sizeof(client_entry *) * num_buckets); if (!client_list) { - log_error_and_cleanup("failed to allocate shared memory", -1, s); - return !OK; + return init_failed(s, 0, "failed to allocate shared memory"); } client_list->table = (client_entry**) (client_list + 1); for (idx = 0; idx < num_buckets; idx++) { @@ -367,8 +337,7 @@ static int initialize_tables(server_rec *s, apr_pool_t *ctx) sts = ap_global_mutex_create(&client_lock, NULL, client_mutex_type, NULL, s, ctx, 0); if (sts != APR_SUCCESS) { - log_error_and_cleanup("failed to create lock (client_lock)", sts, s); - return !OK; + return init_failed(s, sts, "failed to create lock (client_lock)"); } return OK; @@ -422,6 +391,10 @@ static int initialize_module(apr_pool_t *p, apr_pool_t *plog, return initialize_tables(s, p); } +/* A failure here is logged and the child carries on with the handles it + * inherited: nothing is torn down, since the shared memory and above all + * the mutex are shared with every other child, and this child has no + * fallback for serving Digest requests without them. */ static void initialize_child(apr_pool_t *p, server_rec *s) { apr_status_t sts; @@ -432,7 +405,8 @@ static void initialize_child(apr_pool_t *p, server_rec *s) apr_shm_baseaddr_get(client_shm), p); if (sts != APR_SUCCESS) { - log_error_and_cleanup("failed to attach to rmm", sts, s); + ap_log_error(APLOG_MARK, APLOG_CRIT, sts, s, APLOGNO() + "failed to attach to rmm in child"); return; } @@ -440,8 +414,8 @@ static void initialize_child(apr_pool_t *p, server_rec *s) apr_global_mutex_lockfile(client_lock), p); if (sts != APR_SUCCESS) { - log_error_and_cleanup("failed to create lock (client_lock)", sts, s); - return; + ap_log_error(APLOG_MARK, APLOG_CRIT, sts, s, APLOGNO() + "failed to initialize lock (client_lock) in child"); } } From 61ced6407acf616485883b932b6c35646fbe0760 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Sun, 18 Oct 2026 16:05:01 +0100 Subject: [PATCH 04/10] mod_auth_digest: Challenge a failed nonce-count check as stale: * modules/aaa/mod_auth_digest.c (check_and_record_nonce): Answer NONCE_BAD_COUNT with stale=true like NONCE_STALE. (client_update_nonce): Log it at INFO rather than ERR, and without the "possible replay attack" wording: an out-of-order nonce-count is what a client's concurrent requests look like, and a replay cannot answer the new nonce. Alerting on AH01774 at ERR will no longer see it. (authenticate_digest_user): Expand the comment on the check order. * test/modules/aaa/test_003_nccheck.py (test_digest_032), test/modules/aaa/test_007_replay.py (test_digest_074): Expect stale. Co-Authored-By: Claude Fable 5.1 --- modules/aaa/mod_auth_digest.c | 31 ++++++++++++++++------------ test/modules/aaa/test_003_nccheck.py | 8 ++++--- test/modules/aaa/test_007_replay.py | 6 +++--- 3 files changed, 26 insertions(+), 19 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 177872c8133..df859eb73ae 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -821,10 +821,9 @@ static enum nonce_state client_update_nonce(request_rec *r, "new nonce", r->user, nonce); } else if (state == NONCE_BAD_COUNT) { - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01774) - "Warning, possible replay attack: nonce-count check " - "failed: %lu is not above %lu for nonce %s", nc, - tracked, nonce); + ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(01774) + "user %s: nonce-count %lu is not above %lu for nonce " + "%s - sending new nonce", r->user, nc, tracked, nonce); } return state; @@ -1426,8 +1425,12 @@ static int check_and_record_nonce(request_rec *r, digest_header_rec *resp, return OK; case NONCE_STALE: + case NONCE_BAD_COUNT: /* the credentials were good, so the client can silently retry with - * the nonce from this challenge */ + * the nonce from this challenge. A nonce-count which has not + * increased is what concurrent requests from one client look like + * when they arrive out of order, and a replay gains nothing from + * the stale flag, having no way to answer the new nonce. */ return note_digest_auth_failure(r, conf, resp, 1); case NONCE_NOT_OWNER: @@ -1438,10 +1441,9 @@ static int check_and_record_nonce(request_rec *r, digest_header_rec *resp, resp->opaque = NULL; resp->opaque_num = 0; return note_digest_auth_failure(r, conf, resp, 1); - - default: - return note_digest_auth_failure(r, conf, resp, 0); } + + return HTTP_INTERNAL_SERVER_ERROR; /* not reached */ } static int check_nonce(request_rec *r, digest_header_rec *resp, @@ -1758,11 +1760,14 @@ static int authenticate_digest_user(request_rec *r) } } - /* Note: the nonce is checked before the nonce-count so that the - * nonce-count state is only ever updated for a request which is using - * a nonce this server issued, and so that a request using an expired - * nonce gets a "stale=true" challenge (and hence a silent retry with a - * fresh nonce-count) rather than being reported as a replay. */ + /* The nonce and nonce-count are checked only after the digest has been + * verified above, so the stale=true they can answer with never reveals + * whether the credentials were right. The nonce is checked before the + * nonce-count so that the nonce-count state is only ever updated for a + * request which is using a nonce this server issued, and so that a + * request using an expired nonce gets a "stale=true" challenge (and + * hence a silent retry with a fresh nonce-count) rather than being + * reported as a replay. */ if ((res = check_nonce(r, resp, conf))) { return res; } diff --git a/test/modules/aaa/test_003_nccheck.py b/test/modules/aaa/test_003_nccheck.py index 54b8f0b52cd..b878f65ded4 100644 --- a/test/modules/aaa/test_003_nccheck.py +++ b/test/modules/aaa/test_003_nccheck.py @@ -68,12 +68,14 @@ def test_digest_032_nccheck_replay_rejected(self, env): r2 = self.authenticate(env, "nccheck", challenge, nc="00000002") assert r2.response["status"] == 200 - # replay an already-used nc -> rejected, and NOT reported as stale - # (this is a distinct failure mode from an invalid/expired nonce). + # replay an already-used nc -> rejected, and reported as stale: the + # digest verified, so the credentials are good, and a client which + # sent requests out of nonce-count order retries silently on the + # fresh nonce rather than being told its password was wrong. r3 = self.authenticate(env, "nccheck", challenge, nc="00000001") assert r3.response["status"] == 401 new_challenge = dc.DigestChallenge.parse(r3.response["header"]["www-authenticate"]) - assert new_challenge.stale is False + assert new_challenge.stale is True env.httpd_error_log.ignore_recent(lognos=["AH01774"]) # recovery: the rejected attempt handed out a fresh challenge for diff --git a/test/modules/aaa/test_007_replay.py b/test/modules/aaa/test_007_replay.py index ba61ed9eaaa..2ecee70b13f 100644 --- a/test/modules/aaa/test_007_replay.py +++ b/test/modules/aaa/test_007_replay.py @@ -34,8 +34,8 @@ from . import digest_client as dc from .env import AAATestEnv -# See the note in test_003_nccheck.py: a failed nc check is not reported as -# stale, since it is a distinct failure mode from an invalid/expired nonce. +# A failed nc check is reported as stale (the digest verified, so the +# credentials are good): the client retries silently on the fresh nonce. NC_FAILED = "AH01774" NONCE_HASH_INVALID = "AH01776" PASSWORD_MISMATCH = "AH01794" @@ -156,7 +156,7 @@ def test_digest_074_legit_client_recovers_via_fresh_challenge(self, env): assert r.response["status"] == 401 env.httpd_error_log.ignore_recent(lognos=[NC_FAILED]) fresh = dc.DigestChallenge.parse(r.response["header"]["www-authenticate"]) - assert fresh.stale is False + assert fresh.stale is True assert fresh.opaque == challenge.opaque, \ "the client keeps its identity across a re-challenge" assert fresh.nonce != challenge.nonce From 7a4e251c65e1fb3c5c7707ff1027ab8291d25564 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Sun, 18 Oct 2026 16:07:10 +0100 Subject: [PATCH 05/10] mod_auth_digest: Don't force stale=true for an unknown opaque: * modules/aaa/mod_auth_digest.c (note_digest_auth_failure): Leave the caller's stale flag alone when issuing a new opaque. * test/modules/aaa/test_001_challenge_response.py (test_digest_016): New. Co-Authored-By: Claude Fable 5.1 --- modules/aaa/mod_auth_digest.c | 8 ++++---- .../aaa/test_001_challenge_response.py | 20 +++++++++++++++++++ 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index df859eb73ae..66b2003cb4b 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -1262,14 +1262,14 @@ static int note_digest_auth_failure(request_rec *r, else if (!client_exists(resp->opaque_num, r)) { /* We have no record of this client: its entry may have been * garbage collected, the segment may have been recreated by a - * restart, or the opaque may never have been issued by us. - * Nothing was wrong with the credentials, so the challenge is - * stale and an RFC-compliant client retries silently. */ + * restart, or the opaque may never have been issued by us. It gets + * a new entry; whether the challenge is stale is the caller's + * call, since credentials which failed are no less wrong for the + * opaque being unknown. */ if ((client_key = client_generate(r)) == 0) { return HTTP_SERVICE_UNAVAILABLE; } opaque = client_id_to_opaque(r->pool, client_key); - stale = 1; client_note_renewed(); } else { diff --git a/test/modules/aaa/test_001_challenge_response.py b/test/modules/aaa/test_001_challenge_response.py index 92b54b5c45c..14c1887c939 100644 --- a/test/modules/aaa/test_001_challenge_response.py +++ b/test/modules/aaa/test_001_challenge_response.py @@ -195,3 +195,23 @@ def test_digest_015_uri_bad_escaping_rejected(self, env): assert r.response["status"] == 400, \ f"uri={bad_uri!r} was not rejected" env.httpd_error_log.ignore_recent(lognos=["AH01783"]) + + def test_digest_016_wrong_password_unknown_opaque_not_stale(self, env): + # A wrong password is wrong whatever opaque the request quotes. An + # opaque this server has no entry for (never issued, or garbage + # collected) used to force stale=true onto the challenge, so the + # client retried silently with the password it had just been told + # was wrong -- once -- before being re-prompted. + challenge = self.challenge(env, location="nccheck") + auth = dc.build_authorization( + AAATestEnv.DIGEST_USER, challenge, "not-the-password", + method="GET", uri="/digest/nccheck/secret.txt", + opaque="deadbeef") + r = env.curl_get(self.url(env, location="nccheck"), + options=["-H", f"Authorization: {auth}"]) + assert r.response["status"] == 401 + fresh = dc.DigestChallenge.parse(r.response["header"]["www-authenticate"]) + assert fresh.stale is False + # ...though the unknown opaque is still replaced by one of ours. + assert fresh.opaque not in (None, "deadbeef") + env.httpd_error_log.ignore_recent(lognos=["AH01794"]) From 1748dc3f49170cab203f0b1f0d726f21fd052f81 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Sun, 18 Oct 2026 17:16:37 +0100 Subject: [PATCH 06/10] * modules/aaa/mod_auth_digest.c (gen_nonce_hash, gen_nonce): Drop the unused server argument. (note_digest_auth_failure, check_nonce, add_auth_info): Adjust. (authenticate_digest_user): Remove the unreachable missing-realm checks and the NULL check on new_digest(). (init_digest_request): Fix the function name in the comment. No functional change. Co-Authored-By: Claude Fable 5.1 --- modules/aaa/mod_auth_digest.c | 30 +++++++++--------------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 66b2003cb4b..cd1a640afcd 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -1101,7 +1101,7 @@ static digest_header_rec *make_digest_rec(request_rec *store, request_rec *r) * updated here: the state of an authenticated client must not be altered * by a request which has not (yet) been authenticated, or a replayed or * bogus request quoting the client's opaque would be able to rewind that - * state. See check_and_update_nc(). + * state. See check_and_record_nonce(). * * Note that this must be called after mod_proxy had its go so that * r->proxyreq is set correctly. @@ -1144,7 +1144,6 @@ static digest_header_rec *get_digest_rec(request_rec *r) * minimum size (NONCE_HASH_LEN+1). */ static void gen_nonce_hash(apr_pool_t *p, char hash[NONCE_HASH_LEN+1], const char *timestr, const char *opaque, - const server_rec *server, const digest_config_rec *conf, const char *realm) { @@ -1197,7 +1196,6 @@ static void gen_nonce_hash(apr_pool_t *p, char hash[NONCE_HASH_LEN+1], /* The nonce has the format b64(time)+hash . */ static const char *gen_nonce(apr_pool_t *p, apr_time_t now, const char *opaque, - const server_rec *server, const digest_config_rec *conf, const char *realm) { @@ -1219,7 +1217,7 @@ static const char *gen_nonce(apr_pool_t *p, apr_time_t now, const char *opaque, apr_global_mutex_unlock(client_lock); } apr_base64_encode_binary(nonce, t.arr, sizeof(t.arr)); - gen_nonce_hash(p, nonce+NONCE_TIME_LEN, nonce, opaque, server, conf, realm); + gen_nonce_hash(p, nonce+NONCE_TIME_LEN, nonce, opaque, conf, realm); return nonce; } @@ -1286,7 +1284,7 @@ static int note_digest_auth_failure(request_rec *r, /* Setup nonce */ - nonce = gen_nonce(r->pool, r->request_time, opaque, r->server, conf, ap_auth_name(r)); + nonce = gen_nonce(r->pool, r->request_time, opaque, conf, ap_auth_name(r)); /* Setup domain, which tells the client which URIs share this * protection space, so that it does not send the Authorization header @@ -1456,7 +1454,7 @@ static int check_nonce(request_rec *r, digest_header_rec *resp, tmp = resp->nonce[NONCE_TIME_LEN]; resp->nonce[NONCE_TIME_LEN] = '\0'; apr_base64_decode_binary(nonce_time.arr, resp->nonce); - gen_nonce_hash(r->pool, hash, resp->nonce, resp->opaque, r->server, conf, + gen_nonce_hash(r->pool, hash, resp->nonce, resp->opaque, conf, ap_auth_name(r)); resp->nonce[NONCE_TIME_LEN] = tmp; resp->nonce_time = nonce_time.time; @@ -1686,14 +1684,7 @@ static int authenticate_digest_user(request_rec *r) } } - if (!realm) { - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02533) - "realm mismatch - got `%s' but no realm specified", - resp->realm); - return note_digest_auth_failure(r, conf, resp, 0); - } - - if (!resp->realm || strcmp(resp->realm, realm)) { + if (strcmp(resp->realm, realm)) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01788) "realm mismatch - got `%s' but expected `%s'", resp->realm, realm); @@ -1748,10 +1739,7 @@ static int authenticate_digest_user(request_rec *r) else { /* RFC 2617 (or 7616)-style Digest hash calculation. */ const char *exp_digest = new_digest(r, resp); - if (!exp_digest) { - /* we failed to allocate a client struct */ - return HTTP_INTERNAL_SERVER_ERROR; - } + if (!ap_memeq_timingsafe(exp_digest, resp->digest, MD5_DIGEST_LEN)) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01794) "user %s: password mismatch: %s", r->user, @@ -1803,13 +1791,13 @@ static int add_auth_info(request_rec *r) if ((r->request_time - resp->nonce_time) > (conf->nonce_lifetime-NEXTNONCE_DELTA)) { nextnonce = apr_pstrcat(r->pool, ", nextnonce=\"", gen_nonce(r->pool, r->request_time, - resp->opaque, r->server, conf, ap_auth_name(r)), + resp->opaque, conf, ap_auth_name(r)), "\"", NULL); } } else if (conf->nonce_lifetime == 0 && resp->opaque_num) { - const char *nonce = gen_nonce(r->pool, 0, resp->opaque, r->server, - conf, ap_auth_name(r)); + const char *nonce = gen_nonce(r->pool, 0, resp->opaque, conf, + ap_auth_name(r)); nextnonce = apr_pstrcat(r->pool, ", nextnonce=\"", nonce, "\"", NULL); } /* else nonce never expires, hence no nextnonce */ From 2f90ac861854ea2ce10a2a0e28a0a5b938ccbbaf Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Sun, 18 Oct 2026 17:17:00 +0100 Subject: [PATCH 07/10] * modules/aaa/mod_auth_digest.c (authenticate_digest_user): Compare the hostnames with ap_cstr_casecmp(). Co-Authored-By: Claude Fable 5.1 --- modules/aaa/mod_auth_digest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index cd1a640afcd..688ab52e717 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -1658,7 +1658,7 @@ static int authenticate_digest_user(request_rec *r) else if ( /* check hostname matches, if present */ (d_uri.hostname && d_uri.hostname[0] != '\0' - && strcasecmp(d_uri.hostname, r_uri.hostname)) + && ap_cstr_casecmp(d_uri.hostname, r_uri.hostname)) /* check port matches, if present */ || (d_uri.port_str && d_uri.port != r_uri.port) /* check that server-port is default port if no port present */ From 1b1423e406fd51117a193db8b5d440b3a3441ce8 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Sun, 18 Oct 2026 17:24:08 +0100 Subject: [PATCH 08/10] mod_auth_digest: Do less while holding client_lock: * modules/aaa/mod_auth_digest.c (client_note_renewed): Removed. (client_generate): Take a renewed flag and count it under the lock already held; log the gc summary and any rmm_free failure after releasing the lock, so the summary now counts the entry just added. (gc): Return the first rmm_free failure as the result, and the count through a new argument, instead of logging. (note_digest_auth_failure): Adjust. Co-Authored-By: Claude Fable 5.1 --- modules/aaa/mod_auth_digest.c | 119 +++++++++++++++++++--------------- 1 file changed, 66 insertions(+), 53 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 688ab52e717..d3d75c1fb39 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -693,15 +693,6 @@ static int client_exists(client_id_t key, const request_rec *r) return found; } -/* Note that a client entry was created to replace one which had been - * garbage collected. */ -static void client_note_renewed(void) -{ - apr_global_mutex_lock(client_lock); - client_list->num_renewed++; - apr_global_mutex_unlock(client_lock); -} - /* Identifies the authenticated user of r, within the origin server and * realm, for binding a client entry to. A keyed hash, so that it is * fixed-size in shared memory and a username colliding with another's @@ -830,12 +821,17 @@ static enum nonce_state client_update_nonce(request_rec *r, } /* A simple garbage-collecter to remove unused clients. It removes the - * last entry in each bucket and updates the counters. Returns the - * number of removed entries. + * last entry in each bucket and updates the counters, returning the + * number of removed entries in *num_removed. The result is the first + * failure to free an entry, if any, for the caller to log once the lock + * is released. */ -static unsigned long gc(server_rec *s) +static apr_status_t gc(unsigned long *num_removed) { - unsigned long num_removed = 0, idx; + apr_status_t rv = APR_SUCCESS; + unsigned long idx; + + *num_removed = 0; /* garbage collect one entry from each bucket */ @@ -865,35 +861,37 @@ static unsigned long gc(server_rec *s) *victim = entry->next; err = rmm_free(client_rmm, entry); - num_removed++; + (*num_removed)++; - if (err) { - /* Nothing we can really do but log... */ - ap_log_error(APLOG_MARK, APLOG_ERR, err, s, APLOGNO(10007) - "Failed to free auth_digest client allocation"); + if (err && rv == APR_SUCCESS) { + rv = err; } } } - /* update counters and log */ + /* update counters */ - client_list->num_entries -= num_removed; - client_list->num_removed += num_removed; + client_list->num_entries -= *num_removed; + client_list->num_removed += *num_removed; - return num_removed; + return rv; } /* - * Add a new client to the list, under a newly issued id. Returns the id if - * successful, zero otherwise. This triggers the garbage collection if - * memory is low. (The new entry is not returned: see find_client().) + * Add a new client to the list, under a newly issued id; renewed is set + * when it replaces an entry which had been garbage collected. Returns the + * id if successful, zero otherwise. This triggers the garbage collection + * if memory is low. (The new entry is not returned: see find_client().) */ -static client_id_t client_generate(request_rec *r) +static client_id_t client_generate(request_rec *r, int renewed) { server_rec *s = r->server; int bucket; client_entry *entry; - client_id_t key; + client_id_t key = 0; + int ran_gc = 0; + unsigned long gc_removed = 0, created, removed, renewals; + apr_status_t gc_rv = APR_SUCCESS; apr_global_mutex_lock(client_lock); @@ -901,37 +899,53 @@ static client_id_t client_generate(request_rec *r) entry = rmm_malloc(client_rmm, sizeof(client_entry)); if (!entry) { - unsigned long num_removed = gc(s); - ap_log_error(APLOG_MARK, APLOG_INFO, 0, s, APLOGNO(01766) - "gc'd %lu client entries. Total new clients: " - "%lu; Total removed clients: %lu; Total renewed clients: " - "%lu", num_removed, - client_list->num_created - client_list->num_renewed, - client_list->num_removed, client_list->num_renewed); + gc_rv = gc(&gc_removed); + ran_gc = 1; entry = rmm_malloc(client_rmm, sizeof(client_entry)); - if (!entry) { - apr_global_mutex_unlock(client_lock); - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01769) - "unable to allocate a client entry - failing the " - "request, since this configuration needs one"); - return 0; - } } - /* now issue the id, in 1..CLIENT_ID_MAX, and add the entry */ + if (entry) { + /* now issue the id, in 1..CLIENT_ID_MAX, and add the entry */ - key = client_list->next_id = client_list->next_id % CLIENT_ID_MAX + 1; - bucket = key % client_list->tbl_len; + key = client_list->next_id = client_list->next_id % CLIENT_ID_MAX + 1; + bucket = key % client_list->tbl_len; - memset(entry, 0, sizeof(client_entry)); - entry->key = key; - entry->next = client_list->table[bucket]; - client_list->table[bucket] = entry; - client_list->num_created++; - client_list->num_entries++; + memset(entry, 0, sizeof(client_entry)); + entry->key = key; + entry->next = client_list->table[bucket]; + client_list->table[bucket] = entry; + client_list->num_created++; + client_list->num_entries++; + client_list->num_renewed += !!renewed; + } + + /* for logging after the lock is released */ + created = client_list->num_created; + removed = client_list->num_removed; + renewals = client_list->num_renewed; apr_global_mutex_unlock(client_lock); + if (gc_rv) { + /* Nothing we can really do but log... */ + ap_log_error(APLOG_MARK, APLOG_ERR, gc_rv, s, APLOGNO(10007) + "Failed to free auth_digest client allocation"); + } + if (ran_gc) { + ap_log_error(APLOG_MARK, APLOG_INFO, 0, s, APLOGNO(01766) + "gc'd %lu client entries. Total new clients: " + "%lu; Total removed clients: %lu; Total renewed clients: " + "%lu", gc_removed, created - renewals, removed, + renewals); + } + + if (!key) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01769) + "unable to allocate a client entry - failing the " + "request, since this configuration needs one"); + return 0; + } + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(01768) "allocated new client %" APR_UINT64_T_FMT, key); @@ -1249,7 +1263,7 @@ static int note_digest_auth_failure(request_rec *r, if (resp->opaque == NULL) { /* new client */ if (conf->check_nc || conf->nonce_lifetime == 0) { - if ((client_key = client_generate(r)) == 0) { + if ((client_key = client_generate(r, 0)) == 0) { return HTTP_SERVICE_UNAVAILABLE; } opaque = client_id_to_opaque(r->pool, client_key); @@ -1264,11 +1278,10 @@ static int note_digest_auth_failure(request_rec *r, * a new entry; whether the challenge is stale is the caller's * call, since credentials which failed are no less wrong for the * opaque being unknown. */ - if ((client_key = client_generate(r)) == 0) { + if ((client_key = client_generate(r, 1)) == 0) { return HTTP_SERVICE_UNAVAILABLE; } opaque = client_id_to_opaque(r->pool, client_key); - client_note_renewed(); } else { /* Note that the nonce-count tracked for this client is left alone From 7343d2da6543a9ec3344e7eee9fc3415a32681a4 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Mon, 19 Oct 2026 19:10:54 +0100 Subject: [PATCH 09/10] mod_auth_digest: Refuse a nonce whose time field does not fully decode: * modules/aaa/mod_auth_digest.c (check_nonce): The base64 decoder stops at the first byte which is not base64, so a nonce of the right length could leave part of the decoded time unset. Check the decoded length and challenge as stale if it is short, before using the time. * test/modules/aaa/test_012_truncated_nonce.py (test_digest_012_02_time_not_base64): New test. Co-Authored-By: Claude Mythos 5.1 --- modules/aaa/mod_auth_digest.c | 14 +++++++++- test/modules/aaa/test_012_truncated_nonce.py | 29 ++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index d3d75c1fb39..22d4889ceb4 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -1463,13 +1463,25 @@ static int check_nonce(request_rec *r, digest_header_rec *resp, apr_time_t dt; time_rec nonce_time; char tmp, hash[NONCE_HASH_LEN+1]; + int len; + /* The decoder stops at the first byte which is not base64, so a + * nonce of the right shape can still decode to fewer bytes than the + * time field holds, leaving the rest of it unset. VALID_NONCE() + * bounds what it can decode to at most sizeof(nonce_time.arr). */ tmp = resp->nonce[NONCE_TIME_LEN]; resp->nonce[NONCE_TIME_LEN] = '\0'; - apr_base64_decode_binary(nonce_time.arr, resp->nonce); + len = apr_base64_decode_binary(nonce_time.arr, resp->nonce); gen_nonce_hash(r->pool, hash, resp->nonce, resp->opaque, conf, ap_auth_name(r)); resp->nonce[NONCE_TIME_LEN] = tmp; + + if (len != sizeof(nonce_time.arr)) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO() + "invalid nonce %s received - time is not base64", + resp->nonce); + return note_digest_auth_failure(r, conf, resp, 1); + } resp->nonce_time = nonce_time.time; if (!ap_memeq_timingsafe(hash, resp->nonce+NONCE_TIME_LEN, NONCE_HASH_LEN)) { diff --git a/test/modules/aaa/test_012_truncated_nonce.py b/test/modules/aaa/test_012_truncated_nonce.py index 7db50b2bced..a89dfb338f7 100644 --- a/test/modules/aaa/test_012_truncated_nonce.py +++ b/test/modules/aaa/test_012_truncated_nonce.py @@ -27,3 +27,32 @@ def test_digest_012_01(self, env): assert r2.response["status"] == 401 env.httpd_error_log.ignore_recent(lognos=["AH01782"]) + + def test_digest_012_02_time_not_base64(self, env): + # A nonce of the right length, ending in the '=' the time field + # must end in, whose time field is not base64 beyond its first + # four characters. The decoder stops there, so the time is only + # partly decoded; the nonce must be refused for that, before the + # partial time is used for anything. + url = env.mkurl("http", "aaa", "/digest/default/secret.txt") + r1 = env.curl_get(url) + assert r1.response["status"] == 401 + challenge = dc.DigestChallenge.parse( + r1.response["header"]["www-authenticate"]) + eq_pos = challenge.nonce.index("=") + bad_nonce = "AAAA" + "!" * (eq_pos - 4) + challenge.nonce[eq_pos:] + assert len(bad_nonce) == len(challenge.nonce) + + auth_header = dc.build_authorization( + AAATestEnv.DIGEST_USER, challenge, AAATestEnv.DIGEST_PASSWORD, + method="GET", uri="/digest/default/secret.txt", + nonce_val=bad_nonce) + + r2 = env.curl_get(url, options=['-H', f'Authorization: {auth_header}']) + assert r2.response["status"] == 401 + stale = dc.DigestChallenge.parse(r2.response["header"]["www-authenticate"]) + assert stale.stale is True + + not_base64 = re.compile(r".*invalid nonce .* - time is not base64") + assert env.httpd_error_log.scan_recent(not_base64) + env.httpd_error_log.ignore_recent(matches=[not_base64.pattern]) From 7fea70d44b788d0cf99173e508f11753cc4b6be2 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Tue, 20 Oct 2026 21:02:38 +0100 Subject: [PATCH 10/10] mod_auth_digest: Fold nonce handling functions together, no functional change: * modules/aaa/mod_auth_digest.c (check_nonce): Fold check_and_record_nonce() in as the tail, so the nonce is checked in one place: that this server issued it, then its age, then its use by the client. Log per nonce_state in a switch, with one shared stale challenge for every failure. Merge the two lifetime > 0 tests into one block. (check_and_record_nonce): Remove. (client_update_nonce): Drop the logging and the nonce argument; return the tracked nonce-count through a new out parameter. (enum nonce_state): Add NONCE_UNKNOWN for a client with no entry; NONCE_STALE is now only a used one-time nonce. (tracks_clients): New predicate for whether state is kept per client. (note_digest_auth_failure, check_nonce): Use it. (authenticate_digest_user): Call check_nonce() alone; move the ordering rationale to its header comment. Drop the unused res. (init_digest_request): Adjust comment. Co-Authored-By: Claude Mythos 5.1 --- modules/aaa/mod_auth_digest.c | 196 +++++++++++++++++----------------- 1 file changed, 99 insertions(+), 97 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 22d4889ceb4..d6bfbb207ac 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -108,6 +108,15 @@ typedef struct digest_config_struct { char *uri_list; } digest_config_rec; +/* Whether state is tracked per client under conf: a one-time nonce is + * retired by its use, and AuthDigestNcCheck needs the last nonce-count. A + * client is issued an opaque iff so, since the opaque is the key to that + * state. */ +static int tracks_clients(const digest_config_rec *conf) +{ + return conf->check_nc || conf->nonce_lifetime == 0; +} + #define DFLT_ALGORITHM "MD5" #define DFLT_NONCE_LIFE apr_time_from_sec(300) @@ -181,7 +190,8 @@ enum hdr_sts { NO_HEADER, NOT_DIGEST, INVALID, VALID }; * tracked for its client. */ enum nonce_state { NONCE_ACCEPTED, /* recorded as the latest used by this client */ - NONCE_STALE, /* already used, or the client is unknown */ + NONCE_UNKNOWN, /* the client has no entry (garbage collected?) */ + NONCE_STALE, /* one-time nonce already used */ NONCE_BAD_COUNT, /* nonce-count did not increase: possible replay */ NONCE_NOT_OWNER /* the entry quoted belongs to another user */ }; @@ -749,38 +759,41 @@ static apr_uint64_t owner_hash(request_rec *r) * The opaque is public, so a verified request from anybody with an account * on the realm could otherwise do so too. An entry is bound to the user * who first used it, and a request from another user quoting it is - * NONCE_NOT_OWNER, leaving the entry alone. */ + * NONCE_NOT_OWNER, leaving the entry alone. + * + * The nonce-count last used by the client is returned in *tracked_nc (zero + * if the client is unknown or not this user's), for the caller to log. */ static enum nonce_state client_update_nonce(request_rec *r, client_id_t key, const digest_config_rec *conf, apr_time_t nonce_time, unsigned long nc, - const char *nonce) + unsigned long *tracked_nc) { client_entry *client; - unsigned long tracked = 0; enum nonce_state state; - int known; apr_uint64_t owner = owner_hash(r); + *tracked_nc = 0; + apr_global_mutex_lock(client_lock); client = find_client(key); - known = (client != NULL); - if (!known) { - state = NONCE_STALE; + if (client == NULL) { + state = NONCE_UNKNOWN; } else if (client->owner && client->owner != owner) { state = NONCE_NOT_OWNER; } else { - tracked = client->nonce_count; + *tracked_nc = client->nonce_count; if (conf->nonce_lifetime == 0) { /* one-time nonce: usable until it has been used */ state = (nonce_time > client->last_nonce_time) ? NONCE_ACCEPTED : NONCE_STALE; } else if (nonce_time > client->last_nonce_time - || (nonce_time == client->last_nonce_time && nc > tracked)) { + || (nonce_time == client->last_nonce_time + && nc > *tracked_nc)) { state = NONCE_ACCEPTED; } else { @@ -795,28 +808,6 @@ static enum nonce_state client_update_nonce(request_rec *r, } apr_global_mutex_unlock(client_lock); - if (!known) { - ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(10618) - "client %" APR_UINT64_T_FMT " is no longer known - " - "sending new nonce", key); - } - else if (state == NONCE_NOT_OWNER) { - ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO() - "user %s: client %" APR_UINT64_T_FMT " belongs to " - "another user - sending new nonce and opaque", - r->user, key); - } - else if (state == NONCE_STALE) { - ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(01779) - "user %s: one-time-nonce %s already used - sending " - "new nonce", r->user, nonce); - } - else if (state == NONCE_BAD_COUNT) { - ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(01774) - "user %s: nonce-count %lu is not above %lu for nonce " - "%s - sending new nonce", r->user, nc, tracked, nonce); - } - return state; } @@ -1115,7 +1106,7 @@ static digest_header_rec *make_digest_rec(request_rec *store, request_rec *r) * updated here: the state of an authenticated client must not be altered * by a request which has not (yet) been authenticated, or a replayed or * bogus request quoting the client's opaque would be able to rewind that - * state. See check_and_record_nonce(). + * state. See check_nonce(). * * Note that this must be called after mod_proxy had its go so that * r->proxyreq is set correctly. @@ -1262,7 +1253,7 @@ static int note_digest_auth_failure(request_rec *r, if (resp->opaque == NULL) { /* new client */ - if (conf->check_nc || conf->nonce_lifetime == 0) { + if (tracks_clients(conf)) { if ((client_key = client_generate(r, 0)) == 0) { return HTTP_SERVICE_UNAVAILABLE; } @@ -1411,9 +1402,17 @@ static authn_status get_hash(request_rec *r, const char *user, return auth_result; } -/* Check the nonce and nonce-count of a fully verified request against the - * state tracked for its client, record them, and generate a new challenge - * if they are not acceptable. +/* Check the nonce of a fully verified request, record its use, and + * generate a new challenge if it is not acceptable. + * + * This runs only once the response digest has been verified, so the + * stale=true it answers with never reveals whether the credentials were + * right. The nonce itself is checked first - that this server issued it, + * for this opaque and realm, and that it is within its lifetime - and only + * then its use by the client, so that the state tracked for a client is + * altered only by a request using a nonce this server issued, and a request + * using an expired nonce gets a stale challenge (and hence a silent retry + * with a fresh nonce-count) rather than being refused as a replay. * * The nonce-count is counted by the client per-nonce (RFC 7616 3.4.3), so * the count tracked here is tied to the nonce it was counted for: a request @@ -1423,47 +1422,13 @@ static authn_status get_hash(request_rec *r, const char *user, * which turn out not to need authentication, and this server never sees * those. */ -static int check_and_record_nonce(request_rec *r, digest_header_rec *resp, - const digest_config_rec *conf) -{ - if (!conf->check_nc && conf->nonce_lifetime != 0) { - return OK; /* nothing is tracked per-client */ - } - - switch (client_update_nonce(r, resp->opaque_num, conf, resp->nonce_time, - resp->nc, resp->nonce)) { - case NONCE_ACCEPTED: - return OK; - - case NONCE_STALE: - case NONCE_BAD_COUNT: - /* the credentials were good, so the client can silently retry with - * the nonce from this challenge. A nonce-count which has not - * increased is what concurrent requests from one client look like - * when they arrive out of order, and a replay gains nothing from - * the stale flag, having no way to answer the new nonce. */ - return note_digest_auth_failure(r, conf, resp, 1); - - case NONCE_NOT_OWNER: - /* likewise, but with an entry of its own rather than the one it - * quoted: dropping the opaque has the challenge issue a new one. - * Both forms go, since the record is shared with any later - * request in this tree. */ - resp->opaque = NULL; - resp->opaque_num = 0; - return note_digest_auth_failure(r, conf, resp, 1); - } - - return HTTP_INTERNAL_SERVER_ERROR; /* not reached */ -} - static int check_nonce(request_rec *r, digest_header_rec *resp, const digest_config_rec *conf) { - apr_time_t dt; time_rec nonce_time; char tmp, hash[NONCE_HASH_LEN+1]; int len; + unsigned long tracked_nc; /* The decoder stops at the first byte which is not base64, so a * nonce of the right shape can still decode to fewer bytes than the @@ -1491,15 +1456,16 @@ static int check_nonce(request_rec *r, digest_header_rec *resp, return note_digest_auth_failure(r, conf, resp, 1); } - dt = r->request_time - nonce_time.time; - if (conf->nonce_lifetime > 0 && dt < 0) { - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01777) - "invalid nonce %s received - user attempted " - "time travel", resp->nonce); - return note_digest_auth_failure(r, conf, resp, 1); - } - if (conf->nonce_lifetime > 0) { + apr_time_t dt = r->request_time - nonce_time.time; + + if (dt < 0) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01777) + "invalid nonce %s received - user attempted " + "time travel", resp->nonce); + return note_digest_auth_failure(r, conf, resp, 1); + } + if (dt > conf->nonce_lifetime) { ap_log_rerror(APLOG_MARK, APLOG_INFO, 0,r, APLOGNO(01778) "user %s: nonce expired (%.2f seconds old " @@ -1509,10 +1475,57 @@ static int check_nonce(request_rec *r, digest_header_rec *resp, return note_digest_auth_failure(r, conf, resp, 1); } } - /* else (lifetime <= 0) => never expires by time; a one-time nonce is - * retired by use, in check_and_record_nonce() */ - return OK; + if (!tracks_clients(conf)) { + return OK; /* a timed nonce, not counted: that's all */ + } + + /* A one-time nonce is retired by its use, and with AuthDigestNcCheck + * the count must rise: both are checked against, and recorded in, the + * state tracked for the client. */ + switch (client_update_nonce(r, resp->opaque_num, conf, nonce_time.time, + resp->nc, &tracked_nc)) { + case NONCE_ACCEPTED: + return OK; + + case NONCE_UNKNOWN: + ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(10618) + "client %" APR_UINT64_T_FMT " is no longer known - " + "sending new nonce", resp->opaque_num); + break; + + case NONCE_STALE: + ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(01779) + "user %s: one-time-nonce %s already used - sending " + "new nonce", r->user, resp->nonce); + break; + + case NONCE_BAD_COUNT: + ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(01774) + "user %s: nonce-count %lu is not above %lu for nonce " + "%s - sending new nonce", r->user, resp->nc, tracked_nc, + resp->nonce); + break; + + case NONCE_NOT_OWNER: + ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO() + "user %s: client %" APR_UINT64_T_FMT " belongs to " + "another user - sending new nonce and opaque", + r->user, resp->opaque_num); + /* dropping the opaque has the challenge issue this user an entry of + * its own rather than the one it quoted. Both forms go, since the + * record is shared with any later request in this tree. */ + resp->opaque = NULL; + resp->opaque_num = 0; + break; + } + + /* the credentials were good, so the client can silently retry with the + * nonce from this challenge. A nonce-count which has not increased is + * what concurrent requests from one client look like when they arrive + * out of order, and a replay gains nothing from the stale flag, having + * no way to answer the new nonce. */ + return note_digest_auth_failure(r, conf, resp, 1); } /* Compute the response digest expected for request r (RFC 2617). */ @@ -1605,7 +1618,6 @@ static int authenticate_digest_user(request_rec *r) digest_config_rec *conf; digest_header_rec *resp; const char *t; - int res; authn_status return_code; const char *realm; @@ -1773,19 +1785,9 @@ static int authenticate_digest_user(request_rec *r) } } - /* The nonce and nonce-count are checked only after the digest has been - * verified above, so the stale=true they can answer with never reveals - * whether the credentials were right. The nonce is checked before the - * nonce-count so that the nonce-count state is only ever updated for a - * request which is using a nonce this server issued, and so that a - * request using an expired nonce gets a "stale=true" challenge (and - * hence a silent retry with a fresh nonce-count) rather than being - * reported as a replay. */ - if ((res = check_nonce(r, resp, conf))) { - return res; - } - - return check_and_record_nonce(r, resp, conf); + /* Only now that the digest is verified is the nonce checked; see + * check_nonce() for why. */ + return check_nonce(r, resp, conf); } /* Authentication-Info header code. */