You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by ja...@apache.org on 2014/04/04 22:30:39 UTC
svn commit: r1584896 - /httpd/httpd/trunk/modules/filters/mod_proxy_html.c
Author: jailletc36
Date: Fri Apr 4 20:30:38 2014
New Revision: 1584896
URL: http://svn.apache.org/r1584896
Log:
Do not perform a p+= 7 that could go past the end of the buffer in case we find a 'content' without a corresponding '='.
Should we need to deal with this case, a new search should be performed to find the real starting position of another potential 'content=' pattern.
Modified:
httpd/httpd/trunk/modules/filters/mod_proxy_html.c
Modified: httpd/httpd/trunk/modules/filters/mod_proxy_html.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_proxy_html.c?rev=1584896&r1=1584895&r2=1584896&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/filters/mod_proxy_html.c (original)
+++ httpd/httpd/trunk/modules/filters/mod_proxy_html.c Fri Apr 4 20:30:38 2014
@@ -672,8 +672,9 @@ static meta *metafix(request_rec *r, con
p += 7;
while (apr_isspace(*p))
++p;
+ /* XXX Should we search for another content= pattern? */
if (*p != '=')
- continue;
+ break;
while (*p && apr_isspace(*++p));
if ((*p == '\'') || (*p == '"')) {
delim = *p++;
Re: svn commit: r1584896 - /httpd/httpd/trunk/modules/filters/mod_proxy_html.c
Posted by Marion & Christophe JAILLET <ch...@wanadoo.fr>.
r1588356
> Should you share my analysis and should a CHANGE be useful for what I
> think is a corner case, feel free to add something, or I can do it by
> the end of the week.
>
>>
>> Does this fix a crash or a parsing error or ...? (CHANGES)
>>
Re: svn commit: r1584896 - /httpd/httpd/trunk/modules/filters/mod_proxy_html.c
Posted by Marion & Christophe JAILLET <ch...@wanadoo.fr>.
Hi,
AFAIK, no crash has ever been reported for that.
I just noted this while looking at PR56287 and found it odd.
A file such as:
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-
<html><head>
<meta http-equiv="Conten" content><head><body></body></html>
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-
will trigger the scan past the end of the buffer behavior.
line 658: ap_regexec(<meta[^>]*(http-equiv)[^>]*>) OK
line 665: strncasecmp(header, "Content-" OK, string not
found
line 667: apr_strmatch("content" OK
then looping with +7 every time until reaching a \0 because no '=' is in
the buffer
In the example above, we go past then end of 'buf' because of the
unconditional +7 and no check that we are about to scan past the end of
'buf'
So, the ocde with a 'continue' can:
- eat time until we find a \0 somewhere
- do a 'content = apr_pstrndup' with unexpected data if we finally
reach a "=" (however, the strdup will not copy data past the end of buf)
- crash if we try to read memory that does not belong to the process ?
So I imagine that someone who can write a file on the server behind the
proxy could set up what looks like a DoS, expecting to crash a process
from time to time.
All this is true if 'ProxyHTMLMeta' is on, which is not the default.
This is all theoretical and with my tests, I never managed to crash my
server.
Anyway, the logic that was in place was broken. My change is not perfect
(we should try to find another 'content', if any, to keep the spirit of
the code), but is, IMO, safer because avoid a possible crash.
Should you share my analysis and should a CHANGE be useful for what I
think is a corner case, feel free to add something, or I can do it by
the end of the week.
Best regards,
CJ
Le 15/04/2014 21:16, Jeff Trawick a écrit :
> On Fri, Apr 4, 2014 at 4:30 PM, <jailletc36@apache.org
> <ma...@apache.org>> wrote:
>
> Author: jailletc36
> Date: Fri Apr 4 20:30:38 2014
> New Revision: 1584896
>
> URL: http://svn.apache.org/r1584896
> Log:
> Do not perform a p+= 7 that could go past the end of the buffer in
> case we find a 'content' without a corresponding '='.
>
>
> Does this fix a crash or a parsing error or ...? (CHANGES)
>
>
> Should we need to deal with this case, a new search should be
> performed to find the real starting position of another potential
> 'content=' pattern.
>
> Modified:
> httpd/httpd/trunk/modules/filters/mod_proxy_html.c
>
> Modified: httpd/httpd/trunk/modules/filters/mod_proxy_html.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_proxy_html.c?rev=1584896&r1=1584895&r2=1584896&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/filters/mod_proxy_html.c (original)
> +++ httpd/httpd/trunk/modules/filters/mod_proxy_html.c Fri Apr 4
> 20:30:38 2014
> @@ -672,8 +672,9 @@ static meta *metafix(request_rec *r, con
> p += 7;
> while (apr_isspace(*p))
> ++p;
> + /* XXX Should we search for another content=
> pattern? */
> if (*p != '=')
> - continue;
> + break;
> while (*p && apr_isspace(*++p));
> if ((*p == '\'') || (*p == '"')) {
> delim = *p++;
>
>
>
>
>
> --
> Born in Roswell... married an alien...
> http://emptyhammock.com/
> http://edjective.org/
>
Re: svn commit: r1584896 - /httpd/httpd/trunk/modules/filters/mod_proxy_html.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Fri, Apr 4, 2014 at 4:30 PM, <ja...@apache.org> wrote:
> Author: jailletc36
> Date: Fri Apr 4 20:30:38 2014
> New Revision: 1584896
>
> URL: http://svn.apache.org/r1584896
> Log:
> Do not perform a p+= 7 that could go past the end of the buffer in case we
> find a 'content' without a corresponding '='.
>
Does this fix a crash or a parsing error or ...? (CHANGES)
>
> Should we need to deal with this case, a new search should be performed to
> find the real starting position of another potential 'content=' pattern.
>
> Modified:
> httpd/httpd/trunk/modules/filters/mod_proxy_html.c
>
> Modified: httpd/httpd/trunk/modules/filters/mod_proxy_html.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_proxy_html.c?rev=1584896&r1=1584895&r2=1584896&view=diff
>
> ==============================================================================
> --- httpd/httpd/trunk/modules/filters/mod_proxy_html.c (original)
> +++ httpd/httpd/trunk/modules/filters/mod_proxy_html.c Fri Apr 4 20:30:38
> 2014
> @@ -672,8 +672,9 @@ static meta *metafix(request_rec *r, con
> p += 7;
> while (apr_isspace(*p))
> ++p;
> + /* XXX Should we search for another content= pattern?
> */
> if (*p != '=')
> - continue;
> + break;
> while (*p && apr_isspace(*++p));
> if ((*p == '\'') || (*p == '"')) {
> delim = *p++;
>
>
>
--
Born in Roswell... married an alien...
http://emptyhammock.com/
http://edjective.org/