Re: [PATCH] don't corrupt heap upon empty response from OCSP server
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? 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. 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: 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 Regards, Joe
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!
[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
