From: William A. Rowe Jr Date: Wed, 16 Jul 2014 21:03:30 +0000 (+0000) Subject: Fix PR 56480: PROPFIND walker doesn't encode hrefs properly X-Git-Tag: 2.2.28~42 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3068349cd25921012ffdcfcc1a1c278e309b342a;p=thirdparty%2Fapache%2Fhttpd.git Fix PR 56480: PROPFIND walker doesn't encode hrefs properly Reverts r1529559 partially (specifically the dav_xml_escape_uri) bit. Reverts r1531505 entirely. * modules/dav/main/mod_dav.c (dav_xml_escape_uri): Revert the piece of r1529559 that removes the URI escaping from this function. * modules/dav/main/props.c (dav_do_prop_subreq): Escape the URI before doing a sub request with it. This resolves some properties like getcontenttype from failing to be returned for files that contain characters that require encoding in their path. * modules/dav/main/mod_dav.h (dav_resource): Note the inconsistency in the documentation. * modules/dav/fs/repos.c (dav_fs_get_resource): Don't use the unparsed_uri to set the uri field of the resource. This is the correct fix for the double encoding in mod_dav_fs that led to the dav_xml_escape_uri() change and r1531505. (dav_fs_walker, dav_fs_append_uri): Revert r1531505 changes. Submitted by: breser PR: 56480 Backports: r1602338 Reviewed by: breser, rpluem, ylavic git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.2.x@1611189 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/CHANGES b/CHANGES index f860266b0eb..27fd8bfa4ab 100644 --- a/CHANGES +++ b/CHANGES @@ -1,4 +1,4 @@ - -*- coding: utf-8 -*- + -*- coding: utf-8 -*- Changes with Apache 2.2.28 *) SECURITY: CVE-2014-0231 (cve.mitre.org) @@ -14,6 +14,8 @@ Changes with Apache 2.2.28 Fix a race condition in scoreboard handling, which could lead to a heap buffer overflow. [Joe Orton, Eric Covener, Jeff Trawick] + *) mod_dav: Fix improper encoding in PROPFIND responses. PR 56480. + *) mod_ssl: Extend the scope of SSLSessionCacheTimeout to sessions resumed by TLS session resumption (RFC 5077). [Rainer Jung] diff --git a/STATUS b/STATUS index 9d55a4e706b..d5c289d22ff 100644 --- a/STATUS +++ b/STATUS @@ -99,17 +99,6 @@ RELEASE SHOWSTOPPERS: PATCHES ACCEPTED TO BACKPORT FROM TRUNK: [ start all new proposals below, under PATCHES PROPOSED. ] - * mod_dav: PROPFIND not encoding hrefs properly. PR 56480. - This was an unintentional regression in the PR 55397 fixes previously - released to fix the regressions caused by PR 54611. Causes problems with - mod_dav_svn. Subversion's trunk test suite will show an XPASS test when - this is fixed. - trunk patch: http://svn.apache.org/r1602338 - 2.2.x patch: run svn merge -c 1602338 ^/httpd/httpd/trunk - and add the following CHANGES entry: - *) mod_dav: Fix improper encoding in PROPFIND responses. PR 56480. - +1: breser, rpluem, ylavic - PATCHES PROPOSED TO BACKPORT FROM TRUNK: [ New proposals should be added at the end of the list ] diff --git a/modules/dav/fs/repos.c b/modules/dav/fs/repos.c index 02daca0b79c..6723becbf87 100644 --- a/modules/dav/fs/repos.c +++ b/modules/dav/fs/repos.c @@ -683,13 +683,13 @@ static dav_error * dav_fs_get_resource( resource->pool = r->pool; /* make sure the URI does not have a trailing "/" */ - len = strlen(r->unparsed_uri); - if (len > 1 && r->unparsed_uri[len - 1] == '/') { - s = apr_pstrmemdup(r->pool, r->unparsed_uri, len-1); + len = strlen(r->uri); + if (len > 1 && r->uri[len - 1] == '/') { + s = apr_pstrmemdup(r->pool, r->uri, len-1); resource->uri = s; } else { - resource->uri = r->unparsed_uri; + resource->uri = r->uri; } if (r->finfo.filetype != 0) { @@ -1406,18 +1406,6 @@ static dav_error * dav_fs_remove_resource(dav_resource *resource, return dav_fs_deleteset(info->pool, resource); } -/* Take an unescaped path component and escape it and append it onto a - * dav_buffer for a URI */ -static apr_size_t dav_fs_append_uri(apr_pool_t *p, dav_buffer *pbuf, - const char *path, apr_size_t pad) -{ - const char *epath = ap_escape_uri(p, path); - apr_size_t epath_len = strlen(epath); - - dav_buffer_place_mem(p, pbuf, epath, epath_len + 1, pad); - return epath_len; -} - /* ### move this to dav_util? */ /* Walk recursively down through directories, * * including lock-null resources as we go. */ @@ -1472,7 +1460,6 @@ static dav_error * dav_fs_walker(dav_fs_walker_context *fsctx, int depth) } while ((apr_dir_read(&dirent, APR_FINFO_DIRENT, dirp)) == APR_SUCCESS) { apr_size_t len; - apr_size_t escaped_len; apr_status_t status; len = strlen(dirent.name); @@ -1512,7 +1499,7 @@ static dav_error * dav_fs_walker(dav_fs_walker_context *fsctx, int depth) /* copy the file to the URI, too. NOTE: we will pad an extra byte for the trailing slash later. */ - escaped_len = dav_fs_append_uri(pool, &fsctx->uri_buf, dirent.name, 1); + dav_buffer_place_mem(pool, &fsctx->uri_buf, dirent.name, len + 1, 1); /* if there is a secondary path, then do that, too */ if (fsctx->path2.buf != NULL) { @@ -1545,7 +1532,7 @@ static dav_error * dav_fs_walker(dav_fs_walker_context *fsctx, int depth) fsctx->path2.cur_len += len; /* adjust URI length to incorporate subdir and a slash */ - fsctx->uri_buf.cur_len += escaped_len + 1; + fsctx->uri_buf.cur_len += len + 1; fsctx->uri_buf.buf[fsctx->uri_buf.cur_len - 1] = '/'; fsctx->uri_buf.buf[fsctx->uri_buf.cur_len] = '\0'; @@ -1611,8 +1598,8 @@ static dav_error * dav_fs_walker(dav_fs_walker_context *fsctx, int depth) */ dav_buffer_place_mem(pool, &fsctx->path1, fsctx->locknull_buf.buf + offset, len + 1, 0); - dav_fs_append_uri(pool, &fsctx->uri_buf, - fsctx->locknull_buf.buf + offset, 0); + dav_buffer_place_mem(pool, &fsctx->uri_buf, + fsctx->locknull_buf.buf + offset, len + 1, 0); if (fsctx->path2.buf != NULL) { dav_buffer_place_mem(pool, &fsctx->path2, fsctx->locknull_buf.buf + offset, diff --git a/modules/dav/main/mod_dav.c b/modules/dav/main/mod_dav.c index ba0117e51ee..83f3dd1243e 100644 --- a/modules/dav/main/mod_dav.c +++ b/modules/dav/main/mod_dav.c @@ -400,9 +400,11 @@ static int dav_error_response_tag(request_rec *r, */ static const char *dav_xml_escape_uri(apr_pool_t *p, const char *uri) { + const char *e_uri = ap_escape_uri(p, uri); + /* check the easy case... */ - if (ap_strchr_c(uri, '&') == NULL) - return uri; + if (ap_strchr_c(e_uri, '&') == NULL) + return e_uri; /* there was a '&', so more work is needed... sigh. */ @@ -410,7 +412,7 @@ static const char *dav_xml_escape_uri(apr_pool_t *p, const char *uri) * Note: this is a teeny bit of overkill since we know there are no * '<' or '>' characters, but who cares. */ - return apr_xml_quote_string(p, uri, 0); + return apr_xml_quote_string(p, e_uri, 0); } diff --git a/modules/dav/main/mod_dav.h b/modules/dav/main/mod_dav.h index 64e0e22764e..827e520550b 100644 --- a/modules/dav/main/mod_dav.h +++ b/modules/dav/main/mod_dav.h @@ -370,7 +370,9 @@ typedef struct dav_resource { * REGULAR and WORKSPACE resources, * and is always 1 for WORKING */ - const char *uri; /* the escaped URI for this resource */ + const char *uri; /* the URI for this resource; + * currently has an ABI flaw where sometimes it is + * assumed to be encoded and sometimes not */ dav_resource_private *info; /* the provider's private info */ diff --git a/modules/dav/main/props.c b/modules/dav/main/props.c index 27967377729..5f7b1424f9d 100644 --- a/modules/dav/main/props.c +++ b/modules/dav/main/props.c @@ -321,10 +321,14 @@ static int dav_rw_liveprop(dav_propdb *propdb, dav_elem_private *priv) /* do a sub-request to fetch properties for the target resource's URI. */ static void dav_do_prop_subreq(dav_propdb *propdb) { + /* need to escape the uri that's in the resource struct because during + * the property walker it's not encoded. */ + const char *e_uri = ap_escape_uri(propdb->resource->pool, + propdb->resource->uri); + /* perform a "GET" on the resource's URI (note that the resource may not correspond to the current request!). */ - propdb->subreq = ap_sub_req_lookup_uri(propdb->resource->uri, propdb->r, - NULL); + propdb->subreq = ap_sub_req_lookup_uri(e_uri, propdb->r, NULL); } static dav_error * dav_insert_coreprop(dav_propdb *propdb,