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