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