You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Yann Ylavic <yl...@apache.org> on 2014/11/17 17:06:03 UTC
Re: svn commit: r1640036 - in /httpd/httpd/trunk: CHANGES
modules/aaa/mod_authnz_fcgi.c modules/proxy/mod_proxy_fcgi.c
On Sun, Nov 16, 2014 at 11:04 PM, <yl...@apache.org> wrote:
> Author: ylavic
> Date: Sun Nov 16 22:04:39 2014
> New Revision: 1640036
>
> URL: http://svn.apache.org/r1640036
> Log:
> mod_proxy_fcgi, mod_authnz_fcgi: SECURITY: CVE-2014-3583 (cve.mitre.org)
> Fix a potential crash with response headers' size above 8K.
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/modules/aaa/mod_authnz_fcgi.c
Hmm, in fact mod_authnz_fcgi is *not* vulerable to this CVE (unlike
mod_proxy_fcgi) since it explicitely reserves and forces the trailing
'\0' before calling handle_headers().
It participates in responses splitting, though, by ignoring anything
after the first '\0' encountered.
So should I limit this CVE to mod_proxy_fcgi only, or by an (shameful)
abuse let the proposed backport as is, or even go through with it and
add the below follow-up (we don't need an extra char now)?
Index: modules/aaa/mod_authnz_fcgi.c
===================================================================
--- modules/aaa/mod_authnz_fcgi.c (revision 1640040)
+++ modules/aaa/mod_authnz_fcgi.c (working copy)
@@ -491,7 +491,7 @@ static apr_status_t handle_response(const fcgi_pro
apr_size_t readbuflen;
apr_uint16_t clen;
apr_uint16_t rid;
- char readbuf[AP_IOBUFSIZE + 1];
+ char readbuf[AP_IOBUFSIZE];
unsigned char farray[AP_FCGI_HEADER_LEN];
unsigned char plen;
unsigned char type;
@@ -527,8 +527,8 @@ static apr_status_t handle_response(const fcgi_pro
recv_again: /* if we need to keep reading more of a record's content */
- if (clen > sizeof(readbuf) - 1) {
- readbuflen = sizeof(readbuf) - 1;
+ if (clen > sizeof(readbuf)) {
+ readbuflen = sizeof(readbuf);
} else {
readbuflen = clen;
}
@@ -541,7 +541,6 @@ static apr_status_t handle_response(const fcgi_pro
if (rv != APR_SUCCESS) {
break;
}
- readbuf[readbuflen] = '\0';
}
switch (type) {
[END]
Also note that both modules are still vulnerable to an infinite header
(no LF) sent by the backend (the buckets are setaside in memory until
then).
Shouldn't we apply some reasonable hard limit (8/16/32K)?
Regards,
Yann.
Re: svn commit: r1640036 - in /httpd/httpd/trunk: CHANGES
modules/aaa/mod_authnz_fcgi.c modules/proxy/mod_proxy_fcgi.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Mon, Nov 17, 2014 at 11:06 AM, Yann Ylavic <yl...@apache.org> wrote:
> On Sun, Nov 16, 2014 at 11:04 PM, <yl...@apache.org> wrote:
> > Author: ylavic
> > Date: Sun Nov 16 22:04:39 2014
> > New Revision: 1640036
> >
> > URL: http://svn.apache.org/r1640036
> > Log:
> > mod_proxy_fcgi, mod_authnz_fcgi: SECURITY: CVE-2014-3583 (cve.mitre.org)
> > Fix a potential crash with response headers' size above 8K.
> >
> > Modified:
> > httpd/httpd/trunk/CHANGES
> > httpd/httpd/trunk/modules/aaa/mod_authnz_fcgi.c
>
> Hmm, in fact mod_authnz_fcgi is *not* vulerable to this CVE (unlike
> mod_proxy_fcgi) since it explicitely reserves and forces the trailing
> '\0' before calling handle_headers().
>
> It participates in responses splitting, though, by ignoring anything
> after the first '\0' encountered.
> So should I limit this CVE to mod_proxy_fcgi only, or by an (shameful)
> abuse let the proposed backport as is, or even go through with it and
> add the below follow-up (we don't need an extra char now)?
>
If it were me, I'd let the unshameful backport go through as is (but with
CHANGES and revision log update to reflect that authnz_fcgi was not
vulnerable, but the changes to handle_headers() were replicated to keep
them in sync). And then propose this small optimization separately.
>
> Index: modules/aaa/mod_authnz_fcgi.c
> ===================================================================
> --- modules/aaa/mod_authnz_fcgi.c (revision 1640040)
> +++ modules/aaa/mod_authnz_fcgi.c (working copy)
> @@ -491,7 +491,7 @@ static apr_status_t handle_response(const fcgi_pro
> apr_size_t readbuflen;
> apr_uint16_t clen;
> apr_uint16_t rid;
> - char readbuf[AP_IOBUFSIZE + 1];
> + char readbuf[AP_IOBUFSIZE];
> unsigned char farray[AP_FCGI_HEADER_LEN];
> unsigned char plen;
> unsigned char type;
> @@ -527,8 +527,8 @@ static apr_status_t handle_response(const fcgi_pro
>
> recv_again: /* if we need to keep reading more of a record's content
> */
>
> - if (clen > sizeof(readbuf) - 1) {
> - readbuflen = sizeof(readbuf) - 1;
> + if (clen > sizeof(readbuf)) {
> + readbuflen = sizeof(readbuf);
> } else {
> readbuflen = clen;
> }
> @@ -541,7 +541,6 @@ static apr_status_t handle_response(const fcgi_pro
> if (rv != APR_SUCCESS) {
> break;
> }
> - readbuf[readbuflen] = '\0';
> }
>
> switch (type) {
> [END]
>
> Also note that both modules are still vulnerable to an infinite header
> (no LF) sent by the backend (the buckets are setaside in memory until
> then).
> Shouldn't we apply some reasonable hard limit (8/16/32K)?
>
> Regards,
> Yann.
>
--
Born in Roswell... married an alien...
http://emptyhammock.com/