Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
From: Philip Martin <philip.martin@wandisco.com>
To: dev@subversion.apache.org, Ben Reser <ben@reser.org>,
 James McCoy <jamessan@debian.org>
Subject: [PATCH] mod_auth_kerb/mod_auth_ntlm and mod_authz_svn
Date: Fri, 09 Oct 2015 19:20:22 +0100
Message-ID: <87io6fewex.fsf@wandisco.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.4 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain

CVE-2015-3185 for httpd and CVE-2015-3184 for Subversion combined to fix
a Subversion authz vulnerability.  Following the release (httpd 2.4.16
and Subversion 1.8.14) there have been reports of authn failures with
mod_auth_kerb and mod_auth_ntlm.  These are 3rd party modules
maintained outside the httpd tree.

  https://bugs.debian.org/797216
  https://bugs.debian.org/799105
  http://mail-archives.apache.org/mod_mbox/subversion-users/201509.mbox/%3CF3CBF2565B5E6046BC0D960DFE30DDCE78B01E62%40AL-MAIL01.aliconahq.com%3E
  http://svn.haxx.se/users/archive-2015-09/0063.shtml

I believe I have reproduced the mod_auth_kerb problem:

  $ curl -D- http://localhost:8888/krb/repo/A/B/
  HTTP/1.1 401 Unauthorized
  Date: Fri, 09 Oct 2015 17:32:46 GMT
  Server: Apache/2.4.17-dev (Unix) OpenSSL/1.0.1k SVN/1.10.0-dev mod_auth_kerb/5.4
  Content-Length: 381
  Content-Type: text/html; charset=iso-8859-1

  <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
  ...

The 401 is expected here, but the server is sending it without a
WWW-Authenticate header.  A client that could authenticate, like
Subversion, cannot do it because the header is missing and so the client
gives up.

The cause is this code in mod_authz_svn.c:

      if (authn_configured) {
          /* We have to check to see if authn is required because if so we must
           * return UNAUTHORIZED (401) rather than FORBIDDEN (403) since returning
           * the 403 leaks information about what paths may exist to
           * unauthenticated users.  We must set a note here in order
           * to use ap_some_authn_rquired() without triggering an infinite
           * loop since the call will trigger this function to be called again. */
          apr_table_setn(r->notes, IN_SOME_AUTHN_NOTE, (const char*)1);
          authn_required = ap_some_authn_required(r);
          apr_table_unset(r->notes, IN_SOME_AUTHN_NOTE);
          if (authn_required)
            {
              ap_note_auth_failure(r);
              return HTTP_UNAUTHORIZED;
            }
      }

Returning HTTP_UNAUTHORIZED is part of the CVE fix, mod_authz_svn didn't
do this before.  The call to ap_note_auth_failure() runs hooks in the
authn modules to add the authenticate header and works with modules like
mod_auth_basic and mod_auth_digest which use ap_hook_note_auth_failure()
to register the hook.  However, note_auth_failure is new in 2.4 and the
3rd party mod_auth_kerb or mod_auth_ntlm do not register.  That explains
why they don't add the header and why authn fails.

I don't know whether ap_note_auth_failure is optional or manatory for
authn modules in 2.4 so I don't know if the fault lies with mod_dav_svn
or mod_auth_kerb/mod_auth_ntlm.  I do know that httpd's mod_authz_core
uses ap_note_auth_failure().

We could attempt to add the hook to mod_auth_kerb, but we would need to
do the same for mod_auth_ntlm (of which there appears to be several
variations) as well as any other module that doesn't implement the hook.

Before the CVE fix mod_authz_svn would return DECLINED rather than
HTTP_UNAUTHORIZED and this allows httpd's ap_process_request_internal()
to procede further and invoke mod_auth_kerb via an api it does implement
and that adds the authenticate header.  I've been experimenting with the
patch below that makes mod_authz_svn return DECLINED again.  It passes
the Subversion regression tests, including the ones added for the CVE.

All this authn/authz stuff is very complex and I don't know if this is
correct.

Index: subversion/mod_authz_svn/mod_authz_svn.c
===================================================================
--- subversion/mod_authz_svn/mod_authz_svn.c	(revision 1707771)
+++ subversion/mod_authz_svn/mod_authz_svn.c	(working copy)
@@ -954,7 +954,7 @@ access_checker(request_rec *r)
 #if USE_FORCE_AUTHN
       if (authn_configured) {
           /* We have to check to see if authn is required because if so we must
-           * return UNAUTHORIZED (401) rather than FORBIDDEN (403) since returning
+           * return DECLINED rather than FORBIDDEN (403) since returning
            * the 403 leaks information about what paths may exist to
            * unauthenticated users.  We must set a note here in order
            * to use ap_some_authn_rquired() without triggering an infinite
@@ -963,10 +963,7 @@ access_checker(request_rec *r)
           authn_required = ap_some_authn_required(r);
           apr_table_unset(r->notes, IN_SOME_AUTHN_NOTE);
           if (authn_required)
-            {
-              ap_note_auth_failure(r);
-              return HTTP_UNAUTHORIZED;
-            }
+            return DECLINED;
       }
 #else
       if (!authn_required)

-- 
Philip Martin
WANdisco