From 95baad3342aa7ef7172ad4922eb9f0d605dc469b Mon Sep 17 00:00:00 2001 From: Hans Zandbelt Date: Thu, 6 Dec 2018 14:55:44 +0100 Subject: [PATCH 12/13] optionally delete the oldest state cookie(s); see #399 bump to 2.3.10rc3 Signed-off-by: Hans Zandbelt (cherry picked from commit 46758a75eef8e3c91f2917fc7c6302136eb18809) --- auth_openidc.conf | 8 +++-- src/config.c | 27 +++++++++++++---- src/mod_auth_openidc.c | 66 +++++++++++++++++++++++++++++++++++++++--- src/mod_auth_openidc.h | 2 ++ src/parse.c | 9 ++++-- src/parse.h | 2 +- 6 files changed, 100 insertions(+), 14 deletions(-) diff --git a/auth_openidc.conf b/auth_openidc.conf index 48dd027..33cea64 100644 --- a/auth_openidc.conf +++ b/auth_openidc.conf @@ -450,8 +450,12 @@ # authentication requests. See: https://github.com/zmartzone/mod_auth_openidc/issues/331 # Setting this to 0 means unlimited, until the browser or server gives up which is the # behavior of mod_auth_openidc < 2.3.8, which did not have this configuration option. -# When not defined, the default is 7. -#OIDCStateMaxNumberOfCookies +# +# The optional second boolean parameter if the oldest state cookie(s) will be deleted, +# even if still valid; see #399. +# +# When not defined, the default is 7 and "false", thus the oldest cookie(s) will not be deleted. +#OIDCStateMaxNumberOfCookies [false|true] ######################################################################################## # diff --git a/src/config.c b/src/config.c index 6fa6227..8e56716 100644 --- a/src/config.c +++ b/src/config.c @@ -106,6 +106,8 @@ #define OIDC_DEFAULT_STATE_TIMEOUT 300 /* maximum number of parallel state cookies; 0 means unlimited, until the browser or server gives up */ #define OIDC_DEFAULT_MAX_NUMBER_OF_STATE_COOKIES 7 +/* default setting for deleting the oldest state cookies */ +#define OIDC_DEFAULT_DELETE_OLDEST_STATE_COOKIES 0 /* default session inactivity timeout */ #define OIDC_DEFAULT_SESSION_INACTIVITY_TIMEOUT 300 /* default session max duration */ @@ -1001,11 +1003,12 @@ static const char *oidc_set_client_auth_bearer_token(cmd_parms *cmd, * set the maximun number of parallel state cookies */ static const char *oidc_set_max_number_of_state_cookies(cmd_parms *cmd, - void *struct_ptr, const char *arg) { + void *struct_ptr, const char *arg1, const char *arg2) { oidc_cfg *cfg = (oidc_cfg *) ap_get_module_config( cmd->server->module_config, &auth_openidc_module); - const char *rv = oidc_parse_max_number_of_state_cookies(cmd->pool, arg, - &cfg->max_number_of_state_cookies); + const char *rv = oidc_parse_max_number_of_state_cookies(cmd->pool, arg1, + arg2, &cfg->max_number_of_state_cookies, + &cfg->delete_oldest_state_cookies); return OIDC_CONFIG_DIR_RV(cmd, rv); } @@ -1018,6 +1021,15 @@ int oidc_cfg_max_number_of_state_cookies(oidc_cfg *cfg) { return cfg->max_number_of_state_cookies; } +/* + * return the number of oldest state cookies that need to be deleted + */ +int oidc_cfg_delete_oldest_state_cookies(oidc_cfg *cfg) { + if (cfg->delete_oldest_state_cookies == OIDC_CONFIG_POS_INT_UNSET) + return OIDC_DEFAULT_DELETE_OLDEST_STATE_COOKIES; + return cfg->delete_oldest_state_cookies; +} + /* * create a new server config record with defaults */ @@ -1127,6 +1139,7 @@ void *oidc_create_server_config(apr_pool_t *pool, server_rec *svr) { c->http_timeout_short = OIDC_DEFAULT_HTTP_TIMEOUT_SHORT; c->state_timeout = OIDC_DEFAULT_STATE_TIMEOUT; c->max_number_of_state_cookies = OIDC_CONFIG_POS_INT_UNSET; + c->delete_oldest_state_cookies = OIDC_CONFIG_POS_INT_UNSET; c->session_inactivity_timeout = OIDC_DEFAULT_SESSION_INACTIVITY_TIMEOUT; c->cookie_domain = NULL; @@ -1445,6 +1458,10 @@ void *oidc_merge_server_config(apr_pool_t *pool, void *BASE, void *ADD) { add->max_number_of_state_cookies != OIDC_CONFIG_POS_INT_UNSET ? add->max_number_of_state_cookies : base->max_number_of_state_cookies; + c->delete_oldest_state_cookies = + add->delete_oldest_state_cookies != OIDC_CONFIG_POS_INT_UNSET ? + add->delete_oldest_state_cookies : + base->delete_oldest_state_cookies; c->session_inactivity_timeout = add->session_inactivity_timeout != OIDC_DEFAULT_SESSION_INACTIVITY_TIMEOUT ? @@ -2656,11 +2673,11 @@ const command_rec oidc_config_cmds[] = { (void*)APR_OFFSETOF(oidc_cfg, state_timeout), RSRC_CONF, "Time to live in seconds for state parameter (cq. interval in which the authorization request and the corresponding response need to be completed)."), - AP_INIT_TAKE1(OIDCStateMaxNumberOfCookies, + AP_INIT_TAKE12(OIDCStateMaxNumberOfCookies, oidc_set_max_number_of_state_cookies, (void*)APR_OFFSETOF(oidc_cfg, max_number_of_state_cookies), RSRC_CONF, - "Maximun number of parallel state cookies i.e. outstanding authorization requests."), + "Maximun number of parallel state cookies i.e. outstanding authorization requests and whether to delete the oldest cookie(s)."), AP_INIT_TAKE1(OIDCSessionInactivityTimeout, oidc_set_session_inactivity_timeout, (void*)APR_OFFSETOF(oidc_cfg, session_inactivity_timeout), diff --git a/src/mod_auth_openidc.c b/src/mod_auth_openidc.c index 897a449..8740e02 100644 --- a/src/mod_auth_openidc.c +++ b/src/mod_auth_openidc.c @@ -686,15 +686,53 @@ static apr_byte_t oidc_unsolicited_proto_state(request_rec *r, oidc_cfg *c, return TRUE; } +typedef struct oidc_state_cookies_t { + char *name; + apr_time_t timestamp; + struct oidc_state_cookies_t *next; +} oidc_state_cookies_t; + +static int oidc_delete_oldest_state_cookies(request_rec *r, + int number_of_valid_state_cookies, int max_number_of_state_cookies, + oidc_state_cookies_t *first) { + oidc_state_cookies_t *cur = NULL, *prev = NULL, *prev_oldest = NULL, + *oldest = NULL; + while (number_of_valid_state_cookies >= max_number_of_state_cookies) { + oldest = first; + prev_oldest = NULL; + prev = first; + cur = first->next; + while (cur) { + if ((cur->timestamp < oldest->timestamp)) { + oldest = cur; + prev_oldest = prev; + } + prev = cur; + cur = cur->next; + } + oidc_warn(r, + "deleting oldest state cookie: %s (time until expiry " APR_TIME_T_FMT " seconds)", + oldest->name, apr_time_sec(oldest->timestamp - apr_time_now())); + oidc_util_set_cookie(r, oldest->name, "", 0, NULL); + if (prev_oldest) + prev_oldest->next = oldest->next; + else + first = first->next; + number_of_valid_state_cookies--; + } + return number_of_valid_state_cookies; +} + /* * clean state cookies that have expired i.e. for outstanding requests that will never return * successfully and return the number of remaining valid cookies/outstanding-requests while * doing so */ static int oidc_clean_expired_state_cookies(request_rec *r, oidc_cfg *c, - const char *currentCookieName) { + const char *currentCookieName, int delete_oldest) { int number_of_valid_state_cookies = 0; - char *cookie, *tokenizerCtx; + oidc_state_cookies_t *first = NULL, *last = NULL; + char *cookie, *tokenizerCtx = NULL; char *cookies = apr_pstrdup(r->pool, oidc_util_hdr_in_cookie_get(r)); if (cookies != NULL) { cookie = apr_strtok(cookies, OIDC_STR_SEMI_COLON, &tokenizerCtx); @@ -722,6 +760,18 @@ static int oidc_clean_expired_state_cookies(request_rec *r, oidc_cfg *c, oidc_util_set_cookie(r, cookieName, "", 0, NULL); } else { + if (first == NULL) { + first = apr_pcalloc(r->pool, + sizeof(oidc_state_cookies_t)); + last = first; + } else { + last->next = apr_pcalloc(r->pool, + sizeof(oidc_state_cookies_t)); + last = last->next; + } + last->name = cookieName; + last->timestamp = ts; + last->next = NULL; number_of_valid_state_cookies++; } oidc_proto_state_destroy(proto_state); @@ -732,6 +782,12 @@ static int oidc_clean_expired_state_cookies(request_rec *r, oidc_cfg *c, cookie = apr_strtok(NULL, OIDC_STR_SEMI_COLON, &tokenizerCtx); } } + + if (delete_oldest > 0) + number_of_valid_state_cookies = oidc_delete_oldest_state_cookies(r, + number_of_valid_state_cookies, c->max_number_of_state_cookies, + first); + return number_of_valid_state_cookies; } @@ -746,7 +802,7 @@ static apr_byte_t oidc_restore_proto_state(request_rec *r, oidc_cfg *c, const char *cookieName = oidc_get_state_cookie_name(r, state); /* clean expired state cookies to avoid pollution */ - oidc_clean_expired_state_cookies(r, c, cookieName); + oidc_clean_expired_state_cookies(r, c, cookieName, FALSE); /* get the state cookie value first */ char *cookieValue = oidc_util_get_cookie(r, cookieName); @@ -820,10 +876,12 @@ static int oidc_authorization_request_set_cookie(request_rec *r, oidc_cfg *c, * clean expired state cookies to avoid pollution and optionally * try to avoid the number of state cookies exceeding a max */ - int number_of_cookies = oidc_clean_expired_state_cookies(r, c, NULL); + int number_of_cookies = oidc_clean_expired_state_cookies(r, c, NULL, + oidc_cfg_delete_oldest_state_cookies(c)); int max_number_of_cookies = oidc_cfg_max_number_of_state_cookies(c); if ((max_number_of_cookies > 0) && (number_of_cookies >= max_number_of_cookies)) { + oidc_warn(r, "the number of existing, valid state cookies (%d) has exceeded the limit (%d), no additional authorization request + state cookie can be generated, aborting the request", number_of_cookies, max_number_of_cookies); diff --git a/src/mod_auth_openidc.h b/src/mod_auth_openidc.h index f89f392..c3a0a23 100644 --- a/src/mod_auth_openidc.h +++ b/src/mod_auth_openidc.h @@ -380,6 +380,7 @@ typedef struct oidc_cfg { int http_timeout_short; int state_timeout; int max_number_of_state_cookies; + int delete_oldest_state_cookies; int session_inactivity_timeout; int session_cache_fallback_to_cookie; @@ -691,6 +692,7 @@ int oidc_cfg_session_cache_fallback_to_cookie(request_rec *r); const char *oidc_parse_pkce_type(apr_pool_t *pool, const char *arg, oidc_proto_pkce_t **type); const char *oidc_cfg_claim_prefix(request_rec *r); int oidc_cfg_max_number_of_state_cookies(oidc_cfg *cfg); +int oidc_cfg_delete_oldest_state_cookies(oidc_cfg *cfg); // oidc_util.c int oidc_strnenvcmp(const char *a, const char *b, int len); diff --git a/src/parse.c b/src/parse.c index 0f986fd..2d09584 100644 --- a/src/parse.c +++ b/src/parse.c @@ -1245,7 +1245,12 @@ const char *oidc_parse_auth_request_method(apr_pool_t *pool, const char *arg, * parse the maximum number of parallel state cookies */ const char *oidc_parse_max_number_of_state_cookies(apr_pool_t *pool, - const char *arg, int *int_value) { - return oidc_parse_int_valid(pool, arg, int_value, + const char *arg1, const char *arg2, int *int_value, int *bool_value) { + const char *rv = NULL; + + rv = oidc_parse_int_valid(pool, arg1, int_value, oidc_valid_max_number_of_state_cookies); + if ((rv == NULL) && (arg2 != NULL)) + rv = oidc_parse_boolean(pool, arg2, bool_value); + return rv; } diff --git a/src/parse.h b/src/parse.h index 6355db4..bdf5651 100644 --- a/src/parse.h +++ b/src/parse.h @@ -117,7 +117,7 @@ const char *oidc_parse_info_hook_data(apr_pool_t *pool, const char *arg, apr_has const char *oidc_parse_token_binding_policy(apr_pool_t *pool, const char *arg, int *int_value); const char *oidc_token_binding_policy2str(apr_pool_t *pool, int v); const char *oidc_parse_auth_request_method(apr_pool_t *pool, const char *arg, int *method); -const char *oidc_parse_max_number_of_state_cookies(apr_pool_t *pool, const char *arg, int *int_value); +const char *oidc_parse_max_number_of_state_cookies(apr_pool_t *pool, const char *arg1, const char *arg2, int *int_value, int *bool_value); typedef const char *(*oidc_valid_int_function_t)(apr_pool_t *, int); typedef const char *(*oidc_valid_function_t)(apr_pool_t *, const char *); -- 2.26.2