httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@gmail.com>
Subject Re: svn commit: r1640036 - in /httpd/httpd/trunk: CHANGES modules/aaa/mod_authnz_fcgi.c modules/proxy/mod_proxy_fcgi.c
Date Tue, 18 Nov 2014 12:47:04 GMT
On Mon, Nov 17, 2014 at 11:06 AM, Yann Ylavic <ylavic@apache.org> wrote:

> On Sun, Nov 16, 2014 at 11:04 PM,  <ylavic@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/

Mime
View raw message