You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jim Meyering <ji...@meyering.net> on 2012/07/05 19:33:18 UTC
[PATCH] don't access(r/w) uri[-1] when validating resource w/empty uri string
At first I thought there must be code to guarantee
that a URI (resource->uri) has length > 0, but since I found
similar guards against precisely that case, e.g.,
modules/dav/fs/repos.c-822
char *uri = ap_make_dirstr_parent(ctx->pool, resource->uri);
if (strlen(uri) > 1 && uri[strlen(uri) - 1] == '/')
uri[strlen(uri) - 1] = '\0';
modules/mappers/mod_dir.c-231
/* Redirect requests that are not '/' terminated */
if (r->uri[0] == '\0' || r->uri[strlen(r->uri) - 1] != '/')
modules/metadata/mod_cern_meta.c:293
if (r->finfo.filetype == APR_DIR || r->uri[strlen(r->uri) - 1] == '/') {
[ As I was looking through these other examples, I see that
a zero-length r->uri could cause trouble here, too, since
the above is *not* guarded. ]
it seems best to guard the use below, too:
>From 5609908643d8456c6f56197102161e56d87e56c4 Mon Sep 17 00:00:00 2001
From: Jim Meyering <me...@redhat.com>
Date: Thu, 7 Jun 2012 20:36:16 +0200
Subject: [PATCH] don't access(r/w) uri[-1] when validating resource w/empty
uri string
* modules/dav/main/util.c (dav_validate_resource_state):
Handle a zero-length URI string.
---
modules/dav/main/util.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/modules/dav/main/util.c b/modules/dav/main/util.c
index d076cc4..adddded 100644
--- a/modules/dav/main/util.c
+++ b/modules/dav/main/util.c
@@ -984,11 +984,11 @@ static dav_error * dav_validate_resource_state(apr_pool_t *p,
** URIs, but the majority of URIs provided to us via a resource walk
** will not contain that trailing slash.
*/
uri = resource->uri;
uri_len = strlen(uri);
- if (uri[uri_len - 1] == '/') {
+ if (uri_len > 1 && uri[uri_len - 1] == '/') {
dav_set_bufsize(p, pbuf, uri_len);
memcpy(pbuf->buf, uri, uri_len);
pbuf->buf[--uri_len] = '\0';
uri = pbuf->buf;
}
--
1.7.11.1.116.g8228a23
Re: [PATCH] don't access(r/w) uri[-1] when validating resource w/empty uri string
Posted by Jim Meyering <ji...@meyering.net>.
Nick Kew wrote:
> On Thu, 05 Jul 2012 19:33:18 +0200
> Jim Meyering <ji...@meyering.net> wrote:
>
> Thanks for the patch, but can you clarify?
>
>> At first I thought there must be code to guarantee
>> that a URI (resource->uri) has length > 0,
>
> In principle it must be for an HTTP request to exist.
> Have you found a testcase or code path in which it fails?
Hi Nick,
I found it via inspection.
No reproducer (if I had one, I would have posted it)
>> but since I found
>> similar guards against precisely that case, e.g.,
>
> That could be because it's necessary, but it could also be
> because the authors of those bits of code were over-cautious,
> or because the code is old and extra tests were needed when
> it was written. The mod_dav instance is a different URI.
>
>> it seems best to guard the use below, too:
>
> Indeed, if there is a code path that leaves null or empty
> r->uri then fixing it is right.
>
>> From 5609908643d8456c6f56197102161e56d87e56c4 Mon Sep 17 00:00:00 2001
>
> Huh? Did you send a patch in 2001?
Sure, plenty, but not that one ;-)
I cloned from git://git.apache.org/httpd.git and posted
the output of "git format-patch". That type of patch
always includes such a "From " line. The Date: line
is the one that matters.
>> uri = resource->uri;
>> uri_len = strlen(uri);
>> - if (uri[uri_len - 1] == '/') {
>> + if (uri_len > 1 && uri[uri_len - 1] == '/') {
>
> That fixes empty uri, but segfaults on null URI.
> Makes sense if the first is possible but the second isn't.
If "uri" is NULL, then the preceding strlen would segfault,
regardless of the proposed change. It seems pretty clear,
from all of the unprotected dereferences like that strlen,
that resource->uri cannot be NULL. However, I could not
see anything that guarantees it is not the empty string.
Re: [PATCH] don't access(r/w) uri[-1] when validating resource
w/empty uri string
Posted by Nick Kew <ni...@webthing.com>.
On Thu, 05 Jul 2012 19:33:18 +0200
Jim Meyering <ji...@meyering.net> wrote:
Thanks for the patch, but can you clarify?
> At first I thought there must be code to guarantee
> that a URI (resource->uri) has length > 0,
In principle it must be for an HTTP request to exist.
Have you found a testcase or code path in which it fails?
> but since I found
> similar guards against precisely that case, e.g.,
That could be because it's necessary, but it could also be
because the authors of those bits of code were over-cautious,
or because the code is old and extra tests were needed when
it was written. The mod_dav instance is a different URI.
> it seems best to guard the use below, too:
Indeed, if there is a code path that leaves null or empty
r->uri then fixing it is right.
> From 5609908643d8456c6f56197102161e56d87e56c4 Mon Sep 17 00:00:00 2001
Huh? Did you send a patch in 2001?
> uri = resource->uri;
> uri_len = strlen(uri);
> - if (uri[uri_len - 1] == '/') {
> + if (uri_len > 1 && uri[uri_len - 1] == '/') {
That fixes empty uri, but segfaults on null URI.
Makes sense if the first is possible but the second isn't.
--
Nick Kew