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