Author: jorton
Date: Thu Aug 13 15:15:44 2026
New Revision: 1937107
Log:
* modules/aaa/mod_auth_digest.c: Don't let a client_entry pointer
escape the client_lock critical section; look up and release per
access rather than caching the pointer across the request.
Assisted-by: Claude Opus 5 (1M context) <[email protected]>
GitHub: PR #705
Modified:
httpd/httpd/trunk/modules/aaa/mod_auth_digest.c
httpd/httpd/trunk/test/modules/aaa/test_003_nccheck.py
Modified: httpd/httpd/trunk/modules/aaa/mod_auth_digest.c
==============================================================================
--- httpd/httpd/trunk/modules/aaa/mod_auth_digest.c Thu Aug 13 15:15:30 2026 (r1937106)
+++ httpd/httpd/trunk/modules/aaa/mod_auth_digest.c Thu Aug 13 15:15:44 2026 (r1937107)
@@ -18,7 +18,7 @@
* mod_auth_digest: MD5 digest authentication
*
* Originally by Alexei Kosut <[email protected]>
- * Updated to RFC-2617 by Ronald Tschalär <[email protected]>
+ * Updated to RFC-2617 by Ronald Tschalär <[email protected]>
* based on mod_auth, by Rob McCool and Robert S. Thau
*
* This module an updated version of modules/standard/mod_digest.c
@@ -115,8 +115,15 @@ typedef struct digest_config_struct {
/* 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
+ * 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. */
+typedef apr_uint32_t client_id_t;
+
typedef struct hash_entry {
- unsigned long key; /* the key for this entry */
+ client_id_t key; /* the key for this entry */
struct hash_entry *next; /* next entry in the bucket */
unsigned long nonce_count; /* highest nonce-count seen
* for last_nonce_time */
@@ -157,7 +164,7 @@ typedef struct digest_header_struct {
const char *algorithm;
const char *cnonce;
const char *opaque;
- unsigned long opaque_num;
+ client_id_t opaque_num;
const char *message_qop;
const char *nonce_count;
/* the following fields are not (directly) from the header */
@@ -167,7 +174,6 @@ typedef struct digest_header_struct {
enum hdr_sts auth_hdr_sts;
int needed_auth;
const char *ha1;
- client_entry *client;
} digest_header_rec;
@@ -184,7 +190,7 @@ static unsigned char *secret;
static apr_shm_t *client_shm = NULL;
static apr_rmm_t *client_rmm = NULL;
-static volatile apr_uint32_t *opaque_counter;
+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";
@@ -331,12 +337,12 @@ static int initialize_tables(server_rec
/* setup opaque */
- opaque_counter = rmm_malloc(client_rmm, sizeof *opaque_counter);
- if (opaque_counter == NULL) {
+ 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;
}
- *opaque_counter = 1;
+ *client_id_counter = 1;
/* setup one-time-nonce counter */
@@ -644,38 +650,41 @@ static const command_rec digest_cmds[] =
* above algorithm is really sufficient) a set of counters is kept
* indicating the number of clients held, the number of garbage collected
* clients, and the number of erroneously purged clients. These are printed
- * out at each garbage collection run. Note that access to the counters is
- * not synchronized because they are just indicaters, and whether they are
- * off by a few doesn't matter; and for the same reason no attempt is made
- * to guarantee the num_renewed is correct in the face of clients spoofing
- * the opaque field.
+ * out at each garbage collection run. Note that no attempt is made to
+ * guarantee that num_renewed is correct in the face of clients spoofing
+ * the opaque field; it is just an indicator, and whether it is off by a
+ * few doesn't matter.
*/
/*
- * Get the client given its client number (the key). Returns the entry,
- * or NULL if it's not found.
+ * Find the client given its client number (the key), moving it to the
+ * front of its bucket. Returns the entry, or NULL if it's not found.
+ *
+ * MUST be called with client_lock held, and the entry returned MUST NOT be
+ * used outside that critical section: it lives in the shared memory
+ * segment, where gc() can free it at any time on behalf of another
+ * process. The accessors below are the only supported way to reach a
+ * client entry; each looks it up afresh, so a client which has since been
+ * garbage collected is simply reported as unknown and the caller goes on
+ * to issue a new challenge for it.
*
- * Access to the list itself is synchronized via locks. However, access
- * to the entry returned by get_client() is NOT synchronized. This means
- * that there are potentially problems if a client uses multiple,
- * simultaneous connections to access url's within the same protection
- * space. However, these problems are not new: when using multiple
- * connections you have no guarantee of the order the requests are
- * processed anyway, so you have problems with the nonce-count and
- * one-time nonces anyway.
+ * Note that this still gives no ordering guarantee for a client using
+ * multiple simultaneous connections within the same protection space: the
+ * requests can be processed in any order, so the nonce-count and one-time
+ * nonce checks may reject some of them. That is not new.
*/
-static client_entry *get_client(unsigned long key, const request_rec *r)
+static client_entry *find_client(client_id_t key)
{
int bucket;
client_entry *entry, *prev = NULL;
- if (!key) return NULL;
+ if (!key) {
+ return NULL;
+ }
bucket = key % client_list->tbl_len;
entry = client_list->table[bucket];
- apr_global_mutex_lock(client_lock);
-
while (entry && key != entry->key) {
prev = entry;
entry = entry->next;
@@ -687,18 +696,120 @@ static client_entry *get_client(unsigned
client_list->table[bucket] = entry;
}
+ return entry;
+}
+
+
+/* Determine whether the client identified by key is still known. */
+static int client_exists(client_id_t key, const request_rec *r)
+{
+ int found;
+
+ apr_global_mutex_lock(client_lock);
+ found = find_client(key) != NULL;
apr_global_mutex_unlock(client_lock);
- if (entry) {
+ if (found) {
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01764)
- "get_client(): client %lu found", key);
+ "client %u found", key);
}
else {
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01765)
- "get_client(): client %lu not found", key);
+ "client %u not found", key);
}
- return entry;
+ 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);
+}
+
+
+/* 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.
+ *
+ * Both nonce_time and the count are compared against what the client last
+ * *used*, never against what was last issued to it: a nonce is issued
+ * whenever a challenge is generated, and anything quoting the client's
+ * opaque can provoke a challenge, so tracking what was issued lets an
+ * unauthenticated request invalidate the nonce which the legitimate client
+ * is holding.
+ *
+ * A one-time nonce (AuthDigestNonceLifetime 0) is therefore accepted iff
+ * it is newer than the last nonce this client used, which permits it
+ * exactly once. Otherwise, with AuthDigestNcCheck, a newer nonce starts a
+ * new count and the same nonce must raise it.
+ *
+ * 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,
+ client_id_t key,
+ const digest_config_rec *conf,
+ apr_time_t nonce_time,
+ unsigned long nc,
+ const char *nonce)
+{
+ client_entry *client;
+ unsigned long tracked = 0;
+ enum nonce_state state;
+ int known;
+
+ apr_global_mutex_lock(client_lock);
+ client = find_client(key);
+ known = (client != NULL);
+ if (!known) {
+ state = NONCE_STALE;
+ }
+ else {
+ tracked = 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)) {
+ state = NONCE_ACCEPTED;
+ }
+ else {
+ state = NONCE_BAD_COUNT;
+ }
+
+ if (state == NONCE_ACCEPTED) {
+ client->last_nonce_time = nonce_time;
+ client->nonce_count = nc;
+ }
+ }
+ apr_global_mutex_unlock(client_lock);
+
+ if (!known) {
+ ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(10618)
+ "client %u is no longer known - sending new nonce",
+ 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_ERR, 0, r, APLOGNO(01774)
+ "Warning, possible replay attack: nonce-count check "
+ "failed: %lu is not above %lu for nonce %s", nc,
+ tracked, nonce);
+ }
+
+ return state;
}
@@ -756,17 +867,17 @@ static long gc(server_rec *s)
/*
- * Add a new client to the list. Returns the entry if successful, NULL
- * otherwise. This triggers the garbage collection if memory is low.
+ * 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().)
*/
-static client_entry *add_client(unsigned long key, client_entry *info,
- server_rec *s)
+static int add_client(client_id_t key, client_entry *info, server_rec *s)
{
int bucket;
client_entry *entry;
if (!key) {
- return NULL;
+ return 0;
}
bucket = key % client_list->tbl_len;
@@ -789,7 +900,7 @@ static client_entry *add_client(unsigned
ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(01767)
"unable to allocate new auth_digest client");
apr_global_mutex_unlock(client_lock);
- return NULL; /* give up */
+ return 0; /* give up */
}
}
@@ -805,9 +916,9 @@ static client_entry *add_client(unsigned
apr_global_mutex_unlock(client_lock);
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(01768)
- "allocated new client %lu", key);
+ "allocated new client %u", key);
- return entry;
+ return 1;
}
@@ -932,8 +1043,9 @@ static enum hdr_sts parse_digest_header(
errno = 0;
num = strtol(resp->opaque, &endptr, 16);
- if (errno == 0 && *endptr == '\0' && num > 0)
- resp->opaque_num = (unsigned long)num;
+ if (errno == 0 && *endptr == '\0' && num > 0
+ && num <= APR_UINT32_MAX)
+ resp->opaque_num = (client_id_t)num;
}
return VALID;
@@ -948,7 +1060,7 @@ static enum hdr_sts parse_digest_header(
* 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_and_update_nc().
*
* Note that this must be called after mod_proxy had its go so that
* r->proxyreq is set correctly.
@@ -969,7 +1081,6 @@ static int init_digest_request(request_r
ap_set_module_config(r->request_config, &auth_digest_module, resp);
resp->auth_hdr_sts = parse_digest_header(r, resp);
- resp->client = get_client(resp->opaque_num, r);
return DECLINED;
}
@@ -1031,21 +1142,22 @@ static const char *gen_nonce(apr_pool_t
*/
/*
- * Generate a new client entry, add it to the list, and return the
- * entry. Returns NULL if failed.
+ * 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_entry *gen_client(const request_rec *r)
+static client_id_t client_generate(const request_rec *r)
{
- apr_uint32_t op = apr_atomic_inc32(opaque_counter);
- client_entry new_entry = { 0, NULL, 0, 0 }, *entry;
+ client_id_t op = apr_atomic_inc32(client_id_counter);
+ client_entry new_entry = { 0, NULL, 0, 0 };
- if (!(entry = add_client(op, &new_entry, r->server))) {
+ if (!add_client(op, &new_entry, r->server)) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01769)
"failed to allocate client entry - ignoring client");
- return NULL;
+ return 0;
}
- return entry;
+ return op;
}
@@ -1053,10 +1165,10 @@ static client_entry *gen_client(const re
* Authorization challenge generation code (for WWW-Authenticate)
*/
-static const char *ltox(apr_pool_t *p, unsigned long num)
+static const char *ltox(apr_pool_t *p, client_id_t num)
{
if (num != 0) {
- return apr_psprintf(p, "%lx", num);
+ return apr_psprintf(p, "%x", num);
}
else {
return "";
@@ -1068,6 +1180,7 @@ static void note_digest_auth_failure(req
digest_header_rec *resp, int stale)
{
const char *qop, *opaque, *opaque_param, *domain, *nonce;
+ client_id_t client_key = 0;
/* Setup qop */
qop = ", qop=\"auth\"";
@@ -1077,20 +1190,20 @@ static void note_digest_auth_failure(req
if (resp->opaque == NULL) {
/* new client */
if ((conf->check_nc || conf->nonce_lifetime == 0)
- && (resp->client = gen_client(r)) != NULL) {
- opaque = ltox(r->pool, resp->client->key);
+ && (client_key = client_generate(r)) != 0) {
+ opaque = ltox(r->pool, client_key);
}
else {
opaque = ""; /* opaque not needed */
}
}
- else if (resp->client == NULL) {
+ else if (!client_exists(resp->opaque_num, r)) {
/* client info was gc'd */
- resp->client = gen_client(r);
- if (resp->client != NULL) {
- opaque = ltox(r->pool, resp->client->key);
+ client_key = client_generate(r);
+ if (client_key != 0) {
+ opaque = ltox(r->pool, client_key);
stale = 1;
- client_list->num_renewed++;
+ client_note_renewed();
}
else {
opaque = ""; /* ??? */
@@ -1101,6 +1214,7 @@ static void note_digest_auth_failure(req
* here: the client may not even see this challenge (it may have
* been triggered by somebody else quoting its opaque), and it is
* tied to the nonce it was counted for in any case. */
+ client_key = resp->opaque_num;
opaque = resp->opaque;
}
@@ -1245,34 +1359,20 @@ static authn_status get_hash(request_rec
* state tracked for its client, record them, and generate a new challenge
* if they are not acceptable.
*
- * Both are compared against what the client last *used*, never against what
- * was last issued to it: a nonce is issued whenever a challenge is
- * generated, and anything quoting the client's opaque can provoke a
- * challenge, so tracking what was issued lets an unauthenticated request
- * invalidate the nonce which the legitimate client is holding.
- *
- * A one-time nonce (AuthDigestNonceLifetime 0) is therefore accepted iff it
- * is newer than the last nonce this client used, which permits it exactly
- * once. Otherwise, with AuthDigestNcCheck, a newer nonce starts a new count
- * and the same nonce must raise it: the nonce-count is counted by the client
- * per-nonce (RFC 7616 3.4.3). Within a nonce the count must strictly
- * increase, but it need not increase by exactly one, since the client also
- * counts the requests it sends to URIs in the protection space which turn
- * out not to need authentication, and this server never sees those.
- *
- * This 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.
+ * 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
+ * using a newer nonce starts a new count. Within a single nonce the count
+ * must strictly increase, but it need not increase by exactly one: the
+ * client also counts the requests it sends to URIs in the protection space
+ * 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)
{
- client_entry *client = resp->client;
- unsigned long nc, tracked = 0;
+ unsigned long nc;
const char *snc = resp->nonce_count;
char *endptr;
- enum nonce_state state = NONCE_STALE;
if (!conf->check_nc && conf->nonce_lifetime != 0) {
return OK; /* nothing is tracked per-client */
@@ -1286,50 +1386,8 @@ static int check_and_record_nonce(reques
return HTTP_UNAUTHORIZED;
}
- if (client) {
- apr_global_mutex_lock(client_lock);
-
- tracked = client->nonce_count;
- if (conf->nonce_lifetime == 0) {
- /* one-time nonce: usable until it has been used */
- state = (resp->nonce_time > client->last_nonce_time)
- ? NONCE_ACCEPTED : NONCE_STALE;
- }
- else if (resp->nonce_time > client->last_nonce_time
- || (resp->nonce_time == client->last_nonce_time
- && nc > tracked)) {
- state = NONCE_ACCEPTED;
- }
- else {
- state = NONCE_BAD_COUNT;
- }
-
- if (state == NONCE_ACCEPTED) {
- client->last_nonce_time = resp->nonce_time;
- client->nonce_count = nc;
- }
-
- apr_global_mutex_unlock(client_lock);
- }
-
- if (!client) {
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(10618)
- "client %lu is no longer known - sending new nonce",
- resp->opaque_num);
- }
- 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, resp->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, resp->nonce);
- }
-
- switch (state) {
+ switch (client_update_nonce(r, resp->opaque_num, conf, resp->nonce_time,
+ nc, resp->nonce)) {
case NONCE_ACCEPTED:
return OK;
@@ -1736,7 +1794,7 @@ static int add_auth_info(request_rec *r)
"\"", NULL);
}
}
- else if (conf->nonce_lifetime == 0 && resp->client) {
+ 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));
nextnonce = apr_pstrcat(r->pool, ", nextnonce=\"", nonce, "\"", NULL);
Modified: httpd/httpd/trunk/test/modules/aaa/test_003_nccheck.py
==============================================================================
--- httpd/httpd/trunk/test/modules/aaa/test_003_nccheck.py Thu Aug 13 15:15:30 2026 (r1937106)
+++ httpd/httpd/trunk/test/modules/aaa/test_003_nccheck.py Thu Aug 13 15:15:44 2026 (r1937107)
@@ -114,3 +114,30 @@ class TestDigestNcCheck:
assert r1.response["status"] == 200
r2 = self.authenticate(env, "default", challenge, nc="00000001")
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.
+ 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"
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.