From 78b86cd7aab0413e8d471370c891507243eccc13 Mon Sep 17 00:00:00 2001 From: Jim Jagielski Date: Tue, 30 May 2017 12:27:41 +0000 Subject: [PATCH] SECURITY: CVE-2017-3167 (cve.mitre.org) Use of the ap_get_basic_auth_pw() by third-party modules outside of the authentication phase may lead to authentication requirements being bypassed. Merge r1796348 from trunk: core: Deprecate ap_get_basic_auth_pw() and add ap_get_basic_auth_components(). Submitted By: Emmanuel Dreyfus , jchampion, coverner Reviewed by: covener, ylavic, jim git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1796855 13f79535-47bb-0310-9956-ffa450edef68 --- CHANGES | 4 ++++ STATUS | 6 ------ include/ap_mmn.h | 4 +++- include/http_protocol.h | 25 ++++++++++++++++++++- server/protocol.c | 48 +++++++++++++++++++++++++++++++++++++++++ server/request.c | 17 ++++++++++++--- 6 files changed, 93 insertions(+), 11 deletions(-) diff --git a/CHANGES b/CHANGES index 1b815557a3..9d7fe8e379 100644 --- a/CHANGES +++ b/CHANGES @@ -2,6 +2,10 @@ Changes with Apache 2.4.26 + *) core: Deprecate ap_get_basic_auth_pw() and add + ap_get_basic_auth_components(). + [Emmanuel Dreyfus , Jacob Champion, Eric Covener] + *) mod_rewrite: When a substitution is a fully qualified URL, and the scheme/host/port matches the current virtual host, stop interpreting the path component as a local path just because the first component of the diff --git a/STATUS b/STATUS index 1ce64c6b43..456596a616 100644 --- a/STATUS +++ b/STATUS @@ -120,12 +120,6 @@ RELEASE SHOWSTOPPERS: PATCHES ACCEPTED TO BACKPORT FROM TRUNK: [ start all new proposals below, under PATCHES PROPOSED. ] - *) core: Deprecate ap_get_basic_auth_pw() and add - ap_get_basic_auth_components(). - trunk patch: http://svn.apache.org/r1796348 - 2.4.x patch: http://people.apache.org/~covener/patches/httpd-2.4.x-ap_get_basic_auth_pw.diff + CHANGES - +1: covener, ylavic, jim - *) core: Terminate token processing on NULL. trunk patch: http://svn.apache.org/r1796350 2.4.x patch: svn merge -c 1796350 ^/httpd/httpd/trunk . diff --git a/include/ap_mmn.h b/include/ap_mmn.h index 124057ca7d..2764501833 100644 --- a/include/ap_mmn.h +++ b/include/ap_mmn.h @@ -494,6 +494,8 @@ * and ap_scan_vchar_obstext() * Replaced fold boolean with with multiple bit flags * to ap_[r]getline() + * 20120211.68 (2.4.26-dev) Add ap_get_basic_auth_components() and deprecate + * ap_get_basic_auth_pw() */ #define MODULE_MAGIC_COOKIE 0x41503234UL /* "AP24" */ @@ -501,7 +503,7 @@ #ifndef MODULE_MAGIC_NUMBER_MAJOR #define MODULE_MAGIC_NUMBER_MAJOR 20120211 #endif -#define MODULE_MAGIC_NUMBER_MINOR 67 /* 0...n */ +#define MODULE_MAGIC_NUMBER_MINOR 68 /* 0...n */ /** * Determine if the server's current MODULE_MAGIC_NUMBER is at least a diff --git a/include/http_protocol.h b/include/http_protocol.h index a9e09904bd..29d887c61e 100644 --- a/include/http_protocol.h +++ b/include/http_protocol.h @@ -558,7 +558,11 @@ AP_DECLARE(void) ap_note_digest_auth_failure(request_rec *r); AP_DECLARE_HOOK(int, note_auth_failure, (request_rec *r, const char *auth_type)) /** - * Get the password from the request headers + * Get the password from the request headers. This function has multiple side + * effects due to its prior use in the old authentication framework. + * ap_get_basic_auth_components() should be preferred. + * + * @deprecated @see ap_get_basic_auth_components * @param r The current request * @param pw The password as set in the headers * @return 0 (OK) if it set the 'pw' argument (and assured @@ -571,6 +575,25 @@ AP_DECLARE_HOOK(int, note_auth_failure, (request_rec *r, const char *auth_type)) */ AP_DECLARE(int) ap_get_basic_auth_pw(request_rec *r, const char **pw); +#define AP_GET_BASIC_AUTH_PW_NOTE "AP_GET_BASIC_AUTH_PW_NOTE" + +/** + * Get the username and/or password from the request's Basic authentication + * headers. Unlike ap_get_basic_auth_pw(), calling this function has no side + * effects on the passed request_rec. + * + * @param r The current request + * @param username If not NULL, set to the username sent by the client + * @param password If not NULL, set to the password sent by the client + * @return APR_SUCCESS if the credentials were successfully parsed and returned; + * APR_EINVAL if there was no authentication header sent or if the + * client was not using the Basic authentication scheme. username and + * password are unchanged on failure. + */ +AP_DECLARE(apr_status_t) ap_get_basic_auth_components(const request_rec *r, + const char **username, + const char **password); + /** * parse_uri: break apart the uri * @warning Side Effects: diff --git a/server/protocol.c b/server/protocol.c index 19d087cbea..ff44b3937c 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -1593,6 +1593,7 @@ AP_DECLARE(int) ap_get_basic_auth_pw(request_rec *r, const char **pw) t = ap_pbase64decode(r->pool, auth_line); r->user = ap_getword_nulls (r->pool, &t, ':'); + apr_table_setn(r->notes, AP_GET_BASIC_AUTH_PW_NOTE, "1"); r->ap_auth_type = "Basic"; *pw = t; @@ -1600,6 +1601,53 @@ AP_DECLARE(int) ap_get_basic_auth_pw(request_rec *r, const char **pw) return OK; } +AP_DECLARE(apr_status_t) ap_get_basic_auth_components(const request_rec *r, + const char **username, + const char **password) +{ + const char *auth_header; + const char *credentials; + const char *decoded; + const char *user; + + auth_header = (PROXYREQ_PROXY == r->proxyreq) ? "Proxy-Authorization" + : "Authorization"; + credentials = apr_table_get(r->headers_in, auth_header); + + if (!credentials) { + /* No auth header. */ + return APR_EINVAL; + } + + if (ap_cstr_casecmp(ap_getword(r->pool, &credentials, ' '), "Basic")) { + /* These aren't Basic credentials. */ + return APR_EINVAL; + } + + while (*credentials == ' ' || *credentials == '\t') { + credentials++; + } + + /* XXX Our base64 decoding functions don't actually error out if the string + * we give it isn't base64; they'll just silently stop and hand us whatever + * they've parsed up to that point. + * + * Since this function is supposed to be a drop-in replacement for the + * deprecated ap_get_basic_auth_pw(), don't fix this for 2.4.x. + */ + decoded = ap_pbase64decode(r->pool, credentials); + user = ap_getword_nulls(r->pool, &decoded, ':'); + + if (username) { + *username = user; + } + if (password) { + *password = decoded; + } + + return APR_SUCCESS; +} + struct content_length_ctx { int data_sent; /* true if the C-L filter has already sent at * least one bucket on to the next output filter diff --git a/server/request.c b/server/request.c index b2280cb5a8..fac5f8c7cd 100644 --- a/server/request.c +++ b/server/request.c @@ -124,6 +124,8 @@ static int decl_die(int status, const char *phase, request_rec *r) AP_DECLARE(int) ap_some_authn_required(request_rec *r) { int access_status; + char *olduser = r->user; + int rv = FALSE; switch (ap_satisfies(r)) { case SATISFY_ALL: @@ -134,7 +136,7 @@ AP_DECLARE(int) ap_some_authn_required(request_rec *r) access_status = ap_run_access_checker_ex(r); if (access_status == DECLINED) { - return TRUE; + rv = TRUE; } break; @@ -145,13 +147,14 @@ AP_DECLARE(int) ap_some_authn_required(request_rec *r) access_status = ap_run_access_checker_ex(r); if (access_status == DECLINED) { - return TRUE; + rv = TRUE; } break; } - return FALSE; + r->user = olduser; + return rv; } /* This is the master logic for processing requests. Do NOT duplicate @@ -259,6 +262,14 @@ AP_DECLARE(int) ap_process_request_internal(request_rec *r) r->ap_auth_type = r->main->ap_auth_type; } else { + /* A module using a confusing API (ap_get_basic_auth_pw) caused + ** r->user to be filled out prior to check_authn hook. We treat + ** it is inadvertent. + */ + if (r->user && apr_table_get(r->notes, AP_GET_BASIC_AUTH_PW_NOTE)) { + r->user = NULL; + } + switch (ap_satisfies(r)) { case SATISFY_ALL: case SATISFY_NOSPEC: -- 2.40.0