From 95baad3342aa7ef7172ad4922eb9f0d605dc469b Mon Sep 17 00:00:00 2001

From: Hans Zandbelt <hans.zandbelt@zmartzone.eu>

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 <hans.zandbelt@zmartzone.eu>

(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 <number>

+#

+# 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 <number> [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