From 5151152253b28e786225aaf325c2b9bf19c43873 Mon Sep 17 00:00:00 2001 From: Eric Covener Date: Mon, 22 Feb 2016 15:32:40 +0000 Subject: [PATCH 01/22] PR59039 Digest not working with ap_expr based AuthName Stop caching the configured realm during config processing and always call ap_auth_name(r) to determine (=evaluate potential expression) the configured realm git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1731662 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit c52f4a83f2c090776416e7c8ae7e94aa699df9fd) --- modules/aaa/mod_auth_digest.c | 51 +++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index f10af0da81e..1edf0dc1f7d 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -85,7 +85,6 @@ typedef struct digest_config_struct { const char *dir_name; authn_provider_list *providers; - const char *realm; apr_array_header_t *qop_list; apr_sha1_ctx_t nonce_ctx; apr_time_t nonce_lifetime; @@ -482,6 +481,11 @@ static void *create_digest_dir_config(apr_pool_t *p, char *dir) return conf; } + +/* + * The realm is no longer precomputed because it may be an expression, which + * makes this hooking of AuthName quite weird. + */ static const char *set_realm(cmd_parms *cmd, void *config, const char *realm) { digest_config_rec *conf = (digest_config_rec *) config; @@ -496,21 +500,13 @@ static const char *set_realm(cmd_parms *cmd, void *config, const char *realm) ap_assert(i < SECRET_LEN); #endif - /* The core already handles the realm, but it's just too convenient to - * grab it ourselves too and cache some setups. However, we need to - * let the core get at it too, which is why we decline at the end - - * this relies on the fact that http_core is last in the list. - */ - conf->realm = realm; - /* we precompute the part of the nonce hash that is constant (well, * the host:port would be too, but that varies for .htaccess files * and directives outside a virtual host section) */ apr_sha1_init(&conf->nonce_ctx); apr_sha1_update_binary(&conf->nonce_ctx, secret, SECRET_LEN); - apr_sha1_update_binary(&conf->nonce_ctx, (const unsigned char *) realm, - strlen(realm)); + return DECLINE_CMD; } @@ -1082,7 +1078,8 @@ static int parse_hdr_and_update_nc(request_rec *r) * minimum size (NONCE_HASH_LEN+1). */ static void gen_nonce_hash(char hash[NONCE_HASH_LEN+1], const char *timestr, const char *opaque, const server_rec *server, - const digest_config_rec *conf) + const digest_config_rec *conf, + const char *realm) { unsigned char sha1[APR_SHA1_DIGESTSIZE]; apr_sha1_ctx_t ctx; @@ -1094,6 +1091,9 @@ static void gen_nonce_hash(char hash[NONCE_HASH_LEN+1], const char *timestr, con apr_sha1_update_binary(&ctx, (const unsigned char *) &server->port, sizeof(server->port)); */ + + apr_sha1_update_binary(&ctx, (const unsigned char *) realm, strlen(realm)); + apr_sha1_update_binary(&ctx, (const unsigned char *) timestr, strlen(timestr)); if (opaque) { apr_sha1_update_binary(&ctx, (const unsigned char *) opaque, @@ -1109,7 +1109,8 @@ static void gen_nonce_hash(char hash[NONCE_HASH_LEN+1], const char *timestr, con */ static const char *gen_nonce(apr_pool_t *p, apr_time_t now, const char *opaque, const server_rec *server, - const digest_config_rec *conf) + const digest_config_rec *conf, + const char *realm) { char *nonce = apr_palloc(p, NONCE_LEN+1); time_rec t; @@ -1128,7 +1129,7 @@ static const char *gen_nonce(apr_pool_t *p, apr_time_t now, const char *opaque, t.time = 42; } apr_base64_encode_binary(nonce, t.arr, sizeof(t.arr)); - gen_nonce_hash(nonce+NONCE_TIME_LEN, nonce, opaque, server, conf); + gen_nonce_hash(nonce+NONCE_TIME_LEN, nonce, opaque, server, conf, realm); return nonce; } @@ -1238,7 +1239,7 @@ static void note_digest_auth_failure(request_rec *r, /* Setup nonce */ - nonce = gen_nonce(r->pool, r->request_time, opaque, r->server, conf); + nonce = gen_nonce(r->pool, r->request_time, opaque, r->server, conf, ap_auth_name(r)); if (resp->client && conf->nonce_lifetime == 0) { memcpy(resp->client->last_nonce, nonce, NONCE_LEN+1); } @@ -1344,7 +1345,7 @@ static authn_status get_hash(request_rec *r, const char *user, /* We expect the password to be md5 hash of user:realm:password */ - auth_result = provider->get_realm_hash(r, user, conf->realm, + auth_result = provider->get_realm_hash(r, user, ap_auth_name(r), &password); apr_table_unset(r->notes, AUTHN_PROVIDER_NAME_NOTE); @@ -1432,7 +1433,7 @@ static int check_nonce(request_rec *r, digest_header_rec *resp, tmp = resp->nonce[NONCE_TIME_LEN]; resp->nonce[NONCE_TIME_LEN] = '\0'; apr_base64_decode_binary(nonce_time.arr, resp->nonce); - gen_nonce_hash(hash, resp->nonce, resp->opaque, r->server, conf); + gen_nonce_hash(hash, resp->nonce, resp->opaque, r->server, conf, ap_auth_name(r)); resp->nonce[NONCE_TIME_LEN] = tmp; resp->nonce_time = nonce_time.time; @@ -1577,6 +1578,7 @@ static int authenticate_digest_user(request_rec *r) const char *t; int res; authn_status return_code; + const char *realm; /* do we require Digest auth for this URI? */ @@ -1604,6 +1606,7 @@ static int authenticate_digest_user(request_rec *r) &auth_digest_module); resp->needed_auth = 1; + realm = ap_auth_name(r); /* get our conf */ @@ -1730,8 +1733,10 @@ static int authenticate_digest_user(request_rec *r) note_digest_auth_failure(r, conf, resp, 0); return HTTP_UNAUTHORIZED; } + + - if (!conf->realm) { + if (!realm) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02533) "realm mismatch - got `%s' but no realm specified", resp->realm); @@ -1739,10 +1744,10 @@ static int authenticate_digest_user(request_rec *r) return HTTP_UNAUTHORIZED; } - if (!resp->realm || strcmp(resp->realm, conf->realm)) { + if (!resp->realm || strcmp(resp->realm, realm)) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01788) "realm mismatch - got `%s' but expected `%s'", - resp->realm, conf->realm); + resp->realm, realm); note_digest_auth_failure(r, conf, resp, 0); return HTTP_UNAUTHORIZED; } @@ -1761,7 +1766,7 @@ static int authenticate_digest_user(request_rec *r) if (return_code == AUTH_USER_NOT_FOUND) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01790) "user `%s' in realm `%s' not found: %s", - r->user, conf->realm, r->uri); + r->user, realm, r->uri); note_digest_auth_failure(r, conf, resp, 0); return HTTP_UNAUTHORIZED; } @@ -1772,7 +1777,7 @@ static int authenticate_digest_user(request_rec *r) /* authentication denied in the provider before attempting a match */ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01791) "user `%s' in realm `%s' denied by provider: %s", - r->user, conf->realm, r->uri); + r->user, realm, r->uri); note_digest_auth_failure(r, conf, resp, 0); return HTTP_UNAUTHORIZED; } @@ -1872,7 +1877,7 @@ static int add_auth_info(request_rec *r) if ((r->request_time - resp->nonce_time) > (conf->nonce_lifetime-NEXTNONCE_DELTA)) { nextnonce = apr_pstrcat(r->pool, ", nextnonce=\"", gen_nonce(r->pool, r->request_time, - resp->opaque, r->server, conf), + resp->opaque, r->server, conf, ap_auth_name(r)), "\"", NULL); if (resp->client) resp->client->nonce_count = 0; @@ -1880,7 +1885,7 @@ static int add_auth_info(request_rec *r) } else if (conf->nonce_lifetime == 0 && resp->client) { const char *nonce = gen_nonce(r->pool, 0, resp->opaque, r->server, - conf); + conf, ap_auth_name(r)); nextnonce = apr_pstrcat(r->pool, ", nextnonce=\"", nonce, "\"", NULL); memcpy(resp->client->last_nonce, nonce, NONCE_LEN+1); } From c4faadc61f610eb1dc178758786bbf7edd886bb5 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Wed, 20 May 2026 12:58:03 +0000 Subject: [PATCH 02/22] * modules/aaa/mod_auth_digest.c: Remove undocumented and unimplemented AuthDigestNonceFormat directive. PR: 70056 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1934434 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit 29b83f42e6410b4c14f0723c0b5c1b3ec6ea9448) --- modules/aaa/mod_auth_digest.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 1edf0dc1f7d..f574297de7b 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -595,12 +595,6 @@ static const char *set_nonce_lifetime(cmd_parms *cmd, void *config, return NULL; } -static const char *set_nonce_format(cmd_parms *cmd, void *config, - const char *fmt) -{ - return "AuthDigestNonceFormat is not implemented"; -} - static const char *set_nc_check(cmd_parms *cmd, void *config, int flag) { #if !APR_HAS_SHARED_MEMORY @@ -693,8 +687,6 @@ static const command_rec digest_cmds[] = "A list of quality-of-protection options"), AP_INIT_TAKE1("AuthDigestNonceLifetime", set_nonce_lifetime, NULL, OR_AUTHCFG, "Maximum lifetime of the server nonce (seconds)"), - AP_INIT_TAKE1("AuthDigestNonceFormat", set_nonce_format, NULL, OR_AUTHCFG, - "The format to use when generating the server nonce"), AP_INIT_FLAG("AuthDigestNcCheck", set_nc_check, NULL, OR_AUTHCFG, "Whether or not to check the nonce-count sent by the client"), AP_INIT_TAKE1("AuthDigestAlgorithm", set_algorithm, NULL, OR_AUTHCFG, From 0e0ceb90d2d9f67a1fa062c43068115892bdb2f9 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Mon, 6 Jul 2026 12:50:18 +0000 Subject: [PATCH 03/22] mod_auth_digest: Drop RFC 2069 and configurable qop support. (RFC 2617, which replaced 2069, is now 26 years old) * modules/aaa/mod_auth_digest.c (digest_config_rec): Remove qop_list field. (create_digest_dir_config): Remove qop_list initialization. (set_qop): Deprecate AuthDigestQop, only "auth" is supported. (note_digest_auth_failure): Always send qop="auth". (check_nc): Remove handling for qop=none. (old_digest): Remove function. (authenticate_digest_user): Reject requests with missing or non-"auth" qop value rather than falling back to RFC 2069. Co-Authored-By: Claude Opus 4.6 GitHub: PR #661 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1935944 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit 358a56751f51e0d2cc76ccf9166f66adec34756e) --- modules/aaa/mod_auth_digest.c | 114 +++++----------------------------- 1 file changed, 15 insertions(+), 99 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index f574297de7b..09203282e63 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -85,8 +85,7 @@ typedef struct digest_config_struct { const char *dir_name; authn_provider_list *providers; - apr_array_header_t *qop_list; - apr_sha1_ctx_t nonce_ctx; + apr_sha1_ctx_t nonce_ctx; apr_time_t nonce_lifetime; int check_nc; const char *algorithm; @@ -472,7 +471,6 @@ static void *create_digest_dir_config(apr_pool_t *p, char *dir) conf = (digest_config_rec *) apr_pcalloc(p, sizeof(digest_config_rec)); if (conf) { - conf->qop_list = apr_array_make(p, 2, sizeof(char *)); conf->nonce_lifetime = DFLT_NONCE_LIFE; conf->dir_name = apr_pstrdup(p, dir); conf->algorithm = DFLT_ALGORITHM; @@ -558,22 +556,9 @@ static const char *add_authn_provider(cmd_parms *cmd, void *config, static const char *set_qop(cmd_parms *cmd, void *config, const char *op) { - digest_config_rec *conf = (digest_config_rec *) config; - - if (!ap_cstr_casecmp(op, "none")) { - apr_array_clear(conf->qop_list); - *(const char **)apr_array_push(conf->qop_list) = "none"; - return NULL; - } - - if (!ap_cstr_casecmp(op, "auth-int")) { - return "AuthDigestQop auth-int is not implemented"; + if (ap_cstr_casecmp(op, "auth")) { + return "AuthDigestQop is deprecated: only 'auth' is supported"; } - else if (ap_cstr_casecmp(op, "auth")) { - return apr_pstrcat(cmd->pool, "Unrecognized qop: ", op, NULL); - } - - *(const char **)apr_array_push(conf->qop_list) = op; return NULL; } @@ -1179,18 +1164,7 @@ static void note_digest_auth_failure(request_rec *r, const char *qop, *opaque, *opaque_param, *domain, *nonce; /* Setup qop */ - if (apr_is_empty_array(conf->qop_list)) { - qop = ", qop=\"auth\""; - } - else if (!ap_cstr_casecmp(*(const char **)(conf->qop_list->elts), "none")) { - qop = ""; - } - else { - qop = apr_pstrcat(r->pool, ", qop=\"", - apr_array_pstrcat(r->pool, conf->qop_list, ','), - "\"", - NULL); - } + qop = ", qop=\"auth\""; /* Setup opaque */ @@ -1380,19 +1354,6 @@ static int check_nc(const request_rec *r, const digest_header_rec *resp, return OK; } - if (!apr_is_empty_array(conf->qop_list) && - !ap_cstr_casecmp(*(const char **)(conf->qop_list->elts), "none")) { - /* qop is none, client must not send a nonce count */ - if (snc != NULL) { - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01772) - "invalid nc %s received - no nonce count allowed when qop=none", - snc); - return !OK; - } - /* qop is none, cannot check nonce count */ - return OK; - } - nc = strtol(snc, &endptr, 16); if (endptr < (snc+strlen(snc)) && !apr_isspace(*endptr)) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01773) @@ -1473,19 +1434,6 @@ static int check_nonce(request_rec *r, digest_header_rec *resp, /* The actual MD5 code... whee */ -/* RFC-2069 */ -static const char *old_digest(const request_rec *r, - const digest_header_rec *resp) -{ - const char *ha2; - - ha2 = ap_md5(r->pool, (unsigned char *)apr_pstrcat(r->pool, resp->method, ":", - resp->uri, NULL)); - return ap_md5(r->pool, - (unsigned char *)apr_pstrcat(r->pool, resp->ha1, ":", - resp->nonce, ":", ha2, NULL)); -} - /* RFC-2617 */ static const char *new_digest(const request_rec *r, digest_header_rec *resp) @@ -1781,39 +1729,18 @@ static int authenticate_digest_user(request_rec *r) return HTTP_INTERNAL_SERVER_ERROR; } - if (resp->message_qop == NULL) { - /* old (rfc-2069) style digest */ - if (!ap_memeq_timingsafe(old_digest(r, resp), resp->digest, MD5_DIGEST_LEN)) { - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01792) - "user %s: password mismatch: %s", r->user, - r->uri); - note_digest_auth_failure(r, conf, resp, 0); - return HTTP_UNAUTHORIZED; - } + if (resp->message_qop == NULL + || ap_cstr_casecmp(resp->message_qop, "auth")) { + /* RFC 2069-style Digest is no longer supported. */ + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10560) + "invalid or missing qop value '%s', RFC 2069 is " + "no longer supported: %s", resp->message_qop, r->uri); + note_digest_auth_failure(r, conf, resp, 0); + return HTTP_UNAUTHORIZED; } else { - const char *exp_digest; - int match = 0, idx; - const char **tmp = (const char **)(conf->qop_list->elts); - for (idx = 0; idx < conf->qop_list->nelts; idx++) { - if (!ap_cstr_casecmp(*tmp, resp->message_qop)) { - match = 1; - break; - } - ++tmp; - } - - if (!match - && !(apr_is_empty_array(conf->qop_list) - && !ap_cstr_casecmp(resp->message_qop, "auth"))) { - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01793) - "invalid qop `%s' received: %s", - resp->message_qop, r->uri); - note_digest_auth_failure(r, conf, resp, 0); - return HTTP_UNAUTHORIZED; - } - - exp_digest = new_digest(r, resp); + /* RFC 2617 (or 7616)-style Digest hash calculation. */ + const char *exp_digest = new_digest(r, resp); if (!exp_digest) { /* we failed to allocate a client struct */ return HTTP_INTERNAL_SERVER_ERROR; @@ -1859,9 +1786,6 @@ static int add_auth_info(request_rec *r) return OK; } - /* 2069-style entity-digest is not supported (it's too hard, and - * there are no clients which support 2069 but not 2617). */ - /* setup nextnonce */ if (conf->nonce_lifetime > 0) { @@ -1884,15 +1808,7 @@ static int add_auth_info(request_rec *r) /* else nonce never expires, hence no nextnonce */ - /* do rfc-2069 digest - */ - if (!apr_is_empty_array(conf->qop_list) && - !ap_cstr_casecmp(*(const char **)(conf->qop_list->elts), "none") - && resp->message_qop == NULL) { - /* use only RFC-2069 format */ - ai = nextnonce; - } - else { + { const char *resp_dig, *ha1, *a2, *ha2; /* calculate rspauth attribute From 68657cfb473881cdfa06b1ee9feea4f50315bd50 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Mon, 6 Jul 2026 12:50:48 +0000 Subject: [PATCH 04/22] * modules/aaa/mod_auth_digest.c: Remove "weird" override of AuthName directive, which adds complexity for little benefit (avoids putting 20 bytes through SHA1 for each auth attempt). (set_realm): Remove function. (gen_nonce_hash): Create the nonce hash here from scratch. (create_digest_dir_config): Always allocate a config struct. Remove unused dir_name field from digest_config_rec. Co-Authored-By: Claude Opus 4.6 GitHub: PR #661 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1935945 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit 026753617320880fa9dc19d1e9c113cdaceebcd3) --- modules/aaa/mod_auth_digest.c | 58 +++-------------------------------- 1 file changed, 5 insertions(+), 53 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 09203282e63..f4b0be6897d 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -83,9 +83,7 @@ /* struct to hold the configuration info */ typedef struct digest_config_struct { - const char *dir_name; authn_provider_list *providers; - apr_sha1_ctx_t nonce_ctx; apr_time_t nonce_lifetime; int check_nc; const char *algorithm; @@ -463,52 +461,14 @@ static void initialize_child(apr_pool_t *p, server_rec *s) static void *create_digest_dir_config(apr_pool_t *p, char *dir) { - digest_config_rec *conf; - - if (dir == NULL) { - return NULL; - } + digest_config_rec *conf = apr_pcalloc(p, sizeof *conf); - conf = (digest_config_rec *) apr_pcalloc(p, sizeof(digest_config_rec)); - if (conf) { - conf->nonce_lifetime = DFLT_NONCE_LIFE; - conf->dir_name = apr_pstrdup(p, dir); - conf->algorithm = DFLT_ALGORITHM; - } + conf->nonce_lifetime = DFLT_NONCE_LIFE; + conf->algorithm = DFLT_ALGORITHM; return conf; } - -/* - * The realm is no longer precomputed because it may be an expression, which - * makes this hooking of AuthName quite weird. - */ -static const char *set_realm(cmd_parms *cmd, void *config, const char *realm) -{ - digest_config_rec *conf = (digest_config_rec *) config; -#ifdef AP_DEBUG - int i; - - /* check that we got random numbers */ - for (i = 0; i < SECRET_LEN; i++) { - if (secret[i] != 0) - break; - } - ap_assert(i < SECRET_LEN); -#endif - - /* we precompute the part of the nonce hash that is constant (well, - * the host:port would be too, but that varies for .htaccess files - * and directives outside a virtual host section) - */ - apr_sha1_init(&conf->nonce_ctx); - apr_sha1_update_binary(&conf->nonce_ctx, secret, SECRET_LEN); - - - return DECLINE_CMD; -} - static const char *add_authn_provider(cmd_parms *cmd, void *config, const char *arg) { @@ -664,8 +624,6 @@ static const char *set_shmem_size(cmd_parms *cmd, void *config, static const command_rec digest_cmds[] = { - AP_INIT_TAKE1("AuthName", set_realm, NULL, OR_AUTHCFG, - "The authentication realm (e.g. \"Members Only\")"), AP_INIT_ITERATE("AuthDigestProvider", add_authn_provider, NULL, OR_AUTHCFG, "specify the auth providers for a directory or location"), AP_INIT_ITERATE("AuthDigestQop", set_qop, NULL, OR_AUTHCFG, @@ -1061,14 +1019,8 @@ static void gen_nonce_hash(char hash[NONCE_HASH_LEN+1], const char *timestr, con unsigned char sha1[APR_SHA1_DIGESTSIZE]; apr_sha1_ctx_t ctx; - memcpy(&ctx, &conf->nonce_ctx, sizeof(ctx)); - /* - apr_sha1_update_binary(&ctx, (const unsigned char *) server->server_hostname, - strlen(server->server_hostname)); - apr_sha1_update_binary(&ctx, (const unsigned char *) &server->port, - sizeof(server->port)); - */ - + apr_sha1_init(&ctx); + apr_sha1_update_binary(&ctx, secret, SECRET_LEN); apr_sha1_update_binary(&ctx, (const unsigned char *) realm, strlen(realm)); apr_sha1_update_binary(&ctx, (const unsigned char *) timestr, strlen(timestr)); From 91e96257298508dd7cdc9e241dc6e2ff82a02ab1 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Mon, 6 Jul 2026 12:51:01 +0000 Subject: [PATCH 05/22] * modules/aaa/mod_auth_digest.c (set_algorithm): Note that conf->algorithm is a constant ("MD5") so there is no point in overriding it at runtime. Simplify error case. GitHub: PR #661 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1935946 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit 6bb2685379211df37b8296d46701396807a323f1) --- modules/aaa/mod_auth_digest.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index f4b0be6897d..06f909126ba 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -86,7 +86,7 @@ typedef struct digest_config_struct { authn_provider_list *providers; apr_time_t nonce_lifetime; int check_nc; - const char *algorithm; + const char *algorithm; /* currently a constant (MD5). */ char *uri_list; } digest_config_rec; @@ -556,15 +556,11 @@ static const char *set_nc_check(cmd_parms *cmd, void *config, int flag) static const char *set_algorithm(cmd_parms *cmd, void *config, const char *alg) { - if (!ap_cstr_casecmp(alg, "MD5-sess")) { - return "AuthDigestAlgorithm: ERROR: algorithm `MD5-sess' " - "is not implemented"; - } - else if (ap_cstr_casecmp(alg, "MD5")) { - return apr_pstrcat(cmd->pool, "Invalid algorithm in AuthDigestAlgorithm: ", alg, NULL); + if (ap_cstr_casecmp(alg, "MD5")) { + return apr_pstrcat(cmd->pool, "Unsupported algorithm in AuthDigestAlgorithm: ", alg, NULL); } - ((digest_config_rec *) config)->algorithm = alg; + /* conf->algorithm remains the constant, "MD5". */ return NULL; } From 023ba5781c09c94506d36d824e7207fefe46b6aa Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Mon, 6 Jul 2026 12:51:26 +0000 Subject: [PATCH 06/22] * modules/aaa/config.m4, modules/aaa/mod_auth_digest.c (pre_init, initialize_module): Prevent using mod_auth_digest without APR_HAS_SHARED_MEMORY. GitHub: PR #661 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1935947 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit dc9e1377764f7d9621dc43f3c61f7c781da2fd11) --- modules/aaa/config.m4 | 6 +++--- modules/aaa/mod_auth_digest.c | 25 +++++++------------------ 2 files changed, 10 insertions(+), 21 deletions(-) diff --git a/modules/aaa/config.m4 b/modules/aaa/config.m4 index b443761d2b5..766354a1457 100644 --- a/modules/aaa/config.m4 +++ b/modules/aaa/config.m4 @@ -70,9 +70,9 @@ APACHE_MODULE(auth_basic, basic authentication, , , yes) APACHE_MODULE(auth_form, form authentication, , , most) APACHE_MODULE(auth_digest, RFC2617 Digest authentication, , , most, [ APR_CHECK_APR_DEFINE(APR_HAS_RANDOM) - if test $ac_cv_define_APR_HAS_RANDOM = "no"; then - echo "You need APR random support to use mod_auth_digest." - echo "Look at APR configure options --with-egd and --with-devrandom." + APR_CHECK_APR_DEFINE(APR_HAS_SHARED_MEMORY) + if test "${ac_cv_define_APR_HAS_RANDOM}X${ac_cv_define_APR_HAS_SHARED_MEMORY}" != "yesXyes"; then + AC_MSG_NOTICE([mod_auth_digest requires APR with random and shared memory support]) enable_auth_digest="no" fi ]) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 06f909126ba..ad04c6183e8 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -254,8 +254,6 @@ static apr_status_t rmm_free(apr_rmm_t *rmm, void *alloc) return apr_rmm_free(rmm, offset); } -#if APR_HAS_SHARED_MEMORY - static int initialize_tables(server_rec *s, apr_pool_t *ctx) { unsigned long idx; @@ -360,13 +358,17 @@ static int initialize_tables(server_rec *s, apr_pool_t *ctx) return OK; } -#endif /* APR_HAS_SHARED_MEMORY */ - static int pre_init(apr_pool_t *pconf, apr_pool_t *plog, apr_pool_t *ptemp) { apr_status_t rv; void *retained; + if (!APR_HAS_SHARED_MEMORY) { + ap_log_error(APLOG_MARK, APLOG_CRIT, rv, NULL, APLOGNO(10590) + "mod_auth_digest cannot be used on platforms without shared memory support"); + return !OK; + } + rv = ap_mutex_register(pconf, client_mutex_type, NULL, APR_LOCK_DEFAULT, 0); if (rv != APR_SUCCESS) return !OK; @@ -403,7 +405,6 @@ static int initialize_module(apr_pool_t *p, apr_pool_t *plog, if (ap_state_query(AP_SQ_MAIN_STATE) == AP_SQ_MS_CREATE_PRE_CONFIG) return OK; -#if APR_HAS_SHARED_MEMORY /* Note: this stuff is currently fixed for the lifetime of the server, * i.e. even across restarts. This means that A) any shmem-size * configuration changes are ignored, and B) certain optimizations, @@ -414,11 +415,7 @@ static int initialize_module(apr_pool_t *p, apr_pool_t *plog, * last child dies. Therefore we can never clean up the old stuff, * creating a creeping memory leak. */ - if (initialize_tables(s, p) != OK) { - return !OK; - } -#endif /* APR_HAS_SHARED_MEMORY */ - return OK; + return initialize_tables(s, p); } static void initialize_child(apr_pool_t *p, server_rec *s) @@ -542,14 +539,6 @@ static const char *set_nonce_lifetime(cmd_parms *cmd, void *config, static const char *set_nc_check(cmd_parms *cmd, void *config, int flag) { -#if !APR_HAS_SHARED_MEMORY - if (flag) { - return "AuthDigestNcCheck: ERROR: nonce-count checking " - "is not supported on platforms without shared-memory " - "support"; - } -#endif - ((digest_config_rec *) config)->check_nc = flag; return NULL; } From 6a017722255c8f0a366aa50a69c8e2d3c2874cc5 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Mon, 6 Jul 2026 12:51:39 +0000 Subject: [PATCH 07/22] * modules/aaa/mod_auth_digest.c: Use apr_uint32_t for the one-time-nonce counter. (gen_nonce): Drop the fallback case for non-shmem builds where otn_counter could be NULL previously. Use atomics to increment the counter. GitHub: PR #661 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1935948 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit 584eb25bd111248db000346a3b680e363a87723e) --- modules/aaa/mod_auth_digest.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index ad04c6183e8..191fa4690ab 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -56,6 +56,7 @@ #include "apr_errno.h" #include "apr_global_mutex.h" #include "apr_strings.h" +#include "apr_atomic.h" #define APR_WANT_STRFUNC #include "apr_want.h" @@ -174,7 +175,7 @@ static unsigned char *secret; static apr_shm_t *client_shm = NULL; static apr_rmm_t *client_rmm = NULL; static unsigned long *opaque_cntr; -static apr_time_t *otn_counter; /* one-time-nonce counter */ +static volatile apr_uint32_t *otn_counter; /* one-time-nonce counter */ static apr_global_mutex_t *client_lock = NULL; static apr_global_mutex_t *opaque_lock = NULL; static const char *client_mutex_type = "authdigest-client"; @@ -1032,15 +1033,8 @@ static const char *gen_nonce(apr_pool_t *p, apr_time_t now, const char *opaque, if (conf->nonce_lifetime != 0) { t.time = now; } - else if (otn_counter) { - /* this counter is not synch'd, because it doesn't really matter - * if it counts exactly. - */ - t.time = (*otn_counter)++; - } else { - /* XXX: WHAT IS THIS CONSTANT? */ - t.time = 42; + t.time = apr_atomic_inc32(otn_counter); } apr_base64_encode_binary(nonce, t.arr, sizeof(t.arr)); gen_nonce_hash(nonce+NONCE_TIME_LEN, nonce, opaque, server, conf, realm); From ef959e1e197605072b3287d69115cb430385e054 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Mon, 6 Jul 2026 12:51:50 +0000 Subject: [PATCH 08/22] * modules/aaa/mod_auth_digest.c (get_digest_rec): Parse opaque more strictly. GitHub: PR #661 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1935949 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit 81a13c618ba06680d8ed26335f21d29f34c8f19f) --- modules/aaa/mod_auth_digest.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 191fa4690ab..b1961624773 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -950,7 +950,13 @@ static int get_digest_rec(request_rec *r, digest_header_rec *resp) } if (resp->opaque) { - resp->opaque_num = (unsigned long) strtol(resp->opaque, NULL, 16); + char *endptr; + long num; + + errno = 0; + num = strtol(resp->opaque, &endptr, 16); + if (errno == 0 && *endptr == '\0' && num > 0) + resp->opaque_num = (unsigned long)num; } resp->auth_hdr_sts = VALID; From 69e1cadfc7fccd6cb205d06fddea45cfdfb8f1a5 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Mon, 6 Jul 2026 12:52:00 +0000 Subject: [PATCH 09/22] * modules/aaa/mod_auth_digest.c (cleanup_tables, initialize_tables, initialize_child, gen_client): Replace mutex-protected opaque_cntr with opaque_counter, incremented via atomics. GitHub: PR #661 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1935950 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit a23632f752534c3a31e2654365a600948ad6abae) --- modules/aaa/mod_auth_digest.c | 40 +++++------------------------------ 1 file changed, 5 insertions(+), 35 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index b1961624773..8933de73260 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -174,10 +174,9 @@ static unsigned char *secret; static apr_shm_t *client_shm = NULL; static apr_rmm_t *client_rmm = NULL; -static unsigned long *opaque_cntr; +static volatile apr_uint32_t *opaque_counter; static volatile apr_uint32_t *otn_counter; /* one-time-nonce counter */ static apr_global_mutex_t *client_lock = NULL; -static apr_global_mutex_t *opaque_lock = NULL; static const char *client_mutex_type = "authdigest-client"; static const char *opaque_mutex_type = "authdigest-opaque"; static const char *client_shm_filename; @@ -216,11 +215,6 @@ static apr_status_t cleanup_tables(void *not_used) client_lock = NULL; } - if (opaque_lock) { - apr_global_mutex_destroy(opaque_lock); - opaque_lock = NULL; - } - client_list = NULL; return APR_SUCCESS; @@ -267,7 +261,6 @@ static int initialize_tables(server_rec *s, apr_pool_t *ctx) client_shm = NULL; client_rmm = NULL; client_lock = NULL; - opaque_lock = NULL; client_list = NULL; /* @@ -329,20 +322,12 @@ static int initialize_tables(server_rec *s, apr_pool_t *ctx) /* setup opaque */ - opaque_cntr = rmm_malloc(client_rmm, sizeof(*opaque_cntr)); - if (opaque_cntr == NULL) { + opaque_counter = rmm_malloc(client_rmm, sizeof *opaque_counter); + if (opaque_counter == NULL) { log_error_and_cleanup("failed to allocate shared memory", -1, s); return !OK; } - *opaque_cntr = 1UL; - - sts = ap_global_mutex_create(&opaque_lock, NULL, opaque_mutex_type, NULL, - s, ctx, 0); - if (sts != APR_SUCCESS) { - log_error_and_cleanup("failed to create lock (opaque_lock)", sts, s); - return !OK; - } - + *opaque_counter = 1; /* setup one-time-nonce counter */ @@ -444,13 +429,6 @@ static void initialize_child(apr_pool_t *p, server_rec *s) log_error_and_cleanup("failed to create lock (client_lock)", sts, s); return; } - sts = apr_global_mutex_child_init(&opaque_lock, - apr_global_mutex_lockfile(opaque_lock), - p); - if (sts != APR_SUCCESS) { - log_error_and_cleanup("failed to create lock (opaque_lock)", sts, s); - return; - } } /* @@ -1059,17 +1037,9 @@ static const char *gen_nonce(apr_pool_t *p, apr_time_t now, const char *opaque, */ static client_entry *gen_client(const request_rec *r) { - unsigned long op; + apr_uint32_t op = apr_atomic_inc32(opaque_counter); client_entry new_entry = { 0, NULL, 0, "" }, *entry; - if (!opaque_cntr) { - return NULL; - } - - apr_global_mutex_lock(opaque_lock); - op = (*opaque_cntr)++; - apr_global_mutex_unlock(opaque_lock); - if (!(entry = 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"); From e96a658a414f085ad1d5f08ab0d05470f27fdea4 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Mon, 6 Jul 2026 12:52:10 +0000 Subject: [PATCH 10/22] * modules/aaa/mod_auth_digest.c (pre_init): Don't register now-unused "authdigest-opaque" mutex type. GitHub: PR #661 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1935951 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit 9edc7f541fc77d070d0cf9b7835d81587a892d1f) --- modules/aaa/mod_auth_digest.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 8933de73260..6c0bc28e2fe 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -178,7 +178,6 @@ static volatile apr_uint32_t *opaque_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"; -static const char *opaque_mutex_type = "authdigest-opaque"; static const char *client_shm_filename; #define DEF_SHMEM_SIZE 1000L /* ~ 12 entries */ @@ -356,9 +355,6 @@ static int pre_init(apr_pool_t *pconf, apr_pool_t *plog, apr_pool_t *ptemp) } rv = ap_mutex_register(pconf, client_mutex_type, NULL, APR_LOCK_DEFAULT, 0); - if (rv != APR_SUCCESS) - return !OK; - rv = ap_mutex_register(pconf, opaque_mutex_type, NULL, APR_LOCK_DEFAULT, 0); if (rv != APR_SUCCESS) return !OK; From 644945e524bec20238b5ce4d892d88b708b131a2 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Mon, 6 Jul 2026 12:52:23 +0000 Subject: [PATCH 11/22] * modules/aaa/mod_auth_digest.c: Remove checks for NULL client_shm throughout. GitHub: PR #661 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1935952 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit acc2d562f1e628b4f504557f35caaa618027ff23) --- modules/aaa/mod_auth_digest.c | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 6c0bc28e2fe..ccb3e22de11 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -404,10 +404,6 @@ static void initialize_child(apr_pool_t *p, server_rec *s) { apr_status_t sts; - if (!client_shm) { - return; - } - /* Get access to rmm in child */ sts = apr_rmm_attach(&client_rmm, NULL, @@ -663,8 +659,7 @@ static client_entry *get_client(unsigned long key, const request_rec *r) int bucket; client_entry *entry, *prev = NULL; - - if (!key || !client_shm) return NULL; + if (!key) return NULL; bucket = key % client_list->tbl_len; entry = client_list->table[bucket]; @@ -760,8 +755,7 @@ static client_entry *add_client(unsigned long key, client_entry *info, int bucket; client_entry *entry; - - if (!key || !client_shm) { + if (!key) { return NULL; } @@ -1246,14 +1240,7 @@ static int check_nc(const request_rec *r, const digest_header_rec *resp, const char *snc = resp->nonce_count; char *endptr; - if (conf->check_nc && !client_shm) { - /* Shouldn't happen, but just in case... */ - ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01771) - "cannot check nonce count without shared memory"); - return OK; - } - - if (!conf->check_nc || !client_shm) { + if (!conf->check_nc) { return OK; } From d9af54b1c03cf7432d57998a921a93c4b12b4c34 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Mon, 6 Jul 2026 12:52:32 +0000 Subject: [PATCH 12/22] * modules/aaa/mod_auth_digest.c: Fix comments. GitHub: PR #661 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1935953 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit 41dce1707f863cdc6cf41c34141ce41f5f2ab053) --- modules/aaa/mod_auth_digest.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index ccb3e22de11..14b42f0fa54 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -29,7 +29,7 @@ * Open Issues: * - qop=auth-int (when streams and trailer support available) * - nonce-format configurability - * - Proxy-Authorization-Info header is set by this module, but is + * - Proxy-Authentication-Info header is set by this module, but is * currently ignored by mod_proxy (needs patch to mod_proxy) * - The source of the secret should be run-time directive (with server * scope: RSRC_CONF) @@ -1658,10 +1658,7 @@ static int authenticate_digest_user(request_rec *r) return OK; } -/* - * Authorization-Info header code - */ - +/* Authentication-Info header code. */ static int add_auth_info(request_rec *r) { const digest_config_rec *conf = From ba85c11f750393ef0b3abc6a0e312c1882f3744c Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Mon, 6 Jul 2026 12:52:41 +0000 Subject: [PATCH 13/22] * modules/aaa/mod_auth_digest.c (add_auth_info): Don't add (Proxy-)Authentication-Info headers to 401/407 responses. GitHub: PR #661 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1935954 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit 4e4c7fb9379de42118643f354f241aac6dcf3f76) --- modules/aaa/mod_auth_digest.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 14b42f0fa54..5f51686ea28 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -1673,8 +1673,14 @@ static int add_auth_info(request_rec *r) return OK; } - /* setup nextnonce - */ + /* Don't add Authentication-Info for 401/407 responses. */ + if (apr_table_get(r->err_headers_out, + (r->proxyreq == PROXYREQ_PROXY) + ? "Proxy-Authenticate" : "WWW-Authenticate")) { + return OK; + } + + /* Set up nextnonce for one-time-nonces and expiring-nonce cases. */ if (conf->nonce_lifetime > 0) { /* send nextnonce if current nonce will expire in less than 30 secs */ if ((r->request_time - resp->nonce_time) > (conf->nonce_lifetime-NEXTNONCE_DELTA)) { From e07f87931399d3c8e9f36b096594f6473264c21d Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Thu, 13 Aug 2026 15:15:07 +0000 Subject: [PATCH 14/22] * modules/aaa/mod_auth_digest.c: Fix AuthDigestNcCheck: track the nonce-count per-nonce, require it to increase (RFC 7616 3.4.3), and record it only for a fully verified request. * test/modules/aaa/test_007_replay.py: Add replay tests. * test/modules/aaa/test_003_nccheck.py, test/modules/aaa/conftest.py: Update for the new semantics. Assisted-by: Claude Opus 5 (1M context) GitHub: PR #705 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1937104 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit 113f027c169d2a533f03f7c6f5e4b5af8ef44123) --- modules/aaa/mod_auth_digest.c | 113 ++++++--- test/modules/aaa/conftest.py | 93 +++++++ .../digest/nccheck-shortlife/secret.txt | 1 + test/modules/aaa/test_003_nccheck.py | 116 +++++++++ test/modules/aaa/test_007_replay.py | 237 ++++++++++++++++++ 5 files changed, 525 insertions(+), 35 deletions(-) create mode 100644 test/modules/aaa/conftest.py create mode 100644 test/modules/aaa/htdocs/digest/nccheck-shortlife/secret.txt create mode 100644 test/modules/aaa/test_003_nccheck.py create mode 100644 test/modules/aaa/test_007_replay.py diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 5f51686ea28..4b0c6731e30 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -118,7 +118,10 @@ typedef struct digest_config_struct { typedef struct hash_entry { unsigned long key; /* the key for this entry */ struct hash_entry *next; /* next entry in the bucket */ - unsigned long nonce_count; /* for nonce-count checking */ + unsigned long nonce_count; /* highest nonce-count seen + * for last_nonce_time */ + apr_time_t last_nonce_time; /* nonce of the last request + * accepted for this client */ char last_nonce[NONCE_LEN+1]; /* for one-time nonce's */ } client_entry; @@ -932,21 +935,22 @@ static int get_digest_rec(request_rec *r, digest_header_rec *resp) } -/* Because the browser may preemptively send auth info, incrementing the - * nonce-count when it does, and because the client does not get notified - * if the URI didn't need authentication after all, we need to be sure to - * update the nonce-count each time we receive an Authorization header no - * matter what the final outcome of the request. Furthermore this is a - * convenient place to get the request-uri (before any subrequests etc - * are initiated) and to initialize the request_config. +/* This is a convenient place to parse the Authorization header, to get the + * request-uri (before any subrequests etc are initiated) and to initialize + * the request_config. + * + * Note that the nonce-count tracked for the client is deliberately NOT + * 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(). * * Note that this must be called after mod_proxy had its go so that * r->proxyreq is set correctly. */ -static int parse_hdr_and_update_nc(request_rec *r) +static int parse_digest_header(request_rec *r) { digest_header_rec *resp; - int res; if (!ap_is_initial_req(r)) { return DECLINED; @@ -959,11 +963,8 @@ static int parse_hdr_and_update_nc(request_rec *r) resp->method = r->method; ap_set_module_config(r->request_config, &auth_digest_module, resp); - res = get_digest_rec(r, resp); + get_digest_rec(r, resp); resp->client = get_client(resp->opaque_num, r); - if (res == OK && resp->client) { - resp->client->nonce_count++; - } return DECLINED; } @@ -1028,7 +1029,7 @@ static const char *gen_nonce(apr_pool_t *p, apr_time_t now, const char *opaque, static client_entry *gen_client(const request_rec *r) { apr_uint32_t op = apr_atomic_inc32(opaque_counter); - client_entry new_entry = { 0, NULL, 0, "" }, *entry; + client_entry new_entry = { 0, NULL, 0, 0, "" }, *entry; if (!(entry = add_client(op, &new_entry, r->server))) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01769) @@ -1088,9 +1089,11 @@ static void note_digest_auth_failure(request_rec *r, } } else { + /* Note that the nonce-count tracked for this client is left alone + * 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. */ opaque = resp->opaque; - /* we're generating a new nonce, so reset the nonce-count */ - resp->client->nonce_count = 0; } if (opaque[0]) { @@ -1233,12 +1236,33 @@ static authn_status get_hash(request_rec *r, const char *user, return auth_result; } -static int check_nc(const request_rec *r, const digest_header_rec *resp, - const digest_config_rec *conf) +/* Check the nonce-count of a request against the count tracked for this + * client, and update the tracked count. + * + * 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, and a request using an older + * nonce is replaying a superseded one. 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. + * + * 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. Otherwise a bogus or replayed request could rewind the count + * and so lock out the legitimate client, and line the count up with the + * nonce-count of the replayed request itself. + */ +static int check_and_record_nonce(const request_rec *r, + const digest_header_rec *resp, + const digest_config_rec *conf) { - unsigned long nc; + client_entry *client = resp->client; + unsigned long nc, tracked; const char *snc = resp->nonce_count; char *endptr; + int accepted; if (!conf->check_nc) { return OK; @@ -1251,15 +1275,33 @@ static int check_nc(const request_rec *r, const digest_header_rec *resp, return !OK; } - if (!resp->client) { + if (!client) { + /* Without an opaque identifying the client there is nothing to + * check the nonce-count against. */ return !OK; } - if (nc != resp->client->nonce_count) { + apr_global_mutex_lock(client_lock); + + tracked = client->nonce_count; + + /* Accept, and record, iff the client has moved on to a newer nonce (in + * which case this is the first request counted for that nonce), or is + * still on the tracked nonce and has raised the count. */ + accepted = (resp->nonce_time > client->last_nonce_time) + || (resp->nonce_time == client->last_nonce_time && nc > tracked); + if (accepted) { + client->last_nonce_time = resp->nonce_time; + client->nonce_count = nc; + } + + apr_global_mutex_unlock(client_lock); + + if (!accepted) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01774) - "Warning, possible replay attack: nonce-count " - "check failed: %lu != %lu", nc, - resp->client->nonce_count); + "Warning, possible replay attack: nonce-count check " + "failed: %lu is not above %lu for nonce %s", nc, + tracked, resp->nonce); return !OK; } @@ -1644,17 +1686,20 @@ static int authenticate_digest_user(request_rec *r) } } - if (check_nc(r, resp, conf) != OK) { - note_digest_auth_failure(r, conf, resp, 0); - return HTTP_UNAUTHORIZED; - } - - /* Note: this check is done last so that a "stale=true" can be - generated if the nonce is old */ + /* Note: the nonce is checked before the nonce-count so that the + * nonce-count state is only ever updated for a request which is using + * a nonce this server issued, and so that a request using an expired + * nonce gets a "stale=true" challenge (and hence a silent retry with a + * fresh nonce-count) rather than being reported as a replay. */ if ((res = check_nonce(r, resp, conf))) { return res; } + if (check_and_record_nonce(r, resp, conf) != OK) { + note_digest_auth_failure(r, conf, resp, 0); + return HTTP_UNAUTHORIZED; + } + return OK; } @@ -1688,8 +1733,6 @@ static int add_auth_info(request_rec *r) gen_nonce(r->pool, r->request_time, resp->opaque, r->server, conf, ap_auth_name(r)), "\"", NULL); - if (resp->client) - resp->client->nonce_count = 0; } } else if (conf->nonce_lifetime == 0 && resp->client) { @@ -1756,7 +1799,7 @@ static void register_hooks(apr_pool_t *p) ap_hook_pre_config(pre_init, NULL, NULL, APR_HOOK_MIDDLE); ap_hook_post_config(initialize_module, NULL, cfgPost, APR_HOOK_MIDDLE); ap_hook_child_init(initialize_child, NULL, NULL, APR_HOOK_MIDDLE); - ap_hook_post_read_request(parse_hdr_and_update_nc, parsePre, NULL, APR_HOOK_MIDDLE); + ap_hook_post_read_request(parse_digest_header, parsePre, NULL, APR_HOOK_MIDDLE); ap_hook_check_authn(authenticate_digest_user, NULL, NULL, APR_HOOK_MIDDLE, AP_AUTH_INTERNAL_PER_CONF); diff --git a/test/modules/aaa/conftest.py b/test/modules/aaa/conftest.py new file mode 100644 index 00000000000..09e0f7d7b57 --- /dev/null +++ b/test/modules/aaa/conftest.py @@ -0,0 +1,93 @@ +import logging +import os +import sys + +import pytest + +from .env import AAATestEnv +from pyhttpd.conf import HttpdConf + +sys.path.append(os.path.join(os.path.dirname(__file__), '../..')) + + +def pytest_report_header(config, start_path): + env = AAATestEnv() + return f"mod_auth_digest [apache: {env.get_httpd_version()}, mpm: {env.mpm_module}, {env.prefix}]" + + +def _digest_dir(docs, path, extra_lines): + lines = [ + f'', + ' AuthType Digest', + f' AuthName "{AAATestEnv.REALM}"', + ] + lines.extend(f" {l}" for l in extra_lines) + lines.append(' Require valid-user') + lines.append('') + return lines + + +@pytest.fixture(scope="package") +def env(pytestconfig) -> AAATestEnv: + level = logging.INFO + console = logging.StreamHandler() + console.setLevel(level) + console.setFormatter(logging.Formatter('%(levelname)s: %(message)s')) + logging.getLogger('').addHandler(console) + logging.getLogger('').setLevel(level=level) + env = AAATestEnv(pytestconfig=pytestconfig) + env.setup_httpd() + env.apache_access_log_clear() + env.httpd_error_log.clear_log() + + docs = env.server_docs_dir + pwfile = env.digest_pwfile + conf = HttpdConf(env) + conf.add(_digest_dir(docs, "default", [ + 'AuthDigestProvider file', + f'AuthUserFile "{pwfile}"', + ])) + conf.add(_digest_dir(docs, "nccheck", [ + 'AuthDigestProvider file', + f'AuthUserFile "{pwfile}"', + 'AuthDigestNcCheck On', + ])) + conf.add(_digest_dir(docs, "nccheck-shortlife", [ + 'AuthDigestProvider file', + f'AuthUserFile "{pwfile}"', + 'AuthDigestNcCheck On', + 'AuthDigestNonceLifetime 2', + ])) + conf.add(_digest_dir(docs, "shortlife", [ + 'AuthDigestProvider file', + f'AuthUserFile "{pwfile}"', + 'AuthDigestNonceLifetime 2', + ])) + conf.add(_digest_dir(docs, "neverexpire", [ + 'AuthDigestProvider file', + f'AuthUserFile "{pwfile}"', + 'AuthDigestNonceLifetime -1', + ])) + conf.add(_digest_dir(docs, "onetime", [ + 'AuthDigestProvider file', + f'AuthUserFile "{pwfile}"', + 'AuthDigestNonceLifetime 0', + ])) + conf.add(_digest_dir(docs, "domain", [ + 'AuthDigestProvider file', + f'AuthUserFile "{pwfile}"', + 'AuthDigestDomain "/digest/domain/" "https://mirror.example.org/other/"', + ])) + conf.add(_digest_dir(docs, "noprovider", [ + # AuthDigestProvider intentionally omitted: falls back to "file". + f'AuthUserFile "{pwfile}"', + ])) + conf.install() + assert env.apache_restart() == 0 + return env + + +@pytest.fixture(autouse=True, scope="package") +def _stop_package_scope(env): + yield + assert env.apache_stop() == 0 diff --git a/test/modules/aaa/htdocs/digest/nccheck-shortlife/secret.txt b/test/modules/aaa/htdocs/digest/nccheck-shortlife/secret.txt new file mode 100644 index 00000000000..fe15209e018 --- /dev/null +++ b/test/modules/aaa/htdocs/digest/nccheck-shortlife/secret.txt @@ -0,0 +1 @@ +digest-nccheck-secret diff --git a/test/modules/aaa/test_003_nccheck.py b/test/modules/aaa/test_003_nccheck.py new file mode 100644 index 00000000000..d4375243d68 --- /dev/null +++ b/test/modules/aaa/test_003_nccheck.py @@ -0,0 +1,116 @@ +"""AuthDigestNcCheck replay-detection scenarios. + +The semantics are those of RFC 7616 3.4.3: the nonce-count is counted by +the client per-nonce, so the server tracks a count per (client, nonce) pair +and requires it to strictly increase. Within one nonce, an nc which has +already been seen is a replay and is rejected; a *higher* nc than expected +is not, since the client also counts the requests it sends to URIs in the +protection space which turn out not to need authentication, and the server +never sees those. Moving to a newer nonce starts a fresh count, and a nonce +the client has already moved on from is rejected. + +The tracked count is only ever updated for a fully verified request, so a +failed request cannot disturb the count of the client whose opaque it +quotes; test_007_replay.py covers that property directly. +""" + +from . import digest_client as dc +from .env import AAATestEnv + + +class TestDigestNcCheck: + + def url(self, env, location, path="secret.txt"): + return env.mkurl("http", "aaa", f"/digest/{location}/{path}") + + def challenge(self, env, location): + r = env.curl_get(self.url(env, location)) + assert r.response["status"] == 401 + return dc.DigestChallenge.parse(r.response["header"]["www-authenticate"]) + + def authenticate(self, env, location, challenge, nc, cnonce="ncc-test-cnonce", + include_opaque=True): + uri = f"/digest/{location}/secret.txt" + auth = dc.build_authorization( + AAATestEnv.DIGEST_USER, challenge, AAATestEnv.DIGEST_PASSWORD, + method="GET", uri=uri, nc=nc, cnonce=cnonce, + include_opaque=include_opaque) + return env.curl_get(self.url(env, location), options=["-H", f"Authorization: {auth}"]) + + def test_digest_030_nccheck_requires_opaque(self, env): + # with AuthDigestNcCheck on, the server cannot verify nc without + # having tracked this client via its opaque -- omitting the opaque + # therefore fails, even with nc=00000001. It is rejected before the + # nc check is even reached: the nonce hash is computed over the + # opaque (gen_nonce_hash()), so a nonce quoted without the opaque it + # was issued with does not verify, and that is reported as stale. + challenge = self.challenge(env, "nccheck") + assert challenge.opaque is not None + r = self.authenticate(env, "nccheck", challenge, nc="00000001", include_opaque=False) + assert r.response["status"] == 401 + new_challenge = dc.DigestChallenge.parse(r.response["header"]["www-authenticate"]) + assert new_challenge.stale is True + env.httpd_error_log.ignore_recent(lognos=["AH01776"]) + + def test_digest_031_nccheck_sequential_ok(self, env): + challenge = self.challenge(env, "nccheck") + r1 = self.authenticate(env, "nccheck", challenge, nc="00000001") + assert r1.response["status"] == 200 + r2 = self.authenticate(env, "nccheck", challenge, nc="00000002") + assert r2.response["status"] == 200 + r3 = self.authenticate(env, "nccheck", challenge, nc="00000003") + assert r3.response["status"] == 200 + + def test_digest_032_nccheck_replay_rejected(self, env): + challenge = self.challenge(env, "nccheck") + r1 = self.authenticate(env, "nccheck", challenge, nc="00000001") + assert r1.response["status"] == 200 + r2 = self.authenticate(env, "nccheck", challenge, nc="00000002") + assert r2.response["status"] == 200 + + # replay an already-used nc -> rejected, and NOT reported as stale + # (this is a distinct failure mode from an invalid/expired nonce). + r3 = self.authenticate(env, "nccheck", challenge, nc="00000001") + assert r3.response["status"] == 401 + new_challenge = dc.DigestChallenge.parse(r3.response["header"]["www-authenticate"]) + assert new_challenge.stale is False + env.httpd_error_log.ignore_recent(lognos=["AH01774"]) + + # recovery: the rejected attempt handed out a fresh challenge for + # this client, and following it -- new nonce, so the count starts + # over at 00000001 -- authenticates again. + r4 = self.authenticate(env, "nccheck", new_challenge, nc="00000001") + assert r4.response["status"] == 200 + + # the superseded nonce is not usable any more, at any nc. + r5 = self.authenticate(env, "nccheck", challenge, nc="00000003") + assert r5.response["status"] == 401 + env.httpd_error_log.ignore_recent(lognos=["AH01774"]) + + def test_digest_033_nccheck_skip_ahead_allowed(self, env): + challenge = self.challenge(env, "nccheck") + r1 = self.authenticate(env, "nccheck", challenge, nc="00000001") + assert r1.response["status"] == 200 + + # skipping ahead is allowed: nc only has to be higher than the + # highest already seen for this nonce, not exactly one more. A + # client legitimately produces gaps by sending counted requests to + # URIs in the protection space which don't need authentication, and + # a higher nc is not a replay in any case. + r2 = self.authenticate(env, "nccheck", challenge, nc="00000009") + assert r2.response["status"] == 200 + + # ...and the skipped-over counts are spent: they are no longer + # accepted afterwards. + r3 = self.authenticate(env, "nccheck", challenge, nc="00000005") + assert r3.response["status"] == 401 + env.httpd_error_log.ignore_recent(lognos=["AH01774"]) + + def test_digest_034_no_nccheck_allows_replay(self, env): + # the "default" location has no AuthDigestNcCheck (Off by default), + # so replaying the exact same nc is not detected or rejected. + challenge = self.challenge(env, "default") + r1 = self.authenticate(env, "default", challenge, nc="00000001") + assert r1.response["status"] == 200 + r2 = self.authenticate(env, "default", challenge, nc="00000001") + assert r2.response["status"] == 200 diff --git a/test/modules/aaa/test_007_replay.py b/test/modules/aaa/test_007_replay.py new file mode 100644 index 00000000000..ba61ed9eaaa --- /dev/null +++ b/test/modules/aaa/test_007_replay.py @@ -0,0 +1,237 @@ +"""Replay-attack scenarios against AuthDigestNcCheck. + +AuthDigestNcCheck exists to detect replayed requests: the server tracks the +highest nonce-count it has accepted from a client (identified by its opaque) +for the nonce that client is using, and requires each request to raise it. + +The security property under test here is not just "the replayed request is +rejected", but that rejecting it must not damage the legitimate client: + + With nonce-count checking enabled, a replay attack MUST NOT affect the + original (legitimate) client by resetting its nonce count. + +It used to. On a failed authentication mod_auth_digest issues a fresh +challenge via note_digest_auth_failure(), and for an already-known +(opaque-identified) client that path reset client->nonce_count to 0, while +the post_read_request hook re-incremented the count from 0 on the next +request carrying that opaque. An attacker who could make *any* request fail +for the victim's opaque therefore rewound the victim's counter, with two +consequences: + + * the legitimate client's next in-sequence nc no longer matched, so it + was locked out (denial of service against the victim), and + * the attacker's replayed request lined up with the rewound counter and + was accepted -- 200, 401, 200, 401, ... for one captured header, or + every time if the attacker rewound the counter deliberately first. + +The count is now tracked per (client, nonce) and updated only for a request +which has been fully verified, so a request which fails to authenticate +leaves the victim's state untouched. +""" + +import time + +from . import digest_client as dc +from .env import AAATestEnv + +# See the note in test_003_nccheck.py: a failed nc check is not reported as +# stale, since it is a distinct failure mode from an invalid/expired nonce. +NC_FAILED = "AH01774" +NONCE_HASH_INVALID = "AH01776" +PASSWORD_MISMATCH = "AH01794" + + +class TestDigestReplay: + + LOCATION = "nccheck" + + def url(self, env, path="secret.txt"): + return env.mkurl("http", "aaa", f"/digest/{self.LOCATION}/{path}") + + @property + def uri(self): + return f"/digest/{self.LOCATION}/secret.txt" + + def challenge(self, env): + r = env.curl_get(self.url(env)) + assert r.response["status"] == 401 + return dc.DigestChallenge.parse(r.response["header"]["www-authenticate"]) + + def victim_header(self, challenge, nc, cnonce="victim-cnonce"): + """A correct Authorization header from the legitimate client.""" + return dc.build_authorization( + AAATestEnv.DIGEST_USER, challenge, AAATestEnv.DIGEST_PASSWORD, + method="GET", uri=self.uri, nc=nc, cnonce=cnonce) + + def attacker_header(self, challenge, nc="00000001", cnonce="attacker-cnonce"): + """A well-formed Digest header carrying the victim's opaque and nonce + but a bogus response digest. An attacker who has merely *seen* one of + the victim's requests can build this; no credentials are needed.""" + return dc.build_authorization( + AAATestEnv.DIGEST_USER, challenge, "not-the-password", + method="GET", uri=self.uri, nc=nc, cnonce=cnonce, + response="0" * 32) + + def send(self, env, auth): + return env.curl_get(self.url(env), options=["-H", f"Authorization: {auth}"]) + + def test_digest_070_replay_does_not_lock_out_legit_client(self, env): + # The legitimate client authenticates a few times, in sequence. + challenge = self.challenge(env) + for nc in ["00000001", "00000002", "00000003"]: + assert self.send(env, self.victim_header(challenge, nc)).response["status"] == 200 + + # An attacker replays a request captured earlier in that sequence. + # Rejecting it is correct... + replayed = self.victim_header(challenge, "00000002") + replay_status = self.send(env, replayed).response["status"] + + # ...but it must not disturb the legitimate client, which knows + # nothing of the replay and simply carries on with its next nc. + r = self.send(env, self.victim_header(challenge, "00000004")) + env.httpd_error_log.ignore_recent(lognos=[NC_FAILED]) + assert replay_status == 401 + assert r.response["status"] == 200, \ + "the replay reset the victim's nonce-count and locked it out" + + def test_digest_071_bogus_request_does_not_lock_out_legit_client(self, env): + # Same property, but the attacker does not even need to have captured + # a complete valid request: any well-formed Digest header quoting the + # victim's opaque is enough to rewind the victim's counter. + challenge = self.challenge(env) + for nc in ["00000001", "00000002"]: + assert self.send(env, self.victim_header(challenge, nc)).response["status"] == 200 + + bogus_status = self.send(env, self.attacker_header(challenge)).response["status"] + + r = self.send(env, self.victim_header(challenge, "00000003")) + env.httpd_error_log.ignore_recent(lognos=[NC_FAILED, PASSWORD_MISMATCH]) + assert bogus_status == 401 + assert r.response["status"] == 200, \ + "a bogus request reset the victim's nonce-count and locked it out" + + def test_digest_072_captured_request_is_never_accepted_twice(self, env): + # The flip side of the same defect. One captured Authorization header + # is replayed verbatim; the first send is the genuine request, so it + # succeeds, and every later send must be rejected. Before the fix the + # rejection rewound the counter, so the replay after it lined up + # again: the observed pattern was 200, 401, 200, 401, ... + challenge = self.challenge(env) + captured = self.victim_header(challenge, "00000001", cnonce="captured-cnonce") + + assert self.send(env, captured).response["status"] == 200 + statuses = [self.send(env, captured).response["status"] for _ in range(4)] + env.httpd_error_log.ignore_recent(lognos=[NC_FAILED]) + assert statuses == [401, 401, 401, 401], \ + f"replayed request was accepted again: {statuses}" + + def test_digest_073_attacker_cannot_force_replay_to_succeed(self, env): + # Severity check: the attacker must not be able to line the counter + # up on demand. Before the fix, sending a bogus request first rewound + # the counter to 0, so the replay that followed succeeded every + # single time. + challenge = self.challenge(env) + captured = self.victim_header(challenge, "00000001", cnonce="captured-cnonce") + assert self.send(env, captured).response["status"] == 200 + + statuses = [] + for _ in range(3): + self.send(env, self.attacker_header(challenge)) + statuses.append(self.send(env, captured).response["status"]) + env.httpd_error_log.ignore_recent(lognos=[NC_FAILED, PASSWORD_MISMATCH]) + assert statuses == [401, 401, 401], \ + f"attacker replayed at will by forcing a counter reset: {statuses}" + + def test_digest_074_legit_client_recovers_via_fresh_challenge(self, env): + # Invariant: a client whose nc is rejected is handed a fresh + # challenge, and following that challenge -- new nonce, so the count + # starts over at 1 -- gets it working again. Simply never resetting + # the count, without tying it to the nonce it was counted for, would + # break this. + challenge = self.challenge(env) + assert self.send(env, self.victim_header(challenge, "00000001")).response["status"] == 200 + + # provoke the rejection with a replay of that first request + r = self.send(env, self.victim_header(challenge, "00000001")) + assert r.response["status"] == 401 + env.httpd_error_log.ignore_recent(lognos=[NC_FAILED]) + fresh = dc.DigestChallenge.parse(r.response["header"]["www-authenticate"]) + assert fresh.stale is False + assert fresh.opaque == challenge.opaque, \ + "the client keeps its identity across a re-challenge" + assert fresh.nonce != challenge.nonce + + r = self.send(env, self.victim_header(fresh, "00000001")) + assert r.response["status"] == 200 + + def test_digest_075_nonce_is_bound_to_opaque(self, env): + # A captured header cannot be re-pointed at a *different* client + # session to dodge that session's nonce-count: the nonce hash is + # computed over the opaque (gen_nonce_hash()), so quoting one + # client's nonce under another client's opaque fails the hash check + # outright, and is reported as stale. + victim = self.challenge(env) + captured = self.victim_header(victim, "00000001", cnonce="captured-cnonce") + assert self.send(env, captured).response["status"] == 200 + + attacker = self.challenge(env) + assert attacker.opaque != victim.opaque + spliced = dc.build_authorization( + AAATestEnv.DIGEST_USER, victim, AAATestEnv.DIGEST_PASSWORD, + method="GET", uri=self.uri, nc="00000001", cnonce="captured-cnonce", + opaque=attacker.opaque) + r = self.send(env, spliced) + env.httpd_error_log.ignore_recent(lognos=[NONCE_HASH_INVALID]) + assert r.response["status"] == 401 + assert dc.DigestChallenge.parse( + r.response["header"]["www-authenticate"]).stale is True + + +class TestDigestNcCheckExpiry: + """AuthDigestNcCheck combined with an expiring nonce. + + The nonce is checked before the nonce-count, so that an expired nonce + still produces a "stale=true" challenge rather than being reported as a + replay -- the client then retries silently against the fresh nonce, with + its count restarted at 1. + """ + + LOCATION = "nccheck-shortlife" # AuthDigestNcCheck On, lifetime 2s + + def url(self, env): + return env.mkurl("http", "aaa", f"/digest/{self.LOCATION}/secret.txt") + + def challenge(self, env): + r = env.curl_get(self.url(env)) + assert r.response["status"] == 401 + return dc.DigestChallenge.parse(r.response["header"]["www-authenticate"]) + + def send(self, env, challenge, nc): + auth = dc.build_authorization( + AAATestEnv.DIGEST_USER, challenge, AAATestEnv.DIGEST_PASSWORD, + method="GET", uri=f"/digest/{self.LOCATION}/secret.txt", nc=nc, + cnonce="expiry-cnonce") + return env.curl_get(self.url(env), options=["-H", f"Authorization: {auth}"]) + + def test_digest_076_expired_nonce_restarts_the_count(self, env): + challenge = self.challenge(env) + assert self.send(env, challenge, "00000001").response["status"] == 200 + assert self.send(env, challenge, "00000002").response["status"] == 200 + + time.sleep(3) + + # past its lifetime: reported as stale, not as a nonce-count failure + r = self.send(env, challenge, "00000003") + assert r.response["status"] == 401 + fresh = dc.DigestChallenge.parse(r.response["header"]["www-authenticate"]) + assert fresh.stale is True + + # the client restarts its count for the fresh nonce, which must not + # collide with the count already tracked for the expired one + assert self.send(env, fresh, "00000001").response["status"] == 200 + assert self.send(env, fresh, "00000002").response["status"] == 200 + + # and the expired nonce stays unusable + r = self.send(env, challenge, "00000004") + assert r.response["status"] == 401 + env.httpd_error_log.ignore_recent(lognos=["AH01776", NC_FAILED]) From 69fc3ff8196c90b3ceb9dcfd95fac1ba2118e78c Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Thu, 13 Aug 2026 15:15:19 +0000 Subject: [PATCH 15/22] * modules/aaa/mod_auth_digest.c: Consolidate one-time nonce handling in check_and_record_nonce, accepting a nonce iff it is newer than the last used, and re-challenging as stale when reused. * test/modules/aaa/test_008_onetime_nccheck.py: Add tests for one-time nonces, alone and with AuthDigestNcCheck. * test/modules/aaa/conftest.py: Add an AuthDigestNonceLifetime 0 plus AuthDigestNcCheck location. Assisted-by: Claude Opus 5 (1M context) GitHub: PR #705 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1937105 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit 0c2b5b3a7414695ab5a8a70d3c362019eb38bac5) --- modules/aaa/mod_auth_digest.c | 151 ++++++++------ test/modules/aaa/conftest.py | 6 + .../htdocs/digest/onetime-nccheck/secret.txt | 1 + test/modules/aaa/test_008_onetime_nccheck.py | 185 ++++++++++++++++++ 4 files changed, 280 insertions(+), 63 deletions(-) create mode 100644 test/modules/aaa/htdocs/digest/onetime-nccheck/secret.txt create mode 100644 test/modules/aaa/test_008_onetime_nccheck.py diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 4b0c6731e30..2899296e93c 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -122,7 +122,6 @@ typedef struct hash_entry { * for last_nonce_time */ apr_time_t last_nonce_time; /* nonce of the last request * accepted for this client */ - char last_nonce[NONCE_LEN+1]; /* for one-time nonce's */ } client_entry; static struct hash_table { @@ -139,6 +138,14 @@ static struct hash_table { enum hdr_sts { NO_HEADER, NOT_DIGEST, INVALID, VALID }; +/* Outcome of checking a request's nonce and nonce-count against the state + * tracked for its client. */ +enum nonce_state { + NONCE_ACCEPTED, /* recorded as the latest used by this client */ + NONCE_STALE, /* already used, or the client is unknown */ + NONCE_BAD_COUNT /* nonce-count did not increase: possible replay */ +}; + typedef struct digest_header_struct { const char *scheme; const char *realm; @@ -1009,7 +1016,10 @@ static const char *gen_nonce(apr_pool_t *p, apr_time_t now, const char *opaque, t.time = now; } else { - t.time = apr_atomic_inc32(otn_counter); + /* 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; } apr_base64_encode_binary(nonce, t.arr, sizeof(t.arr)); gen_nonce_hash(nonce+NONCE_TIME_LEN, nonce, opaque, server, conf, realm); @@ -1029,7 +1039,7 @@ static const char *gen_nonce(apr_pool_t *p, apr_time_t now, const char *opaque, static client_entry *gen_client(const request_rec *r) { apr_uint32_t op = apr_atomic_inc32(opaque_counter); - client_entry new_entry = { 0, NULL, 0, 0, "" }, *entry; + client_entry new_entry = { 0, NULL, 0, 0 }, *entry; if (!(entry = add_client(op, &new_entry, r->server))) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01769) @@ -1106,9 +1116,6 @@ static void note_digest_auth_failure(request_rec *r, /* Setup nonce */ nonce = gen_nonce(r->pool, r->request_time, opaque, r->server, conf, ap_auth_name(r)); - if (resp->client && conf->nonce_lifetime == 0) { - memcpy(resp->client->last_nonce, nonce, NONCE_LEN+1); - } /* setup domain attribute. We want to send this attribute wherever * possible so that the client won't send the Authorization header @@ -1236,76 +1243,108 @@ static authn_status get_hash(request_rec *r, const char *user, return auth_result; } -/* Check the nonce-count of a request against the count tracked for this - * client, and update the tracked count. +/* Check the nonce and nonce-count of a fully verified request against the + * state tracked for its client, record them, and generate a new challenge + * if they are not acceptable. * - * 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, and a request using an older - * nonce is replaying a superseded one. 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. + * 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. Otherwise a bogus or replayed request could rewind the count - * and so lock out the legitimate client, and line the count up with the - * nonce-count of the replayed request itself. + * it quotes. */ -static int check_and_record_nonce(const request_rec *r, - const digest_header_rec *resp, - const digest_config_rec *conf) +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; + unsigned long nc, tracked = 0; const char *snc = resp->nonce_count; char *endptr; - int accepted; + enum nonce_state state = NONCE_STALE; - if (!conf->check_nc) { - return OK; + 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 !OK; + note_digest_auth_failure(r, conf, resp, 0); + return HTTP_UNAUTHORIZED; } - if (!client) { - /* Without an opaque identifying the client there is nothing to - * check the nonce-count against. */ - return !OK; - } + if (client) { + apr_global_mutex_lock(client_lock); - 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; + } - tracked = client->nonce_count; + if (state == NONCE_ACCEPTED) { + client->last_nonce_time = resp->nonce_time; + client->nonce_count = nc; + } - /* Accept, and record, iff the client has moved on to a newer nonce (in - * which case this is the first request counted for that nonce), or is - * still on the tracked nonce and has raised the count. */ - accepted = (resp->nonce_time > client->last_nonce_time) - || (resp->nonce_time == client->last_nonce_time && nc > tracked); - if (accepted) { - client->last_nonce_time = resp->nonce_time; - client->nonce_count = nc; + apr_global_mutex_unlock(client_lock); } - apr_global_mutex_unlock(client_lock); - - if (!accepted) { + 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); - return !OK; } - return OK; + switch (state) { + case NONCE_ACCEPTED: + return OK; + + case NONCE_STALE: + /* the credentials were good, so the client can silently retry with + * the nonce from this challenge */ + note_digest_auth_failure(r, conf, resp, 1); + return HTTP_UNAUTHORIZED; + + default: + note_digest_auth_failure(r, conf, resp, 0); + return HTTP_UNAUTHORIZED; + } } static int check_nonce(request_rec *r, digest_header_rec *resp, @@ -1350,16 +1389,8 @@ static int check_nonce(request_rec *r, digest_header_rec *resp, return HTTP_UNAUTHORIZED; } } - else if (conf->nonce_lifetime == 0 && resp->client) { - if (memcmp(resp->client->last_nonce, resp->nonce, NONCE_LEN)) { - ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(01779) - "user %s: one-time-nonce mismatch - sending " - "new nonce", r->user); - note_digest_auth_failure(r, conf, resp, 1); - return HTTP_UNAUTHORIZED; - } - } - /* else (lifetime < 0) => never expires */ + /* else (lifetime <= 0) => never expires by time; a one-time nonce is + * retired by use, in check_and_record_nonce() */ return OK; } @@ -1695,12 +1726,7 @@ static int authenticate_digest_user(request_rec *r) return res; } - if (check_and_record_nonce(r, resp, conf) != OK) { - note_digest_auth_failure(r, conf, resp, 0); - return HTTP_UNAUTHORIZED; - } - - return OK; + return check_and_record_nonce(r, resp, conf); } /* Authentication-Info header code. */ @@ -1739,7 +1765,6 @@ static int add_auth_info(request_rec *r) 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); - memcpy(resp->client->last_nonce, nonce, NONCE_LEN+1); } /* else nonce never expires, hence no nextnonce */ diff --git a/test/modules/aaa/conftest.py b/test/modules/aaa/conftest.py index 09e0f7d7b57..eef98e73e51 100644 --- a/test/modules/aaa/conftest.py +++ b/test/modules/aaa/conftest.py @@ -73,6 +73,12 @@ def env(pytestconfig) -> AAATestEnv: f'AuthUserFile "{pwfile}"', 'AuthDigestNonceLifetime 0', ])) + conf.add(_digest_dir(docs, "onetime-nccheck", [ + 'AuthDigestProvider file', + f'AuthUserFile "{pwfile}"', + 'AuthDigestNonceLifetime 0', + 'AuthDigestNcCheck On', + ])) conf.add(_digest_dir(docs, "domain", [ 'AuthDigestProvider file', f'AuthUserFile "{pwfile}"', diff --git a/test/modules/aaa/htdocs/digest/onetime-nccheck/secret.txt b/test/modules/aaa/htdocs/digest/onetime-nccheck/secret.txt new file mode 100644 index 00000000000..945bf8d92d3 --- /dev/null +++ b/test/modules/aaa/htdocs/digest/onetime-nccheck/secret.txt @@ -0,0 +1 @@ +digest-onetime-secret diff --git a/test/modules/aaa/test_008_onetime_nccheck.py b/test/modules/aaa/test_008_onetime_nccheck.py new file mode 100644 index 00000000000..101abdd549c --- /dev/null +++ b/test/modules/aaa/test_008_onetime_nccheck.py @@ -0,0 +1,185 @@ +"""One-time nonces (AuthDigestNonceLifetime 0), alone and with AuthDigestNcCheck. + +With a lifetime of 0 the server hands the client a nextnonce on every +successful response, and a nonce may be used once: it is accepted only if +it is newer than the last nonce that client used. The client counts from 1 +again for each new nonce, so with AuthDigestNcCheck also on, every request +legitimately carries nc=00000001. + +The security property here is the one from test_007_replay.py, applied to +the other piece of per-client state: + + A request which fails to authenticate MUST NOT invalidate the nonce + which the legitimate client is holding. + +It did, when the client's state was the last nonce *issued* to it: +note_digest_auth_failure() generates a fresh nonce and recorded it there, +and any request quoting the client's opaque can provoke a challenge. So an +eavesdropper who had captured one Authorization header could replay it at +will -- the replay itself was correctly rejected, but it moved the stored +nonce on, and the victim's next request was then refused. The opaque is in +the clear in every challenge and every request, and such a captured header +never goes stale for this purpose, since it works by failing. + +This needed no credentials and, despite where it was first noticed, no +AuthDigestNcCheck: the tests below run against both locations to pin that +the defect was in the one-time-nonce path, not in the combination. + +The state is now the last nonce the client actually *used*, which nothing +unauthenticated can move. +""" + +import pytest + +from . import digest_client as dc +from .env import AAATestEnv + +BOTH = ["onetime", "onetime-nccheck"] + +NC_FAILED = "AH01774" +NONCE_HASH_INVALID = "AH01776" +PASSWORD_MISMATCH = "AH01794" + + +class TestOneTimeNonce: + + def url(self, env, location): + return env.mkurl("http", "aaa", f"/digest/{location}/secret.txt") + + def challenge(self, env, location): + r = env.curl_get(self.url(env, location)) + assert r.response["status"] == 401 + challenge = dc.DigestChallenge.parse( + r.response["header"]["www-authenticate"]) + assert challenge.opaque is not None, \ + "one-time nonces are tracked per client, so an opaque is required" + return challenge + + def header(self, location, challenge, nc="00000001", cnonce="onetime-cnonce", + response=None): + """A correct Authorization header, unless response= overrides the + digest -- an attacker can build that from an observed request + without knowing the password.""" + return dc.build_authorization( + AAATestEnv.DIGEST_USER, challenge, AAATestEnv.DIGEST_PASSWORD, + method="GET", uri=f"/digest/{location}/secret.txt", nc=nc, + cnonce=cnonce, response=response) + + def send(self, env, location, auth): + return env.curl_get(self.url(env, location), + options=["-H", f"Authorization: {auth}"]) + + def follow_nextnonce(self, r, challenge): + """Advance the client to the nextnonce it was just handed.""" + ai = dc.parse_params(r.response["header"]["authentication-info"]) + assert "nextnonce" in ai + assert ai["nextnonce"] != challenge.nonce + challenge.nonce = ai["nextnonce"] + + def test_digest_080_nccheck_does_not_break_the_onetime_chain(self, env): + # Each nonce is new, so the client's count restarts at 1 every time + # and the nonce-count check must not object. (Before the nonce-count + # was tracked per-nonce this alternated 200, 401, 200, 401, ...) + challenge = self.challenge(env, "onetime-nccheck") + for _ in range(4): + r = self.send(env, "onetime-nccheck", self.header( + "onetime-nccheck", challenge, nc="00000001")) + assert r.response["status"] == 200 + self.follow_nextnonce(r, challenge) + + @pytest.mark.parametrize("location", BOTH) + def test_digest_081_onetime_nonce_rejects_immediate_replay(self, env, location): + challenge = self.challenge(env, location) + captured = self.header(location, challenge) + assert self.send(env, location, captured).response["status"] == 200 + + r = self.send(env, location, captured) + env.httpd_error_log.ignore_recent(lognos=[NC_FAILED]) + assert r.response["status"] == 401 + assert dc.DigestChallenge.parse( + r.response["header"]["www-authenticate"]).stale is True + + @pytest.mark.parametrize("location", BOTH) + def test_digest_082_onetime_nonce_rejects_replay_after_rotation(self, env, location): + # The captured header stays rejected once the client has moved on + # through the nextnonce chain. + challenge = self.challenge(env, location) + captured = self.header(location, challenge) + r = self.send(env, location, captured) + assert r.response["status"] == 200 + self.follow_nextnonce(r, challenge) + + r = self.send(env, location, self.header(location, challenge)) + assert r.response["status"] == 200 + self.follow_nextnonce(r, challenge) + + r = self.send(env, location, captured) + env.httpd_error_log.ignore_recent(lognos=[NC_FAILED]) + assert r.response["status"] == 401 + + @pytest.mark.parametrize("location", BOTH) + def test_digest_083_replay_does_not_invalidate_the_clients_nonce(self, env, location): + # The eavesdropper's version: no credentials, no forgery, just one + # captured Authorization header replayed after the client has moved + # on. Rejecting it is correct; denying the client's next request is + # not. + challenge = self.challenge(env, location) + captured = self.header(location, challenge) + r = self.send(env, location, captured) + assert r.response["status"] == 200 + self.follow_nextnonce(r, challenge) + + replay_status = self.send(env, location, captured).response["status"] + + r = self.send(env, location, self.header(location, challenge)) + env.httpd_error_log.ignore_recent(lognos=[NC_FAILED]) + assert replay_status == 401 + assert r.response["status"] == 200, \ + "the replay moved the client's one-time nonce on and locked it out" + + @pytest.mark.parametrize("location", BOTH) + def test_digest_084_bogus_request_does_not_invalidate_the_clients_nonce( + self, env, location): + # Same property with a forged digest rather than a captured one, so + # it holds however the attacker's request comes to fail. + challenge = self.challenge(env, location) + r = self.send(env, location, self.header(location, challenge)) + assert r.response["status"] == 200 + self.follow_nextnonce(r, challenge) + + bogus = self.header(location, challenge, cnonce="bogus", + response="0" * 32) + bogus_status = self.send(env, location, bogus).response["status"] + + r = self.send(env, location, self.header(location, challenge)) + env.httpd_error_log.ignore_recent( + lognos=[NC_FAILED, NONCE_HASH_INVALID, PASSWORD_MISMATCH]) + assert bogus_status == 401 + assert r.response["status"] == 200, \ + "the bogus request moved the client's one-time nonce on and locked it out" + + @pytest.mark.parametrize("location", BOTH) + def test_digest_085_replay_rejected_when_the_client_entry_is_gone(self, env, + location): + # The client table is small -- AuthDigestShmemSize defaults to 1000 + # bytes, "~ 12 entries" -- and a request with no credentials at all + # allocates an entry, since the challenge it gets back has to carry an + # opaque. An attacker can therefore make gc() discard a client's entry + # for the price of a dozen bare requests. + # + # A captured request must still not be replayable once that has + # happened. It used to be: check_nonce() skipped the one-time + # comparison entirely when the client was unknown, so the nonce was + # taken on trust and the replay served the protected resource. + challenge = self.challenge(env, location) + captured = self.header(location, challenge) + assert self.send(env, location, captured).response["status"] == 200 + assert self.send(env, location, captured).response["status"] == 401 + + for _ in range(40): + env.curl_get(self.url(env, location)) + + r = self.send(env, location, captured) + env.httpd_error_log.ignore_recent(lognos=[NC_FAILED]) + assert r.response["status"] == 401, \ + "captured request replayed once the client entry was evicted" From 08a21431b6e90631d2481ea7718ad5d674e3facb Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Thu, 13 Aug 2026 15:15:30 +0000 Subject: [PATCH 16/22] * modules/aaa/mod_auth_digest.c (get_digest_rec): Rename to parse_digest_header, returning the header status the caller stores in resp->auth_hdr_sts. (parse_digest_header): Rename to init_digest_request. Assisted-by: Claude Opus 5 (1M context) GitHub: PR #705 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1937106 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit 6b251695de6269eedf1e125f208a96ab04b62ff8) --- modules/aaa/mod_auth_digest.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 2899296e93c..32b6488cd1a 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -815,8 +815,10 @@ static client_entry *add_client(unsigned long key, client_entry *info, * Authorization header parser code */ -/* Parse the Authorization header, if it exists */ -static int get_digest_rec(request_rec *r, digest_header_rec *resp) +/* 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, + digest_header_rec *resp) { const char *auth_line; apr_size_t l; @@ -828,14 +830,12 @@ static int get_digest_rec(request_rec *r, digest_header_rec *resp) ? "Proxy-Authorization" : "Authorization"); if (!auth_line) { - resp->auth_hdr_sts = NO_HEADER; - return !OK; + return NO_HEADER; } resp->scheme = ap_getword_white(r->pool, &auth_line); if (ap_cstr_casecmp(resp->scheme, "Digest")) { - resp->auth_hdr_sts = NOT_DIGEST; - return !OK; + return NOT_DIGEST; } l = strlen(auth_line); @@ -923,8 +923,7 @@ static int get_digest_rec(request_rec *r, digest_header_rec *resp) || !VALID_NONCE(resp->nonce) || !resp->digest || strlen(resp->digest) != MD5_DIGEST_LEN || (resp->message_qop && (!resp->cnonce || !resp->nonce_count))) { - resp->auth_hdr_sts = INVALID; - return !OK; + return INVALID; } if (resp->opaque) { @@ -937,14 +936,13 @@ static int get_digest_rec(request_rec *r, digest_header_rec *resp) resp->opaque_num = (unsigned long)num; } - resp->auth_hdr_sts = VALID; - return OK; + return VALID; } -/* This is a convenient place to parse the Authorization header, to get the - * request-uri (before any subrequests etc are initiated) and to initialize - * the request_config. +/* 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. * * Note that the nonce-count tracked for the client is deliberately NOT * updated here: the state of an authenticated client must not be altered @@ -955,7 +953,7 @@ static int get_digest_rec(request_rec *r, digest_header_rec *resp) * Note that this must be called after mod_proxy had its go so that * r->proxyreq is set correctly. */ -static int parse_digest_header(request_rec *r) +static int init_digest_request(request_rec *r) { digest_header_rec *resp; @@ -970,7 +968,7 @@ static int parse_digest_header(request_rec *r) resp->method = r->method; ap_set_module_config(r->request_config, &auth_digest_module, resp); - get_digest_rec(r, resp); + resp->auth_hdr_sts = parse_digest_header(r, resp); resp->client = get_client(resp->opaque_num, r); return DECLINED; @@ -1824,7 +1822,7 @@ static void register_hooks(apr_pool_t *p) ap_hook_pre_config(pre_init, NULL, NULL, APR_HOOK_MIDDLE); ap_hook_post_config(initialize_module, NULL, cfgPost, APR_HOOK_MIDDLE); ap_hook_child_init(initialize_child, NULL, NULL, APR_HOOK_MIDDLE); - ap_hook_post_read_request(parse_digest_header, parsePre, NULL, APR_HOOK_MIDDLE); + ap_hook_post_read_request(init_digest_request, parsePre, NULL, APR_HOOK_MIDDLE); ap_hook_check_authn(authenticate_digest_user, NULL, NULL, APR_HOOK_MIDDLE, AP_AUTH_INTERNAL_PER_CONF); From 88bee92cdf7fb343d2c4473bb42cbf52e2578683 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Thu, 13 Aug 2026 15:15:44 +0000 Subject: [PATCH 17/22] * 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) GitHub: PR #705 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1937107 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit 2137354fd75bf414444b2055750fb6f27cbc518b) --- modules/aaa/mod_auth_digest.c | 312 ++++++++++++++++----------- test/modules/aaa/test_003_nccheck.py | 27 +++ 2 files changed, 212 insertions(+), 127 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 32b6488cd1a..fd8d680a93a 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -18,7 +18,7 @@ * mod_auth_digest: MD5 digest authentication * * Originally by Alexei Kosut - * Updated to RFC-2617 by Ronald Tschal�r + * Updated to RFC-2617 by Ronald Tschalär * 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 *s, apr_pool_t *ctx) /* 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 long key, const request_rec *r) 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 long key, client_entry *info, 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 long key, client_entry *info, 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(request_rec *r, 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(request_rec *r, * updated here: the state of an authenticated client must not be altered * by a request which has not (yet) been authenticated, or a replayed or * bogus request quoting the client's opaque would be able to rewind that - * state. See check_and_record_nonce(). + * state. See check_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_rec *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 *p, apr_time_t now, const char *opaque, */ /* - * 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 request_rec *r) * 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(request_rec *r, 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(request_rec *r, 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(request_rec *r, * 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 *r, const char *user, * 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(request_rec *r, digest_header_rec *resp, 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; @@ -1759,7 +1817,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); diff --git a/test/modules/aaa/test_003_nccheck.py b/test/modules/aaa/test_003_nccheck.py index d4375243d68..539c182b66c 100644 --- a/test/modules/aaa/test_003_nccheck.py +++ b/test/modules/aaa/test_003_nccheck.py @@ -114,3 +114,30 @@ def test_digest_034_no_nccheck_allows_replay(self, env): 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" From f52faf74fe9cde2e792aa8a7e4cbcdc546148986 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Thu, 13 Aug 2026 15:15:56 +0000 Subject: [PATCH 18/22] * modules/aaa/mod_auth_digest.c (note_digest_auth_failure): Return 503 when the client entry cannot be allocated, rather than sending a challenge with no opaque. Default the opaque to NULL to match what check_nonce() hashes when the client sends none. Assisted-by: Claude Opus 5 (1M context) GitHub: PR #705 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1937108 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit 25bd92b2b7982295e96a1a26282aa375c23bc46a) --- modules/aaa/mod_auth_digest.c | 106 ++++++++++++++-------------------- 1 file changed, 44 insertions(+), 62 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index fd8d680a93a..d27ab625d7c 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -897,10 +897,8 @@ static int add_client(client_id_t key, client_entry *info, server_rec *s) client_list->num_removed, client_list->num_renewed); entry = rmm_malloc(client_rmm, sizeof(client_entry)); if (!entry) { - 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 0; /* give up */ + return 0; /* give up; the caller logs this */ } } @@ -1153,7 +1151,8 @@ static client_id_t client_generate(const request_rec *r) 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"); + "unable to allocate a client entry - failing the " + "request, since this configuration needs one"); return 0; } @@ -1165,21 +1164,24 @@ static client_id_t client_generate(const request_rec *r) * Authorization challenge generation code (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. */ static const char *ltox(apr_pool_t *p, client_id_t num) { - if (num != 0) { - return apr_psprintf(p, "%x", num); - } - else { - return ""; - } + return apr_psprintf(p, "%x", num); } -static void note_digest_auth_failure(request_rec *r, - const digest_config_rec *conf, - digest_header_rec *resp, int stale) +/* Generate a challenge for the client, and return the status which the + * caller should return for this request: HTTP_UNAUTHORIZED normally, or + * HTTP_SERVICE_UNAVAILABLE if the per-client state which this configuration + * requires could not be allocated. No challenge is sent in that case: it + * could only carry an opaque which identifies nothing, so the client would + * be unable to authenticate through it however often it retried. */ +static int note_digest_auth_failure(request_rec *r, + const digest_config_rec *conf, + digest_header_rec *resp, int stale) { - const char *qop, *opaque, *opaque_param, *domain, *nonce; + const char *qop, *opaque = NULL, *opaque_param = "", *domain, *nonce; client_id_t client_key = 0; /* Setup qop */ @@ -1189,25 +1191,22 @@ static void note_digest_auth_failure(request_rec *r, if (resp->opaque == NULL) { /* new client */ - if ((conf->check_nc || conf->nonce_lifetime == 0) - && (client_key = client_generate(r)) != 0) { + if (conf->check_nc || conf->nonce_lifetime == 0) { + if ((client_key = client_generate(r)) == 0) { + return HTTP_SERVICE_UNAVAILABLE; + } opaque = ltox(r->pool, client_key); } - else { - opaque = ""; /* opaque not needed */ - } + /* else no opaque is needed, and none is sent */ } else if (!client_exists(resp->opaque_num, r)) { /* client info was gc'd */ - client_key = client_generate(r); - if (client_key != 0) { - opaque = ltox(r->pool, client_key); - stale = 1; - client_note_renewed(); - } - else { - opaque = ""; /* ??? */ + if ((client_key = client_generate(r)) == 0) { + return HTTP_SERVICE_UNAVAILABLE; } + opaque = ltox(r->pool, client_key); + stale = 1; + client_note_renewed(); } else { /* Note that the nonce-count tracked for this client is left alone @@ -1218,12 +1217,9 @@ static void note_digest_auth_failure(request_rec *r, opaque = resp->opaque; } - if (opaque[0]) { + if (opaque) { opaque_param = apr_pstrcat(r->pool, ", opaque=\"", opaque, "\"", NULL); } - else { - opaque_param = NULL; - } /* Setup nonce */ @@ -1252,10 +1248,11 @@ static void note_digest_auth_failure(request_rec *r, apr_psprintf(r->pool, "Digest realm=\"%s\", " "nonce=\"%s\", algorithm=%s%s%s%s%s", ap_auth_name(r), nonce, conf->algorithm, - opaque_param ? opaque_param : "", + opaque_param, domain ? domain : "", stale ? ", stale=true" : "", qop)); + return HTTP_UNAUTHORIZED; } static int hook_note_digest_auth_failure(request_rec *r, const char *auth_type) @@ -1382,8 +1379,7 @@ static int check_and_record_nonce(request_rec *r, digest_header_rec *resp, 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); - note_digest_auth_failure(r, conf, resp, 0); - return HTTP_UNAUTHORIZED; + return note_digest_auth_failure(r, conf, resp, 0); } switch (client_update_nonce(r, resp->opaque_num, conf, resp->nonce_time, @@ -1394,12 +1390,10 @@ static int check_and_record_nonce(request_rec *r, digest_header_rec *resp, case NONCE_STALE: /* the credentials were good, so the client can silently retry with * the nonce from this challenge */ - note_digest_auth_failure(r, conf, resp, 1); - return HTTP_UNAUTHORIZED; + return note_digest_auth_failure(r, conf, resp, 1); default: - note_digest_auth_failure(r, conf, resp, 0); - return HTTP_UNAUTHORIZED; + return note_digest_auth_failure(r, conf, resp, 0); } } @@ -1421,8 +1415,7 @@ static int check_nonce(request_rec *r, digest_header_rec *resp, ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01776) "invalid nonce %s received - hash is not %s", resp->nonce, hash); - note_digest_auth_failure(r, conf, resp, 1); - return HTTP_UNAUTHORIZED; + return note_digest_auth_failure(r, conf, resp, 1); } dt = r->request_time - nonce_time.time; @@ -1430,8 +1423,7 @@ static int check_nonce(request_rec *r, digest_header_rec *resp, ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01777) "invalid nonce %s received - user attempted " "time travel", resp->nonce); - note_digest_auth_failure(r, conf, resp, 1); - return HTTP_UNAUTHORIZED; + return note_digest_auth_failure(r, conf, resp, 1); } if (conf->nonce_lifetime > 0) { @@ -1441,8 +1433,7 @@ static int check_nonce(request_rec *r, digest_header_rec *resp, "- max lifetime %.2f) - sending new nonce", r->user, (double)apr_time_sec(dt), (double)apr_time_sec(conf->nonce_lifetime)); - note_digest_auth_failure(r, conf, resp, 1); - return HTTP_UNAUTHORIZED; + return note_digest_auth_failure(r, conf, resp, 1); } } /* else (lifetime <= 0) => never expires by time; a one-time nonce is @@ -1588,8 +1579,7 @@ static int authenticate_digest_user(request_rec *r) r->uri); } /* else (resp->auth_hdr_sts == NO_HEADER) */ - note_digest_auth_failure(r, conf, resp, 0); - return HTTP_UNAUTHORIZED; + return note_digest_auth_failure(r, conf, resp, 0); } r->user = (char *) resp->username; @@ -1689,8 +1679,7 @@ static int authenticate_digest_user(request_rec *r) ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01787) "received invalid opaque - got `%s'", resp->opaque); - note_digest_auth_failure(r, conf, resp, 0); - return HTTP_UNAUTHORIZED; + return note_digest_auth_failure(r, conf, resp, 0); } @@ -1699,16 +1688,14 @@ static int authenticate_digest_user(request_rec *r) ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02533) "realm mismatch - got `%s' but no realm specified", resp->realm); - note_digest_auth_failure(r, conf, resp, 0); - return HTTP_UNAUTHORIZED; + return note_digest_auth_failure(r, conf, resp, 0); } if (!resp->realm || strcmp(resp->realm, realm)) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01788) "realm mismatch - got `%s' but expected `%s'", resp->realm, realm); - note_digest_auth_failure(r, conf, resp, 0); - return HTTP_UNAUTHORIZED; + return note_digest_auth_failure(r, conf, resp, 0); } if (resp->algorithm != NULL @@ -1716,8 +1703,7 @@ static int authenticate_digest_user(request_rec *r) ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01789) "unknown algorithm `%s' received: %s", resp->algorithm, r->uri); - note_digest_auth_failure(r, conf, resp, 0); - return HTTP_UNAUTHORIZED; + return note_digest_auth_failure(r, conf, resp, 0); } return_code = get_hash(r, r->user, conf, &resp->ha1); @@ -1726,8 +1712,7 @@ static int authenticate_digest_user(request_rec *r) ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01790) "user `%s' in realm `%s' not found: %s", r->user, realm, r->uri); - note_digest_auth_failure(r, conf, resp, 0); - return HTTP_UNAUTHORIZED; + return note_digest_auth_failure(r, conf, resp, 0); } else if (return_code == AUTH_USER_FOUND) { /* we have a password, so continue */ @@ -1737,8 +1722,7 @@ static int authenticate_digest_user(request_rec *r) ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01791) "user `%s' in realm `%s' denied by provider: %s", r->user, realm, r->uri); - note_digest_auth_failure(r, conf, resp, 0); - return HTTP_UNAUTHORIZED; + return note_digest_auth_failure(r, conf, resp, 0); } else { /* AUTH_GENERAL_ERROR (or worse) @@ -1754,8 +1738,7 @@ static int authenticate_digest_user(request_rec *r) ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10560) "invalid or missing qop value '%s', RFC 2069 is " "no longer supported: %s", resp->message_qop, r->uri); - note_digest_auth_failure(r, conf, resp, 0); - return HTTP_UNAUTHORIZED; + return note_digest_auth_failure(r, conf, resp, 0); } else { /* RFC 2617 (or 7616)-style Digest hash calculation. */ @@ -1768,8 +1751,7 @@ static int authenticate_digest_user(request_rec *r) ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01794) "user %s: password mismatch: %s", r->user, r->uri); - note_digest_auth_failure(r, conf, resp, 0); - return HTTP_UNAUTHORIZED; + return note_digest_auth_failure(r, conf, resp, 0); } } From 19ba61ebd349a32f7713c6e45f56544850d33293 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Thu, 13 Aug 2026 15:16:01 +0000 Subject: [PATCH 19/22] * modules/aaa/mod_auth_digest.c (client_generate): Skip a client id of zero, which the counter hands out once it wraps: zero means "no client", so add_client() refuses it. Assisted-by: Claude Opus 5 (1M context) GitHub: PR #705 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1937109 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit 5216183dd3cdf8a9f1324b8e7e51788fa29849fa) --- modules/aaa/mod_auth_digest.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index d27ab625d7c..e2a34b6ec10 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -1149,6 +1149,12 @@ 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 " From 396a3b81e8e57789d791008278e8a300c5f6e3c9 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Thu, 13 Aug 2026 15:16:06 +0000 Subject: [PATCH 20/22] * modules/aaa/mod_auth_digest.c (gc): Return unsigned long, the type the count is accumulated in and the type of the client_list counters. (add_client): Log those counters with %lu, all being unsigned. Assisted-by: Claude Opus 5 (1M context) GitHub: PR #705 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1937110 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit 5e7b2c88786ca345ff1e7be5d190adc06129a01f) --- modules/aaa/mod_auth_digest.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index e2a34b6ec10..183e24b4382 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -817,7 +817,7 @@ static enum nonce_state client_update_nonce(const request_rec *r, * last entry in each bucket and updates the counters. Returns the * number of removed entries. */ -static long gc(server_rec *s) +static unsigned long gc(server_rec *s) { client_entry *entry, *prev; unsigned long num_removed = 0, idx; @@ -888,11 +888,11 @@ static int add_client(client_id_t key, client_entry *info, server_rec *s) entry = rmm_malloc(client_rmm, sizeof(client_entry)); if (!entry) { - long num_removed = gc(s); + unsigned long num_removed = gc(s); ap_log_error(APLOG_MARK, APLOG_INFO, 0, s, APLOGNO(01766) - "gc'd %ld client entries. Total new clients: " - "%ld; Total removed clients: %ld; Total renewed clients: " - "%ld", num_removed, + "gc'd %lu client entries. Total new clients: " + "%lu; Total removed clients: %lu; Total renewed clients: " + "%lu", num_removed, client_list->num_created - client_list->num_renewed, client_list->num_removed, client_list->num_renewed); entry = rmm_malloc(client_rmm, sizeof(client_entry)); From 895dd8b70467938ed796a7642faeba563f292bf9 Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Thu, 13 Aug 2026 15:16:10 +0000 Subject: [PATCH 21/22] * modules/aaa/mod_auth_digest.c: Fail the build without APR_HAS_RANDOM or APR_HAS_SHARED_MEMORY, rather than testing at runtime. configure already declines to build the module without them. Assisted-by: Claude Opus 5 (1M context) GitHub: PR #705 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1937111 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit 37d73119bc308117fe2bc3e965832b36ebc39362) --- modules/aaa/mod_auth_digest.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index 183e24b4382..0267a83be0d 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -81,6 +81,12 @@ #include #endif +/* configure declines to build this module without both of these, so this + * only catches builds which don't use it. */ +#if !APR_HAS_RANDOM || !APR_HAS_SHARED_MEMORY +#error mod_auth_digest requires APR with random and shared memory support +#endif + /* struct to hold the configuration info */ typedef struct digest_config_struct { @@ -364,12 +370,6 @@ static int pre_init(apr_pool_t *pconf, apr_pool_t *plog, apr_pool_t *ptemp) apr_status_t rv; void *retained; - if (!APR_HAS_SHARED_MEMORY) { - ap_log_error(APLOG_MARK, APLOG_CRIT, rv, NULL, APLOGNO(10590) - "mod_auth_digest cannot be used on platforms without shared memory support"); - return !OK; - } - rv = ap_mutex_register(pconf, client_mutex_type, NULL, APR_LOCK_DEFAULT, 0); if (rv != APR_SUCCESS) return !OK; @@ -379,11 +379,7 @@ static int pre_init(apr_pool_t *pconf, apr_pool_t *plog, apr_pool_t *ptemp) retained = ap_retained_data_create(RETAINED_DATA_ID, SECRET_LEN); ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, NULL, APLOGNO(01757) "generating secret for digest authentication"); -#if APR_HAS_RANDOM rv = apr_generate_random_bytes(retained, SECRET_LEN); -#else -#error APR random number support is missing -#endif if (rv != APR_SUCCESS) { ap_log_error(APLOG_MARK, APLOG_CRIT, rv, NULL, APLOGNO(01758) "error generating secret"); From f72f17c8701cbed5acf0dec0c7508c4c09b28d4b Mon Sep 17 00:00:00 2001 From: Joe Orton Date: Thu, 13 Aug 2026 17:55:05 +0100 Subject: [PATCH 22/22] Document changes. --- changes-entries/mod_auth_digest-rebase.txt | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 changes-entries/mod_auth_digest-rebase.txt diff --git a/changes-entries/mod_auth_digest-rebase.txt b/changes-entries/mod_auth_digest-rebase.txt new file mode 100644 index 00000000000..52f0230c4ce --- /dev/null +++ b/changes-entries/mod_auth_digest-rebase.txt @@ -0,0 +1,8 @@ + *) mod_auth_digest: Fix compatibility with expression-based AuthName. + PR 59039. [Eric Covener] + + *) mod_auth_digest.c: Drop RFC 2069 support; rewrite shared memory + handling and client nonce handling; "authdigest-opaque" mutex is + now longer needed. [Joe Orton] + +