On Mon, Nov 17, 2014 at 11:06 AM, Yann Ylavic <[email protected]> wrote:
> On Sun, Nov 16, 2014 at 11:04 PM, <[email protected]> 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/
