From 5a1f90847efce160631ee2a16d7b9d1da3496616 Mon Sep 17 00:00:00 2001 From: Hiroyuki Wada 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 []+ +# 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