From 1bb55df94c0db8376f88a568c331802bd282f097 Mon Sep 17 00:00:00 2001

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

Date: Wed, 1 Aug 2018 11:41:57 +0200

Subject: [PATCH 06/11] add OIDCStateMaxNumberOfCookies to limit nr of state

 cookies; see #331

Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>

(cherry picked from commit 5041e0f679f03525f106eaf0edc7c7d245b1d89c)

---

 ChangeLog              |  3 +++

 src/config.c           | 19 +++++++++++++

 src/mod_auth_openidc.c | 61 ++++++++++++++++++++++++++++++++++--------

 src/mod_auth_openidc.h |  2 ++

 4 files changed, 74 insertions(+), 11 deletions(-)

diff --git a/ChangeLog b/ChangeLog

index af9380e..b6ac513 100644

--- a/ChangeLog

+++ b/ChangeLog

@@ -1,3 +1,6 @@

+08/01/2018

+- add option to set an upper limit to the number of concurrent state cookies via OIDCStateMaxNumberOfCookies; see #331

+

 07/06/2018

 - abort when string length for remote user name substitution is larger than 255 characters

 - release 2.3.7

diff --git a/src/config.c b/src/config.c

index 0185cff..2fd63ea 100644

--- a/src/config.c

+++ b/src/config.c

@@ -104,6 +104,8 @@

 #define OIDC_DEFAULT_SESSION_CLIENT_COOKIE_CHUNK_SIZE 4000

 /* timeout in seconds after which state expires */

 #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 0

 /* default session inactivity timeout */

 #define OIDC_DEFAULT_SESSION_INACTIVITY_TIMEOUT 300

 /* default session max duration */

@@ -227,6 +229,7 @@

 #define OIDCHTTPTimeoutLong                  "OIDCHTTPTimeoutLong"

 #define OIDCHTTPTimeoutShort                 "OIDCHTTPTimeoutShort"

 #define OIDCStateTimeout                     "OIDCStateTimeout"

+#define OIDCStateMaxNumberOfCookies          "OIDCStateMaxNumberOfCookies"

 #define OIDCSessionInactivityTimeout         "OIDCSessionInactivityTimeout"

 #define OIDCMetadataDir                      "OIDCMetadataDir"

 #define OIDCSessionCacheFallbackToCookie     "OIDCSessionCacheFallbackToCookie"

@@ -994,6 +997,12 @@ static const char *oidc_set_client_auth_bearer_token(cmd_parms *cmd,

 	return NULL;

 }

+int oidc_cfg_max_number_of_state_cookies(oidc_cfg *cfg) {

+	if (cfg->max_number_of_state_cookies == OIDC_CONFIG_POS_INT_UNSET)

+		return OIDC_DEFAULT_MAX_NUMBER_OF_STATE_COOKIES;

+	return cfg->max_number_of_state_cookies;

+}

+

 /*

  * create a new server config record with defaults

  */

