Fix two security bugs

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

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 <jdennis@redhat.com>
+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/
+    <Location /sp-proxy>
+        AuthType Mellon
+        MellonEnable auth
+        Require valid-user
+    </Location>
+
+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
+

0002-Fix-redirect-URL-validation-bypass.patch

@@ -0,0 +1,44 @@
+From 62041428a32de402e0be6ba45fe12df6a83bedb8 Mon Sep 17 00:00:00 2001
+From: Olav Morken <olav.morken@uninett.no>
+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
+

acs-warning.patch

@@ -1,33 +0,0 @@
-commit 5ba9bb72707a90503cd4d042083ea074a0cb6b8a
-Author: John Dennis <jdennis@redhat.com>
-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 <jdennis@redhat.com>
-
-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);

content-type.patch

@@ -1,36 +0,0 @@
-commit 040a1ae5cb2aab38b2bc716cc3d0d6fa7b998a7a
-Author: John Dennis <jdennis@redhat.com>
-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 <jdennis@redhat.com>
-
-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;

enabled_in_check_uid.patch

@@ -1,34 +0,0 @@
-commit 912aa852ebd78577f59cf7958c709acea98ace4c
-Author: John Dennis <jdennis@redhat.com>
-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 <jdennis@redhat.com>
-
-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) {

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 <jhrozek@redhat.com> - 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 <releng@fedoraproject.org> - 0.14.0-4
 - Rebuilt for https://fedoraproject.org/wiki/Fedora_29_Mass_Rebuild