rpms/mod_auth_openidc
5
0
Fork 0
Code Issues Pull Requests Releases Activity

import mod_auth_openidc-2.4.9.4-1.module+el8.7.0+14797+4085fcb6

Browse Source
This commit is contained in:
CentOS Sources 2022-11-08 01:36:34 -05:00 committed by Stepan Oksanichenko
parent 59d4e05cc1
commit 95b4d74ef6
29 changed files with 13 additions and 3877 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
.gitignore vendored
View File

@ -1 +1 @@
SOURCES/mod_auth_openidc-2.3.7.tar.gz
SOURCES/v2.4.9.4.tar.gz

2
.mod_auth_openidc.metadata
View File

@ -1 +1 @@
0f9d620dad066ae8c415a59055903da1bfa9a4bf SOURCES/mod_auth_openidc-2.3.7.tar.gz
47f8b949552c3d32f019c5cf785c4672dc0f8aae SOURCES/v2.4.9.4.tar.gz

127
SOURCES/0002-Backport-of-improve-validation-of-the-post-logout-UR.patch
View File

@ -1,127 +0,0 @@
From cb5560f016d4f8bbca40670c59898afafb8d0763 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek@redhat.com>
Date: Sun, 10 May 2020 19:56:53 +0200
Subject: [PATCH] Backport of improve validation of the post-logout URL
---
src/mod_auth_openidc.c | 90 +++++++++++++++++++++++++-----------------
1 file changed, 53 insertions(+), 37 deletions(-)
diff --git a/src/mod_auth_openidc.c b/src/mod_auth_openidc.c
index eaaec3c..e86c61e 100644
--- a/src/mod_auth_openidc.c
+++ b/src/mod_auth_openidc.c
@@ -2563,6 +2563,52 @@ 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 (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;
+ }
+
+ /* 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;
+}
+
/*
* perform (single) logout
*/
@@ -2571,6 +2617,8 @@ static int oidc_handle_logout(request_rec *r, oidc_cfg *c,
/* pickup the command or URL where the user wants to go after logout */
char *url = NULL;
+ char *error_str = NULL;
+ char *error_description = NULL;
oidc_util_get_request_parameter(r, OIDC_REDIRECT_URI_REQUEST_LOGOUT, &url);
oidc_debug(r, "enter (url=%s)", url);
@@ -2587,43 +2635,11 @@ static int oidc_handle_logout(request_rec *r, oidc_cfg *c,
/* do input validation on the logout parameter value */
- const char *error_description = NULL;
- apr_uri_t uri;
-
- if (apr_uri_parse(r->pool, url, &uri) != APR_SUCCESS) {
- const char *error_description = apr_psprintf(r->pool,
- "Logout URL malformed: %s", url);
- oidc_error(r, "%s", error_description);
- return oidc_util_html_send_error(r, c->error_template,
- "Malformed URL", error_description,
- HTTP_INTERNAL_SERVER_ERROR);
-
- }
-
- const char *c_host = oidc_get_current_url_host(r);
- if ((uri.hostname != NULL)
- && ((strstr(c_host, uri.hostname) == NULL)
- || (strstr(uri.hostname, c_host) == NULL))) {
- error_description =
- 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", error_description);
- return oidc_util_html_send_error(r, c->error_template,
- "Invalid Request", error_description,
- HTTP_INTERNAL_SERVER_ERROR);
- }
-
- /* validate the URL to prevent HTTP header splitting */
- if (((strstr(url, "\n") != NULL) || strstr(url, "\r") != NULL)) {
- error_description =
- apr_psprintf(r->pool,
- "logout value \"%s\" contains illegal \"\n\" or \"\r\" character(s)",
- url);
- oidc_error(r, "%s", error_description);
- return oidc_util_html_send_error(r, c->error_template,
- "Invalid Request", error_description,
- HTTP_INTERNAL_SERVER_ERROR);
+ 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);
}
}
--
2.21.3

31
SOURCES/0003-Backport-of-Fix-open-redirect-starting-with-a-slash.patch
View File

@ -1,31 +0,0 @@
From ed041f8b5df58c4e612a0d0cbb920dc0b399b921 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek@redhat.com>
Date: Sun, 10 May 2020 20:00:49 +0200
Subject: [PATCH 3/3] Backport of Fix open redirect starting with a slash
---
src/mod_auth_openidc.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/src/mod_auth_openidc.c b/src/mod_auth_openidc.c
index e86c61e..3c6efb4 100644
--- a/src/mod_auth_openidc.c
+++ b/src/mod_auth_openidc.c
@@ -2604,6 +2604,14 @@ static apr_byte_t oidc_validate_post_logout_url(request_rec *r, const char *url,
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;
}
return TRUE;
--
2.21.3

32
SOURCES/0004-Backport-of-Fix-open-redirect-starting-with-a-slash-.patch
View File

@ -1,32 +0,0 @@
From c21228a0f170c025d79625207dc94759f480418f Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek@redhat.com>
Date: Sun, 10 May 2020 20:02:23 +0200
Subject: [PATCH 4/4] Backport of Fix open redirect starting with a slash and a
backslash
---
src/mod_auth_openidc.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/src/mod_auth_openidc.c b/src/mod_auth_openidc.c
index 3c6efb4..e16d500 100644
--- a/src/mod_auth_openidc.c
+++ b/src/mod_auth_openidc.c
@@ -2612,6 +2612,14 @@ static apr_byte_t oidc_validate_post_logout_url(request_rec *r, const char *url,
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;
}
return TRUE;
--
2.21.3

61
SOURCES/0005-Fix-the-previous-backports.patch
View File

