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