From 1bb55df94c0db8376f88a568c331802bd282f097 Mon Sep 17 00:00:00 2001 From: Hans Zandbelt 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 (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