From e4d625b0f1116e50df2737e2738b4c77b35328bb Mon Sep 17 00:00:00 2001
From: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
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 <hans.zandbelt@zmartzone.eu>
(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 [<regexp>]+
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 =
"<style type=\"text/css\">body {text-align: center}</style>";
- 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