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)?

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.

Reply via email to