From 19f8d1f866cf8495f43f010935a030ac910064bd Mon Sep 17 00:00:00 2001 From: Jim Jagielski Date: Mon, 11 Nov 2013 14:03:49 +0000 Subject: [PATCH] Merge r1529559, r1531505 from trunk: Fix PR 55397: dav_resource->uri treated as an unparsed uri. The change made for PR 54611 caused this field to be treated as unescaped. mod_dav_svn however, provided escaped URIs. Essentially breaking support for paths with non-URI safe characters in SVN. Adjust the code so that dav_resource->uri is assumed to be escaped and adjust mod_dav_fs so that it uses escaped URIs in this field. * modules/dav/fs/repos.c (dav_fs_get_resource): Use the unparsed_uri to contruct the resource uri. * modules/dav/main/mod_dav.c (dav_xml_escape_uri): Do not uri escape, just handle xml escaping. (dav_created): Assume that locn if provided is escaped. (dav_method_copymove, dav_method_bind): Use the unparsed_uri on the request when calling dav_created() to adjust to locn assuming it is escaped. * modules/dav/main/mod_dav.h (dav_resource): Document that uri is escaped. Followup to r1529559: mod_dav_fs: Fix encoding of hrefs in PROPFIND response. Previous commit missed encoding the names of the children of the PROPFIND request when the depth wasn't 0. * modules/dav/fs/repos.c (dav_fs_append_uri): New function (dav_fs_walker): Use dav_fs_append_uri() and adjust length calculations to use the encoded length. Submitted by: breser Reviewed/backported by: jim git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.2.x@1540730 13f79535-47bb-0310-9956-ffa450edef68 --- CHANGES | 6 ++++++ STATUS | 8 -------- modules/dav/fs/repos.c | 30 +++++++++++++++++++++--------- modules/dav/main/mod_dav.c | 17 +++++++---------- modules/dav/main/mod_dav.h | 2 +- 5 files changed, 35 insertions(+), 28 deletions(-) diff --git a/CHANGES b/CHANGES index 79e272dc717..0f308878186 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,9 @@ -*- coding: utf-8 -*- Changes with Apache 2.2.26 + *) mod_dav: dav_resource->uri treated as unencoded. This was an + unnecessary ABI changed introduced in 2.2.25 PR 55397. [Ben Reser] + *) mod_dav: Do not validate locks against parent collection of COPY source URI. PR 55304. [Ben Reser] @@ -19,6 +22,9 @@ Changes with Apache 2.2.26 support for SSLv2. Problem was introduced in 2.2.25. PR 55194. [Rainer Jung, Kaspar Brand] + *) mod_dav: Fix double encoding of URIs in XML and Location header (caused + by unintential ABI change in 2.2.25). PR 55397. [Ben Reser] + Changes with Apache 2.2.25 *) SECURITY: CVE-2013-1896 (cve.mitre.org) diff --git a/STATUS b/STATUS index 74772fa0030..fc10f7c989d 100644 --- a/STATUS +++ b/STATUS @@ -105,14 +105,6 @@ PATCHES ACCEPTED TO BACKPORT FROM TRUNK: 2.2.x patch: trunk patch to ssl_engine_config.c (above) applies with offset +1: trawick, jorton, wrowe - * mod_dav: Fix 55397. dav_resource->uri treated as unencoded. This was an - unnecessary ABI changed introduced in 2.2.25 - trunk patches: https://svn.apache.org/r1529559 - https://svn.apache.org/r1531505 - 2.4.x: trunk works, CHANGES needs to be written when merging - 2.2.x: https://people.apache.org/~breser/httpd/2.2.x/patches/pr55397.patch - +1: breser, minfrin, wrowe - 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 0502808c652..02daca0b79c 100644 --- a/modules/dav/fs/repos.c +++ b/modules/dav/fs/repos.c @@ -683,14 +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->uri); - if (len > 1 && r->uri[len - 1] == '/') { - s = apr_pstrdup(r->pool, r->uri); - s[len - 1] = '\0'; + len = strlen(r->unparsed_uri); + if (len > 1 && r->unparsed_uri[len - 1] == '/') { + s = apr_pstrmemdup(r->pool, r->unparsed_uri, len-1); resource->uri = s; } else { - resource->uri = r->uri; + resource->uri = r->unparsed_uri; } if (r->finfo.filetype != 0) { @@ -1407,6 +1406,18 @@ 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. */ @@ -1461,6 +1472,7 @@ 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); @@ -1500,7 +1512,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. */ - dav_buffer_place_mem(pool, &fsctx->uri_buf, dirent.name, len + 1, 1); + escaped_len = dav_fs_append_uri(pool, &fsctx->uri_buf, dirent.name, 1); /* if there is a secondary path, then do that, too */ if (fsctx->path2.buf != NULL) { @@ -1533,7 +1545,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 += len + 1; + fsctx->uri_buf.cur_len += escaped_len + 1; fsctx->uri_buf.buf[fsctx->uri_buf.cur_len - 1] = '/'; fsctx->uri_buf.buf[fsctx->uri_buf.cur_len] = '\0'; @@ -1599,8 +1611,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_buffer_place_mem(pool, &fsctx->uri_buf, - fsctx->locknull_buf.buf + offset, len + 1, 0); + dav_fs_append_uri(pool, &fsctx->uri_buf, + fsctx->locknull_buf.buf + offset, 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 ddb9af01a9c..ba0117e51ee 100644 --- a/modules/dav/main/mod_dav.c +++ b/modules/dav/main/mod_dav.c @@ -400,11 +400,9 @@ 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(e_uri, '&') == NULL) - return e_uri; + if (ap_strchr_c(uri, '&') == NULL) + return uri; /* there was a '&', so more work is needed... sigh. */ @@ -412,7 +410,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, e_uri, 0); + return apr_xml_quote_string(p, uri, 0); } @@ -616,7 +614,8 @@ static int dav_handle_err(request_rec *r, dav_error *err, return DONE; } -/* handy function for return values of methods that (may) create things */ +/* handy function for return values of methods that (may) create things. + * locn if provided is assumed to be escaped. */ static int dav_created(request_rec *r, const char *locn, const char *what, int replaced) { @@ -624,8 +623,6 @@ static int dav_created(request_rec *r, const char *locn, const char *what, if (locn == NULL) { locn = r->unparsed_uri; - } else { - locn = ap_escape_uri(r->pool, locn); } /* did the target resource already exist? */ @@ -2972,7 +2969,7 @@ static int dav_method_copymove(request_rec *r, int is_move) } /* return an appropriate response (HTTP_CREATED or HTTP_NO_CONTENT) */ - return dav_created(r, lookup.rnew->uri, "Destination", + return dav_created(r, lookup.rnew->unparsed_uri, "Destination", resnew_state == DAV_RESOURCE_EXISTS); } @@ -4562,7 +4559,7 @@ static int dav_method_bind(request_rec *r) /* return an appropriate response (HTTP_CREATED) */ /* ### spec doesn't say what happens when destination was replaced */ - return dav_created(r, lookup.rnew->uri, "Binding", 0); + return dav_created(r, lookup.rnew->unparsed_uri, "Binding", 0); } diff --git a/modules/dav/main/mod_dav.h b/modules/dav/main/mod_dav.h index 8f2560dce04..64e0e22764e 100644 --- a/modules/dav/main/mod_dav.h +++ b/modules/dav/main/mod_dav.h @@ -370,7 +370,7 @@ typedef struct dav_resource { * REGULAR and WORKSPACE resources, * and is always 1 for WORKING */ - const char *uri; /* the URI for this resource */ + const char *uri; /* the escaped URI for this resource */ dav_resource_private *info; /* the provider's private info */ -- 2.47.2