diff --git a/0001-Modify-am_handler-setup-to-run-before-mod_proxy.patch b/0001-Modify-am_handler-setup-to-run-before-mod_proxy.patch new file mode 100644 index 0000000..23e4ac8 --- /dev/null +++ b/0001-Modify-am_handler-setup-to-run-before-mod_proxy.patch @@ -0,0 +1,80 @@ +From e09a28a30e13e5c22b481010f26b4a7743a09280 Mon Sep 17 00:00:00 2001 +From: John Dennis +Date: Tue, 5 Mar 2019 10:15:48 +0100 +Subject: [PATCH] Modify am_handler setup to run before mod_proxy + +The way the ECP flow works is that when a client initiates the flow, the +SP's response is HTTP 200, but not the requested content, but a signed XML +document that contains the "samlp:AuthnRequest" element. The idea is that +the ECP client would then determine the IDP and send the document to the +IDP, get a samlp:Response and convey that to the SP to get access to the +protected resource. + +Internally, the auth check which is normally done with am_check_uid() set to +apache's ap_hook_check_user_id() hook, just responds with OK, so it pretends +to authenticate the user. Then in the usual flow, the request reaches the +ap_hook_handler which handles the request. There in the pipeline, mellon +registers functions am_handler() which should run first (APR_HOOK_FIRST), +determine that this request is an ECP one and return the ECP AuthnRequest +document. But in case the proxy module is also in the picture, the proxy +module "races" for who gets to be the first to handle the request in the +pipeline and wins. Therefore, the request reaches the protected resource +via mod_proxy and returns it. + +This fix modifies the ap_hook_handler() call to explicitly run before +handlers from mod_proxy.c + +To reproduce the bug: +0) Have a SP with mellon connected to a Keycloak IDP (or any other IDP I + guess). In the example below, my SAML SP is saml.federation.test +1) Set a Location protected by mellon that proxies requests to another + URL. For example: + + ProxyPass /sp-proxy http://app.federation.test/example_app/ + + AuthType Mellon + MellonEnable auth + Require valid-user + + +2) call: + curl -L -H "Accept: application/vnd.paos+xml" \ + -H 'PAOS: ver="urn:liberty:paos:2003-08";"urn:oasis:names:tc:SAML:2.0:profiles:SSO:ecp"' \ + http://saml.federation.test/sp-proxy + +Before the patch, you would see whatever is served from the proxied +page. With the patch, you should get back a XML document with a +samlp:AuthnRequest. +--- + mod_auth_mellon.c | 8 +++++++- + 1 file changed, 7 insertions(+), 1 deletion(-) + +diff --git a/mod_auth_mellon.c b/mod_auth_mellon.c +index 74bd328..5330f48 100644 +--- a/mod_auth_mellon.c ++++ b/mod_auth_mellon.c +@@ -207,6 +207,12 @@ static int am_create_request(request_rec *r) + + static void register_hooks(apr_pool_t *p) + { ++ /* Our handler needs to run before mod_proxy so that it can properly ++ * return ECP AuthnRequest messages when running as a reverse proxy. ++ * See: https://github.com/Uninett/mod_auth_mellon/pull/196 ++ */ ++ static const char * const run_handler_before[]={ "mod_proxy.c", NULL }; ++ + ap_hook_access_checker(am_auth_mellon_user, NULL, NULL, APR_HOOK_MIDDLE); + ap_hook_check_user_id(am_check_uid, NULL, NULL, APR_HOOK_MIDDLE); + ap_hook_post_config(am_global_init, NULL, NULL, APR_HOOK_MIDDLE); +@@ -222,7 +228,7 @@ static void register_hooks(apr_pool_t *p) + * Therefore this hook must run before any handler that may check + * r->handler and decide that it is the only handler for this URL. + */ +- ap_hook_handler(am_handler, NULL, NULL, APR_HOOK_FIRST); ++ ap_hook_handler(am_handler, NULL, run_handler_before, APR_HOOK_FIRST); + + #ifdef ENABLE_DIAGNOSTICS + ap_hook_open_logs(am_diag_log_init,NULL,NULL,APR_HOOK_MIDDLE); +-- +2.19.2 + diff --git a/0002-Fix-redirect-URL-validation-bypass.patch b/0002-Fix-redirect-URL-validation-bypass.patch new file mode 100644 index 0000000..b3f18c5 --- /dev/null +++ b/0002-Fix-redirect-URL-validation-bypass.patch @@ -0,0 +1,44 @@ +From 62041428a32de402e0be6ba45fe12df6a83bedb8 Mon Sep 17 00:00:00 2001 +From: Olav Morken +Date: Tue, 19 Mar 2019 13:42:22 +0100 +Subject: [PATCH] Fix redirect URL validation bypass + +It turns out that browsers silently convert backslash characters into +forward slashes, while apr_uri_parse() does not. + +This mismatch allows an attacker to bypass the redirect URL validation +by using an URL like: + + https://sp.example.org/mellon/logout?ReturnTo=https:%5c%5cmalicious.example.org/ + +mod_auth_mellon will assume that it is a relative URL and allow the +request to pass through, while the browsers will use it as an absolute +url and redirect to https://malicious.example.org/ . + +This patch fixes this issue by rejecting all redirect URLs with +backslashes. +--- + auth_mellon_util.c | 7 +++++++ + 1 file changed, 7 insertions(+) + +diff --git a/auth_mellon_util.c b/auth_mellon_util.c +index 0fab309..fd442f9 100644 +--- a/auth_mellon_util.c ++++ b/auth_mellon_util.c +@@ -927,6 +927,13 @@ int am_check_url(request_rec *r, const char *url) + "Control character detected in URL."); + return HTTP_BAD_REQUEST; + } ++ if (*i == '\\') { ++ /* Reject backslash character, as it can be used to bypass ++ * redirect URL validation. */ ++ AM_LOG_RERROR(APLOG_MARK, APLOG_ERR, HTTP_BAD_REQUEST, r, ++ "Backslash character detected in URL."); ++ return HTTP_BAD_REQUEST; ++ } + } + + return OK; +-- +2.19.2 + diff --git a/acs-warning.patch b/acs-warning.patch deleted file mode 100644 index 1163574..0000000 --- a/acs-warning.patch +++ /dev/null @@ -1,33 +0,0 @@ -commit 5ba9bb72707a90503cd4d042083ea074a0cb6b8a -Author: John Dennis -Date: Fri Oct 30 15:46:33 2015 -0400 - - Role maybe unknown when assertion consumer url is looked up - - Replace the call to lasso_provider_get_metadata_one() with - lasso_provider_get_metadata_one_for_role() so that we can exlicitly - pass the LASSO_PROVIDER_ROLE_SP role. The former call obtains the - role from the provider object and then calls - lasso_provider_get_metadata_one_for_role() using that role. However - the role will not have been set in the provider until the first request is - processed. This means the first time we call this routine it won't - work correctly because the role will not have been set yet, by - explicitly passing the role we avoid this problem. - - Signed-off-by: John Dennis - -diff --git a/auth_mellon_util.c b/auth_mellon_util.c -index 155bb1a..6c694b7 100644 ---- a/auth_mellon_util.c -+++ b/auth_mellon_util.c -@@ -1827,7 +1827,9 @@ char *am_get_assertion_consumer_service_by_binding(LassoProvider *provider, cons - } - - if (selected_descriptor) { -- url = lasso_provider_get_metadata_one(provider, selected_descriptor); -+ url = lasso_provider_get_metadata_one_for_role(provider, -+ LASSO_PROVIDER_ROLE_SP, -+ selected_descriptor); - } - - lasso_release_list_of_strings(descriptors); diff --git a/content-type.patch b/content-type.patch deleted file mode 100644 index 7b8b54a..0000000 --- a/content-type.patch +++ /dev/null @@ -1,36 +0,0 @@ -commit 040a1ae5cb2aab38b2bc716cc3d0d6fa7b998a7a -Author: John Dennis -Date: Mon Jan 16 09:02:06 2017 -0500 - - Use ap_set_content_type() to set "Content-Type" header - - Formerly we were setting the response header "Content-Type" in - r->headers_out directly via the apr_table_setn() call. Although using - apr_table_setn() is appropriate for many HTTP headers Apache actively - manages a small set of headers in - http_filters.c:ap_http_header_filter(). These managed headers are - derived from values maintained in the request_rec. "Content-Type" is - one of the managed headers. - - Because we didn't set r->content_type field via the - ap_set_content_type() call and instead directly updated the - r->headers_out table our value for "Content-Type" was overwriten when - the ap_http_header_filter() was run just prior to emitting the - response with the result the "Content-Type" header returned to the - client was incorrect. - - Signed-off-by: John Dennis - -diff --git a/auth_mellon_handler.c b/auth_mellon_handler.c -index a55828a..25365de 100644 ---- a/auth_mellon_handler.c -+++ b/auth_mellon_handler.c -@@ -2655,7 +2655,7 @@ static int am_set_authn_request_post_content(request_rec *r, LassoLogin *login) - */ - static int am_set_authn_request_paos_content(request_rec *r, LassoLogin *login) - { -- apr_table_setn(r->headers_out, "Content-Type", MEDIA_TYPE_PAOS); -+ ap_set_content_type(r, MEDIA_TYPE_PAOS); - ap_rputs(LASSO_PROFILE(login)->msg_body, r); - - return OK; diff --git a/enabled_in_check_uid.patch b/enabled_in_check_uid.patch deleted file mode 100644 index ba99055..0000000 --- a/enabled_in_check_uid.patch +++ /dev/null @@ -1,34 +0,0 @@ -commit 912aa852ebd78577f59cf7958c709acea98ace4c -Author: John Dennis -Date: Fri Apr 8 09:01:22 2016 -0400 - - am_check_uid() should be no-op if mellon not enabled - - mod_auth_mellon was interferring with other Apache authentication - modules (e.g. mod_auth_kerb) because when the Apache check_user_id - hook ran the logic in am_check_uid would execute even if mellon was - not enabled for the location. This short circuited the hook execution - and never allowed the authentication enabled for the location to - execute. It resulted in HTTP_UNAUTHORIZED being returned with the - client then expecting a WWW-Authenticate header field causing the - client to attempt to authenticate again. - - Signed-off-by: John Dennis - -diff --git a/auth_mellon_handler.c b/auth_mellon_handler.c -index a72e1ca..864396f 100644 ---- a/auth_mellon_handler.c -+++ b/auth_mellon_handler.c -@@ -3625,6 +3625,12 @@ int am_check_uid(request_rec *r) - return OK; - } - -+ /* Check that the user has enabled authentication for this directory. */ -+ if(dir->enable_mellon == am_enable_off -+ || dir->enable_mellon == am_enable_default) { -+ return DECLINED; -+ } -+ - #ifdef HAVE_ECP - am_req_cfg_rec *req_cfg = am_get_req_cfg(r); - if (req_cfg->ecp_authn_req) { diff --git a/mod_auth_mellon.spec b/mod_auth_mellon.spec index 78f4c2c..218c6ca 100644 --- a/mod_auth_mellon.spec +++ b/mod_auth_mellon.spec @@ -1,7 +1,7 @@ Summary: A SAML 2.0 authentication module for the Apache Httpd Server Name: mod_auth_mellon Version: 0.14.0 -Release: 4%{?dist} +Release: 5%{?dist} Group: System Environment/Daemons Source0: https://github.com/UNINETT/mod_auth_mellon/releases/download/v%{version}/%{name}-%{version}.tar.gz Source1: auth_mellon.conf @@ -22,6 +22,9 @@ Requires: httpd-mmn = %{_httpd_mmn} Requires: lasso >= 2.5.1-13 Url: https://github.com/UNINETT/mod_auth_mellon +Patch0001: 0001-Modify-am_handler-setup-to-run-before-mod_proxy.patch +Patch0002: 0002-Fix-redirect-URL-validation-bypass.patch + %description The mod_auth_mellon module is an authentication service that implements the @@ -30,6 +33,8 @@ received in assertions generated by a IdP server. %prep %setup -q -n %{name}-%{version} +%patch0001 -p1 +%patch0002 -p1 %build export APXS=%{_httpd_apxs} @@ -102,6 +107,12 @@ in the doc directory for instructions on using the diagnostics build. %dir /run/%{name}/ %changelog +* Fri Mar 22 2019 Jakub Hrozek - 0.14.0-5 +- Related: rhbz#1691771 - CVE-2019-3877 mod_auth_mellon: open redirect in + logout url when using URLs with backslashes +- Related: rhbz#1691136 - CVE-2019-3878 mod_auth_mellon: authentication + bypass in ECP flow + * Fri Jul 13 2018 Fedora Release Engineering - 0.14.0-4 - Rebuilt for https://fedoraproject.org/wiki/Fedora_29_Mass_Rebuild