Re: [PATCH] don't corrupt heap upon empty response from OCSP server
Joe Orton wrote: Hi Jim, On Thu, Jul 05, 2012 at 01:49:25PM +0200, Jim Meyering wrote: This is my first httpd patch/report. If you'd prefer that it go to a BZ or a different list, just let me know. This is fine! I found this by inspection: it appears that line[-1] (the heap) can be corrupted. Is it possible for len to be 0 at that point? It looks like it, since the preceding block guards against the len == 0 case. However, I have not tried to trigger the flaw. Interesting. Are you using static analysis tools to find these? No. In this case I used grep with visual inspection. I'm not sure it would be possible for apr_brigade_split_line() to find a zero-length string without error, but certainly the code is wrong. ... See docs/log-message-tags/ for reference here, keeping the existing number is correct. Thanks for the patch, committed: http://svn.apache.org/viewvc?view=revisionrevision=1358061 Thanks!
Re: [PATCH] don't access(r/w) uri[-1] when validating resource w/empty uri string
Nick Kew wrote:
On Thu, 05 Jul 2012 19:33:18 +0200
Jim Meyering j...@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.
[PATCH] don't corrupt heap upon empty response from OCSP server
Hi,
This is my first httpd patch/report.
If you'd prefer that it go to a BZ or a different list, just let me know.
I found this by inspection: it appears that line[-1] (the heap) can be
corrupted. Is it possible for len to be 0 at that point? It looks like
it, since the preceding block guards against the len == 0 case.
However, I have not tried to trigger the flaw.
A minor note: From the documentation of APLOGNO, it was not clear
whether I should change 01979, given that this patch changes its guard
condition in such a small way, so I left it. You may want to burn the
01979 and simply use a new number.
Also, I didn't know of a recommended method for finding a number
for the new diagnostic, so I did a quick and dirty:
git grep -w APLOGNO|sed 's/.*APLOGNO.//'|sort -nr|head
From 71485156919f20d2e0bf57370f5d520d0bff1da0 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Thu, 7 Jun 2012 22:48:15 +0200
Subject: [PATCH] don't corrupt heap upon empty response from OCSP server
* modules/ssl/ssl_util_ocsp.c (get_line): Don't set line[-1] to 0
when len == 0.
---
modules/ssl/ssl_util_ocsp.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/modules/ssl/ssl_util_ocsp.c b/modules/ssl/ssl_util_ocsp.c
index 94ef4cd..e5c5e58 100644
--- a/modules/ssl/ssl_util_ocsp.c
+++ b/modules/ssl/ssl_util_ocsp.c
@@ -147,19 +147,25 @@ static char *get_line(apr_bucket_brigade *bbout,
apr_bucket_brigade *bbin,
}
rv = apr_brigade_pflatten(bbout, line, len, p);
if (rv) {
ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c, APLOGNO(01978)
failed reading line from OCSP server);
return NULL;
}
-if (len line[len-1] != APR_ASCII_LF) {
+if (len == 0) {
+ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c, APLOGNO(02321)
+ empty response from OCSP server);
+return NULL;
+}
+
+if (line[len-1] != APR_ASCII_LF) {
ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c, APLOGNO(01979)
response header line too long from OCSP server);
return NULL;
}
line[len-1] = '\0';
if (len 1 line[len-2] == APR_ASCII_CR) {
line[len-2] = '\0';
}
--
1.7.11.1.116.g8228a23
[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 meyer...@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..aed 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
