From e4d625b0f1116e50df2737e2738b4c77b35328bb Mon Sep 17 00:00:00 2001 From: Hans Zandbelt Date: Wed, 10 Jun 2020 18:14:24 +0200 Subject: [PATCH 1/4] prevent open redirect on refresh token requests; release 2.4.3 add new OIDCRedirectURLsAllowed primitive to handle post logout and refresh-return-to validation; addresses #453; closes #466 Signed-off-by: Hans Zandbelt (cherry picked from commit 8ea550f34ce51d8d41ba47843739c964407fa0ad) --- auth_openidc.conf | 9 ++ src/config.c | 24 ++++- src/mod_auth_openidc.c | 205 ++++++++++++++++++++++++----------------- src/mod_auth_openidc.h | 1 + src/util.c | 16 ++-- 5 files changed, 166 insertions(+), 89 deletions(-) diff --git a/auth_openidc.conf b/auth_openidc.conf index ce2fba7..87685f6 100644 --- a/auth_openidc.conf +++ b/auth_openidc.conf @@ -784,3 +784,12 @@ # for calculating the fingerprint of the state during authentication. # When not defined the default "both" is used. #OIDCStateInputHeaders [none|user-agent|x-forwarded-for|both] + +# Define one or more regular expressions that specify URLs (or domains) allowed for post logout and +# other redirects such as the "return_to" value on refresh token requests, e.g.: +# OIDCRedirectURLsAllowed ^https://www.example.com ^https://(\w+).example.org ^https://example.net/app +# or: +# OIDCRedirectURLsAllowed ^https://www.example.com/logout$ ^https://www.example.com/app/return_to$ +# When not defined, the default is to match the hostname in the URL redirected to against +# the hostname in the current request. +#OIDCRedirectURLsAllowed []+ diff --git a/src/config.c b/src/config.c index 588e1a3..b2c0da7 100644 --- a/src/config.c +++ b/src/config.c @@ -264,6 +264,7 @@ #define OIDCBlackListedClaims "OIDCBlackListedClaims" #define OIDCOAuthServerMetadataURL "OIDCOAuthServerMetadataURL" #define OIDCStateInputHeaders "OIDCStateInputHeaders" +#define OIDCRedirectURLsAllowed "OIDCRedirectURLsAllowed" extern module AP_MODULE_DECLARE_DATA auth_openidc_module; @@ -1044,6 +1045,16 @@ static const char * oidc_set_state_input_headers_as(cmd_parms *cmd, void *m, return OIDC_CONFIG_DIR_RV(cmd, rv); } +static const char * oidc_set_redirect_urls_allowed(cmd_parms *cmd, void *m, + const char *arg) { + oidc_cfg *cfg = (oidc_cfg *) ap_get_module_config( + cmd->server->module_config, &auth_openidc_module); + if (cfg->redirect_urls_allowed == NULL) + cfg->redirect_urls_allowed = apr_hash_make(cmd->pool); + apr_hash_set(cfg->redirect_urls_allowed, arg, APR_HASH_KEY_STRING, arg); + return NULL; +} + /* * create a new server config record with defaults */ @@ -1192,6 +1203,8 @@ void *oidc_create_server_config(apr_pool_t *pool, server_rec *svr) { c->state_input_headers = OIDC_DEFAULT_STATE_INPUT_HEADERS; + c->redirect_urls_allowed = NULL; + return c; } @@ -1637,6 +1650,10 @@ void *oidc_merge_server_config(apr_pool_t *pool, void *BASE, void *ADD) { add->state_input_headers != OIDC_DEFAULT_STATE_INPUT_HEADERS ? add->state_input_headers : base->state_input_headers; + c->redirect_urls_allowed = + add->redirect_urls_allowed != NULL ? + add->redirect_urls_allowed : base->redirect_urls_allowed; + return c; } @@ -2310,7 +2327,7 @@ void oidc_register_hooks(apr_pool_t *pool) { ap_register_auth_provider(pool, AUTHZ_PROVIDER_GROUP, OIDC_REQUIRE_CLAIM_NAME, "0", &oidc_authz_claim_provider, AP_AUTH_INTERNAL_PER_CONF); -#ifdef USE_LIBJQ +#ifdef USE_LIBJQ ap_register_auth_provider(pool, AUTHZ_PROVIDER_GROUP, OIDC_REQUIRE_CLAIMS_EXPR_NAME, "0", &oidc_authz_claims_expr_provider, AP_AUTH_INTERNAL_PER_CONF); @@ -2891,5 +2908,10 @@ const command_rec oidc_config_cmds[] = { NULL, RSRC_CONF, "Specify header name which is used as the input for calculating the fingerprint of the state during authentication; must be one of \"none\", \"user-agent\", \"x-forwarded-for\" or \"both\" (default)."), + AP_INIT_ITERATE(OIDCRedirectURLsAllowed, + oidc_set_redirect_urls_allowed, + (void *) APR_OFFSETOF(oidc_cfg, redirect_urls_allowed), + RSRC_CONF|ACCESS_CONF|OR_AUTHCFG, + "Specify one or more regular expressions that define URLs allowed for post logout and other redirects."), { NULL } }; diff --git a/src/mod_auth_openidc.c b/src/mod_auth_openidc.c index 68fa2ce..215ed5e 100644 --- a/src/mod_auth_openidc.c +++ b/src/mod_auth_openidc.c @@ -226,7 +226,8 @@ void oidc_strip_cookies(request_rec *r) { /* * calculates a hash value based on request fingerprint plus a provided nonce string. */ -static char *oidc_get_browser_state_hash(request_rec *r, oidc_cfg *c, const char *nonce) { +static char *oidc_get_browser_state_hash(request_rec *r, oidc_cfg *c, + const char *nonce) { oidc_debug(r, "enter"); @@ -543,14 +544,13 @@ static apr_byte_t oidc_unsolicited_proto_state(request_rec *r, oidc_cfg *c, oidc_jose_error_t err; oidc_jwk_t *jwk = NULL; if (oidc_util_create_symmetric_key(r, c->provider.client_secret, - oidc_alg2keysize(alg), OIDC_JOSE_ALG_SHA256, - TRUE, &jwk) == FALSE) + oidc_alg2keysize(alg), OIDC_JOSE_ALG_SHA256, TRUE, &jwk) == FALSE) return FALSE; oidc_jwt_t *jwt = NULL; if (oidc_jwt_parse(r->pool, state, &jwt, - oidc_util_merge_symmetric_key(r->pool, c->private_keys, jwk), - &err) == FALSE) { + oidc_util_merge_symmetric_key(r->pool, c->private_keys, jwk), &err) + == FALSE) { oidc_error(r, "could not parse JWT from state: invalid unsolicited response: %s", oidc_jose_e2s(r->pool, err)); @@ -600,8 +600,7 @@ static apr_byte_t oidc_unsolicited_proto_state(request_rec *r, oidc_cfg *c, char *target_link_uri = NULL; oidc_jose_get_string(r->pool, jwt->payload.value.json, - OIDC_CLAIM_TARGET_LINK_URI, - FALSE, &target_link_uri, NULL); + OIDC_CLAIM_TARGET_LINK_URI, FALSE, &target_link_uri, NULL); if (target_link_uri == NULL) { if (c->default_sso_url == NULL) { oidc_error(r, @@ -1224,8 +1223,8 @@ static apr_byte_t oidc_refresh_access_token(request_rec *r, oidc_cfg *c, /* refresh the tokens by calling the token endpoint */ if (oidc_proto_refresh_request(r, c, provider, refresh_token, &s_id_token, - &s_access_token, &s_token_type, &expires_in, - &s_refresh_token) == FALSE) { + &s_access_token, &s_token_type, &expires_in, &s_refresh_token) + == FALSE) { oidc_error(r, "access_token could not be refreshed"); return FALSE; } @@ -2286,8 +2285,8 @@ static int oidc_discovery(request_rec *r, oidc_cfg *cfg) { char *javascript = NULL, *javascript_method = NULL; char *html_head = ""; - if (oidc_post_preserve_javascript(r, NULL, &javascript, - &javascript_method) == TRUE) + if (oidc_post_preserve_javascript(r, NULL, &javascript, &javascript_method) + == TRUE) html_head = apr_psprintf(r->pool, "%s%s", html_head, javascript); /* now send the HTML contents to the user agent */ @@ -2531,8 +2530,8 @@ static int oidc_handle_discovery_response(request_rec *r, oidc_cfg *c) { } /* do open redirect prevention */ - if (oidc_target_link_uri_matches_configuration(r, c, - target_link_uri) == FALSE) { + if (oidc_target_link_uri_matches_configuration(r, c, target_link_uri) + == FALSE) { return oidc_util_html_send_error(r, c->error_template, "Invalid Request", "\"target_link_uri\" parameter does not match configuration settings, aborting to prevent an open redirect.", @@ -2587,7 +2586,8 @@ static int oidc_handle_discovery_response(request_rec *r, oidc_cfg *c) { } /* got an account name as input, perform OP discovery with that */ - if (oidc_proto_account_based_discovery(r, c, issuer, &issuer) == FALSE) { + if (oidc_proto_account_based_discovery(r, c, issuer, &issuer) + == FALSE) { /* something did not work out, show a user facing error */ return oidc_util_html_send_error(r, c->error_template, @@ -2687,66 +2687,84 @@ static int oidc_handle_logout_request(request_rec *r, oidc_cfg *c, return HTTP_MOVED_TEMPORARILY; } -static apr_byte_t oidc_validate_post_logout_url(request_rec *r, const char *url, char **err_str, char **err_desc) { - apr_uri_t uri; - const char *c_host = NULL; - - if (apr_uri_parse(r->pool, url, &uri) != APR_SUCCESS) { - *err_str = apr_pstrdup(r->pool, "Malformed URL"); - *err_desc = apr_psprintf(r->pool, "Logout URL malformed: %s", url); - oidc_error(r, "%s: %s", *err_str, *err_desc); - return FALSE; - } - - c_host = oidc_get_current_url_host(r); - if ((uri.hostname != NULL) - && ((strstr(c_host, uri.hostname) == NULL) - || (strstr(uri.hostname, c_host) == NULL))) { - *err_str = apr_pstrdup(r->pool, "Invalid Request"); - *err_desc = - apr_psprintf(r->pool, - "logout value \"%s\" does not match the hostname of the current request \"%s\"", - apr_uri_unparse(r->pool, &uri, 0), c_host); - oidc_error(r, "%s: %s", *err_str, *err_desc); - return FALSE; - } else if ((uri.hostname == NULL) && (strstr(url, "/") != url)) { - *err_str = apr_pstrdup(r->pool, "Malformed URL"); - *err_desc = - apr_psprintf(r->pool, - "No hostname was parsed and it does not seem to be relative, i.e starting with '/': %s", - url); - oidc_error(r, "%s: %s", *err_str, *err_desc); - return FALSE; - } else if ((uri.hostname == NULL) && (strstr(url, "//") == url)) { - *err_str = apr_pstrdup(r->pool, "Malformed URL"); - *err_desc = - apr_psprintf(r->pool, - "No hostname was parsed and starting with '//': %s", - url); - oidc_error(r, "%s: %s", *err_str, *err_desc); - return FALSE; - } else if ((uri.hostname == NULL) && (strstr(url, "/\\") == url)) { - *err_str = apr_pstrdup(r->pool, "Malformed URL"); - *err_desc = - apr_psprintf(r->pool, - "No hostname was parsed and starting with '/\\': %s", - url); - oidc_error(r, "%s: %s", *err_str, *err_desc); - return FALSE; - } - - /* validate the URL to prevent HTTP header splitting */ - if (((strstr(url, "\n") != NULL) || strstr(url, "\r") != NULL)) { - *err_str = apr_pstrdup(r->pool, "Invalid Request"); - *err_desc = - apr_psprintf(r->pool, - "logout value \"%s\" contains illegal \"\n\" or \"\r\" character(s)", - url); - oidc_error(r, "%s: %s", *err_str, *err_desc); - return FALSE; - } - - return TRUE; +static apr_byte_t oidc_validate_redirect_url(request_rec *r, oidc_cfg *c, + const char *url, char **err_str, char **err_desc) { + apr_uri_t uri; + const char *c_host = NULL; + apr_hash_index_t *hi = NULL; + + if (apr_uri_parse(r->pool, url, &uri) != APR_SUCCESS) { + *err_str = apr_pstrdup(r->pool, "Malformed URL"); + *err_desc = apr_psprintf(r->pool, "not a valid URL value: %s", url); + oidc_error(r, "%s: %s", *err_str, *err_desc); + return FALSE; + } + + if (c->redirect_urls_allowed != NULL) { + for (hi = apr_hash_first(NULL, c->redirect_urls_allowed); hi; hi = + apr_hash_next(hi)) { + apr_hash_this(hi, (const void**) &c_host, NULL, NULL); + if (oidc_util_regexp_first_match(r->pool, url, c_host, + NULL, err_str) == TRUE) + break; + } + if (hi == NULL) { + *err_str = apr_pstrdup(r->pool, "URL not allowed"); + *err_desc = + apr_psprintf(r->pool, + "value does not match the list of allowed redirect URLs: %s", + url); + oidc_error(r, "%s: %s", *err_str, *err_desc); + return FALSE; + } + } else if (uri.hostname != NULL) { + c_host = oidc_get_current_url_host(r); + if ((strstr(c_host, uri.hostname) == NULL) + || (strstr(uri.hostname, c_host) == NULL)) { + *err_str = apr_pstrdup(r->pool, "Invalid Request"); + *err_desc = + apr_psprintf(r->pool, + "URL value \"%s\" does not match the hostname of the current request \"%s\"", + apr_uri_unparse(r->pool, &uri, 0), c_host); + oidc_error(r, "%s: %s", *err_str, *err_desc); + return FALSE; + } + } + + if ((uri.hostname == NULL) && (strstr(url, "/") != url)) { + *err_str = apr_pstrdup(r->pool, "Malformed URL"); + *err_desc = + apr_psprintf(r->pool, + "No hostname was parsed and it does not seem to be relative, i.e starting with '/': %s", + url); + oidc_error(r, "%s: %s", *err_str, *err_desc); + return FALSE; + } else if ((uri.hostname == NULL) && (strstr(url, "//") == url)) { + *err_str = apr_pstrdup(r->pool, "Malformed URL"); + *err_desc = apr_psprintf(r->pool, + "No hostname was parsed and starting with '//': %s", url); + oidc_error(r, "%s: %s", *err_str, *err_desc); + return FALSE; + } else if ((uri.hostname == NULL) && (strstr(url, "/\\") == url)) { + *err_str = apr_pstrdup(r->pool, "Malformed URL"); + *err_desc = apr_psprintf(r->pool, + "No hostname was parsed and starting with '/\\': %s", url); + oidc_error(r, "%s: %s", *err_str, *err_desc); + return FALSE; + } + + /* validate the URL to prevent HTTP header splitting */ + if (((strstr(url, "\n") != NULL) || strstr(url, "\r") != NULL)) { + *err_str = apr_pstrdup(r->pool, "Invalid URL"); + *err_desc = + apr_psprintf(r->pool, + "URL value \"%s\" contains illegal \"\n\" or \"\r\" character(s)", + url); + oidc_error(r, "%s: %s", *err_str, *err_desc); + return FALSE; + } + + return TRUE; } /* @@ -2774,12 +2792,11 @@ static int oidc_handle_logout(request_rec *r, oidc_cfg *c, } else { /* do input validation on the logout parameter value */ - - if (oidc_validate_post_logout_url(r, url, &error_str, - &error_description) == FALSE) { - return oidc_util_html_send_error(r, c->error_template, error_str, - error_description, - HTTP_BAD_REQUEST); + if (oidc_validate_redirect_url(r, c, url, &error_str, + &error_description) == FALSE) { + return oidc_util_html_send_error(r, c->error_template, error_str, + error_description, + HTTP_BAD_REQUEST); } } @@ -3027,6 +3044,8 @@ static int oidc_handle_refresh_token_request(request_rec *r, oidc_cfg *c, char *return_to = NULL; char *r_access_token = NULL; char *error_code = NULL; + char *error_str = NULL; + char *error_description = NULL; /* get the command passed to the session management handler */ oidc_util_get_request_parameter(r, OIDC_REDIRECT_URI_REQUEST_REFRESH, @@ -3041,6 +3060,14 @@ static int oidc_handle_refresh_token_request(request_rec *r, oidc_cfg *c, return HTTP_INTERNAL_SERVER_ERROR; } + /* do input validation on the return to parameter value */ + if (oidc_validate_redirect_url(r, c, return_to, &error_str, + &error_description) == FALSE) { + oidc_error(r, "return_to URL validation failed: %s: %s", error_str, + error_description); + return HTTP_INTERNAL_SERVER_ERROR; + } + if (r_access_token == NULL) { oidc_error(r, "refresh token request handler called with no access_token parameter"); @@ -3201,8 +3228,8 @@ static int oidc_handle_info_request(request_rec *r, oidc_cfg *c, /* get the current provider info */ oidc_provider_t *provider = NULL; - if (oidc_get_provider_from_session(r, c, session, - &provider) == FALSE) + if (oidc_get_provider_from_session(r, c, session, &provider) + == FALSE) return HTTP_INTERNAL_SERVER_ERROR; /* execute the actual refresh grant */ @@ -3319,6 +3346,20 @@ int oidc_handle_redirect_uri_request(request_rec *r, oidc_cfg *c, /* this is an authorization response from the OP using the Basic Client profile or a Hybrid flow*/ return oidc_handle_redirect_authorization_response(r, c, session); + /* + * + * Note that we are checking for logout *before* checking for a POST authorization response + * to handle backchannel POST-based logout + * + * so any POST to the Redirect URI that does not have a logout query parameter will be handled + * as an authorization response; alternatively we could assume that a POST response has no + * parameters + */ + } else if (oidc_util_request_has_parameter(r, + OIDC_REDIRECT_URI_REQUEST_LOGOUT)) { + /* handle logout */ + return oidc_handle_logout(r, c, session); + } else if (oidc_proto_is_post_authorization_response(r, c)) { /* this is an authorization response using the fragment(+POST) response_mode with the Implicit Client profile */ diff --git a/src/mod_auth_openidc.h b/src/mod_auth_openidc.h index 6821d0c..ea79e6b 100644 --- a/src/mod_auth_openidc.h +++ b/src/mod_auth_openidc.h @@ -414,6 +414,7 @@ typedef struct oidc_cfg { apr_byte_t state_input_headers; + apr_hash_t *redirect_urls_allowed; } oidc_cfg; int oidc_check_user_id(request_rec *r); diff --git a/src/util.c b/src/util.c index c1fa5f3..4b4e16b 100644 --- a/src/util.c +++ b/src/util.c @@ -2201,14 +2201,18 @@ apr_byte_t oidc_util_regexp_first_match(apr_pool_t *pool, const char *input, goto out; } - if (pcre_get_substring(input, subStr, rc, OIDC_UTIL_REGEXP_MATCH_NR, - &(psubStrMatchStr)) <= 0) { - *error_str = apr_psprintf(pool, "pcre_get_substring failed (rc=%d)", - rc); - goto out; + if (output) { + + if (pcre_get_substring(input, subStr, rc, OIDC_UTIL_REGEXP_MATCH_NR, + &(psubStrMatchStr)) <= 0) { + *error_str = apr_psprintf(pool, "pcre_get_substring failed (rc=%d)", + rc); + goto out; + } + + *output = apr_pstrdup(pool, psubStrMatchStr); } - *output = apr_pstrdup(pool, psubStrMatchStr); rv = TRUE; out: -- 2.31.1