From def1f34ecdef52cf248ecaefb09bfb37e198242d Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Mon, 5 Oct 2026 18:49:31 +0100 Subject: [PATCH 01/10] mod_auth_digest: Simplify further, moving the client_id counter into the client_list struct; no functional change intended. * modules/aaa/mod_auth_digest.c (struct hash_table): Add next_id field; note that all access must be inside client_lock. (client_id_counter): Remove global. (initialize_tables): Seed client_list->next_id rather than allocating and seeding client_id_counter. (add_client): Remove, merging into... (client_generate): ...here; issue the id from client_list->next_id inside client_lock rather than via atomics, and log allocation failure. Co-Authored-By: Claude Opus 5.5 (1M context) --- modules/aaa/mod_auth_digest.c | 105 ++++++++++++---------------------- 1 file changed, 36 insertions(+), 69 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index a1574fe856d..71ecb8b06e4 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -146,8 +146,8 @@ typedef struct digest_config_struct { /* Identifies a client entry. This is the value sent to the client in the * opaque field of the challenge, and echoed back in its Authorization * header; zero is never a valid id, and means "no client". Ids are counted - * out by client_id_counter, so this must remain the type which the atomics - * used on it take, and the "%u"/"%x" formats below must match it. */ + * out by client_generate() from client_list->next_id, and the "%u"/"%x" + * formats below must match this type. */ typedef apr_uint32_t client_id_t; typedef struct hash_entry { @@ -159,6 +159,9 @@ typedef struct hash_entry { * accepted for this client */ } client_entry; +/* The client table, in the shared memory segment. Once initialize_tables() + * has set it up, the table, the entries reachable from it and every field + * below may only be read or modified with client_lock held. */ static struct hash_table { client_entry **table; unsigned long tbl_len; @@ -166,11 +169,10 @@ static struct hash_table { unsigned long num_created; unsigned long num_removed; unsigned long num_renewed; + client_id_t next_id; /* the last id issued */ } *client_list; - -/* struct to hold a parsed Authorization header */ - +/* Outcome from parsing an Authorization header. */ enum hdr_sts { NO_HEADER, NOT_DIGEST, INVALID, VALID }; /* Outcome of checking a request's nonce and nonce-count against the state @@ -181,6 +183,7 @@ enum nonce_state { NONCE_BAD_COUNT /* nonce-count did not increase: possible replay */ }; +/* struct to hold a parsed Authorization header */ typedef struct digest_header_struct { const char *scheme; const char *realm; @@ -218,7 +221,6 @@ static unsigned char *secret; static apr_shm_t *client_shm = NULL; static apr_rmm_t *client_rmm = NULL; -static volatile client_id_t *client_id_counter; static volatile apr_uint32_t *otn_counter; /* one-time-nonce counter */ static apr_global_mutex_t *client_lock = NULL; static const char *client_mutex_type = "authdigest-client"; @@ -301,7 +303,6 @@ static int initialize_tables(server_rec *s, apr_pool_t *ctx) { unsigned long idx; apr_status_t sts; - client_id_t seed; /* set up client list */ @@ -360,6 +361,15 @@ static int initialize_tables(server_rec *s, apr_pool_t *ctx) } client_list->tbl_len = num_buckets; client_list->num_entries = 0; + /* Start the ids at a random point rather than at 1. This segment does + * not survive a restart, but the nonces naming its entries do, since the + * secret they are hashed with is retained; ids restarting from 1 too + * would hand a returning client's id straight back out, so that client + * would be checked against whichever new client now held it. The ids are + * not secret - they are sent in the clear as the opaque - this only has + * to make them distinct across a restart. */ + ap_random_insecure_bytes(&client_list->next_id, + sizeof client_list->next_id); sts = ap_global_mutex_create(&client_lock, NULL, client_mutex_type, NULL, s, ctx, 0); @@ -369,23 +379,6 @@ static int initialize_tables(server_rec *s, apr_pool_t *ctx) } - /* setup opaque */ - - client_id_counter = rmm_malloc(client_rmm, sizeof *client_id_counter); - if (client_id_counter == NULL) { - log_error_and_cleanup("failed to allocate shared memory", -1, s); - return !OK; - } - /* Start the ids at a random point rather than at 1. This segment does - * not survive a restart, but the nonces naming its entries do, since the - * secret they are hashed with is retained; ids restarting from 1 too - * would hand a returning client's id straight back out, so that client - * would be checked against whichever new client now held it. The ids are - * not secret - they are sent in the clear as the opaque - this only has - * to make them distinct across a restart. */ - ap_random_insecure_bytes(&seed, sizeof seed); - *client_id_counter = seed; - /* setup one-time-nonce counter */ otn_counter = rmm_malloc(client_rmm, sizeof(*otn_counter)); @@ -901,20 +894,16 @@ static unsigned long gc(server_rec *s) /* - * Add a new client to the list. Returns non-zero 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. 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 int add_client(client_id_t key, client_entry *info, server_rec *s) +static client_id_t client_generate(request_rec *r) { + server_rec *s = r->server; int bucket; client_entry *entry; - - if (!key) { - return 0; - } - - bucket = key % client_list->tbl_len; + client_id_t key; apr_global_mutex_lock(client_lock); @@ -932,13 +921,22 @@ static int add_client(client_id_t key, client_entry *info, server_rec *s) entry = rmm_malloc(client_rmm, sizeof(client_entry)); if (!entry) { apr_global_mutex_unlock(client_lock); - return 0; /* give up; the caller logs this */ + 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 add the entry */ + /* now issue the id and add the entry; the id wraps after 2^32 + * clients, so skip zero, which means "no client" */ - memcpy(entry, info, sizeof(client_entry)); + if ((key = ++client_list->next_id) == 0) { + key = ++client_list->next_id; + } + 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; @@ -950,7 +948,7 @@ static int add_client(client_id_t key, client_entry *info, server_rec *s) ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(01768) "allocated new client %u", key); - return 1; + return key; } @@ -1190,37 +1188,6 @@ static const char *gen_nonce(apr_pool_t *p, apr_time_t now, const char *opaque, } -/* - * Opaque and hash-table management - */ - -/* - * Generate a new client entry and add it to the list. Returns the key of - * the new entry, or 0 if it failed. (The entry itself is deliberately not - * returned: see find_client().) - */ -static client_id_t client_generate(const request_rec *r) -{ - client_id_t op = apr_atomic_inc32(client_id_counter); - client_entry new_entry = { 0, NULL, 0, 0 }; - - /* The counter wraps after 2^32 clients: skip an id of zero, which means - * "no client" and which add_client() would refuse. */ - if (op == 0) { - op = apr_atomic_inc32(client_id_counter); - } - - if (!add_client(op, &new_entry, r->server)) { - 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; - } - - return op; -} - - /* * Authorization challenge generation code (for WWW-Authenticate) */ From 4a05c775a1b2b54b3883e3885aef78312ccdf66c Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Mon, 5 Oct 2026 20:43:16 +0100 Subject: [PATCH 02/10] mod_auth_digest: Widen client ids to 64 bits: * modules/aaa/mod_auth_digest.c (client_id_t): Widen to apr_uint64_t. (CLIENT_ID_MAX): New. (initialize_tables): Cap the seed at CLIENT_ID_MAX. (client_generate): Issue ids in 1..CLIENT_ID_MAX. (client_exists, client_update_nonce, client_generate): Log ids with APR_UINT64_T_FMT. (parse_digest_header): Parse the opaque with apr_strtoi64(), returning INVALID if it is not in 1..CLIENT_ID_MAX. (authenticate_digest_user): Remove the now-unreachable invalid opaque check (AH01787); mention an invalid opaque in AH01782. (ltox): Rename to... (client_id_to_opaque): ...this; format with APR_UINT64_T_HEX_FMT. (note_digest_auth_failure): Update callers. * test/modules/aaa/test_001_challenge_response.py (test_digest_013_invalid_opaque): Accept AH01782 as well as AH01787. * test/modules/aaa/test_003_nccheck.py (test_digest_035_out_of_range_opaque_is_not_truncated): Try opaques 2^32 and 2^64 above the live id. Co-Authored-By: Claude Opus 5.5 (1M context) --- modules/aaa/mod_auth_digest.c | 61 ++++++++----------- .../aaa/test_001_challenge_response.py | 2 +- test/modules/aaa/test_003_nccheck.py | 46 +++++++------- 3 files changed, 53 insertions(+), 56 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 71ecb8b06e4..d57ec2776ab 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -146,9 +146,10 @@ typedef struct digest_config_struct { /* Identifies a client entry. This is the value sent to the client in the * opaque field of the challenge, and echoed back in its Authorization * header; zero is never a valid id, and means "no client". Ids are counted - * out by client_generate() from client_list->next_id, and the "%u"/"%x" - * formats below must match this type. */ -typedef apr_uint32_t client_id_t; + * out by client_generate() from client_list->next_id, and are kept within + * 1..CLIENT_ID_MAX so that the opaque can be parsed by apr_strtoi64(). */ +typedef apr_uint64_t client_id_t; +#define CLIENT_ID_MAX APR_INT64_MAX typedef struct hash_entry { client_id_t key; /* the key for this entry */ @@ -366,10 +367,12 @@ static int initialize_tables(server_rec *s, apr_pool_t *ctx) * secret they are hashed with is retained; ids restarting from 1 too * would hand a returning client's id straight back out, so that client * would be checked against whichever new client now held it. The ids are - * not secret - they are sent in the clear as the opaque - this only has - * to make them distinct across a restart. */ + * not secret - they are sent in the clear as the opaque - but over a + * range of 2^63 neither wrapping the counter nor walking it from a + * random start to an id issued before the restart is feasible. */ ap_random_insecure_bytes(&client_list->next_id, sizeof client_list->next_id); + client_list->next_id %= CLIENT_ID_MAX; sts = ap_global_mutex_create(&client_lock, NULL, client_mutex_type, NULL, s, ctx, 0); @@ -736,11 +739,11 @@ static int client_exists(client_id_t key, const request_rec *r) if (found) { ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01764) - "client %u found", key); + "client %" APR_UINT64_T_FMT " found", key); } else { ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01765) - "client %u not found", key); + "client %" APR_UINT64_T_FMT " not found", key); } return found; @@ -819,8 +822,8 @@ static enum nonce_state client_update_nonce(const request_rec *r, if (!known) { ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(10618) - "client %u is no longer known - sending new nonce", - key); + "client %" APR_UINT64_T_FMT " is no longer known - " + "sending new nonce", key); } else if (state == NONCE_STALE) { ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(01779) @@ -928,12 +931,9 @@ static client_id_t client_generate(request_rec *r) } } - /* now issue the id and add the entry; the id wraps after 2^32 - * clients, so skip zero, which means "no client" */ + /* now issue the id, in 1..CLIENT_ID_MAX, and add the entry */ - if ((key = ++client_list->next_id) == 0) { - key = ++client_list->next_id; - } + 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)); @@ -946,7 +946,7 @@ static client_id_t client_generate(request_rec *r) apr_global_mutex_unlock(client_lock); ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(01768) - "allocated new client %u", key); + "allocated new client %" APR_UINT64_T_FMT, key); return key; } @@ -1069,13 +1069,13 @@ static enum hdr_sts parse_digest_header(request_rec *r, if (resp->opaque) { char *endptr; - unsigned long num; + apr_int64_t num; - errno = 0; - num = strtoul(resp->opaque, &endptr, 16); - if (errno == 0 && *endptr == '\0' && num > 0 - && num <= APR_UINT32_MAX) - resp->opaque_num = (client_id_t)num; + num = apr_strtoi64(resp->opaque, &endptr, 16); + if (errno || *endptr != '\0' || num <= 0 || num > CLIENT_ID_MAX) { + return INVALID; + } + resp->opaque_num = (client_id_t)num; } return VALID; @@ -1194,9 +1194,9 @@ static const char *gen_nonce(apr_pool_t *p, apr_time_t now, const char *opaque, /* Format a client id as the opaque sent to the client. Never called with * zero: the callers check client_generate() for failure first. */ -static const char *ltox(apr_pool_t *p, client_id_t num) +static const char *client_id_to_opaque(apr_pool_t *p, client_id_t num) { - return apr_psprintf(p, "%x", num); + return apr_psprintf(p, "%" APR_UINT64_T_HEX_FMT, num); } /* Generate a challenge for the client, and return the status which the @@ -1220,7 +1220,7 @@ static int note_digest_auth_failure(request_rec *r, if ((client_key = client_generate(r)) == 0) { return HTTP_SERVICE_UNAVAILABLE; } - opaque = ltox(r->pool, client_key); + opaque = client_id_to_opaque(r->pool, client_key); } /* else this configuration tracks no per-client state, so no entry * is allocated and no opaque is sent */ @@ -1234,7 +1234,7 @@ static int note_digest_auth_failure(request_rec *r, if ((client_key = client_generate(r)) == 0) { return HTTP_SERVICE_UNAVAILABLE; } - opaque = ltox(r->pool, client_key); + opaque = client_id_to_opaque(r->pool, client_key); stale = 1; client_note_renewed(); } @@ -1607,8 +1607,8 @@ static int authenticate_digest_user(request_rec *r) else if (resp->auth_hdr_sts == INVALID) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01782) "missing user, realm, nonce, uri, digest, " - "cnonce, or nonce_count in authorization header: %s", - r->uri); + "cnonce, or nonce_count, or invalid opaque, in " + "authorization header: %s", r->uri); } /* else (resp->auth_hdr_sts == NO_HEADER) */ return note_digest_auth_failure(r, conf, resp, 0); @@ -1680,13 +1680,6 @@ static int authenticate_digest_user(request_rec *r) return HTTP_BAD_REQUEST; } } - - if (resp->opaque && resp->opaque_num == 0) { - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01787) - "received invalid opaque - got `%s'", - resp->opaque); - return note_digest_auth_failure(r, conf, resp, 0); - } diff --git a/test/modules/aaa/test_001_challenge_response.py b/test/modules/aaa/test_001_challenge_response.py index aa6ff1217b2..71080dab075 100644 --- a/test/modules/aaa/test_001_challenge_response.py +++ b/test/modules/aaa/test_001_challenge_response.py @@ -167,7 +167,7 @@ def test_digest_013_invalid_opaque(self, env): opaque="not-a-hex-number") r = env.curl_get(self.url(env), options=["-H", f"Authorization: {auth}"]) assert r.response["status"] == 401 - env.httpd_error_log.ignore_recent(lognos=["AH01787"]) + env.httpd_error_log.ignore_recent(lognos=["AH01782", "AH01787"]) def test_digest_014_tampered_response_hash(self, env): challenge = self.challenge(env) diff --git a/test/modules/aaa/test_003_nccheck.py b/test/modules/aaa/test_003_nccheck.py index 539c182b66c..aad89a02482 100644 --- a/test/modules/aaa/test_003_nccheck.py +++ b/test/modules/aaa/test_003_nccheck.py @@ -116,28 +116,32 @@ def test_digest_034_no_nccheck_allows_replay(self, env): assert r2.response["status"] == 200 def test_digest_035_out_of_range_opaque_is_not_truncated(self, env): - # The opaque is a 32-bit client id. A value which would truncate onto - # a live id must not select that client. This is observable in the - # challenge which comes back: a client the server still knows is - # re-challenged with its own opaque, whereas an unknown one is given a - # freshly minted opaque and stale=true. + # The opaque is a 32- or 64-bit client id. A value which would + # truncate onto a live id must not select that client. This is + # observable in the challenge which comes back: a client the server + # still knows is re-challenged with its own opaque, whereas an unknown + # one is given a freshly minted opaque and stale=true. challenge = self.challenge(env, "nccheck") assert self.authenticate(env, "nccheck", challenge, nc="00000001").response["status"] == 200 - # 2^32 + the live id, which truncates to the live id in 32 bits - crafted = "%x" % ((1 << 32) + int(challenge.opaque, 16)) - auth = dc.build_authorization( - AAATestEnv.DIGEST_USER, challenge, AAATestEnv.DIGEST_PASSWORD, - method="GET", uri="/digest/nccheck/secret.txt", nc="00000002", - cnonce="trunc-cnonce", opaque=crafted) - r = env.curl_get(env.mkurl("http", "aaa", "/digest/nccheck/secret.txt"), - options=["-H", f"Authorization: {auth}"]) - # AH01787 with the range check in place; AH01776 (nonce hash) if the - # opaque were truncated onto the live client instead - env.httpd_error_log.ignore_recent(lognos=["AH01787", "AH01776"]) - assert r.response["status"] == 401 - new_challenge = dc.DigestChallenge.parse( - r.response["header"]["www-authenticate"]) - assert new_challenge.opaque != crafted, \ - "an out-of-range opaque was truncated onto a live client id" + # 2^bits + the live id, which truncates to the live id in that many + # bits; one of the two is out of range whichever width is in use + for bits in (32, 64): + crafted = "%x" % ((1 << bits) + int(challenge.opaque, 16)) + auth = dc.build_authorization( + AAATestEnv.DIGEST_USER, challenge, AAATestEnv.DIGEST_PASSWORD, + method="GET", uri="/digest/nccheck/secret.txt", nc="00000002", + cnonce="trunc-cnonce", opaque=crafted) + r = env.curl_get(env.mkurl("http", "aaa", "/digest/nccheck/secret.txt"), + options=["-H", f"Authorization: {auth}"]) + # AH01782 (AH01787 before 64-bit ids) for an opaque out of range, + # AH01776 (nonce hash) for one in range but not the opaque the + # nonce was issued with + env.httpd_error_log.ignore_recent(lognos=["AH01782", "AH01787", + "AH01776"]) + assert r.response["status"] == 401 + new_challenge = dc.DigestChallenge.parse( + r.response["header"]["www-authenticate"]) + assert new_challenge.opaque != crafted, \ + f"an opaque 2^{bits} above a live client id was truncated onto it" From 93dc0244558c57ab8b81363c6fc4f35630dd599e Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Mon, 5 Oct 2026 21:49:04 +0100 Subject: [PATCH 03/10] * modules/aaa/mod_auth_digest.c (struct hash_table): Clarify that the next_id seed is never itself issued. (authenticate_digest_user): Remove stray whitespace left by the removal of the opaque check. No functional change. [skip ci] Co-Authored-By: Claude Fable 5 --- modules/aaa/mod_auth_digest.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index d57ec2776ab..05565a939c2 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -170,7 +170,8 @@ static struct hash_table { unsigned long num_created; unsigned long num_removed; unsigned long num_renewed; - client_id_t next_id; /* the last id issued */ + client_id_t next_id; /* the last id issued (the random + * initial seed is never issued) */ } *client_list; /* Outcome from parsing an Authorization header. */ @@ -1680,8 +1681,6 @@ static int authenticate_digest_user(request_rec *r) return HTTP_BAD_REQUEST; } } - - if (!realm) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02533) From edec297b64c6679bc07884bd9205b0a6b438c42c Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Mon, 5 Oct 2026 23:25:27 +0100 Subject: [PATCH 04/10] mod_auth_digest: Fix handling of a request with no per-request record: The per-request record is created in a post_read_request hook, which does not run for a request rejected earlier (e.g. no Host on HTTP/1.1). An ErrorDocument directing such a request into a Digest-protected location then reached the auth hooks, which walked ->prev to the original request and used its record without checking it had been created. * modules/aaa/mod_auth_digest.c (make_digest_rec): New, split from init_digest_request, storing the record on a given request. (get_digest_rec): New; walk to the shared record and create it if the originating request never ran init_digest_request. (authenticate_digest_user, hook_note_digest_auth_failure): Use it in place of the open-coded walk, which did not check for a missing record. * test/modules/aaa/conftest.py: Add an ErrorDocument 400 into a Digest-protected location. * test/modules/aaa/test_012_errordoc.py: New test suite. Co-Authored-By: Claude Opus 4.8 --- modules/aaa/mod_auth_digest.c | 81 ++++++++++++++------------- test/modules/aaa/conftest.py | 5 ++ test/modules/aaa/test_012_errordoc.py | 37 ++++++++++++ 3 files changed, 83 insertions(+), 40 deletions(-) create mode 100644 test/modules/aaa/test_012_errordoc.py diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 05565a939c2..3d7693e82a8 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -1083,6 +1083,21 @@ static enum hdr_sts parse_digest_header(request_rec *r, } +/* Create the per-request Digest record describing request r, store it on + * store's request_config, and parse r's Authorization header into it. */ +static digest_header_rec *make_digest_rec(request_rec *store, request_rec *r) +{ + digest_header_rec *resp = apr_pcalloc(store->pool, sizeof(*resp)); + + resp->raw_request_uri = r->unparsed_uri; + resp->psd_request_uri = &r->parsed_uri; + resp->method = r->method; + ap_set_module_config(store->request_config, &auth_digest_module, resp); + + resp->auth_hdr_sts = parse_digest_header(r, resp); + return resp; +} + /* Set up the per-request record: this is the place to get the request-uri * (before any subrequests etc are initiated), to initialize the * request_config, and to parse the Authorization header. @@ -1098,22 +1113,36 @@ static enum hdr_sts parse_digest_header(request_rec *r, */ static int init_digest_request(request_rec *r) { - digest_header_rec *resp; - if (!ap_is_initial_req(r)) { return DECLINED; } - resp = apr_pcalloc(r->pool, sizeof(digest_header_rec)); - resp->raw_request_uri = r->unparsed_uri; - resp->psd_request_uri = &r->parsed_uri; - resp->needed_auth = 0; - resp->method = r->method; - ap_set_module_config(r->request_config, &auth_digest_module, resp); + make_digest_rec(r, r); + return DECLINED; +} - resp->auth_hdr_sts = parse_digest_header(r, resp); +/* Return the Digest record shared across this request tree, which lives on + * the initial request. init_digest_request creates it in post_read_request; + * a request which never ran that hook (one rejected before it and brought + * here by an ErrorDocument or other internal redirect) has none, so create + * it now rather than dereferencing NULL. */ +static digest_header_rec *get_digest_rec(request_rec *r) +{ + request_rec *mainreq = r; + digest_header_rec *resp; - return DECLINED; + while (mainreq->main != NULL) { + mainreq = mainreq->main; + } + while (mainreq->prev != NULL) { + mainreq = mainreq->prev; + } + + resp = ap_get_module_config(mainreq->request_config, &auth_digest_module); + if (resp == NULL) { + resp = make_digest_rec(mainreq, r); + } + return resp; } @@ -1289,29 +1318,15 @@ static int note_digest_auth_failure(request_rec *r, static int hook_note_digest_auth_failure(request_rec *r, const char *auth_type) { - request_rec *mainreq; digest_header_rec *resp; digest_config_rec *conf; if (ap_cstr_casecmp(auth_type, "Digest")) return DECLINED; - /* get the client response and mark */ - - mainreq = r; - while (mainreq->main != NULL) { - mainreq = mainreq->main; - } - while (mainreq->prev != NULL) { - mainreq = mainreq->prev; - } - resp = (digest_header_rec *) ap_get_module_config(mainreq->request_config, - &auth_digest_module); + resp = get_digest_rec(r); resp->needed_auth = 1; - - /* get our conf */ - conf = (digest_config_rec *) ap_get_module_config(r->per_dir_config, &auth_digest_module); @@ -1557,7 +1572,6 @@ static int authenticate_digest_user(request_rec *r) { digest_config_rec *conf; digest_header_rec *resp; - request_rec *mainreq; const char *t; int res; authn_status return_code; @@ -1575,24 +1589,11 @@ static int authenticate_digest_user(request_rec *r) return HTTP_INTERNAL_SERVER_ERROR; } - - /* get the client response and mark */ - - mainreq = r; - while (mainreq->main != NULL) { - mainreq = mainreq->main; - } - while (mainreq->prev != NULL) { - mainreq = mainreq->prev; - } - resp = (digest_header_rec *) ap_get_module_config(mainreq->request_config, - &auth_digest_module); + resp = get_digest_rec(r); resp->needed_auth = 1; realm = ap_auth_name(r); - /* get our conf */ - conf = (digest_config_rec *) ap_get_module_config(r->per_dir_config, &auth_digest_module); diff --git a/test/modules/aaa/conftest.py b/test/modules/aaa/conftest.py index 1ea2aba7966..bf030e80304 100644 --- a/test/modules/aaa/conftest.py +++ b/test/modules/aaa/conftest.py @@ -94,6 +94,11 @@ def env(pytestconfig) -> AAATestEnv: # AuthDigestProvider intentionally omitted: falls back to "file". f'AuthUserFile "{pwfile}"', ])) + # A request which fails header validation (e.g. no Host on HTTP/1.1) + # is answered before the Digest module's post_read_request runs, so it + # has no per-request Digest record; an ErrorDocument redirecting it into + # a Digest-protected location must not then dereference that record. + conf.add('ErrorDocument 400 /digest/default/secret.txt') conf.install() assert env.apache_restart() == 0 return env diff --git a/test/modules/aaa/test_012_errordoc.py b/test/modules/aaa/test_012_errordoc.py new file mode 100644 index 00000000000..6868267540b --- /dev/null +++ b/test/modules/aaa/test_012_errordoc.py @@ -0,0 +1,37 @@ +"""An ErrorDocument redirecting into a Digest-protected location. + +A request which fails header validation -- e.g. an HTTP/1.1 request with no +Host header -- is rejected in ap_check_request_header, before +ap_post_read_request runs. The Digest module sets up its per-request record +in a post_read_request hook, so that record was never created for such a +request. + +If ErrorDocument then redirects the failure into a Digest-protected +location, the auth hook runs for the redirect and walks back along ->prev +to the original request to find the shared record -- which is NULL here. +Dereferencing it crashes the child, so an unauthenticated client can take +down a worker with a single malformed request. +""" + +from .env import AAATestEnv + + +class TestDigestErrorDocument: + + def test_digest_125_errordoc_into_digest_without_prr(self, env): + # Send an HTTP/1.1 request with the Host header removed; the server + # answers 400 before the Digest post_read_request hook runs, and the + # ErrorDocument points into a Digest-protected location. + url = env.mkurl("http", "aaa", "/anything") + r = env.curl_get(url, options=["--http1.1", "-H", "Host:"]) + + # The request must get an HTTP response, not a reset connection from + # a crashed child. + assert r.exit_code == 0, \ + f"no HTTP response (curl exit {r.exit_code}): the worker likely " \ + f"crashed handling the ErrorDocument" + assert r.response["status"] in (400, 401) + + # The server is still serving afterwards. + r = env.curl_get(env.mkurl("http", "aaa", "/digest/default/secret.txt")) + assert r.response["status"] == 401 From c80bedb938591588e47146a6caceb9e608a2b9e0 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Tue, 6 Oct 2026 10:18:16 +0100 Subject: [PATCH 05/10] mod_auth_digest: Validate the nonce-count when parsing the header: The nonce-count was parsed with strtol into an unsigned long only where AuthDigestNcCheck or one-time nonces are in use, so a negative value was stored as a huge count (locking the client out) and overflow was clamped silently; the raw string was also echoed unquoted into Authentication-Info. * modules/aaa/mod_auth_digest.c (digest_header_rec): Add nc, the parsed nonce-count. (parse_digest_header): Require nc to be 8 hex digits (RFC 7616 3.4) and store its value, so it is well-formed everywhere it is used or echoed. (check_and_record_nonce): Use the parsed value; drop the local strtol and the now-unreachable AH01773. * test/modules/aaa/test_003_nccheck.py (test_digest_036_malformed_nc_rejected, test_digest_037_malformed_nc_does_not_poison_client): New. Co-Authored-By: Claude Opus 4.8 --- modules/aaa/mod_auth_digest.c | 29 ++++++++++++++++----------- test/modules/aaa/test_003_nccheck.py | 30 ++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 12 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 3d7693e82a8..2f774dd26d3 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -200,6 +200,7 @@ typedef struct digest_header_struct { client_id_t opaque_num; const char *message_qop; const char *nonce_count; + unsigned long nc; /* nonce_count parsed, when valid */ /* the following fields are not (directly) from the header */ const char *raw_request_uri; apr_uri_t *psd_request_uri; @@ -1079,6 +1080,21 @@ static enum hdr_sts parse_digest_header(request_rec *r, resp->opaque_num = (client_id_t)num; } + /* The nonce-count, when present, is 8 hex digits (RFC 7616 3.4); check + * it here so it is a well-formed count everywhere it is later used or + * echoed, rather than parsed leniently into a wrapped or negative value + * or reflected verbatim into the Authentication-Info header. */ + if (resp->nonce_count != NULL) { + int i; + for (i = 0; i < 8 && apr_isxdigit(resp->nonce_count[i]); i++) { + continue; + } + if (i != 8 || resp->nonce_count[8] != '\0') { + return INVALID; + } + resp->nc = strtoul(resp->nonce_count, NULL, 16); + } + return VALID; } @@ -1414,23 +1430,12 @@ static authn_status get_hash(request_rec *r, const char *user, static int check_and_record_nonce(request_rec *r, digest_header_rec *resp, const digest_config_rec *conf) { - unsigned long nc; - const char *snc = resp->nonce_count; - char *endptr; - if (!conf->check_nc && conf->nonce_lifetime != 0) { return OK; /* nothing is tracked per-client */ } - nc = strtol(snc, &endptr, 16); - if (endptr < (snc+strlen(snc)) && !apr_isspace(*endptr)) { - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01773) - "invalid nc %s received - not a number", snc); - return note_digest_auth_failure(r, conf, resp, 0); - } - switch (client_update_nonce(r, resp->opaque_num, conf, resp->nonce_time, - nc, resp->nonce)) { + resp->nc, resp->nonce)) { case NONCE_ACCEPTED: return OK; diff --git a/test/modules/aaa/test_003_nccheck.py b/test/modules/aaa/test_003_nccheck.py index aad89a02482..54b8f0b52cd 100644 --- a/test/modules/aaa/test_003_nccheck.py +++ b/test/modules/aaa/test_003_nccheck.py @@ -145,3 +145,33 @@ def test_digest_035_out_of_range_opaque_is_not_truncated(self, env): r.response["header"]["www-authenticate"]) assert new_challenge.opaque != crafted, \ f"an opaque 2^{bits} above a live client id was truncated onto it" + + def test_digest_036_malformed_nc_rejected(self, env): + # The nonce-count is 8 hex digits. A value which is not -- a negative + # number, non-hex, or the wrong length -- must be rejected rather + # than parsed leniently into a wrapped count. + challenge = self.challenge(env, "nccheck") + for bad in ["-0000001", "zzzzzzzz", "000000001", "1", "0000 001"]: + r = self.authenticate(env, "nccheck", challenge, nc=bad, + cnonce=f"nc-{len(bad)}") + assert r.response["status"] == 401, f"nc={bad!r} was accepted" + env.httpd_error_log.ignore_recent(lognos=["AH01782"]) + + def test_digest_037_malformed_nc_does_not_poison_client(self, env): + # A correctly-signed request carrying nc="-0000001" must not be + # accepted: before the count was validated it was read as a huge + # unsigned value and stored, locking the client out of every later + # (smaller) in-sequence count. + challenge = self.challenge(env, "nccheck") + assert self.authenticate(env, "nccheck", challenge, + nc="00000001").response["status"] == 200 + + r = self.authenticate(env, "nccheck", challenge, nc="-0000001", + cnonce="poison-cnonce") + assert r.response["status"] == 401, "a negative nc was accepted" + + # the legitimate client carries on with its next in-sequence count + assert self.authenticate(env, "nccheck", challenge, + nc="00000002").response["status"] == 200, \ + "a malformed nc locked the client out" + env.httpd_error_log.ignore_recent(lognos=["AH01782"]) From 8bace247d29e372ffc9632eb8032a81aa6a25bc6 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Tue, 6 Oct 2026 10:47:04 +0100 Subject: [PATCH 06/10] mod_auth_digest: Reject an Authorization uri which fails to unescape: The uri-consistency check %-decoded both the request-target and the Authorization uri= but ignored the result, so an encoded NUL or slash in uri= was truncated at the bad octet and its prefix matched the request-target rather than being rejected. * modules/aaa/mod_auth_digest.c (copy_uri_components): Return the ap_unescape_url status rather than discarding it. (authenticate_digest_user): Fail the request when the request-target or the Authorization uri does not unescape. * test/modules/aaa/test_001_challenge_response.py (test_digest_015_uri_bad_escaping_rejected): New. Co-Authored-By: Claude Opus 4.8 --- modules/aaa/mod_auth_digest.c | 41 +++++++++++-------- .../aaa/test_001_challenge_response.py | 17 ++++++++ 2 files changed, 40 insertions(+), 18 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 2f774dd26d3..8ad86981259 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -1516,8 +1516,10 @@ static const char *new_digest(const request_rec *r, NULL)); } -static void copy_uri_components(apr_uri_t *dst, - apr_uri_t *src, request_rec *r) { +static int copy_uri_components(apr_uri_t *dst, + apr_uri_t *src, request_rec *r) { + int rv; + if (src->scheme && src->scheme[0] != '\0') { dst->scheme = src->scheme; } @@ -1527,7 +1529,9 @@ static void copy_uri_components(apr_uri_t *dst, if (src->hostname && src->hostname[0] != '\0') { dst->hostname = apr_pstrdup(r->pool, src->hostname); - ap_unescape_url(dst->hostname); + if ((rv = ap_unescape_url(dst->hostname)) != OK) { + return rv; + } } else { dst->hostname = (char *) ap_get_server_name(r); @@ -1542,7 +1546,9 @@ static void copy_uri_components(apr_uri_t *dst, if (src->path && src->path[0] != '\0') { dst->path = apr_pstrdup(r->pool, src->path); - ap_unescape_url(dst->path); + if ((rv = ap_unescape_url(dst->path)) != OK) { + return rv; + } } else { dst->path = src->path; @@ -1550,13 +1556,16 @@ static void copy_uri_components(apr_uri_t *dst, if (src->query && src->query[0] != '\0') { dst->query = apr_pstrdup(r->pool, src->query); - ap_unescape_url(dst->query); + if ((rv = ap_unescape_url(dst->query)) != OK) { + return rv; + } } else { dst->query = src->query; } dst->hostinfo = src->hostinfo; + return OK; } /* These functions return 0 if client is OK, and proper error status @@ -1632,25 +1641,21 @@ static int authenticate_digest_user(request_rec *r) */ apr_uri_t r_uri, d_uri; - copy_uri_components(&r_uri, resp->psd_request_uri, r); - if (apr_uri_parse(r->pool, resp->uri, &d_uri) != APR_SUCCESS) { + if (copy_uri_components(&r_uri, resp->psd_request_uri, r) != OK) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO() + "invalid request-uri <%s>", r->unparsed_uri); + return HTTP_BAD_REQUEST; + } + if (apr_uri_parse(r->pool, resp->uri, &d_uri) != APR_SUCCESS + || (d_uri.hostname && ap_unescape_url(d_uri.hostname) != OK) + || (d_uri.path && ap_unescape_url(d_uri.path) != OK) + || (d_uri.query && ap_unescape_url(d_uri.query) != OK)) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01783) "invalid uri <%s> in Authorization header", resp->uri); return HTTP_BAD_REQUEST; } - if (d_uri.hostname) { - ap_unescape_url(d_uri.hostname); - } - if (d_uri.path) { - ap_unescape_url(d_uri.path); - } - - if (d_uri.query) { - ap_unescape_url(d_uri.query); - } - if (r->method_number == M_CONNECT) { if (!r_uri.hostinfo || strcmp(resp->uri, r_uri.hostinfo)) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01785) diff --git a/test/modules/aaa/test_001_challenge_response.py b/test/modules/aaa/test_001_challenge_response.py index 71080dab075..92b54b5c45c 100644 --- a/test/modules/aaa/test_001_challenge_response.py +++ b/test/modules/aaa/test_001_challenge_response.py @@ -178,3 +178,20 @@ def test_digest_014_tampered_response_hash(self, env): r = env.curl_get(self.url(env), options=["-H", f"Authorization: {auth}"]) assert r.response["status"] == 401 env.httpd_error_log.ignore_recent(lognos=["AH01794"]) + + def test_digest_015_uri_bad_escaping_rejected(self, env): + # The Authorization uri= is compared against the request-target after + # %-decoding both. A uri= that fails to decode -- an encoded NUL or + # slash -- must be rejected, not silently truncated at the bad octet + # so that its prefix matches the request-target. + challenge = self.challenge(env) + for bad_uri in ["/digest/default/secret.txt%00ignored", + "/digest/default%2fsecret.txt"]: + auth = dc.build_authorization( + AAATestEnv.DIGEST_USER, challenge, AAATestEnv.DIGEST_PASSWORD, + method="GET", uri=bad_uri) + r = env.curl_get(self.url(env), + options=["-H", f"Authorization: {auth}"]) + assert r.response["status"] == 400, \ + f"uri={bad_uri!r} was not rejected" + env.httpd_error_log.ignore_recent(lognos=["AH01783"]) From bb8a7495e24d0d57fd30b4bb2e4144b75a497d07 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Tue, 6 Oct 2026 10:49:36 +0100 Subject: [PATCH 07/10] mod_auth_digest: Send AuthDigestDomain for a reverse-proxied request: The domain was suppressed for any proxy request, but a reverse-proxied request authenticates to this server with an origin-style WWW-Authenticate, so AuthDigestDomain applies to it as it does to a non-proxied request; only a forward-proxy (PROXYREQ_PROXY) request, whose challenge is Proxy-Authenticate, should omit it. * modules/aaa/mod_auth_digest.c (note_digest_auth_failure): Suppress domain= only for PROXYREQ_PROXY, matching the rest of the file. * test/modules/aaa/env.py: Load mod_proxy and mod_proxy_http. * test/modules/aaa/conftest.py: Add a reverse-proxied location which authenticates with Digest and sets AuthDigestDomain. * test/modules/aaa/test_013_proxy_domain.py: New test suite. Co-Authored-By: Claude Opus 4.8 --- modules/aaa/mod_auth_digest.c | 2 +- test/modules/aaa/conftest.py | 15 ++++++++++++++ test/modules/aaa/env.py | 2 +- test/modules/aaa/test_013_proxy_domain.py | 25 +++++++++++++++++++++++ 4 files changed, 42 insertions(+), 2 deletions(-) create mode 100644 test/modules/aaa/test_013_proxy_domain.py diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 8ad86981259..3106edbeca8 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -1303,7 +1303,7 @@ static int note_digest_auth_failure(request_rec *r, /* Setup domain, which tells the client which URIs share this * protection space, so that it does not send the Authorization header * (usually more than 200 bytes) where it is not needed. */ - if (r->proxyreq || !conf->uri_list) { + if (r->proxyreq == PROXYREQ_PROXY || !conf->uri_list) { domain = NULL; } else { diff --git a/test/modules/aaa/conftest.py b/test/modules/aaa/conftest.py index bf030e80304..8c943a6685a 100644 --- a/test/modules/aaa/conftest.py +++ b/test/modules/aaa/conftest.py @@ -99,6 +99,21 @@ def env(pytestconfig) -> AAATestEnv: # has no per-request Digest record; an ErrorDocument redirecting it into # a Digest-protected location must not then dereference that record. conf.add('ErrorDocument 400 /digest/default/secret.txt') + # A reverse-proxied location which authenticates with Digest at the + # proxy (so the backend is never reached and need not be reachable). + # The challenge is an origin-style WWW-Authenticate, so AuthDigestDomain + # applies to it just as for a non-proxied location. + conf.add([ + 'ProxyPass "/pxdomain/" "http://127.0.0.1:1/unused/"', + '', + ' AuthType Digest', + f' AuthName "{AAATestEnv.REALM}"', + ' AuthDigestProvider file', + f' AuthUserFile "{pwfile}"', + ' AuthDigestDomain "/pxdomain/"', + ' Require valid-user', + '', + ]) conf.install() assert env.apache_restart() == 0 return env diff --git a/test/modules/aaa/env.py b/test/modules/aaa/env.py index 0e8ed377e9c..342afe0c96a 100644 --- a/test/modules/aaa/env.py +++ b/test/modules/aaa/env.py @@ -16,7 +16,7 @@ def __init__(self, env: 'HttpdTestEnv'): super().__init__(env=env) self.add_source_dir(os.path.dirname(inspect.getfile(AAATestSetup))) self.add_modules(["auth_digest", "authn_file", "authn_core", - "authz_core", "authz_user"]) + "authz_core", "authz_user", "proxy", "proxy_http"]) class AAATestEnv(HttpdTestEnv): diff --git a/test/modules/aaa/test_013_proxy_domain.py b/test/modules/aaa/test_013_proxy_domain.py new file mode 100644 index 00000000000..ffd4f227e57 --- /dev/null +++ b/test/modules/aaa/test_013_proxy_domain.py @@ -0,0 +1,25 @@ +"""AuthDigestDomain on a reverse-proxied location. + +Digest auth can run at a reverse proxy, for a location mapped by ProxyPass. +Such a request carries PROXYREQ_REVERSE, but its 401 is an ordinary +origin-style WWW-Authenticate (the client authenticates to this server, not +through it), so the configured AuthDigestDomain belongs in the challenge +just as for a non-proxied location. + +The suppression test treated any proxy request alike and dropped domain= +for the reverse-proxied case, so clients re-authenticated per URI prefix +and sent the Authorization header outside the intended protection space. +""" + +from . import digest_client as dc + + +class TestDigestProxyDomain: + + def test_digest_130_domain_sent_for_reverse_proxy(self, env): + r = env.curl_get(env.mkurl("http", "aaa", "/pxdomain/secret.txt")) + assert r.response["status"] == 401 + challenge = dc.DigestChallenge.parse( + r.response["header"]["www-authenticate"]) + assert "/pxdomain/" in challenge.domain_list(), \ + "AuthDigestDomain was dropped from a reverse-proxied challenge" From 21eb9100276709af348a3de2f0e72b4a22ffea10 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Tue, 6 Oct 2026 10:55:19 +0100 Subject: [PATCH 08/10] mod_auth_digest: Validate AuthDigestShmemSize properly: The size was parsed with strtol with no overflow check (the unit multiply could overflow), junk after the unit character was ignored ("64Kfoo" read as 64K), and the minimum counted neither the apr_rmm per-allocation overhead nor the one-time-nonce counter, so an accepted size could leave no room to allocate any client entry -- a 503 on every request the config check did not warn about. * modules/aaa/mod_auth_digest.c (set_shmem_size): Parse with apr_strtoff, reject overflow and trailing junk, and require room for the table, the counter, and at least one entry including rmm overhead. * test/modules/aaa/test_006_config_errors.py (test_digest_068_shmemsize_trailing_junk_rejected, test_digest_069_shmemsize_no_room_for_entry_rejected): New. Co-Authored-By: Claude Opus 4.8 --- modules/aaa/mod_auth_digest.c | 46 ++++++++++++++++------ test/modules/aaa/test_006_config_errors.py | 16 ++++++++ 2 files changed, 49 insertions(+), 13 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 3106edbeca8..cd8a10d9c52 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -588,32 +588,52 @@ static const char *set_shmem_size(cmd_parms *cmd, void *config, const char *size_str) { char *endptr; - long size, min; + apr_off_t size; + apr_size_t min; - size = strtol(size_str, &endptr, 10); - while (apr_isspace(*endptr)) endptr++; - if (*endptr == '\0' || *endptr == 'b' || *endptr == 'B') { - ; + if (apr_strtoff(&size, size_str, &endptr, 10) != APR_SUCCESS || size < 0) { + return apr_pstrcat(cmd->pool, "Invalid size in AuthDigestShmemSize: ", + size_str, NULL); } - else if (*endptr == 'k' || *endptr == 'K') { + if (*endptr == 'k' || *endptr == 'K') { + if (size > APR_INT64_MAX / 1024) { + return "AuthDigestShmemSize value is too large"; + } size *= 1024; + endptr++; } else if (*endptr == 'm' || *endptr == 'M') { - size *= 1048576; + if (size > APR_INT64_MAX / (1024 * 1024)) { + return "AuthDigestShmemSize value is too large"; + } + size *= 1024 * 1024; + endptr++; } - else { + else if (*endptr == 'b' || *endptr == 'B') { + endptr++; + } + while (apr_isspace(*endptr)) { + endptr++; + } + if (*endptr != '\0') { return apr_pstrcat(cmd->pool, "Invalid size in AuthDigestShmemSize: ", size_str, NULL); } - min = sizeof(*client_list) + sizeof(client_entry*) + sizeof(client_entry); - if (size < min) { + /* The segment must hold three separate rmm allocations -- the client + * list with at least one bucket, the one-time-nonce counter, and at + * least one client entry -- each with its rmm overhead. */ + min = apr_rmm_overhead_get(3) + + sizeof(*client_list) + sizeof(client_entry *) + + sizeof(*otn_counter) + + sizeof(client_entry); + if (size < (apr_off_t)min) { return apr_psprintf(cmd->pool, "size in AuthDigestShmemSize too small: " - "%ld < %ld", size, min); + "%" APR_OFF_T_FMT " < %" APR_SIZE_T_FMT, size, min); } - shmem_size = size; - num_buckets = NUM_BUCKETS(size); + shmem_size = (apr_size_t)size; + num_buckets = NUM_BUCKETS(shmem_size); if (num_buckets == 0) { num_buckets = 1; } diff --git a/test/modules/aaa/test_006_config_errors.py b/test/modules/aaa/test_006_config_errors.py index e1284abfdf0..2206ee4c2ec 100644 --- a/test/modules/aaa/test_006_config_errors.py +++ b/test/modules/aaa/test_006_config_errors.py @@ -84,3 +84,19 @@ def test_digest_066_shmemsize_valid_accepted(self, env): def test_digest_067_shmemsize_units_accepted(self, env): r = env.configtest([], extra_top_lines=["AuthDigestShmemSize 64K"]) assert r.exit_code == 0 + + def test_digest_068_shmemsize_trailing_junk_rejected(self, env): + # Junk after the unit character must be rejected, not silently + # ignored (which read "64Kfoo" as 64K). + r = env.configtest([], extra_top_lines=["AuthDigestShmemSize 64Kfoo"]) + assert r.exit_code != 0 + assert "AuthDigestShmemSize" in r.stderr + + def test_digest_069_shmemsize_no_room_for_entry_rejected(self, env): + # A segment large enough for the table header but with no room for a + # single client entry (once rmm overhead is counted) must be + # rejected: otherwise every request needing an entry gets a 503 the + # config check did not warn about. + r = env.configtest([], extra_top_lines=["AuthDigestShmemSize 200"]) + assert r.exit_code != 0 + assert "AuthDigestShmemSize" in r.stderr From f6e0062faf3516012dc61c9526afb771ba8823b7 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Tue, 6 Oct 2026 11:31:12 +0100 Subject: [PATCH 09/10] * modules/aaa/mod_auth_digest.c: Code style tweaks only, no functional change. [skip ci] Co-Authored-By: Claude Opus 4.8 --- modules/aaa/mod_auth_digest.c | 77 +++++------------------------------ 1 file changed, 11 insertions(+), 66 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index cd8a10d9c52..9fe978253c8 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -108,7 +108,6 @@ typedef struct digest_config_struct { char *uri_list; } digest_config_rec; - #define DFLT_ALGORITHM "MD5" #define DFLT_NONCE_LIFE apr_time_from_sec(300) @@ -140,9 +139,6 @@ typedef struct digest_config_struct { #error the secret is too short to key siphash #endif - -/* client list definitions */ - /* Identifies a client entry. This is the value sent to the client in the * opaque field of the challenge, and echoed back in its Authorization * header; zero is never a valid id, and means "no client". Ids are counted @@ -210,9 +206,6 @@ typedef struct digest_header_struct { const char *ha1; } digest_header_rec; - -/* (mostly) nonce stuff */ - typedef union time_union { apr_time_t time; unsigned char arr[sizeof(apr_time_t)]; @@ -220,8 +213,6 @@ typedef union time_union { static unsigned char *secret; -/* client-list, opaque, and one-time-nonce stuff */ - static apr_shm_t *client_shm = NULL; static apr_rmm_t *client_rmm = NULL; static volatile apr_uint32_t *otn_counter; /* one-time-nonce counter */ @@ -241,13 +232,9 @@ static const char *client_shm_filename; static apr_size_t shmem_size = DEF_SHMEM_SIZE; static unsigned long num_buckets = DEF_NUM_BUCKETS; - module AP_MODULE_DECLARE_DATA auth_digest_module; -/* - * initialization code - */ - +/* Initialization. */ static apr_status_t cleanup_tables(void *not_used) { ap_log_error(APLOG_MARK, APLOG_INFO, 0, NULL, APLOGNO(01756) @@ -307,10 +294,6 @@ static int initialize_tables(server_rec *s, apr_pool_t *ctx) unsigned long idx; apr_status_t sts; - /* set up client list */ - - /* Create the shared memory segment */ - client_shm = NULL; client_rmm = NULL; client_lock = NULL; @@ -383,19 +366,13 @@ static int initialize_tables(server_rec *s, apr_pool_t *ctx) return !OK; } - - /* setup one-time-nonce counter */ - otn_counter = rmm_malloc(client_rmm, sizeof(*otn_counter)); if (otn_counter == NULL) { log_error_and_cleanup("failed to allocate shared memory", -1, s); return !OK; } *otn_counter = 0; - /* no lock here */ - - /* success */ return OK; } @@ -470,10 +447,7 @@ static void initialize_child(apr_pool_t *p, server_rec *s) } } -/* - * configuration code - */ - +/* Configuration handling. */ static void *create_digest_dir_config(apr_pool_t *p, char *dir) { digest_config_rec *conf = apr_pcalloc(p, sizeof *conf); @@ -663,9 +637,7 @@ static const command_rec digest_cmds[] = {NULL} }; - -/* - * client list code +/* The client list. * * Each client is assigned a number, which is transferred in the opaque * field of the WWW-Authenticate and Authorization headers. The number @@ -749,7 +721,6 @@ static client_entry *find_client(client_id_t key) return entry; } - /* Determine whether the client identified by key is still known. */ static int client_exists(client_id_t key, const request_rec *r) { @@ -771,7 +742,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) @@ -781,7 +751,6 @@ static void client_note_renewed(void) apr_global_mutex_unlock(client_lock); } - /* 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. @@ -862,7 +831,6 @@ static enum nonce_state client_update_nonce(const request_rec *r, return state; } - /* 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. @@ -917,7 +885,6 @@ static unsigned long gc(server_rec *s) return num_removed; } - /* * 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 @@ -973,11 +940,6 @@ static client_id_t client_generate(request_rec *r) return key; } - -/* - * Authorization header parser code - */ - /* Parse the Authorization header, if it exists, into resp; returns the * status of the header. */ static enum hdr_sts parse_digest_header(request_rec *r, @@ -1118,7 +1080,6 @@ static enum hdr_sts parse_digest_header(request_rec *r, return VALID; } - /* Create the per-request Digest record describing request r, store it on * store's request_config, and parse r's Authorization header into it. */ static digest_header_rec *make_digest_rec(request_rec *store, request_rec *r) @@ -1181,7 +1142,6 @@ static digest_header_rec *get_digest_rec(request_rec *r) return resp; } - /* Writes the hash part of the server nonce to hash, which must be of * minimum size (NONCE_HASH_LEN+1). */ static void gen_nonce_hash(apr_pool_t *p, char hash[NONCE_HASH_LEN+1], @@ -1227,7 +1187,6 @@ static void gen_nonce_hash(apr_pool_t *p, char hash[NONCE_HASH_LEN+1], #endif } - /* The nonce has the format b64(time)+hash . */ static const char *gen_nonce(apr_pool_t *p, apr_time_t now, const char *opaque, @@ -1253,10 +1212,7 @@ static const char *gen_nonce(apr_pool_t *p, apr_time_t now, const char *opaque, return nonce; } - -/* - * Authorization challenge generation code (for WWW-Authenticate) - */ +/* Authorization challenge generation (for WWW-Authenticate). */ /* Format a client id as the opaque sent to the client. Never called with * zero: the callers check client_generate() for failure first. */ @@ -1371,11 +1327,9 @@ static int hook_note_digest_auth_failure(request_rec *r, const char *auth_type) return OK; } - -/* - * Authorization header verification code - */ - +/* Look up the stored HA1 hash, md5(user:realm:password), for user through + * the configured authn providers. Returns the provider's authn_status and, + * on success, the hash in *rethash. */ static authn_status get_hash(request_rec *r, const char *user, digest_config_rec *conf, const char **rethash) { @@ -1408,7 +1362,6 @@ static authn_status get_hash(request_rec *r, const char *user, apr_table_setn(r->notes, AUTHN_PROVIDER_NAME_NOTE, current_provider->provider_name); } - /* We expect the password to be md5 hash of user:realm:password */ auth_result = provider->get_realm_hash(r, user, ap_auth_name(r), &password); @@ -1515,9 +1468,7 @@ static int check_nonce(request_rec *r, digest_header_rec *resp, return OK; } -/* The actual MD5 code... whee */ - -/* RFC-2617 */ +/* Compute the response digest expected for request r (RFC 2617). */ static const char *new_digest(const request_rec *r, digest_header_rec *resp) { @@ -1631,9 +1582,7 @@ static int authenticate_digest_user(request_rec *r) conf = (digest_config_rec *) ap_get_module_config(r->per_dir_config, &auth_digest_module); - - /* check for existence and syntax of Auth header */ - + /* Check for existence and syntax of the Auth header. */ if (resp->auth_hdr_sts != VALID) { if (resp->auth_hdr_sts == NOT_DIGEST) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01781) @@ -1838,12 +1787,10 @@ static int add_auth_info(request_rec *r) } /* else nonce never expires, hence no nextnonce */ - { const char *resp_dig, *ha1, *a2, *ha2; - /* calculate rspauth attribute - */ + /* Calculate the rspauth attribute. */ ha1 = resp->ha1; a2 = apr_pstrcat(r->pool, ":", resp->uri, NULL); @@ -1858,8 +1805,7 @@ static int add_auth_info(request_rec *r) resp->message_qop : "", ":", ha2, NULL)); - /* assemble Authentication-Info header - */ + /* Assemble the Authentication-Info header. */ ai = apr_pstrcat(r->pool, "rspauth=\"", resp_dig, "\"", nextnonce, @@ -1904,7 +1850,6 @@ static void register_hooks(apr_pool_t *p) ap_hook_fixups(add_auth_info, NULL, NULL, APR_HOOK_MIDDLE); ap_hook_note_auth_failure(hook_note_digest_auth_failure, NULL, NULL, APR_HOOK_MIDDLE); - } AP_DECLARE_MODULE(auth_digest) = From d8f674e48df6123724a47378ffd205058dcfb62c Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Tue, 6 Oct 2026 11:47:14 +0100 Subject: [PATCH 10/10] mod_auth_digest: Widen the one-time-nonce counter to 64 bits: The one-time-nonce counter was a 32-bit value incremented with atomics. After 2^32 one-time nonces it wrapped, re-issuing ordinals below those already recorded for established clients, so each was answered as stale on every request until its entry was evicted; the wrap also produced the reserved ordinal 0. * modules/aaa/mod_auth_digest.c (struct hash_table): Hold the counter here as a 64-bit otn_counter, issued under client_lock like next_id rather than with 32-bit atomics (apr_atomic_inc64 is not available on the minimum APR). (initialize_tables): Seed it; drop its separate shared-memory allocation. (gen_nonce): Issue the ordinal under client_lock. (set_shmem_size): Account for two allocations rather than three. * test/modules/aaa/test_006_config_errors.py (test_digest_069_shmemsize_no_room_for_entry_rejected): Adjust for the smaller minimum. Co-Authored-By: Claude Opus 4.8 --- modules/aaa/mod_auth_digest.c | 32 ++++++++++------------ test/modules/aaa/test_006_config_errors.py | 12 ++++---- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 9fe978253c8..51cf2f23fcc 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -168,6 +168,7 @@ static struct hash_table { unsigned long num_renewed; client_id_t next_id; /* the last id issued (the random * initial seed is never issued) */ + apr_uint64_t otn_counter; /* the last one-time nonce ordinal */ } *client_list; /* Outcome from parsing an Authorization header. */ @@ -215,7 +216,6 @@ static unsigned char *secret; static apr_shm_t *client_shm = NULL; static apr_rmm_t *client_rmm = NULL; -static volatile apr_uint32_t *otn_counter; /* one-time-nonce counter */ static apr_global_mutex_t *client_lock = NULL; static const char *client_mutex_type = "authdigest-client"; static const char *client_shm_filename; @@ -358,6 +358,7 @@ static int initialize_tables(server_rec *s, apr_pool_t *ctx) ap_random_insecure_bytes(&client_list->next_id, sizeof client_list->next_id); client_list->next_id %= CLIENT_ID_MAX; + client_list->otn_counter = 0; sts = ap_global_mutex_create(&client_lock, NULL, client_mutex_type, NULL, s, ctx, 0); @@ -366,13 +367,6 @@ static int initialize_tables(server_rec *s, apr_pool_t *ctx) return !OK; } - otn_counter = rmm_malloc(client_rmm, sizeof(*otn_counter)); - if (otn_counter == NULL) { - log_error_and_cleanup("failed to allocate shared memory", -1, s); - return !OK; - } - *otn_counter = 0; - return OK; } @@ -594,12 +588,11 @@ static const char *set_shmem_size(cmd_parms *cmd, void *config, size_str, NULL); } - /* The segment must hold three separate rmm allocations -- the client - * list with at least one bucket, the one-time-nonce counter, and at - * least one client entry -- each with its rmm overhead. */ - min = apr_rmm_overhead_get(3) + /* The segment must hold two separate rmm allocations -- the client + * list with at least one bucket, and at least one client entry -- each + * with its rmm overhead. */ + min = apr_rmm_overhead_get(2) + sizeof(*client_list) + sizeof(client_entry *) - + sizeof(*otn_counter) + sizeof(client_entry); if (size < (apr_off_t)min) { return apr_psprintf(cmd->pool, "size in AuthDigestShmemSize too small: " @@ -1201,10 +1194,15 @@ static const char *gen_nonce(apr_pool_t *p, apr_time_t now, const char *opaque, t.time = now; } else { - /* Nonces are ordered by this counter rather than by time; the +1 - * is because apr_atomic_inc32() returns the previous value, and a - * nonce time of zero means "no nonce used yet" in a client entry. */ - t.time = apr_atomic_inc32(otn_counter) + 1; + /* One-time nonces are ordered by this counter rather than by time. + * It is 64-bit and issued under the lock rather than with 32-bit + * atomics: a counter which wrapped would re-issue ordinals below + * those already recorded, locking every established client out. A + * nonce time of zero means "no nonce used yet" in a client entry, + * so the first value handed out is 1. */ + apr_global_mutex_lock(client_lock); + t.time = ++client_list->otn_counter; + 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); diff --git a/test/modules/aaa/test_006_config_errors.py b/test/modules/aaa/test_006_config_errors.py index 2206ee4c2ec..3565cee9f85 100644 --- a/test/modules/aaa/test_006_config_errors.py +++ b/test/modules/aaa/test_006_config_errors.py @@ -93,10 +93,12 @@ def test_digest_068_shmemsize_trailing_junk_rejected(self, env): assert "AuthDigestShmemSize" in r.stderr def test_digest_069_shmemsize_no_room_for_entry_rejected(self, env): - # A segment large enough for the table header but with no room for a - # single client entry (once rmm overhead is counted) must be - # rejected: otherwise every request needing an entry gets a 503 the - # config check did not warn about. - r = env.configtest([], extra_top_lines=["AuthDigestShmemSize 200"]) + # A size which would hold the table and an entry by their bare + # struct sizes, but not once the per-allocation rmm overhead is + # counted, must be rejected: otherwise every request needing an + # entry gets a 503 the config check did not warn about. 120 bytes is + # above those struct sizes but below the overhead-aware minimum on + # both 32- and 64-bit. + r = env.configtest([], extra_top_lines=["AuthDigestShmemSize 120"]) assert r.exit_code != 0 assert "AuthDigestShmemSize" in r.stderr