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