@@ -1102,6 +1111,7 @@ void *oidc_create_server_config(apr_pool_t *pool, server_rec *svr) {

 	c->http_timeout_long = OIDC_DEFAULT_HTTP_TIMEOUT_LONG;

 	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->session_inactivity_timeout = OIDC_DEFAULT_SESSION_INACTIVITY_TIMEOUT;

 	c->cookie_domain = NULL;

@@ -1416,6 +1426,10 @@ void *oidc_merge_server_config(apr_pool_t *pool, void *BASE, void *ADD) {

 	c->state_timeout =

 			add->state_timeout != OIDC_DEFAULT_STATE_TIMEOUT ?

 					add->state_timeout : base->state_timeout;

+	c->max_number_of_state_cookies =

+			add->max_number_of_state_cookies != OIDC_CONFIG_POS_INT_UNSET ?

+					add->max_number_of_state_cookies :

+					base->max_number_of_state_cookies;

 	c->session_inactivity_timeout =

 			add->session_inactivity_timeout

 			!= OIDC_DEFAULT_SESSION_INACTIVITY_TIMEOUT ?

@@ -2627,6 +2641,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,

+				oidc_set_int_slot,

+				(void*)APR_OFFSETOF(oidc_cfg, max_number_of_state_cookies),

+				RSRC_CONF,

+				"Maximun number of parallel state cookies i.e. outstanding authorization requests."),

 		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 74f206b..c0f65c6 100644

--- a/src/mod_auth_openidc.c

+++ b/src/mod_auth_openidc.c

@@ -686,8 +686,14 @@ static apr_byte_t oidc_unsolicited_proto_state(request_rec *r, oidc_cfg *c,

 	return TRUE;

 }

-static void oidc_clean_expired_state_cookies(request_rec *r, oidc_cfg *c,

+/*

+ * 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) {

+	int number_of_valid_state_cookies = 0;

 	char *cookie, *tokenizerCtx;

 	char *cookies = apr_pstrdup(r->pool, oidc_util_hdr_in_cookie_get(r));

 	if (cookies != NULL) {

@@ -715,6 +721,8 @@ static void oidc_clean_expired_state_cookies(request_rec *r, oidc_cfg *c,

 										cookieName);

 								oidc_util_set_cookie(r, cookieName, "", 0,

 										NULL);

+							} else {

+								number_of_valid_state_cookies++;

 							}

 							oidc_proto_state_destroy(proto_state);

 						}

@@ -724,6 +732,7 @@ static void oidc_clean_expired_state_cookies(request_rec *r, oidc_cfg *c,

 			cookie = apr_strtok(NULL, OIDC_STR_SEMI_COLON, &tokenizerCtx);

 		}

 	}

+	return number_of_valid_state_cookies;

 }

 /*

@@ -796,7 +805,7 @@ static apr_byte_t oidc_restore_proto_state(request_rec *r, oidc_cfg *c,

  * set the state that is maintained between an authorization request and an authorization response

  * in a cookie in the browser that is cryptographically bound to that state

  */

-static apr_byte_t oidc_authorization_request_set_cookie(request_rec *r,

+static int oidc_authorization_request_set_cookie(request_rec *r,

 		oidc_cfg *c, const char *state, oidc_proto_state_t *proto_state) {

 	/*

 	 * create a cookie consisting of 8 elements:

@@ -805,10 +814,32 @@ static apr_byte_t oidc_authorization_request_set_cookie(request_rec *r,

 	 */

 	char *cookieValue = oidc_proto_state_to_cookie(r, c, proto_state);

 	if (cookieValue == NULL)

-		return FALSE;

+		return HTTP_INTERNAL_SERVER_ERROR;

-	/* clean expired state cookies to avoid pollution */

-	oidc_clean_expired_state_cookies(r, c, NULL);

+	/*

+	 * 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 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);

+		/*

+		 * TODO: the html_send code below caters for the case that there's a user behind a

+		 * browser generating this request, rather than a piece of XHR code; how would an

+		 * XHR client handle this?

+		 */

+

+		return oidc_util_html_send_error(r, c->error_template,

+				"Too Many Outstanding Requests",

+				apr_psprintf(r->pool,

+						"No authentication request could be generated since there are too many outstanding authentication requests already; you may have to wait up to %d seconds to be able to create a new request",

+						c->state_timeout),

+						HTTP_SERVICE_UNAVAILABLE);

+	}

 	/* assemble the cookie name for the state cookie */

 	const char *cookieName = oidc_get_state_cookie_name(r, state);

@@ -817,9 +848,7 @@ static apr_byte_t oidc_authorization_request_set_cookie(request_rec *r,

 	oidc_util_set_cookie(r, cookieName, cookieValue, -1,

 			c->cookie_same_site ? OIDC_COOKIE_EXT_SAME_SITE_LAX : NULL);

-	//free(s_value);

-

-	return TRUE;

+	return HTTP_OK;

 }

 /*

@@ -2245,12 +2274,19 @@ static int oidc_authenticate_user(request_rec *r, oidc_cfg *c,

 	/* get a hash value that fingerprints the browser concatenated with the random input */

 	char *state = oidc_get_browser_state_hash(r, nonce);

-	/* create state that restores the context when the authorization response comes in; cryptographically bind it to the browser */

-	if (oidc_authorization_request_set_cookie(r, c, state, proto_state) == FALSE)

-		return HTTP_INTERNAL_SERVER_ERROR;

+	/*

+	 * create state that restores the context when the authorization response comes in

+	 * and cryptographically bind it to the browser

+	 */

+	int rc = oidc_authorization_request_set_cookie(r, c, state, proto_state);

+	if (rc != HTTP_OK) {

+		oidc_proto_state_destroy(proto_state);

+		return rc;

+	}

 	/*

 	 * printout errors if Cookie settings are not going to work

+	 * TODO: separate this code out into its own function

 	 */

 	apr_uri_t o_uri;

 	memset(&o_uri, 0, sizeof(apr_uri_t));

@@ -2263,6 +2299,7 @@ static int oidc_authenticate_user(request_rec *r, oidc_cfg *c,

 		oidc_error(r,

 				"the URL scheme (%s) of the configured " OIDCRedirectURI " does not match the URL scheme of the URL being accessed (%s): the \"state\" and \"session\" cookies will not be shared between the two!",

 				r_uri.scheme, o_uri.scheme);

+		oidc_proto_state_destroy(proto_state);

 		return HTTP_INTERNAL_SERVER_ERROR;

 	}

@@ -2273,6 +2310,7 @@ static int oidc_authenticate_user(request_rec *r, oidc_cfg *c,

 				oidc_error(r,

 						"the URL hostname (%s) of the configured " OIDCRedirectURI " does not match the URL hostname of the URL being accessed (%s): the \"state\" and \"session\" cookies will not be shared between the two!",

 						r_uri.hostname, o_uri.hostname);

+				oidc_proto_state_destroy(proto_state);

 				return HTTP_INTERNAL_SERVER_ERROR;

 			}

 		}

@@ -2281,6 +2319,7 @@ static int oidc_authenticate_user(request_rec *r, oidc_cfg *c,

 			oidc_error(r,

 					"the domain (%s) configured in " OIDCCookieDomain " does not match the URL hostname (%s) of the URL being accessed (%s): setting \"state\" and \"session\" cookies will not work!!",

 					c->cookie_domain, o_uri.hostname, original_url);

+			oidc_proto_state_destroy(proto_state);

 			return HTTP_INTERNAL_SERVER_ERROR;

 		}

 	}

diff --git a/src/mod_auth_openidc.h b/src/mod_auth_openidc.h

index 48bb8fe..5162ed4 100644

--- a/src/mod_auth_openidc.h

+++ b/src/mod_auth_openidc.h

@@ -379,6 +379,7 @@ typedef struct oidc_cfg {

 	int http_timeout_long;

 	int http_timeout_short;

 	int state_timeout;

+	int max_number_of_state_cookies;

 	int session_inactivity_timeout;

 	int session_cache_fallback_to_cookie;

@@ -686,6 +687,7 @@ int oidc_cfg_cache_encrypt(request_rec *r);

 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);

 // oidc_util.c

 int oidc_strnenvcmp(const char *a, const char *b, int len);

-- 

2.26.2