You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by jo...@apache.org on 2026/08/13 15:15:44 UTC
svn commit: r1937107 - in httpd/httpd/trunk: modules/aaa test/modules/aaa
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) <no...@anthropic.com>
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 <ak...@nueva.pvt.k12.ca.us>
- * Updated to RFC-2617 by Ronald Tschal�r <ro...@innovation.ch>
+ * Updated to RFC-2617 by Ronald Tschalär <ro...@innovation.ch>
* 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"