@ -1,61 +0,0 @@
From a5c9f79516fd4097817ac75a37af3b191a3d1448 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek@redhat.com>
Date: Mon, 1 Jun 2020 21:47:28 +0200
Subject: [PATCH] Fix the previous backports
---
src/mod_auth_openidc.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/src/mod_auth_openidc.c b/src/mod_auth_openidc.c
index e16d500..74f206b 100644
--- a/src/mod_auth_openidc.c
+++ b/src/mod_auth_openidc.c
@@ -2585,7 +2585,7 @@ static apr_byte_t oidc_validate_post_logout_url(request_rec *r, const char *url,
apr_uri_unparse(r->pool, &uri, 0), c_host);
oidc_error(r, "%s: %s", *err_str, *err_desc);
return FALSE;
- } else if (strstr(url, "/") != url) {
+ } else if ((uri.hostname == NULL) && (strstr(url, "/") != url)) {
*err_str = apr_pstrdup(r->pool, "Malformed URL");
*err_desc =
apr_psprintf(r->pool,
@@ -2593,17 +2593,6 @@ static apr_byte_t oidc_validate_post_logout_url(request_rec *r, const char *url,
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;
} else if ((uri.hostname == NULL) && (strstr(url, "//") == url)) {
*err_str = apr_pstrdup(r->pool, "Malformed URL");
*err_desc =
@@ -2622,6 +2611,17 @@ static apr_byte_t oidc_validate_post_logout_url(request_rec *r, const char *url,
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;
}
--
2.21.3

254
SOURCES/0006-add-OIDCStateMaxNumberOfCookies-to-limit-nr-of-state.patch
View File

@ -1,254 +0,0 @@
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

118
SOURCES/0007-set-boundaries-on-min-and-max-values-on-number-of-pa.patch
View File

@ -1,118 +0,0 @@
From 284537dfc0585e08cfc0702c89b241d8986c7236 Mon Sep 17 00:00:00 2001
From: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
Date: Fri, 3 Aug 2018 12:22:45 +0200
Subject: [PATCH 07/11] set boundaries on min and max values on number of
parallel state cookies
Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
(cherry picked from commit b8c53d7e0439f190afe0c6eeb2e2e12e881c65ac)
---
src/config.c | 17 ++++++++++++++++-
src/parse.c | 31 +++++++++++++++++++++++++++++++
src/parse.h | 2 ++
3 files changed, 49 insertions(+), 1 deletion(-)
diff --git a/src/config.c b/src/config.c
index 2fd63ea..c793818 100644
--- a/src/config.c
+++ b/src/config.c
@@ -997,6 +997,21 @@ static const char *oidc_set_client_auth_bearer_token(cmd_parms *cmd,
return NULL;
}
+/*
+ * set the maximun number of parallel state cookies
+ */
+static const char *oidc_set_max_number_of_state_cookies(cmd_parms *cmd,
+ void *struct_ptr, const char *arg) {
+ oidc_cfg *cfg = (oidc_cfg *) ap_get_module_config(
+ cmd->server->module_config, &auth_openidc_module);
+ const char *rv = oidc_parse_max_number_of_state_cookies(cmd->pool, arg,
+ &cfg->max_number_of_state_cookies);
+ return OIDC_CONFIG_DIR_RV(cmd, rv);
+}
+
+/*
+ * return the maximun number of parallel state cookies
+ */
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;
@@ -2642,7 +2657,7 @@ const command_rec oidc_config_cmds[] = {
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,
+ oidc_set_max_number_of_state_cookies,
(void*)APR_OFFSETOF(oidc_cfg, max_number_of_state_cookies),
RSRC_CONF,
"Maximun number of parallel state cookies i.e. outstanding authorization requests."),
diff --git a/src/parse.c b/src/parse.c
index 9d3763c..0f986fd 100644
--- a/src/parse.c
+++ b/src/parse.c
@@ -530,6 +530,28 @@ const char *oidc_valid_session_max_duration(apr_pool_t *pool, int v) {
return NULL;
}
+#define OIDC_MAX_NUMBER_OF_STATE_COOKIES_MIN 0
+#define OIDC_MAX_NUMBER_OF_STATE_COOKIES_MAX 255
+
+/*
+ * check the maximum number of parallel state cookies
+ */
+const char *oidc_valid_max_number_of_state_cookies(apr_pool_t *pool, int v) {
+ if (v == 0) {
+ return NULL;
+ }
+ if (v < OIDC_MAX_NUMBER_OF_STATE_COOKIES_MIN) {
+ return apr_psprintf(pool, "maximum must not be less than %d",
+ OIDC_MAX_NUMBER_OF_STATE_COOKIES_MIN);
+ }
+ if (v > OIDC_MAX_NUMBER_OF_STATE_COOKIES_MAX) {
+ return apr_psprintf(pool, "maximum must not be greater than %d",
+ OIDC_MAX_NUMBER_OF_STATE_COOKIES_MAX);
+ }
+ return NULL;
+}
+
+
/*
* parse a session max duration value from the provided string
*/
@@ -1218,3 +1240,12 @@ const char *oidc_parse_auth_request_method(apr_pool_t *pool, const char *arg,
return NULL;
}
+
+/*
+ * parse the maximum number of parallel state cookies
+ */
+const char *oidc_parse_max_number_of_state_cookies(apr_pool_t *pool,
+ const char *arg, int *int_value) {
+ return oidc_parse_int_valid(pool, arg, int_value,
+ oidc_valid_max_number_of_state_cookies);
+}
diff --git a/src/parse.h b/src/parse.h
index 853e98f..6355db4 100644
--- a/src/parse.h
+++ b/src/parse.h
@@ -90,6 +90,7 @@ const char *oidc_valid_userinfo_refresh_interval(apr_pool_t *pool, int v);
const char *oidc_valid_userinfo_token_method(apr_pool_t *pool, const char *arg);
const char *oidc_valid_token_binding_policy(apr_pool_t *pool, const char *arg);
const char *oidc_valid_auth_request_method(apr_pool_t *pool, const char *arg);
+const char *oidc_valid_max_number_of_state_cookies(apr_pool_t *pool, int v);
const char *oidc_parse_int(apr_pool_t *pool, const char *arg, int *int_value);
const char *oidc_parse_boolean(apr_pool_t *pool, const char *arg, int *bool_value);
@@ -116,6 +117,7 @@ const char *oidc_parse_info_hook_data(apr_pool_t *pool, const char *arg, apr_has
const char *oidc_parse_token_binding_policy(apr_pool_t *pool, const char *arg, int *int_value);
const char *oidc_token_binding_policy2str(apr_pool_t *pool, int v);
const char *oidc_parse_auth_request_method(apr_pool_t *pool, const char *arg, int *method);
+const char *oidc_parse_max_number_of_state_cookies(apr_pool_t *pool, const char *arg, int *int_value);
typedef const char *(*oidc_valid_int_function_t)(apr_pool_t *, int);
typedef const char *(*oidc_valid_function_t)(apr_pool_t *, const char *);
--
2.26.2

45
SOURCES/0008-make-the-default-max-number-of-state-cookies-7-inste.patch
View File

@ -1,45 +0,0 @@
From 623163348f74fc1bd019a676fa24af69dde79654 Mon Sep 17 00:00:00 2001
From: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
Date: Fri, 3 Aug 2018 21:41:34 +0200
Subject: [PATCH 08/11] make the default max number of state cookies 7 instead
of unlimited
bump to 2.3.8rc1
Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
(cherry picked from commit 6616372af77df04a9b0b197e759790ecf3f2399a)
---
ChangeLog | 5 ++++-
src/config.c | 2 +-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index b6ac513..27f45be 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,8 @@
-08/01/2018
+
+08/03/2018
- add option to set an upper limit to the number of concurrent state cookies via OIDCStateMaxNumberOfCookies; see #331
+- make the default maximum number of parallel state cookies 7 instead of unlimited; see #331
+- bump o 2.3.8rc1
07/06/2018
- abort when string length for remote user name substitution is larger than 255 characters
diff --git a/src/config.c b/src/config.c
index c793818..6fa6227 100644
--- a/src/config.c
+++ b/src/config.c
@@ -105,7 +105,7 @@
/* 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
+#define OIDC_DEFAULT_MAX_NUMBER_OF_STATE_COOKIES 7
/* default session inactivity timeout */
#define OIDC_DEFAULT_SESSION_INACTIVITY_TIMEOUT 300
/* default session max duration */
--
2.26.2

59
SOURCES/0009-don-t-return-content-with-503-see-331.patch
View File

@ -1,59 +0,0 @@
From 7f5666375a3351e9c37589456b6fb3c92ef987c0 Mon Sep 17 00:00:00 2001
From: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
Date: Sat, 4 Aug 2018 08:55:33 +0200
Subject: [PATCH 09/11] don't return content with 503; see #331
since it turns the HTTP 503 status code into a 200 which we don't prefer
for XHR clients; users will see Apache specific readable text
Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
(cherry picked from commit 9e98f1a042fa14d6b0892638a0d87c2b951837b6)
---
ChangeLog | 4 +++-
src/mod_auth_openidc.c | 8 ++++++++
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/ChangeLog b/ChangeLog
index 27f45be..dfe4bd6 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,8 +1,10 @@
+08/04/2018
+- don't return content with 503 since it will turn the HTTP status code into a 200; see #331
08/03/2018
- add option to set an upper limit to the number of concurrent state cookies via OIDCStateMaxNumberOfCookies; see #331
- make the default maximum number of parallel state cookies 7 instead of unlimited; see #331
-- bump o 2.3.8rc1
+- bump to 2.3.8rc1
07/06/2018
- abort when string length for remote user name substitution is larger than 255 characters
diff --git a/src/mod_auth_openidc.c b/src/mod_auth_openidc.c
index c0f65c6..e3817a9 100644
--- a/src/mod_auth_openidc.c
+++ b/src/mod_auth_openidc.c
@@ -833,12 +833,20 @@ static int oidc_authorization_request_set_cookie(request_rec *r,
* XHR client handle this?
*/
+ /*
+ * it appears that sending content with a 503 turns the HTTP status code
+ * into a 200 so we'll avoid that for now: the user will see Apache specific
+ * readable text anyway
+ *
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);
+ */
+
+ return HTTP_SERVICE_UNAVAILABLE;
}
/* assemble the cookie name for the state cookie */
--
2.26.2

305
SOURCES/0010-improve-auto-detection-of-XMLHttpRequests-via-Accept.patch
View File

@ -1,305 +0,0 @@
From 9a0827a18ec4d16cf9e79afede901194d6ef5cc6 Mon Sep 17 00:00:00 2001
From: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
Date: Thu, 23 Aug 2018 00:04:38 +0200
Subject: [PATCH 10/11] improve auto-detection of XMLHttpRequests via Accept
header; see #331
version 2.3.8rc5
Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
(cherry picked from commit cff35bba8861ddb09d694ecfcd88d5fac1018453)
---
src/mod_auth_openidc.c | 41 +++---
src/mod_auth_openidc.h | 6 +-
src/util.c | 80 +++++++---
diff --git a/src/mod_auth_openidc.c b/src/mod_auth_openidc.c
index e3817a9..897a449 100644
--- a/src/mod_auth_openidc.c
+++ b/src/mod_auth_openidc.c
@@ -805,8 +805,8 @@ 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 int oidc_authorization_request_set_cookie(request_rec *r,
- oidc_cfg *c, const char *state, oidc_proto_state_t *proto_state) {
+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:
* random value, original URL, original method, issuer, response_type, response_mod, prompt and timestamp
@@ -818,7 +818,7 @@ static int oidc_authorization_request_set_cookie(request_rec *r,
/*
* clean expired state cookies to avoid pollution and optionally
- * try to avoid the number of state cookies exceeding a max
+ * 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);
@@ -953,6 +953,26 @@ static void oidc_log_session_expires(request_rec *r, const char *msg,
apr_time_sec(session_expires - apr_time_now()));
}
+/*
+ * see if this is a non-browser request
+ */
+static apr_byte_t oidc_is_xml_http_request(request_rec *r) {
+
+ if ((oidc_util_hdr_in_x_requested_with_get(r) != NULL)
+ && (apr_strnatcasecmp(oidc_util_hdr_in_x_requested_with_get(r),
+ OIDC_HTTP_HDR_VAL_XML_HTTP_REQUEST) == 0))
+ return TRUE;
+
+ if ((oidc_util_hdr_in_accept_contains(r, OIDC_CONTENT_TYPE_TEXT_HTML)
+ == FALSE) && (oidc_util_hdr_in_accept_contains(r,
+ OIDC_CONTENT_TYPE_APP_XHTML_XML) == FALSE)
+ && (oidc_util_hdr_in_accept_contains(r,
+ OIDC_CONTENT_TYPE_ANY) == FALSE))
+ return TRUE;
+
+ return FALSE;
+}
+
/*
* find out which action we need to take when encountering an unauthenticated request
*/
@@ -982,9 +1002,7 @@ static int oidc_handle_unauthenticated_user(request_rec *r, oidc_cfg *c) {
* won't redirect the user and thus avoid creating a state cookie
* for a non-browser (= Javascript) call that will never return from the OP
*/
- if ((oidc_util_hdr_in_x_requested_with_get(r) != NULL)
- && (apr_strnatcasecmp(oidc_util_hdr_in_x_requested_with_get(r),
- OIDC_HTTP_HDR_VAL_XML_HTTP_REQUEST) == 0))
+ if (oidc_is_xml_http_request(r) == TRUE)
return HTTP_UNAUTHORIZED;
}
@@ -997,17 +1015,6 @@ static int oidc_handle_unauthenticated_user(request_rec *r, oidc_cfg *c) {
oidc_dir_cfg_path_scope(r));
}
-/*
- * see if this is a non-browser request
- */
-static apr_byte_t oidc_is_xml_http_request(request_rec *r) {
- if ((oidc_util_hdr_in_x_requested_with_get(r) != NULL)
- && (apr_strnatcasecmp(oidc_util_hdr_in_x_requested_with_get(r),
- OIDC_HTTP_HDR_VAL_XML_HTTP_REQUEST) == 0))
- return TRUE;
- return FALSE;
-}
-
/*
* check if maximum session duration was exceeded
*/
diff --git a/src/mod_auth_openidc.h b/src/mod_auth_openidc.h
index 5162ed4..f89f392 100644
--- a/src/mod_auth_openidc.h
+++ b/src/mod_auth_openidc.h
@@ -537,7 +537,9 @@ apr_byte_t oidc_oauth_get_bearer_token(request_rec *r, const char **access_token
#define OIDC_CONTENT_TYPE_JWT "application/jwt"
#define OIDC_CONTENT_TYPE_FORM_ENCODED "application/x-www-form-urlencoded"
#define OIDC_CONTENT_TYPE_IMAGE_PNG "image/png"
-#define OIDC_CONTENT_TYPE_HTML "text/html"
+#define OIDC_CONTENT_TYPE_TEXT_HTML "text/html"
+#define OIDC_CONTENT_TYPE_APP_XHTML_XML "application/xhtml+xml"
+#define OIDC_CONTENT_TYPE_ANY "*/*"
#define OIDC_STR_SPACE " "
#define OIDC_STR_EQUAL "="
@@ -560,6 +562,7 @@ apr_byte_t oidc_oauth_get_bearer_token(request_rec *r, const char **access_token
#define OIDC_CHAR_FORWARD_SLASH '/'
#define OIDC_CHAR_PIPE '|'
#define OIDC_CHAR_AMP '&'
+#define OIDC_CHAR_SEMI_COLON ';'
#define OIDC_APP_INFO_REFRESH_TOKEN "refresh_token"
#define OIDC_APP_INFO_ACCESS_TOKEN "access_token"
@@ -787,6 +790,7 @@ const char *oidc_util_hdr_in_host_get(const request_rec *r);
void oidc_util_hdr_out_location_set(const request_rec *r, const char *value);
const char *oidc_util_hdr_out_location_get(const request_rec *r);
void oidc_util_hdr_err_out_add(const request_rec *r, const char *name, const char *value);
+apr_byte_t oidc_util_hdr_in_accept_contains(const request_rec *r, const char *needle);
// oidc_metadata.c
apr_byte_t oidc_metadata_provider_retrieve(request_rec *r, oidc_cfg *cfg, const char *issuer, const char *url, json_t **j_metadata, char **response);
diff --git a/src/util.c b/src/util.c
index 2fd79ec..67b2fc3 100644
--- a/src/util.c
+++ b/src/util.c
@@ -97,7 +97,7 @@ int oidc_base64url_encode(request_rec *r, char **dst, const char *src,
enc_len--;
if ((enc_len > 0) && (enc[enc_len - 1] == ','))
enc_len--;
- if ((enc_len > 0) &&(enc[enc_len - 1] == ','))
+ if ((enc_len > 0) && (enc[enc_len - 1] == ','))
enc_len--;
enc[enc_len] = '\0';
}
@@ -320,9 +320,9 @@ char *oidc_util_unescape_string(const request_rec *r, const char *str) {
return NULL;
}
int counter = 0;
- char *replaced = (char *)str;
- while(str[counter] != '\0') {
- if(str[counter] == '+') {
+ char *replaced = (char *) str;
+ while (str[counter] != '\0') {
+ if (str[counter] == '+') {
replaced[counter] = ' ';
}
counter++;
@@ -353,7 +353,7 @@ char *oidc_util_html_escape(apr_pool_t *pool, const char *s) {
for (i = 0; i < strlen(s); i++) {
for (n = 0; n < len; n++) {
if (s[i] == chars[n]) {
- m = (unsigned int)strlen(replace[n]);
+ m = (unsigned int) strlen(replace[n]);
for (k = 0; k < m; k++)
r[j + k] = replace[n][k];
j += m;
@@ -530,12 +530,13 @@ const char *oidc_get_redirect_uri_iss(request_rec *r, oidc_cfg *cfg,
const char *redirect_uri = oidc_get_redirect_uri(r, cfg);
if (provider->issuer_specific_redirect_uri != 0) {
redirect_uri = apr_psprintf(r->pool, "%s%s%s=%s", redirect_uri,
- strchr(redirect_uri ? redirect_uri : "", OIDC_CHAR_QUERY) != NULL ?
- OIDC_STR_AMP :
- OIDC_STR_QUERY,
- OIDC_PROTO_ISS, oidc_util_escape_string(r, provider->issuer));
-// OIDC_PROTO_CLIENT_ID,
-// oidc_util_escape_string(r, provider->client_id));
+ strchr(redirect_uri ? redirect_uri : "",
+ OIDC_CHAR_QUERY) != NULL ?
+ OIDC_STR_AMP :
+ OIDC_STR_QUERY,
+ OIDC_PROTO_ISS, oidc_util_escape_string(r, provider->issuer));
+ // OIDC_PROTO_CLIENT_ID,
+ // oidc_util_escape_string(r, provider->client_id));
oidc_debug(r, "determined issuer specific redirect uri: %s",
redirect_uri);
}
@@ -1346,8 +1347,8 @@ int oidc_util_html_send(request_rec *r, const char *title,
on_load ? apr_psprintf(r->pool, " onload=\"%s()\"", on_load) : "",
html_body ? html_body : "<p></p>");
- return oidc_util_http_send(r, html, strlen(html), OIDC_CONTENT_TYPE_HTML,
- status_code);
+ return oidc_util_http_send(r, html, strlen(html),
+ OIDC_CONTENT_TYPE_TEXT_HTML, status_code);
}
static char *html_error_template_contents = NULL;
@@ -1357,7 +1358,8 @@ static char *html_error_template_contents = NULL;
* that is relative to the Apache root directory
*/
char *oidc_util_get_full_path(apr_pool_t *pool, const char *abs_or_rel_filename) {
- return (abs_or_rel_filename) ? ap_server_root_relative(pool, abs_or_rel_filename) : NULL;
+ return (abs_or_rel_filename) ?
+ ap_server_root_relative(pool, abs_or_rel_filename) : NULL;
}
/*
@@ -1389,7 +1391,7 @@ int oidc_util_html_send_error(request_rec *r, const char *html_template,
description ? description : ""));
return oidc_util_http_send(r, html, strlen(html),
- OIDC_CONTENT_TYPE_HTML, status_code);
+ OIDC_CONTENT_TYPE_TEXT_HTML, status_code);
}
}
@@ -1980,8 +1982,8 @@ void oidc_util_table_add_query_encoded_params(apr_pool_t *pool,
* create a symmetric key from a client_secret
*/
apr_byte_t oidc_util_create_symmetric_key(request_rec *r,
- const char *client_secret, unsigned int r_key_len, const char *hash_algo,
- apr_byte_t set_kid, oidc_jwk_t **jwk) {
+ const char *client_secret, unsigned int r_key_len,
+ const char *hash_algo, apr_byte_t set_kid, oidc_jwk_t **jwk) {
oidc_jose_error_t err;
unsigned char *key = NULL;
unsigned int key_len;
@@ -2216,7 +2218,8 @@ static const char *oidc_util_hdr_in_get(const request_rec *r, const char *name)
return value;
}
-static const char *oidc_util_hdr_in_get_left_most_only(const request_rec *r, const char *name, const char *separator) {
+static const char *oidc_util_hdr_in_get_left_most_only(const request_rec *r,
+ const char *name, const char *separator) {
char *last = NULL;
const char *value = oidc_util_hdr_in_get(r, name);
if (value)
@@ -2224,6 +2227,29 @@ static const char *oidc_util_hdr_in_get_left_most_only(const request_rec *r, con
return NULL;
}
+static apr_byte_t oidc_util_hdr_in_contains(const request_rec *r,
+ const char *name, const char *separator, const char postfix_separator,
+ const char *needle) {
+ char *ctx = NULL, *elem = NULL;
+ const char *value = oidc_util_hdr_in_get(r, name);
+ apr_byte_t rc = FALSE;
+ if (value) {
+ elem = apr_strtok(apr_pstrdup(r->pool, value), separator, &ctx);
+ while (elem != NULL) {
+ while (*elem == OIDC_CHAR_SPACE)
+ elem++;
+ if ((strncmp(elem, needle, strlen(needle)) == 0)
+ && ((elem[strlen(needle)] == '\0')
+ || (elem[strlen(needle)] == postfix_separator))) {
+ rc = TRUE;
+ break;
+ }
+ elem = apr_strtok(NULL, separator, &ctx);
+ }
+ }
+ return rc;
+}
+
static void oidc_util_hdr_table_set(const request_rec *r, apr_table_t *table,
const char *name, const char *value) {
@@ -2288,7 +2314,8 @@ const char *oidc_util_hdr_in_user_agent_get(const request_rec *r) {
}
const char *oidc_util_hdr_in_x_forwarded_for_get(const request_rec *r) {
- return oidc_util_hdr_in_get_left_most_only(r, OIDC_HTTP_HDR_X_FORWARDED_FOR, OIDC_STR_COMMA);
+ return oidc_util_hdr_in_get_left_most_only(r, OIDC_HTTP_HDR_X_FORWARDED_FOR,
+ OIDC_STR_COMMA);
}
const char *oidc_util_hdr_in_content_type_get(const request_rec *r) {
@@ -2303,20 +2330,29 @@ const char *oidc_util_hdr_in_accept_get(const request_rec *r) {
return oidc_util_hdr_in_get(r, OIDC_HTTP_HDR_ACCEPT);
}
+apr_byte_t oidc_util_hdr_in_accept_contains(const request_rec *r,
+ const char *needle) {
+ return oidc_util_hdr_in_contains(r, OIDC_HTTP_HDR_ACCEPT, OIDC_STR_COMMA,
+ OIDC_CHAR_SEMI_COLON, needle);
+}
+
const char *oidc_util_hdr_in_authorization_get(const request_rec *r) {
return oidc_util_hdr_in_get(r, OIDC_HTTP_HDR_AUTHORIZATION);
}
const char *oidc_util_hdr_in_x_forwarded_proto_get(const request_rec *r) {
- return oidc_util_hdr_in_get_left_most_only(r, OIDC_HTTP_HDR_X_FORWARDED_PROTO, OIDC_STR_COMMA);
+ return oidc_util_hdr_in_get_left_most_only(r,
+ OIDC_HTTP_HDR_X_FORWARDED_PROTO, OIDC_STR_COMMA);
}
const char *oidc_util_hdr_in_x_forwarded_port_get(const request_rec *r) {
- return oidc_util_hdr_in_get_left_most_only(r, OIDC_HTTP_HDR_X_FORWARDED_PORT, OIDC_STR_COMMA);
+ return oidc_util_hdr_in_get_left_most_only(r,
+ OIDC_HTTP_HDR_X_FORWARDED_PORT, OIDC_STR_COMMA);
}
const char *oidc_util_hdr_in_x_forwarded_host_get(const request_rec *r) {
- return oidc_util_hdr_in_get_left_most_only(r, OIDC_HTTP_HDR_X_FORWARDED_HOST, OIDC_STR_COMMA);
+ return oidc_util_hdr_in_get_left_most_only(r,
+ OIDC_HTTP_HDR_X_FORWARDED_HOST, OIDC_STR_COMMA);
}
const char *oidc_util_hdr_in_host_get(const request_rec *r) {

33
SOURCES/0011-oops-document-OIDCStateMaxNumberOfCookies-for-releas.patch
View File

@ -1,33 +0,0 @@
From 7f81371f55a4a28000675023ad949e217d22bea3 Mon Sep 17 00:00:00 2001
From: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
Date: Wed, 12 Sep 2018 20:05:34 +0200
Subject: [PATCH 11/11] oops: document OIDCStateMaxNumberOfCookies for release
2.3.8
Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
(cherry picked from commit 406a4915d3747d4cfdc2283f287a99ba1c7b446a)
---
auth_openidc.conf | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/auth_openidc.conf b/auth_openidc.conf
index eb83c24..48dd027 100644
--- a/auth_openidc.conf
+++ b/auth_openidc.conf
@@ -446,6 +446,13 @@
# When not defined, no cookies are stripped.
#OIDCStripCookies [<cookie-name>]+
+# Specify the maximum number of state cookies i.e. the maximum number of parallel outstanding
+# authentication requests. See: https://github.com/zmartzone/mod_auth_openidc/issues/331
+# Setting this to 0 means unlimited, until the browser or server gives up which is the
+# behavior of mod_auth_openidc < 2.3.8, which did not have this configuration option.
+# When not defined, the default is 7.
+#OIDCStateMaxNumberOfCookies <number>
+
########################################################################################
#
# Session Settings (only relevant in an OpenID Connect Relying Party setup)
--
2.26.2

285
SOURCES/0012-optionally-delete-the-oldest-state-cookie-s-see-399.patch
View File

@ -1,285 +0,0 @@
From 95baad3342aa7ef7172ad4922eb9f0d605dc469b Mon Sep 17 00:00:00 2001
From: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
Date: Thu, 6 Dec 2018 14:55:44 +0100
Subject: [PATCH 12/13] optionally delete the oldest state cookie(s); see #399
bump to 2.3.10rc3
Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
(cherry picked from commit 46758a75eef8e3c91f2917fc7c6302136eb18809)
---
auth_openidc.conf | 8 +++--
src/config.c | 27 +++++++++++++----
src/mod_auth_openidc.c | 66 +++++++++++++++++++++++++++++++++++++++---
src/mod_auth_openidc.h | 2 ++
src/parse.c | 9 ++++--
src/parse.h | 2 +-
6 files changed, 100 insertions(+), 14 deletions(-)
diff --git a/auth_openidc.conf b/auth_openidc.conf
index 48dd027..33cea64 100644
--- a/auth_openidc.conf
+++ b/auth_openidc.conf
@@ -450,8 +450,12 @@
# authentication requests. See: https://github.com/zmartzone/mod_auth_openidc/issues/331
# Setting this to 0 means unlimited, until the browser or server gives up which is the
# behavior of mod_auth_openidc < 2.3.8, which did not have this configuration option.
-# When not defined, the default is 7.
-#OIDCStateMaxNumberOfCookies <number>
+#
+# The optional second boolean parameter if the oldest state cookie(s) will be deleted,
+# even if still valid; see #399.
+#
+# When not defined, the default is 7 and "false", thus the oldest cookie(s) will not be deleted.
+#OIDCStateMaxNumberOfCookies <number> [false|true]
########################################################################################
#
diff --git a/src/config.c b/src/config.c
index 6fa6227..8e56716 100644
--- a/src/config.c
+++ b/src/config.c
@@ -106,6 +106,8 @@
#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 7
+/* default setting for deleting the oldest state cookies */
+#define OIDC_DEFAULT_DELETE_OLDEST_STATE_COOKIES 0
/* default session inactivity timeout */
#define OIDC_DEFAULT_SESSION_INACTIVITY_TIMEOUT 300
/* default session max duration */
@@ -1001,11 +1003,12 @@ static const char *oidc_set_client_auth_bearer_token(cmd_parms *cmd,
* set the maximun number of parallel state cookies
*/
static const char *oidc_set_max_number_of_state_cookies(cmd_parms *cmd,
- void *struct_ptr, const char *arg) {
+ void *struct_ptr, const char *arg1, const char *arg2) {
oidc_cfg *cfg = (oidc_cfg *) ap_get_module_config(
cmd->server->module_config, &auth_openidc_module);
- const char *rv = oidc_parse_max_number_of_state_cookies(cmd->pool, arg,
- &cfg->max_number_of_state_cookies);
+ const char *rv = oidc_parse_max_number_of_state_cookies(cmd->pool, arg1,
+ arg2, &cfg->max_number_of_state_cookies,
+ &cfg->delete_oldest_state_cookies);
return OIDC_CONFIG_DIR_RV(cmd, rv);
}
@@ -1018,6 +1021,15 @@ int oidc_cfg_max_number_of_state_cookies(oidc_cfg *cfg) {
return cfg->max_number_of_state_cookies;
}
+/*
+ * return the number of oldest state cookies that need to be deleted
+ */
+int oidc_cfg_delete_oldest_state_cookies(oidc_cfg *cfg) {
+ if (cfg->delete_oldest_state_cookies == OIDC_CONFIG_POS_INT_UNSET)
+ return OIDC_DEFAULT_DELETE_OLDEST_STATE_COOKIES;
+ return cfg->delete_oldest_state_cookies;
+}
+
/*
* create a new server config record with defaults
*/
@@ -1127,6 +1139,7 @@ void *oidc_create_server_config(apr_pool_t *pool, server_rec *svr) {
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->delete_oldest_state_cookies = OIDC_CONFIG_POS_INT_UNSET;
c->session_inactivity_timeout = OIDC_DEFAULT_SESSION_INACTIVITY_TIMEOUT;
c->cookie_domain = NULL;
@@ -1445,6 +1458,10 @@ void *oidc_merge_server_config(apr_pool_t *pool, void *BASE, void *ADD) {
add->max_number_of_state_cookies != OIDC_CONFIG_POS_INT_UNSET ?
add->max_number_of_state_cookies :
base->max_number_of_state_cookies;
+ c->delete_oldest_state_cookies =
+ add->delete_oldest_state_cookies != OIDC_CONFIG_POS_INT_UNSET ?
+ add->delete_oldest_state_cookies :
+ base->delete_oldest_state_cookies;
c->session_inactivity_timeout =
add->session_inactivity_timeout
!= OIDC_DEFAULT_SESSION_INACTIVITY_TIMEOUT ?
@@ -2656,11 +2673,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,
+ AP_INIT_TAKE12(OIDCStateMaxNumberOfCookies,
oidc_set_max_number_of_state_cookies,
(void*)APR_OFFSETOF(oidc_cfg, max_number_of_state_cookies),
RSRC_CONF,
- "Maximun number of parallel state cookies i.e. outstanding authorization requests."),
+ "Maximun number of parallel state cookies i.e. outstanding authorization requests and whether to delete the oldest cookie(s)."),
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 897a449..8740e02 100644
--- a/src/mod_auth_openidc.c
+++ b/src/mod_auth_openidc.c
@@ -686,15 +686,53 @@ static apr_byte_t oidc_unsolicited_proto_state(request_rec *r, oidc_cfg *c,
return TRUE;
}
+typedef struct oidc_state_cookies_t {
+ char *name;
+ apr_time_t timestamp;
+ struct oidc_state_cookies_t *next;
+} oidc_state_cookies_t;
+
+static int oidc_delete_oldest_state_cookies(request_rec *r,
+ int number_of_valid_state_cookies, int max_number_of_state_cookies,
+ oidc_state_cookies_t *first) {
+ oidc_state_cookies_t *cur = NULL, *prev = NULL, *prev_oldest = NULL,
+ *oldest = NULL;
+ while (number_of_valid_state_cookies >= max_number_of_state_cookies) {
+ oldest = first;
+ prev_oldest = NULL;
+ prev = first;
+ cur = first->next;
+ while (cur) {
+ if ((cur->timestamp < oldest->timestamp)) {
+ oldest = cur;
+ prev_oldest = prev;
+ }
+ prev = cur;
+ cur = cur->next;
+ }
+ oidc_warn(r,
+ "deleting oldest state cookie: %s (time until expiry " APR_TIME_T_FMT " seconds)",
+ oldest->name, apr_time_sec(oldest->timestamp - apr_time_now()));
+ oidc_util_set_cookie(r, oldest->name, "", 0, NULL);
+ if (prev_oldest)
+ prev_oldest->next = oldest->next;
+ else
+ first = first->next;
+ number_of_valid_state_cookies--;
+ }
+ return number_of_valid_state_cookies;
+}
+
/*
* 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) {
+ const char *currentCookieName, int delete_oldest) {
int number_of_valid_state_cookies = 0;
- char *cookie, *tokenizerCtx;
+ oidc_state_cookies_t *first = NULL, *last = NULL;
+ char *cookie, *tokenizerCtx = NULL;
char *cookies = apr_pstrdup(r->pool, oidc_util_hdr_in_cookie_get(r));
if (cookies != NULL) {
cookie = apr_strtok(cookies, OIDC_STR_SEMI_COLON, &tokenizerCtx);
@@ -722,6 +760,18 @@ static int oidc_clean_expired_state_cookies(request_rec *r, oidc_cfg *c,
oidc_util_set_cookie(r, cookieName, "", 0,
NULL);
} else {
+ if (first == NULL) {
+ first = apr_pcalloc(r->pool,
+ sizeof(oidc_state_cookies_t));
+ last = first;
+ } else {
+ last->next = apr_pcalloc(r->pool,
+ sizeof(oidc_state_cookies_t));
+ last = last->next;
+ }
+ last->name = cookieName;
+ last->timestamp = ts;
+ last->next = NULL;
number_of_valid_state_cookies++;
}
oidc_proto_state_destroy(proto_state);
@@ -732,6 +782,12 @@ static int oidc_clean_expired_state_cookies(request_rec *r, oidc_cfg *c,
cookie = apr_strtok(NULL, OIDC_STR_SEMI_COLON, &tokenizerCtx);
}
}
+
+ if (delete_oldest > 0)
+ number_of_valid_state_cookies = oidc_delete_oldest_state_cookies(r,
+ number_of_valid_state_cookies, c->max_number_of_state_cookies,
+ first);
+
return number_of_valid_state_cookies;
}
@@ -746,7 +802,7 @@ static apr_byte_t oidc_restore_proto_state(request_rec *r, oidc_cfg *c,
const char *cookieName = oidc_get_state_cookie_name(r, state);
/* clean expired state cookies to avoid pollution */
- oidc_clean_expired_state_cookies(r, c, cookieName);
+ oidc_clean_expired_state_cookies(r, c, cookieName, FALSE);
/* get the state cookie value first */
char *cookieValue = oidc_util_get_cookie(r, cookieName);
@@ -820,10 +876,12 @@ static int oidc_authorization_request_set_cookie(request_rec *r, oidc_cfg *c,
* 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 number_of_cookies = oidc_clean_expired_state_cookies(r, c, NULL,
+ oidc_cfg_delete_oldest_state_cookies(c));
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);
diff --git a/src/mod_auth_openidc.h b/src/mod_auth_openidc.h
index f89f392..c3a0a23 100644
--- a/src/mod_auth_openidc.h
+++ b/src/mod_auth_openidc.h
@@ -380,6 +380,7 @@ typedef struct oidc_cfg {
int http_timeout_short;
int state_timeout;
int max_number_of_state_cookies;
+ int delete_oldest_state_cookies;
int session_inactivity_timeout;
int session_cache_fallback_to_cookie;
@@ -691,6 +692,7 @@ 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);
+int oidc_cfg_delete_oldest_state_cookies(oidc_cfg *cfg);
// oidc_util.c
int oidc_strnenvcmp(const char *a, const char *b, int len);
diff --git a/src/parse.c b/src/parse.c
index 0f986fd..2d09584 100644
--- a/src/parse.c
+++ b/src/parse.c
@@ -1245,7 +1245,12 @@ const char *oidc_parse_auth_request_method(apr_pool_t *pool, const char *arg,
* parse the maximum number of parallel state cookies
*/
const char *oidc_parse_max_number_of_state_cookies(apr_pool_t *pool,
- const char *arg, int *int_value) {
- return oidc_parse_int_valid(pool, arg, int_value,
+ const char *arg1, const char *arg2, int *int_value, int *bool_value) {
+ const char *rv = NULL;
+
+ rv = oidc_parse_int_valid(pool, arg1, int_value,
oidc_valid_max_number_of_state_cookies);
+ if ((rv == NULL) && (arg2 != NULL))
+ rv = oidc_parse_boolean(pool, arg2, bool_value);
+ return rv;
}
diff --git a/src/parse.h b/src/parse.h
index 6355db4..bdf5651 100644
--- a/src/parse.h
+++ b/src/parse.h
@@ -117,7 +117,7 @@ const char *oidc_parse_info_hook_data(apr_pool_t *pool, const char *arg, apr_has
const char *oidc_parse_token_binding_policy(apr_pool_t *pool, const char *arg, int *int_value);
const char *oidc_token_binding_policy2str(apr_pool_t *pool, int v);
const char *oidc_parse_auth_request_method(apr_pool_t *pool, const char *arg, int *method);
-const char *oidc_parse_max_number_of_state_cookies(apr_pool_t *pool, const char *arg, int *int_value);
+const char *oidc_parse_max_number_of_state_cookies(apr_pool_t *pool, const char *arg1, const char *arg2, int *int_value, int *bool_value);
typedef const char *(*oidc_valid_int_function_t)(apr_pool_t *, int);
typedef const char *(*oidc_valid_function_t)(apr_pool_t *, const char *);
--
2.26.2

240
SOURCES/0013-Allow-configuring-which-header-value-is-used-to-calc.patch
View File

@ -1,240 +0,0 @@
From 5a1f90847efce160631ee2a16d7b9d1da3496616 Mon Sep 17 00:00:00 2001
From: Hiroyuki Wada <h2-wada@nri.co.jp>
Date: Tue, 21 Apr 2020 20:29:25 +0900
Subject: [PATCH 13/13] Allow configuring which header value is used to
calculate the fingerprint of the state during authentication
(cherry picked from commit 2a97eced569ebbff82fc0370c4f741d04ba7cb13)
---
auth_openidc.conf | 4 ++++
src/config.c | 25 +++++++++++++++++++++++++
src/mod_auth_openidc.c | 30 +++++++++++++++++-------------
src/mod_auth_openidc.h | 5 +++++
src/parse.c | 33 +++++++++++++++++++++++++++++++++
src/parse.h | 1 +
6 files changed, 85 insertions(+), 13 deletions(-)
diff --git a/auth_openidc.conf b/auth_openidc.conf
index 33cea64..4012df3 100644
--- a/auth_openidc.conf
+++ b/auth_openidc.conf
@@ -774,3 +774,7 @@
# When not defined no claims are whitelisted and all claims are stored except when blacklisted with OIDCBlackListedClaims.
#OIDCWhiteListedClaims [<claim>]+
+# Defines whether the value of the User-Agent and X-Forwarded-For headers will be used as the input
+# 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]
diff --git a/src/config.c b/src/config.c
index 8e56716..588e1a3 100644
--- a/src/config.c
+++ b/src/config.c
@@ -166,6 +166,8 @@
#define OIDC_DEFAULT_AUTH_REQUEST_METHOD OIDC_AUTH_REQUEST_METHOD_GET
/* define whether the issuer will be added to the redirect uri by default to mitigate the IDP mixup attack */
#define OIDC_DEFAULT_PROVIDER_ISSUER_SPECIFIC_REDIRECT_URI 0
+/* default setting for calculating the fingerprint of the state from request headers during authentication */
+#define OIDC_DEFAULT_STATE_INPUT_HEADERS OIDC_STATE_INPUT_HEADERS_USER_AGENT | OIDC_STATE_INPUT_HEADERS_X_FORWARDED_FOR
#define OIDCProviderMetadataURL "OIDCProviderMetadataURL"
#define OIDCProviderIssuer "OIDCProviderIssuer"
@@ -261,6 +263,7 @@
#define OIDCProviderAuthRequestMethod "OIDCProviderAuthRequestMethod"
#define OIDCBlackListedClaims "OIDCBlackListedClaims"
#define OIDCOAuthServerMetadataURL "OIDCOAuthServerMetadataURL"
+#define OIDCStateInputHeaders "OIDCStateInputHeaders"
extern module AP_MODULE_DECLARE_DATA auth_openidc_module;
@@ -1030,6 +1033,17 @@ int oidc_cfg_delete_oldest_state_cookies(oidc_cfg *cfg) {
return cfg->delete_oldest_state_cookies;
}
+/*
+ * define which header we use for calculating the fingerprint of the state during authentication
+ */
+static const char * oidc_set_state_input_headers_as(cmd_parms *cmd, void *m,
+ const char *arg) {
+ oidc_cfg *cfg = (oidc_cfg *) ap_get_module_config(
+ cmd->server->module_config, &auth_openidc_module);
+ const char *rv = oidc_parse_set_state_input_headers_as(cmd->pool, arg, &cfg->state_input_headers);
+ return OIDC_CONFIG_DIR_RV(cmd, rv);
+}
+
/*
* create a new server config record with defaults
*/
@@ -1176,6 +1190,8 @@ void *oidc_create_server_config(apr_pool_t *pool, server_rec *svr) {
c->provider.issuer_specific_redirect_uri =
OIDC_DEFAULT_PROVIDER_ISSUER_SPECIFIC_REDIRECT_URI;
+ c->state_input_headers = OIDC_DEFAULT_STATE_INPUT_HEADERS;
+
return c;
}
@@ -1617,6 +1633,10 @@ void *oidc_merge_server_config(apr_pool_t *pool, void *BASE, void *ADD) {
add->provider.issuer_specific_redirect_uri :
base->provider.issuer_specific_redirect_uri;
+ c->state_input_headers =
+ add->state_input_headers != OIDC_DEFAULT_STATE_INPUT_HEADERS ?
+ add->state_input_headers : base->state_input_headers;
+
return c;
}
@@ -2866,5 +2886,10 @@ const command_rec oidc_config_cmds[] = {
(void*)APR_OFFSETOF(oidc_cfg, oauth.metadata_url),
RSRC_CONF,
"Authorization Server metadata URL."),
+ AP_INIT_TAKE123(OIDCStateInputHeaders,
+ oidc_set_state_input_headers_as,
+ 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)."),
{ NULL }
};
diff --git a/src/mod_auth_openidc.c b/src/mod_auth_openidc.c
index 8740e02..38558d2 100644
--- a/src/mod_auth_openidc.c
+++ b/src/mod_auth_openidc.c
@@ -226,7 +226,7 @@ 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, const char *nonce) {
+static char *oidc_get_browser_state_hash(request_rec *r, oidc_cfg *c, const char *nonce) {
oidc_debug(r, "enter");
@@ -238,17 +238,21 @@ static char *oidc_get_browser_state_hash(request_rec *r, const char *nonce) {
/* Initialize the hash context */
apr_sha1_init(&sha1);
- /* get the X-FORWARDED-FOR header value */
- value = oidc_util_hdr_in_x_forwarded_for_get(r);
- /* if we have a value for this header, concat it to the hash input */
- if (value != NULL)
- apr_sha1_update(&sha1, value, strlen(value));
+ if (c->state_input_headers & OIDC_STATE_INPUT_HEADERS_X_FORWARDED_FOR) {
+ /* get the X-FORWARDED-FOR header value */
+ value = oidc_util_hdr_in_x_forwarded_for_get(r);
+ /* if we have a value for this header, concat it to the hash input */
+ if (value != NULL)
+ apr_sha1_update(&sha1, value, strlen(value));
+ }
- /* get the USER-AGENT header value */
- value = oidc_util_hdr_in_user_agent_get(r);
- /* if we have a value for this header, concat it to the hash input */
- if (value != NULL)
- apr_sha1_update(&sha1, value, strlen(value));
+ if (c->state_input_headers & OIDC_STATE_INPUT_HEADERS_USER_AGENT) {
+ /* get the USER-AGENT header value */
+ value = oidc_util_hdr_in_user_agent_get(r);
+ /* if we have a value for this header, concat it to the hash input */
+ if (value != NULL)
+ apr_sha1_update(&sha1, value, strlen(value));
+ }
/* get the remote client IP address or host name */
/*
@@ -821,7 +825,7 @@ static apr_byte_t oidc_restore_proto_state(request_rec *r, oidc_cfg *c,
const char *nonce = oidc_proto_state_get_nonce(*proto_state);
/* calculate the hash of the browser fingerprint concatenated with the nonce */
- char *calc = oidc_get_browser_state_hash(r, nonce);
+ char *calc = oidc_get_browser_state_hash(r, c, nonce);
/* compare the calculated hash with the value provided in the authorization response */
if (apr_strnatcmp(calc, state) != 0) {
oidc_error(r,
@@ -2345,7 +2349,7 @@ static int oidc_authenticate_user(request_rec *r, oidc_cfg *c,
oidc_proto_state_set_pkce_state(proto_state, pkce_state);
/* get a hash value that fingerprints the browser concatenated with the random input */
- char *state = oidc_get_browser_state_hash(r, nonce);
+ char *state = oidc_get_browser_state_hash(r, c, nonce);
/*
* create state that restores the context when the authorization response comes in
diff --git a/src/mod_auth_openidc.h b/src/mod_auth_openidc.h
index c3a0a23..fada56d 100644
--- a/src/mod_auth_openidc.h
+++ b/src/mod_auth_openidc.h
@@ -222,6 +222,9 @@ APLOG_USE_MODULE(auth_openidc);
#define OIDC_TOKEN_BINDING_POLICY_REQUIRED 2
#define OIDC_TOKEN_BINDING_POLICY_ENFORCED 3
+#define OIDC_STATE_INPUT_HEADERS_USER_AGENT 1
+#define OIDC_STATE_INPUT_HEADERS_X_FORWARDED_FOR 2
+
typedef apr_byte_t (*oidc_proto_pkce_state)(request_rec *r, char **state);
typedef apr_byte_t (*oidc_proto_pkce_challenge)(request_rec *r, const char *state, char **code_challenge);
typedef apr_byte_t (*oidc_proto_pkce_verifier)(request_rec *r, const char *state, char **code_verifier);
@@ -403,6 +406,8 @@ typedef struct oidc_cfg {
apr_hash_t *black_listed_claims;
apr_hash_t *white_listed_claims;
+ apr_byte_t state_input_headers;
+
} oidc_cfg;
int oidc_check_user_id(request_rec *r);
diff --git a/src/parse.c b/src/parse.c
index 2d09584..a0cedcb 100644
--- a/src/parse.c
+++ b/src/parse.c
@@ -1254,3 +1254,36 @@ const char *oidc_parse_max_number_of_state_cookies(apr_pool_t *pool,
rv = oidc_parse_boolean(pool, arg2, bool_value);
return rv;
}
+
+#define OIDC_STATE_INPUT_HEADERS_AS_BOTH "both"
+#define OIDC_STATE_INPUT_HEADERS_AS_USER_AGENT "user-agent"
+#define OIDC_STATE_INPUT_HEADERS_AS_X_FORWARDED_FOR "x-forwarded-for"
+#define OIDC_STATE_INPUT_HEADERS_AS_NONE "none"
+
+/*
+ * parse a "set state input headers as" value from the provided string
+ */
+const char *oidc_parse_set_state_input_headers_as(apr_pool_t *pool, const char *arg,
+ apr_byte_t *state_input_headers) {
+ static char *options[] = {
+ OIDC_STATE_INPUT_HEADERS_AS_BOTH,
+ OIDC_STATE_INPUT_HEADERS_AS_USER_AGENT,
+ OIDC_STATE_INPUT_HEADERS_AS_X_FORWARDED_FOR,
+ OIDC_STATE_INPUT_HEADERS_AS_NONE,
+ NULL };
+ const char *rv = oidc_valid_string_option(pool, arg, options);
+ if (rv != NULL)
+ return rv;
+
+ if (apr_strnatcmp(arg, OIDC_STATE_INPUT_HEADERS_AS_BOTH) == 0) {
+ *state_input_headers = OIDC_STATE_INPUT_HEADERS_USER_AGENT | OIDC_STATE_INPUT_HEADERS_X_FORWARDED_FOR;
+ } else if (apr_strnatcmp(arg, OIDC_STATE_INPUT_HEADERS_AS_USER_AGENT) == 0) {
+ *state_input_headers = OIDC_STATE_INPUT_HEADERS_USER_AGENT;
+ } else if (apr_strnatcmp(arg, OIDC_STATE_INPUT_HEADERS_AS_X_FORWARDED_FOR) == 0) {
+ *state_input_headers = OIDC_STATE_INPUT_HEADERS_X_FORWARDED_FOR;
+ } else if (apr_strnatcmp(arg, OIDC_STATE_INPUT_HEADERS_AS_NONE) == 0) {
+ *state_input_headers = 0;
+ }
+
+ return NULL;
+}
diff --git a/src/parse.h b/src/parse.h
index bdf5651..c4301a3 100644
--- a/src/parse.h
+++ b/src/parse.h
@@ -118,6 +118,7 @@ const char *oidc_parse_token_binding_policy(apr_pool_t *pool, const char *arg, i
const char *oidc_token_binding_policy2str(apr_pool_t *pool, int v);
const char *oidc_parse_auth_request_method(apr_pool_t *pool, const char *arg, int *method);
const char *oidc_parse_max_number_of_state_cookies(apr_pool_t *pool, const char *arg1, const char *arg2, int *int_value, int *bool_value);
+const char *oidc_parse_set_state_input_headers_as(apr_pool_t *pool, const char *arg, apr_byte_t *state_input_headers);
typedef const char *(*oidc_valid_int_function_t)(apr_pool_t *, int);
typedef const char *(*oidc_valid_function_t)(apr_pool_t *, const char *);
--
2.26.2

86
SOURCES/0014-add-value-of-OIDC_SET_COOKIE_APPEND-env-var-to-Set-C.patch
View File

@ -1,86 +0,0 @@
From 0bd084eb058361517b64a2c10a46c332adc9aeea Mon Sep 17 00:00:00 2001
From: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
Date: Wed, 15 Jan 2020 17:58:53 +0100
Subject: [PATCH 14/19] add value of OIDC_SET_COOKIE_APPEND env var to
Set-Cookie headers
- useful for handling changing/upcoming SameSite behaviors across
different browsers, e.g.:
SetEnvIf User-Agent ".*IOS.*" OIDC_SET_COOKIE_APPEND=SameSite=None
- bump to 2.4.1rc4
Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
(cherry picked from commit a326dbe843a755124ecee883db52dcdc26284c26)
---
ChangeLog | 5 +++++
src/util.c | 27 +++++++++++++++++++++++++++
2 files changed, 32 insertions(+)
diff --git a/ChangeLog b/ChangeLog
index dfe4bd6..fc7c5ae 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+01/15/2020
+- add value of OIDC_SET_COOKIE_APPEND env var to Set-Cookie headers
+ useful for handling changing/upcoming SameSite behaviors across different browsers, e.g.:
+ SetEnvIf User-Agent ".*IOS.*" OIDC_SET_COOKIE_APPEND=SameSite=None
+
08/04/2018
- don't return content with 503 since it will turn the HTTP status code into a 200; see #331
diff --git a/src/util.c b/src/util.c
index 67b2fc3..993718e 100644
--- a/src/util.c
+++ b/src/util.c
@@ -914,6 +914,27 @@ static char *oidc_util_get_cookie_path(request_rec *r) {
#define OIDC_COOKIE_MAX_SIZE 4093
+#define OIDC_SET_COOKIE_APPEND_ENV_VAR "OIDC_SET_COOKIE_APPEND"
+
+const char *oidc_util_set_cookie_append_value(request_rec *r, oidc_cfg *c) {
+ const char *env_var_value = NULL;
+
+ if (r->subprocess_env != NULL)
+ env_var_value = apr_table_get(r->subprocess_env,
+ OIDC_SET_COOKIE_APPEND_ENV_VAR);
+
+ if (env_var_value == NULL) {
+ oidc_debug(r, "no cookie append environment variable %s found",
+ OIDC_SET_COOKIE_APPEND_ENV_VAR);
+ return NULL;
+ }
+
+ oidc_debug(r, "cookie append environment variable %s=%s found",
+ OIDC_SET_COOKIE_APPEND_ENV_VAR, env_var_value);
+
+ return env_var_value;
+}
+
/*
* set a cookie in the HTTP response headers
*/
@@ -923,6 +944,7 @@ void oidc_util_set_cookie(request_rec *r, const char *cookieName,
oidc_cfg *c = ap_get_module_config(r->server->module_config,
&auth_openidc_module);
char *headerString, *expiresString = NULL;
+ const char *appendString = NULL;
/* see if we need to clear the cookie */
if (apr_strnatcmp(cookieValue, "") == 0)
@@ -961,6 +983,11 @@ void oidc_util_set_cookie(request_rec *r, const char *cookieName,
if (ext != NULL)
headerString = apr_psprintf(r->pool, "%s; %s", headerString, ext);
+ appendString = oidc_util_set_cookie_append_value(r, c);
+ if (appendString != NULL)
+ headerString = apr_psprintf(r->pool, "%s; %s", headerString,
+ appendString);
+
/* sanity check on overall cookie value size */
if (strlen(headerString) > OIDC_COOKIE_MAX_SIZE) {
oidc_warn(r,
--
2.26.2

35
SOURCES/0015-pick-OIDC_SET_COOKIE_APPEND-over-ext-passed-in-to-oi.patch
View File

@ -1,35 +0,0 @@
From 914f700cd791d370cf363d408e938598023980dc Mon Sep 17 00:00:00 2001
From: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
Date: Sun, 19 Jan 2020 16:00:31 +0100
Subject: [PATCH 15/19] pick OIDC_SET_COOKIE_APPEND over ext passed in to
oidc_util_set_cookie
Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
(cherry picked from commit 5aa73817172acbb9e86287a54bc4532af7e394ee)
---
src/util.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/util.c b/src/util.c
index 993718e..c1fa5f3 100644
--- a/src/util.c
+++ b/src/util.c
@@ -980,13 +980,12 @@ void oidc_util_set_cookie(request_rec *r, const char *cookieName,
headerString = apr_psprintf(r->pool, "%s; %s", headerString,
OIDC_COOKIE_FLAG_HTTP_ONLY);
- if (ext != NULL)
- headerString = apr_psprintf(r->pool, "%s; %s", headerString, ext);
-
appendString = oidc_util_set_cookie_append_value(r, c);
if (appendString != NULL)
headerString = apr_psprintf(r->pool, "%s; %s", headerString,
appendString);
+ else if (ext != NULL)
+ headerString = apr_psprintf(r->pool, "%s; %s", headerString, ext);
/* sanity check on overall cookie value size */
if (strlen(headerString) > OIDC_COOKIE_MAX_SIZE) {
--
2.26.2

95
SOURCES/0016-always-add-a-SameSite-value-to-the-Set-Cookie-header.patch
View File

@ -1,95 +0,0 @@
From 2c999448c87b286744ac9802cb8e4277d5c38b71 Mon Sep 17 00:00:00 2001
From: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
Date: Wed, 29 Jan 2020 13:27:44 +0100
Subject: [PATCH 16/19] always add a SameSite value to the Set-Cookie header
- to satisfy upcoming Chrome/Firefox changes
this can be overridden by using, e.g.:
SetEnvIf User-Agent ".*IOS.*" OIDC_SET_COOKIE_APPEND=;
- release 2.4.1rc6
Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
(cherry picked from commit 3b4770f49cc67b9b0ae8732e9908895683ea556c)
---
ChangeLog | 5 +++++
src/mod_auth_openidc.c | 10 +++++++---
src/mod_auth_openidc.h | 1 +
src/session.c | 2 +-
4 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index fc7c5ae..b67f764 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+01/29/2020
+- always add a SameSite value to the Set-Cookie header to satisfy upcoming Chrome/Firefox changes
+ this can be overridden by using, e.g.:
+ SetEnvIf User-Agent ".*IOS.*" OIDC_SET_COOKIE_APPEND=;
+
01/15/2020
- add value of OIDC_SET_COOKIE_APPEND env var to Set-Cookie headers
useful for handling changing/upcoming SameSite behaviors across different browsers, e.g.:
diff --git a/src/mod_auth_openidc.c b/src/mod_auth_openidc.c
index 38558d2..0d2b37c 100644
--- a/src/mod_auth_openidc.c
+++ b/src/mod_auth_openidc.c
@@ -916,7 +916,9 @@ static int oidc_authorization_request_set_cookie(request_rec *r, oidc_cfg *c,
/* set it as a cookie */
oidc_util_set_cookie(r, cookieName, cookieValue, -1,
- c->cookie_same_site ? OIDC_COOKIE_EXT_SAME_SITE_LAX : NULL);
+ c->cookie_same_site ?
+ OIDC_COOKIE_EXT_SAME_SITE_LAX :
+ OIDC_COOKIE_EXT_SAME_SITE_NONE);
return HTTP_OK;
}
@@ -2183,7 +2185,7 @@ static int oidc_discovery(request_rec *r, oidc_cfg *cfg) {
oidc_util_set_cookie(r, OIDC_CSRF_NAME, csrf, -1,
cfg->cookie_same_site ?
OIDC_COOKIE_EXT_SAME_SITE_STRICT :
- NULL);
+ OIDC_COOKIE_EXT_SAME_SITE_NONE);
/* see if we need to preserve POST parameters through Javascript/HTML5 storage */
if (oidc_post_preserve_javascript(r, url, NULL, NULL) == TRUE)
@@ -2276,7 +2278,9 @@ static int oidc_discovery(request_rec *r, oidc_cfg *cfg) {
s = apr_psprintf(r->pool, "%s</form>\n", s);
oidc_util_set_cookie(r, OIDC_CSRF_NAME, csrf, -1,
- cfg->cookie_same_site ? OIDC_COOKIE_EXT_SAME_SITE_STRICT : NULL);
+ cfg->cookie_same_site ?
+ OIDC_COOKIE_EXT_SAME_SITE_STRICT :
+ OIDC_COOKIE_EXT_SAME_SITE_NONE);
char *javascript = NULL, *javascript_method = NULL;
char *html_head =
diff --git a/src/mod_auth_openidc.h b/src/mod_auth_openidc.h
index fada56d..5f1a79a 100644
--- a/src/mod_auth_openidc.h
+++ b/src/mod_auth_openidc.h
@@ -213,6 +213,7 @@ APLOG_USE_MODULE(auth_openidc);
#define OIDC_COOKIE_EXT_SAME_SITE_LAX "SameSite=Lax"
#define OIDC_COOKIE_EXT_SAME_SITE_STRICT "SameSite=Strict"
+#define OIDC_COOKIE_EXT_SAME_SITE_NONE "SameSite=None"
/* https://tools.ietf.org/html/draft-ietf-tokbind-ttrp-01 */
#define OIDC_TB_CFG_PROVIDED_ENV_VAR "Sec-Provided-Token-Binding-ID"
diff --git a/src/session.c b/src/session.c
index 1c6e118..cd9ccb8 100644
--- a/src/session.c
+++ b/src/session.c
@@ -204,7 +204,7 @@ static apr_byte_t oidc_session_save_cache(request_rec *r, oidc_session_t *z,
(first_time ?
OIDC_COOKIE_EXT_SAME_SITE_LAX :
OIDC_COOKIE_EXT_SAME_SITE_STRICT) :
- NULL);
+ OIDC_COOKIE_EXT_SAME_SITE_NONE);
} else {
/* clear the cookie */
--
2.26.2

42
SOURCES/0017-fix-also-add-SameSite-None-to-by-value-session-cooki.patch
View File

@ -1,42 +0,0 @@
From ca43d64e722f80ed91871c9ea31fbc7660aa9147 Mon Sep 17 00:00:00 2001
From: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
Date: Mon, 3 Feb 2020 10:34:10 +0100
Subject: [PATCH 17/19] fix: also add SameSite=None to by-value session cookies
bump to 2.4.2rc0
Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
(cherry picked from commit f6798246abc8fd8f865db313439882ac9f5771f3)
---
ChangeLog | 4 ++++
src/session.c | 2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/ChangeLog b/ChangeLog
index b67f764..3db7110 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+02/03/2020
+- fix: also add SameSite=None to by-value session cookies
+- bump to 2.4.2rc0
+
01/29/2020
- always add a SameSite value to the Set-Cookie header to satisfy upcoming Chrome/Firefox changes
this can be overridden by using, e.g.:
diff --git a/src/session.c b/src/session.c
index cd9ccb8..e7194bd 100644
--- a/src/session.c
+++ b/src/session.c
@@ -249,7 +249,7 @@ static apr_byte_t oidc_session_save_cookie(request_rec *r, oidc_session_t *z,
(first_time ?
OIDC_COOKIE_EXT_SAME_SITE_LAX :
OIDC_COOKIE_EXT_SAME_SITE_STRICT) :
- NULL);
+ OIDC_COOKIE_EXT_SAME_SITE_NONE);
return TRUE;
}
--
2.26.2

32
SOURCES/0018-add-note-on-usage-of-OIDC_SET_COOKIE_APPEND-in-the-s.patch
View File

@ -1,32 +0,0 @@
From d2f6572e93446d611fc66cf68d0b71cd13366d55 Mon Sep 17 00:00:00 2001
From: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
Date: Thu, 30 Jul 2020 10:10:04 +0200
Subject: [PATCH 18/19] add note on usage of OIDC_SET_COOKIE_APPEND in the
sample config/doc
Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
(cherry picked from commit bcbdd1993e7449446cb34df696826bd8bc9d2977)
---
auth_openidc.conf | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/auth_openidc.conf b/auth_openidc.conf
index 4012df3..ce2fba7 100644
--- a/auth_openidc.conf
+++ b/auth_openidc.conf
@@ -431,6 +431,12 @@
# state cookie: Lax
# session cookie: first time set Lax, updates (e.g. after inactivity timeout) Strict
# x_csrf discovery: Strict:
+#
+# The default `SameSite=None` cookie appendix on `Set-Cookie` response headers can be
+# conditionally overridden using an environment variable in the Apache config as in:
+# SetEnvIf User-Agent ".*IOS.*" OIDC_SET_COOKIE_APPEND=;
+# (since version 2.4.1)
+#
# When not defined the default is Off.
#OIDCCookieSameSite [On|Off]
--
2.26.2

204
SOURCES/0019-add-SameSite-attribute-on-cookie-clearance-logout.patch
View File

@ -1,204 +0,0 @@
From b68ac577b465f54e7eb38e48a65e5cf4c88c74c5 Mon Sep 17 00:00:00 2001
From: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
Date: Thu, 3 Sep 2020 16:52:30 +0200
Subject: [PATCH 19/19] add SameSite attribute on cookie clearance / logout
bump to 2.4.4.1; thanks @v0gler
Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
(cherry picked from commit 314000d179c2d08af6897725f37980e1f0891aa6)
---
ChangeLog | 4 ++++
src/mod_auth_openidc.c | 42 +++++++++++++++++++++++-------------------
src/mod_auth_openidc.h | 5 +++++
src/session.c | 16 +++++++++-------
4 files changed, 41 insertions(+), 26 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 3db7110..eba2ebc 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+09/03/2020
+- add SameSite attribute on cookie clearance / logout; thanks @v0gler
+- bump to 2.4.4.1
+
02/03/2020
- fix: also add SameSite=None to by-value session cookies
- bump to 2.4.2rc0
diff --git a/src/mod_auth_openidc.c b/src/mod_auth_openidc.c
index 0d2b37c..68fa2ce 100644
--- a/src/mod_auth_openidc.c
+++ b/src/mod_auth_openidc.c
@@ -717,7 +717,8 @@ static int oidc_delete_oldest_state_cookies(request_rec *r,
oidc_warn(r,
"deleting oldest state cookie: %s (time until expiry " APR_TIME_T_FMT " seconds)",
oldest->name, apr_time_sec(oldest->timestamp - apr_time_now()));
- oidc_util_set_cookie(r, oldest->name, "", 0, NULL);
+ oidc_util_set_cookie(r, oldest->name, "", 0,
+ OIDC_COOKIE_EXT_SAME_SITE_NONE);
if (prev_oldest)
prev_oldest->next = oldest->next;
else
@@ -762,7 +763,7 @@ static int oidc_clean_expired_state_cookies(request_rec *r, oidc_cfg *c,
oidc_error(r, "state (%s) has expired",
cookieName);
oidc_util_set_cookie(r, cookieName, "", 0,
- NULL);
+ OIDC_COOKIE_EXT_SAME_SITE_NONE);
} else {
if (first == NULL) {
first = apr_pcalloc(r->pool,
@@ -779,6 +780,12 @@ static int oidc_clean_expired_state_cookies(request_rec *r, oidc_cfg *c,
number_of_valid_state_cookies++;
}
oidc_proto_state_destroy(proto_state);
+ } else {
+ oidc_warn(r,
+ "state cookie could not be retrieved/decoded, deleting: %s",
+ cookieName);
+ oidc_util_set_cookie(r, cookieName, "", 0,
+ OIDC_COOKIE_EXT_SAME_SITE_NONE);
}
}
}
@@ -816,7 +823,7 @@ static apr_byte_t oidc_restore_proto_state(request_rec *r, oidc_cfg *c,
}
/* clear state cookie because we don't need it anymore */
- oidc_util_set_cookie(r, cookieName, "", 0, NULL);
+ oidc_util_set_cookie(r, cookieName, "", 0, OIDC_COOKIE_EXT_SAME_SITE_NONE);
*proto_state = oidc_proto_state_from_cookie(r, c, cookieValue);
if (*proto_state == NULL)
@@ -916,9 +923,7 @@ static int oidc_authorization_request_set_cookie(request_rec *r, oidc_cfg *c,
/* set it as a cookie */
oidc_util_set_cookie(r, cookieName, cookieValue, -1,
- c->cookie_same_site ?
- OIDC_COOKIE_EXT_SAME_SITE_LAX :
- OIDC_COOKIE_EXT_SAME_SITE_NONE);
+ OIDC_COOKIE_SAMESITE_LAX(c));
return HTTP_OK;
}
@@ -2183,9 +2188,7 @@ static int oidc_discovery(request_rec *r, oidc_cfg *cfg) {
/* set CSRF cookie */
oidc_util_set_cookie(r, OIDC_CSRF_NAME, csrf, -1,
- cfg->cookie_same_site ?
- OIDC_COOKIE_EXT_SAME_SITE_STRICT :
- OIDC_COOKIE_EXT_SAME_SITE_NONE);
+ OIDC_COOKIE_SAMESITE_STRICT(cfg));
/* see if we need to preserve POST parameters through Javascript/HTML5 storage */
if (oidc_post_preserve_javascript(r, url, NULL, NULL) == TRUE)
@@ -2278,9 +2281,7 @@ static int oidc_discovery(request_rec *r, oidc_cfg *cfg) {
s = apr_psprintf(r->pool, "%s</form>\n", s);
oidc_util_set_cookie(r, OIDC_CSRF_NAME, csrf, -1,
- cfg->cookie_same_site ?
- OIDC_COOKIE_EXT_SAME_SITE_STRICT :
- OIDC_COOKIE_EXT_SAME_SITE_NONE);
+ OIDC_COOKIE_SAMESITE_STRICT(cfg));
char *javascript = NULL, *javascript_method = NULL;
char *html_head =
@@ -2501,7 +2502,8 @@ static int oidc_handle_discovery_response(request_rec *r, oidc_cfg *c) {
if (csrf_cookie) {
/* clean CSRF cookie */
- oidc_util_set_cookie(r, OIDC_CSRF_NAME, "", 0, NULL);
+ oidc_util_set_cookie(r, OIDC_CSRF_NAME, "", 0,
+ OIDC_COOKIE_EXT_SAME_SITE_NONE);
/* compare CSRF cookie value with query parameter value */
if ((csrf_query == NULL)
@@ -2639,12 +2641,12 @@ static int oidc_handle_logout_request(request_rec *r, oidc_cfg *c,
oidc_debug(r, "enter (url=%s)", url);
- /* if there's no remote_user then there's no (stored) session to kill */
- if (session->remote_user != NULL) {
-
- /* remove session state (cq. cache entry and cookie) */
- oidc_session_kill(r, session);
- }
+ /*
+ * remove session state (cq. cache entry and cookie)
+ * always clear the session cookie because the cookie may be not sent (but still in the browser)
+ * due to SameSite policies
+ */
+ oidc_session_kill(r, session);
/* see if this is the OP calling us */
if (oidc_is_front_channel_logout(url)) {
@@ -2661,6 +2663,8 @@ static int oidc_handle_logout_request(request_rec *r, oidc_cfg *c,
const char *accept = oidc_util_hdr_in_accept_get(r);
if ((apr_strnatcmp(url, OIDC_IMG_STYLE_LOGOUT_PARAM_VALUE) == 0)
|| ((accept) && strstr(accept, OIDC_CONTENT_TYPE_IMAGE_PNG))) {
+ // terminate with DONE instead of OK
+ // to avoid Apache returning auth/authz error 401 for the redirect URI
return oidc_util_http_send(r,
(const char *) &oidc_transparent_pixel,
sizeof(oidc_transparent_pixel), OIDC_CONTENT_TYPE_IMAGE_PNG,
diff --git a/src/mod_auth_openidc.h b/src/mod_auth_openidc.h
index 5f1a79a..6821d0c 100644
--- a/src/mod_auth_openidc.h
+++ b/src/mod_auth_openidc.h
@@ -215,6 +215,11 @@ APLOG_USE_MODULE(auth_openidc);
#define OIDC_COOKIE_EXT_SAME_SITE_STRICT "SameSite=Strict"
#define OIDC_COOKIE_EXT_SAME_SITE_NONE "SameSite=None"
+#define OIDC_COOKIE_SAMESITE_STRICT(c) \
+ c->cookie_same_site ? OIDC_COOKIE_EXT_SAME_SITE_STRICT : OIDC_COOKIE_EXT_SAME_SITE_NONE
+#define OIDC_COOKIE_SAMESITE_LAX(c) \
+ c->cookie_same_site ? OIDC_COOKIE_EXT_SAME_SITE_LAX : OIDC_COOKIE_EXT_SAME_SITE_NONE
+
/* https://tools.ietf.org/html/draft-ietf-tokbind-ttrp-01 */
#define OIDC_TB_CFG_PROVIDED_ENV_VAR "Sec-Provided-Token-Binding-ID"
diff --git a/src/session.c b/src/session.c
index e7194bd..539ea21 100644
--- a/src/session.c
+++ b/src/session.c
@@ -155,7 +155,7 @@ static apr_byte_t oidc_session_load_cache(request_rec *r, oidc_session_t *z) {
/* delete the session cookie */
oidc_util_set_cookie(r, oidc_cfg_dir_cookie(r), "", 0,
- NULL);
+ OIDC_COOKIE_EXT_SAME_SITE_NONE);
/* delete the cache entry */
rc = oidc_cache_set_session(r, z->uuid, NULL, 0);
/* clear the session */
@@ -208,7 +208,8 @@ static apr_byte_t oidc_session_save_cache(request_rec *r, oidc_session_t *z,
} else {
/* clear the cookie */
- oidc_util_set_cookie(r, oidc_cfg_dir_cookie(r), "", 0, NULL);
+ oidc_util_set_cookie(r, oidc_cfg_dir_cookie(r), "", 0,
+ OIDC_COOKIE_EXT_SAME_SITE_NONE);
/* remove the session from the cache */
rc = oidc_cache_set_session(r, z->uuid, NULL, 0);
@@ -245,11 +246,12 @@ static apr_byte_t oidc_session_save_cookie(request_rec *r, oidc_session_t *z,
oidc_util_set_chunked_cookie(r, oidc_cfg_dir_cookie(r), cookieValue,
c->persistent_session_cookie ? z->expiry : -1,
c->session_cookie_chunk_size,
- c->cookie_same_site ?
- (first_time ?
- OIDC_COOKIE_EXT_SAME_SITE_LAX :
- OIDC_COOKIE_EXT_SAME_SITE_STRICT) :
- OIDC_COOKIE_EXT_SAME_SITE_NONE);
+ (z->state == NULL) ? OIDC_COOKIE_EXT_SAME_SITE_NONE :
+ c->cookie_same_site ?
+ (first_time ?
+ OIDC_COOKIE_EXT_SAME_SITE_LAX :
+ OIDC_COOKIE_EXT_SAME_SITE_STRICT) :
+ OIDC_COOKIE_EXT_SAME_SITE_NONE);
return TRUE;
}
--
2.26.2

453
SOURCES/0020-prevent-open-redirect-on-refresh-token-requests-rele.patch
View File

@ -1,453 +0,0 @@
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

167
SOURCES/0021-prevent-XSS-and-open-redirect-on-OIDC-session-manage.patch
View File

@ -1,167 +0,0 @@
From a1b8e7aa92e5e624a5f90bb736c307dae22230a1 Mon Sep 17 00:00:00 2001
From: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
Date: Mon, 27 Jul 2020 19:35:29 +0200
Subject: [PATCH 2/4] prevent XSS and open redirect on OIDC session
managemement OP iframe
- apply OIDCRedirectURLsAllowed on the login_uri parameter
- thanks Andrew Brady
- bump to 2.4.4rc3
Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
(cherry picked from commit 51d997899afea6ea454abda49bd4cd41aa7c0cdc)
---
ChangeLog | 12 ++++++------
auth_openidc.conf | 3 ++-
configure.ac | 2 +-
src/mod_auth_openidc.c | 21 +++++++++++++++++----
4 files changed, 26 insertions(+), 12 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index eba2ebc..075f98d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -86,7 +86,7 @@
- bump to 2.3.5rc0
04/27/2018
-- avoid crash when a relative logout URL parameter is passed in; thanks Vivien Delenne
+- avoid crash when a relative logout URL parameter is passed in; thanks Vivien Delenne
- release 2.3.4
03/22/2018
@@ -258,7 +258,7 @@
- bump to 2.2.1rc6
05/18/2017
-- fix parse function of OIDCRequestObject configuration option; thanks @suttod
+- fix parse function of OIDCRequestObject configuration option; thanks @suttod
05/17/2017
- avoid crash when the X-Forwarded-Proto header is not correctly set by a reverse proxy in front of mod_auth_openidc
@@ -325,7 +325,7 @@
02/20/2017
- security fix: scrub headers for "AuthType oauth20"
-- release 2.1.6
+- release 2.1.6
02/15/2017
- improve logging of session max duration and session inactivity timeout
@@ -534,7 +534,7 @@
- bump to 1.9.0rc3
7/19/2016
-- add support for chunked session cookies; closes #153; thanks @glatzert
+- add support for chunked session cookies; closes #153; thanks @glatzert
- bump to 1.9.0rc2
7/9/2016
@@ -911,7 +911,7 @@
1/1/2015
- update copyright to 2015
-- use json_int_t (seconds) for "exp" and "iat" fields, instead of apr_time_t (microseconds)
+- use json_int_t (seconds) for "exp" and "iat" fields, instead of apr_time_t (microseconds)
- correct expiry debug printout
- bump to 1.7.2rc1
@@ -1191,7 +1191,7 @@
- support using a Bearer token on client registration calls
4/22/2014
-- match request and response type
+- match request and response type
- check at_hash value on "token id_token" implicit flow
- use shared memory caching by default
- release 1.2
diff --git a/auth_openidc.conf b/auth_openidc.conf
index 87685f6..75cdb8e 100644
--- a/auth_openidc.conf
+++ b/auth_openidc.conf
@@ -786,7 +786,8 @@
#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.:
+# other redirects such as the "return_to" value on refresh token requests, and the "login_uri" value
+# on session management based logins through the OP iframe, 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$
diff --git a/configure.ac b/configure.ac
index ad5ba0e..c61d117 100644
--- a/configure.ac
+++ b/configure.ac
@@ -91,7 +91,7 @@ HAVE_LIBJQ=0
AC_ARG_WITH(jq,
[ --with-jq=PATH location of your libjq installation])
-
+
if test -n "$with_jq"
then
JQ_CFLAGS="-I$with_jq/include"
diff --git a/src/mod_auth_openidc.c b/src/mod_auth_openidc.c
index 215ed5e..68fbca5 100644
--- a/src/mod_auth_openidc.c
+++ b/src/mod_auth_openidc.c
@@ -2688,7 +2688,8 @@ static int oidc_handle_logout_request(request_rec *r, oidc_cfg *c,
}
static apr_byte_t oidc_validate_redirect_url(request_rec *r, oidc_cfg *c,
- const char *url, char **err_str, char **err_desc) {
+ const char *url, apr_byte_t restrict_to_host, char **err_str,
+ char **err_desc) {
apr_uri_t uri;
const char *c_host = NULL;
apr_hash_index_t *hi = NULL;
@@ -2717,7 +2718,7 @@ static apr_byte_t oidc_validate_redirect_url(request_rec *r, oidc_cfg *c,
oidc_error(r, "%s: %s", *err_str, *err_desc);
return FALSE;
}
- } else if (uri.hostname != NULL) {
+ } else if ((uri.hostname != NULL) && (restrict_to_host == TRUE)) {
c_host = oidc_get_current_url_host(r);
if ((strstr(c_host, uri.hostname) == NULL)
|| (strstr(uri.hostname, c_host) == NULL)) {
@@ -2792,7 +2793,7 @@ static int oidc_handle_logout(request_rec *r, oidc_cfg *c,
} else {
/* do input validation on the logout parameter value */
- if (oidc_validate_redirect_url(r, c, url, &error_str,
+ if (oidc_validate_redirect_url(r, c, url, TRUE, &error_str,
&error_description) == FALSE) {
return oidc_util_html_send_error(r, c->error_template, error_str,
error_description,
@@ -2948,6 +2949,18 @@ static int oidc_handle_session_management_iframe_rp(request_rec *r, oidc_cfg *c,
if (s_poll_interval == NULL)
s_poll_interval = "3000";
+ int poll_interval = s_poll_interval ? strtol(s_poll_interval, NULL, 10) : 0;
+ if ((poll_interval <= 0) || (poll_interval > 3600 * 24))
+ poll_interval = 3000;
+
+ char *login_uri = NULL, *error_str = NULL, *error_description = NULL;
+ oidc_util_get_request_parameter(r, "login_uri", &login_uri);
+ if ((login_uri != NULL)
+ && (oidc_validate_redirect_url(r, c, login_uri, FALSE, &error_str,
+ &error_description) == FALSE)) {
+ return HTTP_BAD_REQUEST;
+ }
+
const char *redirect_uri = oidc_get_redirect_uri(r, c);
java_script = apr_psprintf(r->pool, java_script, origin, client_id,
session_state, op_iframe_id, s_poll_interval, redirect_uri,
@@ -3061,7 +3074,7 @@ static int oidc_handle_refresh_token_request(request_rec *r, oidc_cfg *c,
}
/* do input validation on the return to parameter value */
- if (oidc_validate_redirect_url(r, c, return_to, &error_str,
+ if (oidc_validate_redirect_url(r, c, return_to, TRUE, &error_str,
&error_description) == FALSE) {
oidc_error(r, "return_to URL validation failed: %s: %s", error_str,
error_description);
--
2.31.1

42
SOURCES/0022-replace-potentially-harmful-backslashes-with-forward.patch
View File

@ -1,42 +0,0 @@
From 466f470265554e0e3ae27a6d82375456d2c133e6 Mon Sep 17 00:00:00 2001
From: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
Date: Thu, 22 Jul 2021 15:32:12 +0200
Subject: [PATCH 3/4] replace potentially harmful backslashes with forward
slashes when validating redirection URLs
(cherry picked from commit 69cb206225c749b51db980d44dc268eee5623f2b)
---
src/mod_auth_openidc.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/src/mod_auth_openidc.c b/src/mod_auth_openidc.c
index 68fbca5..c96af75 100644
--- a/src/mod_auth_openidc.c
+++ b/src/mod_auth_openidc.c
@@ -2687,12 +2687,22 @@ static int oidc_handle_logout_request(request_rec *r, oidc_cfg *c,
return HTTP_MOVED_TEMPORARILY;
}
+
+#define OIDC_MAX_URL_LENGTH DEFAULT_LIMIT_REQUEST_LINE * 2
+
static apr_byte_t oidc_validate_redirect_url(request_rec *r, oidc_cfg *c,
- const char *url, apr_byte_t restrict_to_host, char **err_str,
+ const char *redirect_to_url, apr_byte_t restrict_to_host, char **err_str,
char **err_desc) {
apr_uri_t uri;
const char *c_host = NULL;
apr_hash_index_t *hi = NULL;
+ size_t i = 0;
+ char *url = apr_pstrndup(r->pool, redirect_to_url, OIDC_MAX_URL_LENGTH);
+
+ // replace potentially harmful backslashes with forward slashes
+ for (i = 0; i < strlen(url); i++)
+ if (url[i] == '\\')
+ url[i] = '/';
if (apr_uri_parse(r->pool, url, &uri) != APR_SUCCESS) {
*err_str = apr_pstrdup(r->pool, "Malformed URL");
--
2.31.1

306
SOURCES/0023-apply-OIDCRedirectURLsAllowed-setting-to-target_link.patch
View File

@ -1,306 +0,0 @@
From eac10ec54971ff021113f96c80deab9991faafc4 Mon Sep 17 00:00:00 2001
From: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
Date: Fri, 3 Sep 2021 10:41:21 +0200
Subject: [PATCH 4/4] apply OIDCRedirectURLsAllowed setting to target_link_uri
closes #672; thanks @Meheni
release 2.4.9.4
Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
(cherry picked from commit 03e6bfb446f4e3f27c003d30d6a433e5dd8e2b3d)
---
AUTHORS | 31 +++++++
auth_openidc.conf | 5 +-
src/mod_auth_openidc.c | 193 +++++++++++++++++++++--------------------
3 files changed, 135 insertions(+), 94 deletions(-)
diff --git a/AUTHORS b/AUTHORS
index 1d91449..4f77eab 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -46,3 +46,34 @@ reporting bugs, providing fixes, suggesting useful features or other:
timpuri <https://github.com/timpuri>
Eldar Zaitov <https://github.com/kyprizel>
Gergan Penkov <https://github.com/gergan>
+ Florian Weimer <https://github.com/fweimer>
+ Aaron Donovan <https://github.com/amdonov>
+ Hans Petter Bieker <https://github.com/hpbieker>
+ archzone <https://github.com/archzone>
+ Petteri Stenius <https://github.com/psteniusubi>
+ Lance Fannin <lancekf08@gmail.com>
+ Ricardo Martin Camarero <https://github.com/rickyepoderi>
+ Filip Vujicic <https://github.com/FilipVujicic>
+ Janusz Ulanowski <https://github.com/janul>
+ Aimoto Norihito <https://github.com/oss-aimoto>
+ Andy Lindeman <https://github.com/alindeman>
+ Stefan Wachter <https://github.com/swachter>
+ Paolo Battino
+ absynth76 <https://github.com/absynth76>
+ Aaron Jones <https://github.com/wwaaron>
+ Bryan Ingram <https://github/bcingram>
+ Tim Deisser <https://github.com/deisser>
+ Peter Hurtenbach <https://github.com/Peter0x48>
+ Paul Spangler <https://github.com/spanglerco>
+ Chris Pawling <https://github.com/chris468>
+ Matthias Fleschütz <https://github.com/blindzero>
+ Harri Rautila <https://github.com/hrautila>
+ Tatsuhiko Yasumatsu <https://github.com/ty60>
+ Adam Stadler <https://github.com/tzfx>
+ Steffen Greber <https://github.com/codemaker219>
+ Iain Heggie <https://github.com/iainh>
+ Dirk Kok <https://github.com/Foxite>
+ Meheni https://github.com/Meheni
+
+
+
diff --git a/auth_openidc.conf b/auth_openidc.conf
index 75cdb8e..55febe5 100644
--- a/auth_openidc.conf
+++ b/auth_openidc.conf
@@ -786,8 +786,9 @@
#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, and the "login_uri" value
-# on session management based logins through the OP iframe, e.g.:
+# other redirects such as the "return_to" value on refresh token requests, the "login_uri" value
+# on session management based logins through the OP iframe, and the "target_link_uri" parameter in
+# 3rd-party initiated logins, 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$
diff --git a/src/mod_auth_openidc.c b/src/mod_auth_openidc.c
index c96af75..4cc7976 100644
--- a/src/mod_auth_openidc.c
+++ b/src/mod_auth_openidc.c
@@ -2476,6 +2476,96 @@ static int oidc_target_link_uri_matches_configuration(request_rec *r,
return TRUE;
}
+#define OIDC_MAX_URL_LENGTH 8192 * 2
+
+static apr_byte_t oidc_validate_redirect_url(request_rec *r, oidc_cfg *c,
+ const char *redirect_to_url, apr_byte_t restrict_to_host, char **err_str,
+ char **err_desc) {
+ apr_uri_t uri;
+ const char *c_host = NULL;
+ apr_hash_index_t *hi = NULL;
+ size_t i = 0;
+ char *url = apr_pstrndup(r->pool, redirect_to_url, OIDC_MAX_URL_LENGTH);
+
+ // replace potentially harmful backslashes with forward slashes
+ for (i = 0; i < strlen(url); i++)
+ if (url[i] == '\\')
+ url[i] = '/';
+
+ 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) && (restrict_to_host == TRUE)) {
+ 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;
+}
+
/*
* handle a response from an IDP discovery page and/or handle 3rd-party initiated SSO
*/
@@ -2486,6 +2576,8 @@ static int oidc_handle_discovery_response(request_rec *r, oidc_cfg *c) {
*auth_request_params = NULL, *csrf_cookie, *csrf_query = NULL,
*user = NULL, *path_scopes;
oidc_provider_t *provider = NULL;
+ char *error_str = NULL;
+ char *error_description = NULL;
oidc_util_get_request_parameter(r, OIDC_DISC_OP_PARAM, &issuer);
oidc_util_get_request_parameter(r, OIDC_DISC_USER_PARAM, &user);
@@ -2529,7 +2621,7 @@ static int oidc_handle_discovery_response(request_rec *r, oidc_cfg *c) {
target_link_uri = c->default_sso_url;
}
- /* do open redirect prevention */
+ /* do open redirect prevention, step 1 */
if (oidc_target_link_uri_matches_configuration(r, c, target_link_uri)
== FALSE) {
return oidc_util_html_send_error(r, c->error_template,
@@ -2538,6 +2630,14 @@ static int oidc_handle_discovery_response(request_rec *r, oidc_cfg *c) {
HTTP_UNAUTHORIZED);
}
+ /* do input validation on the target_link_uri parameter value, step 2 */
+ if (oidc_validate_redirect_url(r, c, target_link_uri, TRUE, &error_str,
+ &error_description) == FALSE) {
+ return oidc_util_html_send_error(r, c->error_template, error_str,
+ error_description,
+ HTTP_UNAUTHORIZED);
+ }
+
/* see if this is a static setup */
if (c->metadata_dir == NULL) {
if ((oidc_provider_static_config(r, c, &provider) == TRUE)
@@ -2687,97 +2787,6 @@ static int oidc_handle_logout_request(request_rec *r, oidc_cfg *c,
return HTTP_MOVED_TEMPORARILY;
}
-
-#define OIDC_MAX_URL_LENGTH DEFAULT_LIMIT_REQUEST_LINE * 2
-
-static apr_byte_t oidc_validate_redirect_url(request_rec *r, oidc_cfg *c,
- const char *redirect_to_url, apr_byte_t restrict_to_host, char **err_str,
- char **err_desc) {
- apr_uri_t uri;
- const char *c_host = NULL;
- apr_hash_index_t *hi = NULL;
- size_t i = 0;
- char *url = apr_pstrndup(r->pool, redirect_to_url, OIDC_MAX_URL_LENGTH);
-
- // replace potentially harmful backslashes with forward slashes
- for (i = 0; i < strlen(url); i++)
- if (url[i] == '\\')
- url[i] = '/';
-
- 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) && (restrict_to_host == TRUE)) {
- 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;
-}
-
/*
* perform (single) logout
*/
--
2.31.1

406
SOURCES/0024-use-encrypted-JWTs-for-storing-encrypted-cache-conte.patch
View File

@ -1,406 +0,0 @@
From 3c4949e1436473644836f0c4634b809c7526c43a Mon Sep 17 00:00:00 2001
From: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
Date: Thu, 10 Jun 2021 15:32:48 +0200
Subject: [PATCH 1/3] use encrypted JWTs for storing encrypted cache contents
- avoid using static AAD/IV; thanks @niebardzo
- bump to 2.4.9-dev
Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
---
ChangeLog | 4 +
src/cache/common.c | 331 ++++----------------------------------
diff --git a/ChangeLog b/ChangeLog
index 075f98d..b4ab0a1 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+06/10/2021
+- use encrypted JWTs for storing encrypted cache contents and avoid using static AAD/IV; thanks @niebardzo
+- bump to 2.4.9-dev
+
09/03/2020
- add SameSite attribute on cookie clearance / logout; thanks @v0gler
- bump to 2.4.4.1
diff --git a/src/cache/common.c b/src/cache/common.c
index c33876a..e665370 100644
--- a/src/cache/common.c
+++ b/src/cache/common.c
@@ -229,324 +229,59 @@ apr_byte_t oidc_cache_mutex_destroy(server_rec *s, oidc_cache_mutex_t *m) {
return rv;
}
-#define oidc_cache_crypto_openssl_error(r, fmt, ...) \
- oidc_error(r, "%s: %s", apr_psprintf(r->pool, fmt, ##__VA_ARGS__), ERR_error_string(ERR_get_error(), NULL))
-
-#define OIDC_CACHE_CIPHER EVP_aes_256_gcm()
-#define OIDC_CACHE_TAG_LEN 16
-
-#if (OPENSSL_VERSION_NUMBER >= 0x10100005L && !defined(LIBRESSL_VERSION_NUMBER))
-#define OIDC_CACHE_CRYPTO_GET_TAG EVP_CTRL_AEAD_GET_TAG
-#define OIDC_CACHE_CRYPTO_SET_TAG EVP_CTRL_AEAD_SET_TAG
-#define OIDC_CACHE_CRYPTO_SET_IVLEN EVP_CTRL_AEAD_SET_IVLEN
-#else
-#define OIDC_CACHE_CRYPTO_GET_TAG EVP_CTRL_GCM_GET_TAG
-#define OIDC_CACHE_CRYPTO_SET_TAG EVP_CTRL_GCM_SET_TAG
-#define OIDC_CACHE_CRYPTO_SET_IVLEN EVP_CTRL_GCM_SET_IVLEN
-#endif
+#define OIDC_CACHE_CRYPTO_JSON_KEY "c"
/*
- * AES GCM encrypt
+ * AES GCM encrypt using the crypto passphrase as symmetric key
*/
-static int oidc_cache_crypto_encrypt_impl(request_rec *r,
- unsigned char *plaintext, int plaintext_len, const unsigned char *aad,
- int aad_len, unsigned char *key, const unsigned char *iv, int iv_len,
- unsigned char *ciphertext, const unsigned char *tag, int tag_len) {
- EVP_CIPHER_CTX *ctx;
-
- int len;
-
- int ciphertext_len;
-
- /* create and initialize the context */
- if (!(ctx = EVP_CIPHER_CTX_new())) {
- oidc_cache_crypto_openssl_error(r, "EVP_CIPHER_CTX_new");
- return -1;
- }
-
- /* initialize the encryption cipher */
- if (!EVP_EncryptInit_ex(ctx, OIDC_CACHE_CIPHER, NULL, NULL, NULL)) {
- oidc_cache_crypto_openssl_error(r, "EVP_EncryptInit_ex");
- return -1;
- }
-
- /* set IV length */
- if (!EVP_CIPHER_CTX_ctrl(ctx, OIDC_CACHE_CRYPTO_SET_IVLEN, iv_len, NULL)) {
- oidc_cache_crypto_openssl_error(r, "EVP_CIPHER_CTX_ctrl");
- return -1;
- }
-
- /* initialize key and IV */
- if (!EVP_EncryptInit_ex(ctx, NULL, NULL, key, iv)) {
- oidc_cache_crypto_openssl_error(r, "EVP_EncryptInit_ex");
- return -1;
- }
-
- /* provide AAD data */
- if (!EVP_EncryptUpdate(ctx, NULL, &len, aad, aad_len)) {
- oidc_cache_crypto_openssl_error(r, "EVP_DecryptUpdate aad: aad_len=%d",
- aad_len);
- return -1;
- }
-
- /* provide the message to be encrypted and obtain the encrypted output */
- if (!EVP_EncryptUpdate(ctx, ciphertext, &len, plaintext, plaintext_len)) {
- oidc_cache_crypto_openssl_error(r, "EVP_EncryptUpdate ciphertext");
- return -1;
- }
- ciphertext_len = len;
-
- /*
- * finalize the encryption; normally ciphertext bytes may be written at
- * this stage, but this does not occur in GCM mode
- */
- if (!EVP_EncryptFinal_ex(ctx, ciphertext + len, &len)) {
- oidc_cache_crypto_openssl_error(r, "EVP_EncryptFinal_ex");
- return -1;
- }
- ciphertext_len += len;
-
- /* get the tag */
- if (!EVP_CIPHER_CTX_ctrl(ctx, OIDC_CACHE_CRYPTO_GET_TAG, tag_len,
- (void *) tag)) {
- oidc_cache_crypto_openssl_error(r, "EVP_CIPHER_CTX_ctrl");
- return -1;
- }
-
- /* clean up */
- EVP_CIPHER_CTX_free(ctx);
-
- return ciphertext_len;
-}
-
-/*
- * AES GCM decrypt
- */
-static int oidc_cache_crypto_decrypt_impl(request_rec *r,
- unsigned char *ciphertext, int ciphertext_len, const unsigned char *aad,
- int aad_len, const unsigned char *tag, int tag_len, unsigned char *key,
- const unsigned char *iv, int iv_len, unsigned char *plaintext) {
- EVP_CIPHER_CTX *ctx;
- int len;
- int plaintext_len;
- int ret;
-
- /* create and initialize the context */
- if (!(ctx = EVP_CIPHER_CTX_new())) {
- oidc_cache_crypto_openssl_error(r, "EVP_CIPHER_CTX_new");
- return -1;
- }
-
- /* initialize the decryption cipher */
- if (!EVP_DecryptInit_ex(ctx, OIDC_CACHE_CIPHER, NULL, NULL, NULL)) {
- oidc_cache_crypto_openssl_error(r, "EVP_DecryptInit_ex");
- return -1;
- }
-
- /* set IV length */
- if (!EVP_CIPHER_CTX_ctrl(ctx, OIDC_CACHE_CRYPTO_SET_IVLEN, iv_len, NULL)) {
- oidc_cache_crypto_openssl_error(r, "EVP_CIPHER_CTX_ctrl");
- return -1;
- }
-
- /* initialize key and IV */
- if (!EVP_DecryptInit_ex(ctx, NULL, NULL, key, iv)) {
- oidc_cache_crypto_openssl_error(r, "EVP_DecryptInit_ex");
- return -1;
- }
-
- /* provide AAD data */
- if (!EVP_DecryptUpdate(ctx, NULL, &len, aad, aad_len)) {
- oidc_cache_crypto_openssl_error(r, "EVP_DecryptUpdate aad: aad_len=%d",
- aad_len);
- return -1;
- }
-
- /* provide the message to be decrypted and obtain the plaintext output */
- if (!EVP_DecryptUpdate(ctx, plaintext, &len, ciphertext, ciphertext_len)) {
- oidc_cache_crypto_openssl_error(r, "EVP_DecryptUpdate ciphertext");
- return -1;
- }
- plaintext_len = len;
-
- /* set expected tag value; works in OpenSSL 1.0.1d and later */
- if (!EVP_CIPHER_CTX_ctrl(ctx, OIDC_CACHE_CRYPTO_SET_TAG, tag_len,
- (void *) tag)) {
- oidc_cache_crypto_openssl_error(r, "EVP_CIPHER_CTX_ctrl");
- return -1;
- }
+static apr_byte_t oidc_cache_crypto_encrypt(request_rec *r, const char *plaintext, const char *key,
+ char **result) {
+ apr_byte_t rv = FALSE;
+ json_t *json = NULL;
- /*
- * finalize the decryption; a positive return value indicates success,
- * anything else is a failure - the plaintext is not trustworthy
- */
- ret = EVP_DecryptFinal_ex(ctx, plaintext + len, &len);
+ json = json_object();
+ json_object_set_new(json, OIDC_CACHE_CRYPTO_JSON_KEY, json_string(plaintext));
- /* clean up */
- EVP_CIPHER_CTX_free(ctx);
-
- if (ret > 0) {
- /* success */
- plaintext_len += len;
- return plaintext_len;
- } else {
- /* verify failed */
- oidc_cache_crypto_openssl_error(r, "EVP_DecryptFinal_ex");
- return -1;
- }
-}
+ rv = oidc_util_jwt_create(r, (const char*) key, json, result);
-/*
- * static AAD value for encryption/decryption
- */
-static const unsigned char OIDC_CACHE_CRYPTO_GCM_AAD[] = { 0x4d, 0x23, 0xc3,
- 0xce, 0xc3, 0x34, 0xb4, 0x9b, 0xdb, 0x37, 0x0c, 0x43, 0x7f, 0xec, 0x78,
- 0xde };
+ if (json)
+ json_decref(json);
-/*
- * static IV value for encryption/decryption
- */
-static const unsigned char OIDC_CACHE_CRYPTO_GCM_IV[] = { 0x00, 0x01, 0x02,
- 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e,
- 0x0f };
-
-/*
- * AES GCM encrypt using the static AAD and IV
- */
-static int oidc_cache_crypto_encrypt(request_rec *r, const char *plaintext,
- unsigned char *key, char **result) {
- char *encoded = NULL, *p = NULL, *e_tag = NULL;
- unsigned char *ciphertext = NULL;
- int plaintext_len, ciphertext_len, encoded_len, e_tag_len;
- unsigned char tag[OIDC_CACHE_TAG_LEN];
-
- /* allocate space for the ciphertext */
- plaintext_len = strlen(plaintext) + 1;
- ciphertext = apr_pcalloc(r->pool,
- (plaintext_len + EVP_CIPHER_block_size(OIDC_CACHE_CIPHER)));
-
- ciphertext_len = oidc_cache_crypto_encrypt_impl(r,
- (unsigned char *) plaintext, plaintext_len,
- OIDC_CACHE_CRYPTO_GCM_AAD, sizeof(OIDC_CACHE_CRYPTO_GCM_AAD), key,
- OIDC_CACHE_CRYPTO_GCM_IV, sizeof(OIDC_CACHE_CRYPTO_GCM_IV),
- ciphertext, tag, sizeof(tag));
-
- /* base64url encode the resulting ciphertext */
- encoded_len = oidc_base64url_encode(r, &encoded, (const char *) ciphertext,
- ciphertext_len, 1);
- if (encoded_len > 0) {
- p = encoded;
-
- /* now allocated space for the concatenated base64url encoded ciphertext and tag */
- encoded = apr_pcalloc(r->pool,
- encoded_len + 1 + OIDC_CACHE_TAG_LEN + 1);
- memcpy(encoded, p, encoded_len);
- p = encoded + encoded_len;
- *p = OIDC_CHAR_DOT;
- p++;
-
- /* base64url encode the tag and append it in the buffer */
- e_tag_len = oidc_base64url_encode(r, &e_tag, (const char *) tag,
- OIDC_CACHE_TAG_LEN, 1);
- memcpy(p, e_tag, e_tag_len);
- encoded_len += e_tag_len + 1;
-
- /* make sure the result is \0 terminated */
- encoded[encoded_len] = '\0';
-
- *result = encoded;
- }
-
- return encoded_len;
+ return rv;
}
/*
- * AES GCM decrypt using the static AAD and IV
+ * AES GCM decrypt using the crypto passphrase as symmetric key
*/
-static int oidc_cache_crypto_decrypt(request_rec *r, const char *cache_value,
- unsigned char *key, unsigned char **plaintext) {
+static apr_byte_t oidc_cache_crypto_decrypt(request_rec *r, const char *cache_value,
+ const char *key, char **plaintext) {
- int len = -1;
+ apr_byte_t rv = FALSE;
+ json_t *json = NULL;
- /* grab the base64url-encoded tag after the "." */
- char *encoded_tag = strstr(cache_value, ".");
- if (encoded_tag == NULL) {
- oidc_error(r,
- "corrupted cache value: no tag separator found in encrypted value");
- return FALSE;
- }
+ rv = oidc_util_jwt_verify(r, (const char*) key, cache_value, &json);
+ if (rv == FALSE)
+ goto end;
- /* make sure we don't modify the original string since it may be just a pointer into the cache (shm) */
- cache_value = apr_pstrmemdup(r->pool, cache_value,
- strlen(cache_value) - strlen(encoded_tag));
- encoded_tag++;
+ rv = oidc_json_object_get_string(r->pool, json, OIDC_CACHE_CRYPTO_JSON_KEY, plaintext, NULL);
- /* base64url decode the ciphertext */
- char *d_bytes = NULL;
- int d_len = oidc_base64url_decode(r->pool, &d_bytes, cache_value);
+ end:
- /* base64url decode the tag */
- char *t_bytes = NULL;
- int t_len = oidc_base64url_decode(r->pool, &t_bytes, encoded_tag);
+ if (json)
+ json_decref(json);
- /* see if we're still good to go */
- if ((d_len > 0) && (t_len > 0)) {
-
- /* allocated space for the plaintext */
- *plaintext = apr_pcalloc(r->pool,
- (d_len + EVP_CIPHER_block_size(OIDC_CACHE_CIPHER) - 1));
-
- /* decrypt the ciphertext providing the tag value */
-
- len = oidc_cache_crypto_decrypt_impl(r, (unsigned char *) d_bytes,
- d_len, OIDC_CACHE_CRYPTO_GCM_AAD,
- sizeof(OIDC_CACHE_CRYPTO_GCM_AAD), (unsigned char *) t_bytes,
- t_len, key, OIDC_CACHE_CRYPTO_GCM_IV,
- sizeof(OIDC_CACHE_CRYPTO_GCM_IV), *plaintext);
-
- /* check the result and make sure it is \0 terminated */
- if (len > -1) {
- (*plaintext)[len] = '\0';
- } else {
- *plaintext = NULL;
- }
-
- }
-
- return len;
-}
-
-/*
- * hash the crypto passhphrase so it has enough key length for AES GCM 256
- */
-static unsigned char *oidc_cache_hash_passphrase(request_rec *r,
- const char *passphrase) {
-
- unsigned char *key = NULL;
- unsigned int key_len = 0;
- oidc_jose_error_t err;
-
- if (oidc_jose_hash_bytes(r->pool, OIDC_JOSE_ALG_SHA256,
- (const unsigned char *) passphrase, strlen(passphrase), &key,
- &key_len, &err) == FALSE) {
- oidc_error(r, "oidc_jose_hash_bytes returned an error: %s", err.text);
- return NULL;
- }
-
- return key;
+ return rv;
}
/*
* hash a cache key and a crypto passphrase so the result is suitable as an randomized cache key
*/
-static char *oidc_cache_get_hashed_key(request_rec *r, const char *passphrase,
- const char *key) {
+static char* oidc_cache_get_hashed_key(request_rec *r, const char *passphrase, const char *key) {
char *input = apr_psprintf(r->pool, "%s:%s", passphrase, key);
char *output = NULL;
- if (oidc_util_hash_string_and_base64url_encode(r, OIDC_JOSE_ALG_SHA256,
- input, &output) == FALSE) {
- oidc_error(r,
- "oidc_util_hash_string_and_base64url_encode returned an error");
+ if (oidc_util_hash_string_and_base64url_encode(r, OIDC_JOSE_ALG_SHA256, input, &output)
+ == FALSE) {
+ oidc_error(r, "oidc_util_hash_string_and_base64url_encode returned an error");
return NULL;
}
return output;
@@ -588,9 +323,7 @@ apr_byte_t oidc_cache_get(request_rec *r, const char *section, const char *key,
goto out;
}
- rc = (oidc_cache_crypto_decrypt(r, cache_value,
- oidc_cache_hash_passphrase(r, cfg->crypto_passphrase),
- (unsigned char **) value) > 0);
+ rc = oidc_cache_crypto_decrypt(r, cache_value, cfg->crypto_passphrase, value);
out:
/* log the result */
@@ -634,9 +367,7 @@ apr_byte_t oidc_cache_set(request_rec *r, const char *section, const char *key,
goto out;
if (value != NULL) {
- if (oidc_cache_crypto_encrypt(r, value,
- oidc_cache_hash_passphrase(r, cfg->crypto_passphrase),
- &encoded) <= 0)
+ if (oidc_cache_crypto_encrypt(r, value, cfg->crypto_passphrase, &encoded) == FALSE)
goto out;
value = encoded;
}

41
SOURCES/0025-avoid-XSS-vulnerability-when-using-OIDCPreservePost-.patch
View File

@ -1,41 +0,0 @@
From dad95a3ca050910d44ff346edead722e341417ef Mon Sep 17 00:00:00 2001
From: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
Date: Fri, 25 Jun 2021 11:42:57 +0200
Subject: [PATCH 2/3] avoid XSS vulnerability when using OIDCPreservePost On
and supplying URLs that contain single quotes; thanks @oss-aimoto
Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
---
ChangeLog | 4 ++++
src/mod_auth_openidc.c | 2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/ChangeLog b/ChangeLog
index b4ab0a1..7054f0b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+06/25/2021
+- avoid XSS vulnerability when using OIDCPreservePost On and supplying URLs that contain single quotes
+ thanks @oss-aimoto
+
06/10/2021
- use encrypted JWTs for storing encrypted cache contents and avoid using static AAD/IV; thanks @niebardzo
- bump to 2.4.9-dev
diff --git a/src/mod_auth_openidc.c b/src/mod_auth_openidc.c
index 4cc7976..ea84e5e 100644
--- a/src/mod_auth_openidc.c
+++ b/src/mod_auth_openidc.c
@@ -519,7 +519,7 @@ static int oidc_request_post_preserved_restore(request_rec *r,
" input.type = \"hidden\";\n"
" document.forms[0].appendChild(input);\n"
" }\n"
- " document.forms[0].action = '%s';\n"
+ " document.forms[0].action = \"%s\";\n"
" document.forms[0].submit();\n"
" }\n"
" </script>\n", method, original_url);
--
2.27.0

149
SOURCES/0026-Add-a-function-to-escape-Javascript-characters.patch
View File

@ -1,149 +0,0 @@
From 93b81054d4d1ece64a6799cc50a65b0daeabf4d1 Mon Sep 17 00:00:00 2001
From: AIMOTO NORIHITO <aimoto@osstech.co.jp>
Date: Mon, 28 Jun 2021 13:05:52 +0900
Subject: [PATCH 3/3] Add a function to escape Javascript characters
---
src/mod_auth_openidc.c | 6 ++--
src/mod_auth_openidc.h | 1 +
src/util.c | 81 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 85 insertions(+), 3 deletions(-)
diff --git a/src/mod_auth_openidc.c b/src/mod_auth_openidc.c
index ea84e5e..37743b3 100644
--- a/src/mod_auth_openidc.c
+++ b/src/mod_auth_openidc.c
@@ -474,7 +474,7 @@ apr_byte_t oidc_post_preserve_javascript(request_rec *r, const char *location,
" </script>\n", jmethod, json,
location ?
apr_psprintf(r->pool, "window.location='%s';\n",
- location) :
+ oidc_util_javascript_escape(r->pool, location)) :
"");
if (location == NULL) {
if (javascript_method)
@@ -522,7 +522,7 @@ static int oidc_request_post_preserved_restore(request_rec *r,
" document.forms[0].action = \"%s\";\n"
" document.forms[0].submit();\n"
" }\n"
- " </script>\n", method, original_url);
+ " </script>\n", method, oidc_util_javascript_escape(r->pool, original_url));
const char *body = " <p>Restoring...</p>\n"
" <form method=\"post\"></form>\n";
@@ -1626,7 +1626,7 @@ static int oidc_session_redirect_parent_window_to_logout(request_rec *r,
char *java_script = apr_psprintf(r->pool,
" <script type=\"text/javascript\">\n"
" window.top.location.href = '%s?session=logout';\n"
- " </script>\n", oidc_get_redirect_uri(r, c));
+ " </script>\n", oidc_util_javascript_escape(r->pool, oidc_get_redirect_uri(r, c)));
return oidc_util_html_send(r, "Redirecting...", java_script, NULL, NULL,
DONE);
diff --git a/src/mod_auth_openidc.h b/src/mod_auth_openidc.h
index ea79e6b..b88937a 100644
--- a/src/mod_auth_openidc.h
+++ b/src/mod_auth_openidc.h
@@ -747,6 +747,7 @@ apr_byte_t oidc_json_object_get_string(apr_pool_t *pool, json_t *json, const cha
apr_byte_t oidc_json_object_get_int(apr_pool_t *pool, json_t *json, const char *name, int *value, const int default_value);
apr_byte_t oidc_json_object_get_bool(apr_pool_t *pool, json_t *json, const char *name, int *value, const int default_value);
char *oidc_util_html_escape(apr_pool_t *pool, const char *input);
+char *oidc_util_javascript_escape(apr_pool_t *pool, const char *input);
void oidc_util_table_add_query_encoded_params(apr_pool_t *pool, apr_table_t *table, const char *params);
apr_hash_t * oidc_util_merge_key_sets(apr_pool_t *pool, apr_hash_t *k1, apr_hash_t *k2);
apr_byte_t oidc_util_regexp_substitute(apr_pool_t *pool, const char *input, const char *regexp, const char *replace, char **output, char **error_str);
diff --git a/src/util.c b/src/util.c
index 4b4e16b..f98c824 100644
--- a/src/util.c
+++ b/src/util.c
@@ -369,6 +369,87 @@ char *oidc_util_html_escape(apr_pool_t *pool, const char *s) {
return apr_pstrdup(pool, r);
}
+/*
+ * JavaScript escape a string
+ */
+char* oidc_util_javascript_escape(apr_pool_t *pool, const char *s) {
+ const char *cp;
+ char *output;
+ size_t outputlen;
+ int i;
+
+ if (s == NULL) {
+ return NULL;
+ }
+
+ outputlen = 0;
+ for (cp = s; *cp; cp++) {
+ switch (*cp) {
+ case '\'':
+ case '"':
+ case '\\':
+ case '/':
+ case 0x0D:
+ case 0x0A:
+ outputlen += 2;
+ break;
+ case '<':
+ case '>':
+ outputlen += 4;
+ break;
+ default:
+ outputlen += 1;
+ break;
+ }
+ }
+
+ i = 0;
+ output = apr_palloc(pool, outputlen + 1);
+ for (cp = s; *cp; cp++) {
+ switch (*cp) {
+ case '\'':
+ (void)strcpy(&output[i], "\\'");
+ i += 2;
+ break;
+ case '"':
+ (void)strcpy(&output[i], "\\\"");
+ i += 2;
+ break;
+ case '\\':
+ (void)strcpy(&output[i], "\\\\");
+ i += 2;
+ break;
+ case '/':
+ (void)strcpy(&output[i], "\\/");
+ i += 2;
+ break;
+ case 0x0D:
+ (void)strcpy(&output[i], "\\r");
+ i += 2;
+ break;
+ case 0x0A:
+ (void)strcpy(&output[i], "\\n");
+ i += 2;
+ break;
+ case '<':
+ (void)strcpy(&output[i], "\\x3c");
+ i += 4;
+ break;
+ case '>':
+ (void)strcpy(&output[i], "\\x3e");
+ i += 4;
+ break;
+ default:
+ output[i] = *cp;
+ i += 1;
+ break;
+ }
+ }
+ output[i] = '\0';
+ return output;
+}
+
+
/*
* get the URL scheme that is currently being accessed
*/
--
2.27.0

167
SOURCES/test-segfault.patch
View File

@ -1,167 +0,0 @@
commit fe7dfb14c45262df3b15bda374b2ee390b43cfb4
Author: John Dennis <jdennis@redhat.com>
Date: Tue Aug 14 18:08:56 2018 -0400
test_proto_authorization_request() segfault due to uninitialized value
Many thanks to Florian Weimer <fweimer@redhat.com> for his assistence
in helping diagnose the problem.
In test_proto_authorization_request() it creates a provider object but
fails to fully initialize it. In particular it fails to initialize
auth_request_method to OIDC_AUTH_REQUEST_METHOD_GET.
The function oidc_proto_authorization_request() in the file
src/proto.c has a weak test for provider->auth_request_method on line
646.
if (provider->auth_request_method == OIDC_AUTH_REQUEST_METHOD_POST) {
/* construct a HTML POST auto-submit page with the authorization request parameters */
} else {
/* construct the full authorization request URL */
}
The assumption is provider->auth_request_method must be one of
OIDC_AUTH_REQUEST_METHOD_GET or OIDC_AUTH_REQUEST_METHOD_POST but if
the provider struct is not properly initialized it could be any random
value. Therefore the fact provider->auth_request_method does not equal
OIDC_AUTH_REQUEST_METHOD_POST does not imply it's
OIDC_AUTH_REQUEST_METHOD_GET. The test would also be a problem if
OIDC_AUTH_REQUEST_METHOD ever added a new enumerated value.
The defined values for OIDC_AUTH_REQUEST_METHOD are:
define OIDC_AUTH_REQUEST_METHOD_GET 0
define OIDC_AUTH_REQUEST_METHOD_POST 1
So what the test on line src/proto.c:646 is really saying is this:
if provider->auth_request_method != 1 then use the GET method.
The unit test works most of the time because it's unlikely the
unitialized auth_request_method member will be exactly 1. Except on
some architectures it is (e.g. s390x). Of course what's on the stack
is influenced by a variety of factors (all unknown).
The segfault occurs because oidc_proto_authorization_request() takes
the OIDC_AUTH_REQUEST_METHOD_POST branch and calls
oidc_proto_html_post() which then operates on more uninitialized
data. Specfically request->connection->bucket_alloc is
NULL. Fortunately the request object was intialized to zero via
apr_pcalloc() so at least bucket_alloc will consistetnly be NULL.
Here is the stack trace:
Program received signal SIGSEGV, Segmentation fault.
0x00007ffff6b9f67a in apr_bucket_alloc () from /lib64/libaprutil-1.so.0
(gdb) bt
from /lib64/libaprutil-1.so.0
data=0x6adfe0 "<!DOCTYPE HTML PUBLIC \"-//W3C//DTD HTML 4.01//EN\" \"http://www.w3.org/TR/html4/strict.dtd\">\n<html>\n <head>\n <meta http-equiv=\"Content-Type\" content=\"text/html; charset=utf-8\">\n <title>Submitting"...,
data_len=825, content_type=0x47311a "text/html", success_rvalue=-2)
at src/util.c:1307
title=0x46bf28 "Submitting...", html_head=0x0,
on_load=0x46bf0d "document.forms[0].submit()",
html_body=0x6add40 " <p>Submitting Authentication Request...</p>\n <form method=\"post\" action=\"https://idp.example.com/authorize\">\n <p>\n <input type=\"hidden\" name=\"response_type\" value=\"code\">\n <input"..., status_code=-2) at src/util.c:1349
url=0x465758 "https://idp.example.com/authorize", params=0x6acf30)
at src/proto.c:544
provider=0x7fffffffd260, login_hint=0x0,
redirect_uri=0x465790 "https://www.example.com/protected/",
state=0x4657b3 "12345", proto_state=0x68e5f0, id_token_hint=0x0,
nge=0x0, auth_request_params=0x0, path_scope=0x0) at src/proto.c:650
This patch does the following:
1) Initializes the provider struct created on the stack in
test_proto_authorization_request to zero so it's at least
initialized to known consistent values.
2) Initializes provider.auth_request_method to
OIDC_AUTH_REQUEST_METHOD_GET.
3) Initializes request->connection->bucket_alloc via
apr_bucket_alloc_create() so if one does enter that code it won't
segfault.
It's easy to verify the above observations.
If you explicitly intialize provider.auth_request_method to
OIDC_AUTH_REQUEST_METHOD_POST in test_proto_authorization_request()
instead of allowing it to be a random stack value you will segfault
every time. However if you initialize request->connection->bucket_alloc
the segfault goes away and the unit test correctly reports the error,
e.g.
Failed: # test_proto_authorization_request: error in oidc_proto_authorization_request (1): result "0" != expected "1"
WARNING: This patch does not address the test in proto.c:646. That
test should be augmented to assure only valid enumerated values are
operated on and if the enumerated value is not valid it should return
an error.
Note: The above was fixed in the following commit.
Signed-off-by: John Dennis <jdennis@redhat.com>
diff --git a/test/test.c b/test/test.c
index 16f09b5..87d3700 100755
--- a/test/test.c
+++ b/test/test.c
@@ -1019,6 +1019,9 @@ static char *test_proto_validate_code(request_rec *r) {
static char * test_proto_authorization_request(request_rec *r) {
oidc_provider_t provider;
+
+ memset(&provider, 0, sizeof(provider));
+
provider.issuer = "https://idp.example.com";
provider.authorization_endpoint_url = "https://idp.example.com/authorize";
provider.scope = "openid";
@@ -1028,6 +1031,8 @@ static char * test_proto_authorization_request(request_rec *r) {
provider.auth_request_params = NULL;
provider.request_object = NULL;
provider.token_binding_policy = OIDC_TOKEN_BINDING_POLICY_OPTIONAL;
+ provider.auth_request_method = OIDC_AUTH_REQUEST_METHOD_GET;
+
const char *redirect_uri = "https://www.example.com/protected/";
const char *state = "12345";
@@ -1260,6 +1265,7 @@ static request_rec * test_setup(apr_pool_t *pool) {
sizeof(struct process_rec));
request->server->process->pool = request->pool;
request->connection = apr_pcalloc(request->pool, sizeof(struct conn_rec));
+ request->connection->bucket_alloc = apr_bucket_alloc_create(request->pool);
request->connection->local_addr = apr_pcalloc(request->pool,
sizeof(apr_sockaddr_t));
commit aca77a82c1ce2f1ec8f363066ffbc480b3bd75c8
Author: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
Date: Wed Aug 15 07:47:57 2018 +0200
add sanity check on provider->auth_request_method; closes #382
thanks @jdennis; bump to 2.3.8rc4
Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
diff --git a/src/proto.c b/src/proto.c
index e9dbc99..ac7696a 100644
--- a/src/proto.c
+++ b/src/proto.c
@@ -649,7 +649,7 @@ int oidc_proto_authorization_request(request_rec *r,
rv = oidc_proto_html_post(r, provider->authorization_endpoint_url,
params);
- } else {
+ } else if (provider->auth_request_method == OIDC_AUTH_REQUEST_METHOD_GET) {
/* construct the full authorization request URL */
authorization_request = oidc_util_http_query_encoded_url(r,
@@ -666,6 +666,10 @@ int oidc_proto_authorization_request(request_rec *r,
/* and tell Apache to return an HTTP Redirect (302) message */
rv = HTTP_MOVED_TEMPORARILY;
}
+ } else {
+ oidc_error(r, "provider->auth_request_method set to wrong value: %d",
+ provider->auth_request_method);
+ return HTTP_INTERNAL_SERVER_ERROR;
}
/* add a referred token binding request for the provider if enabled */

71
SPECS/mod_auth_openidc.spec
View File

@ -1,4 +1,4 @@
%{!?_httpd_mmn: %{expand: %%global _httpd_mmn %%(cat %{_includedir}/httpd/.mmn || echo 0-0)}}
%{!?_httpd_mmn: %{expand: %%global _httpd_mmn %%(cat %{_includedir}/httpd/.mmn 2>/dev/null || echo 0-0)}}
%{!?_httpd_moddir: %{expand: %%global _httpd_moddir %%{_libdir}/httpd/modules}}
%{!?_httpd_confdir: %{expand: %%global _httpd_confdir %{_sysconfdir}/httpd/conf.d}}
@ -14,41 +14,13 @@
%global httpd_pkg_cache_dir /var/cache/httpd/mod_auth_openidc
Name: mod_auth_openidc
Version: 2.3.7
Release: 11%{?dist}
Version: 2.4.9.4
Release: 1%{?dist}
Summary: OpenID Connect auth module for Apache HTTP Server
Group: System Environment/Daemons
License: ASL 2.0
URL: https://github.com/zmartzone/mod_auth_openidc
Source0: https://github.com/zmartzone/mod_auth_openidc/releases/download/v%{version}/mod_auth_openidc-%{version}.tar.gz
Patch1: test-segfault.patch
Patch2: 0002-Backport-of-improve-validation-of-the-post-logout-UR.patch
Patch3: 0003-Backport-of-Fix-open-redirect-starting-with-a-slash.patch
Patch4: 0004-Backport-of-Fix-open-redirect-starting-with-a-slash-.patch
Patch5: 0005-Fix-the-previous-backports.patch
Patch6: 0006-add-OIDCStateMaxNumberOfCookies-to-limit-nr-of-state.patch
Patch7: 0007-set-boundaries-on-min-and-max-values-on-number-of-pa.patch
Patch8: 0008-make-the-default-max-number-of-state-cookies-7-inste.patch
Patch9: 0009-don-t-return-content-with-503-see-331.patch
Patch10: 0010-improve-auto-detection-of-XMLHttpRequests-via-Accept.patch
Patch11: 0011-oops-document-OIDCStateMaxNumberOfCookies-for-releas.patch
Patch12: 0012-optionally-delete-the-oldest-state-cookie-s-see-399.patch
Patch13: 0013-Allow-configuring-which-header-value-is-used-to-calc.patch
Patch14: 0014-add-value-of-OIDC_SET_COOKIE_APPEND-env-var-to-Set-C.patch
Patch15: 0015-pick-OIDC_SET_COOKIE_APPEND-over-ext-passed-in-to-oi.patch
Patch16: 0016-always-add-a-SameSite-value-to-the-Set-Cookie-header.patch
Patch17: 0017-fix-also-add-SameSite-None-to-by-value-session-cooki.patch
Patch18: 0018-add-note-on-usage-of-OIDC_SET_COOKIE_APPEND-in-the-s.patch
Patch19: 0019-add-SameSite-attribute-on-cookie-clearance-logout.patch
Patch20: 0020-prevent-open-redirect-on-refresh-token-requests-rele.patch
Patch21: 0021-prevent-XSS-and-open-redirect-on-OIDC-session-manage.patch
Patch22: 0022-replace-potentially-harmful-backslashes-with-forward.patch
Patch23: 0023-apply-OIDCRedirectURLsAllowed-setting-to-target_link.patch
Patch24: 0024-use-encrypted-JWTs-for-storing-encrypted-cache-conte.patch
Patch25: 0025-avoid-XSS-vulnerability-when-using-OIDCPreservePost-.patch
Patch26: 0026-Add-a-function-to-escape-Javascript-characters.patch
Source0: https://github.com/zmartzone/mod_auth_openidc/archive/v%{version}.tar.gz
BuildRequires: gcc
BuildRequires: httpd-devel
@ -69,32 +41,6 @@ an OpenID Connect Relying Party and/or OAuth 2.0 Resource Server.
%prep
%setup -q
%patch1 -p1
%patch2 -p1
%patch3 -p1
%patch4 -p1
%patch5 -p1
%patch6 -p1
%patch7 -p1
%patch8 -p1
%patch9 -p1
%patch10 -p1
%patch11 -p1
%patch12 -p1
%patch13 -p1
%patch14 -p1
%patch15 -p1
%patch16 -p1
%patch17 -p1
%patch18 -p1
%patch19 -p1
%patch20 -p1
%patch21 -p1
%patch22 -p1
%patch23 -p1
%patch24 -p1
%patch25 -p1
%patch26 -p1
%build
# workaround rpm-buildroot-usage
@ -104,9 +50,11 @@ autoreconf
%configure \
--with-jq=/usr/lib64/ \
%{?_with_hiredis} \
%{?_without_hiredis}
%{?_without_hiredis} \
--with-apxs2=%{_httpd_apxs}
make %{?_smp_mflags}
%{make_build}
%check
export MODULES_DIR=%{_httpd_moddir}
@ -147,6 +95,9 @@ install -m 700 -d $RPM_BUILD_ROOT%{httpd_pkg_cache_dir}/cache
%dir %attr(0700, apache, apache) %{httpd_pkg_cache_dir}/cache
%changelog
* Fri Apr 8 2022 Tomas Halman <thalman@redhat.com> - 2.4.9.4-1
- Resolves: rhbz#2025368 - Rebase to new version
* Fri Jan 28 2022 Tomas Halman <thalman@redhat.com> - 2.3.7-11
- Resolves: rhbz#1987222 - CVE-2021-32792 XSS when using OIDCPreservePost On