mpm_winnt: remove 'data' AcceptFilter in favor of 'connect'

The 'data' AcceptFilter optimization instructs Windows to wait until
data is received on a connection before completing the AcceptEx
operation. Unfortunately, it seems this isn't performed atomically --
AcceptEx "partially" accepts the incoming connection during the wait for
data, leaving all other incoming connections in the accept queue. This
opens the server to a denial of service.

Since the fix for this requires a substantial rearchitecture (likely
involving multiple outstanding calls to AcceptEx), disable the 'data'
filter for now and replace it with 'connect', which uses the AcceptEx
interface but does not wait for data.

Users running prior releases of httpd on Windows should explicitly move
to a 'connect' AcceptFilter in their configurations if they are
currently using the default 'data' filter.

Many thanks to mludha, Arthur Ramsey, Paul Spangler, and many others for
their assistance in tracking down and diagnosing this issue.

PR: 59970

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1758307 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/CHANGES b/CHANGES
index d8ed6944b14c6167070b9f56f2f1789fe420a0d3..3b1f57ca099606619f46e48ca9585d66e4d60680 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -1,6 +1,9 @@
                                                          -*- coding: utf-8 -*-
 Changes with Apache 2.5.0
 
+  *) mpm_winnt: Prevent a denial of service when the 'data' AcceptFilter is in
+     use by replacing it with the 'connect' filter. PR 59970. [Jacob Champion]
+
   *) mod_cgid: Resolve a case where a short CGI response causes a subsequent
      CGI to be killed prematurely, resulting in a truncated subsequent
      response. [Eric Covener]

diff --git a/docs/log-message-tags/next-number b/docs/log-message-tags/next-number
index a82b2ee2a7015a43e8f2b30243c0e178cd369ec1..c4b489825b33f0d6ee901e57bce26a5cd66f888d 100644 (file)
--- a/docs/log-message-tags/next-number
+++ b/docs/log-message-tags/next-number
@@ -1 +1 @@
-3458
+3459

diff --git a/docs/manual/mod/core.xml b/docs/manual/mod/core.xml
index 56a1f672b3bd23e4027d7a5e56575d5ca2310f54..a5ae5830b4d5319b189b60155abe8629f7392f8e 100644 (file)
--- a/docs/manual/mod/core.xml
+++ b/docs/manual/mod/core.xml
@@ -85,20 +85,15 @@ AcceptFilter https data
 
     <p>The default values on Windows are:</p>
     <highlight language="config">
-AcceptFilter http data
-AcceptFilter https data
+AcceptFilter http connect
+AcceptFilter https connect
     </highlight>
 
     <p>Window's mpm_winnt interprets the AcceptFilter to toggle the AcceptEx()
-       API, and does not support http protocol buffering.  There are two values
-       which utilize the Windows AcceptEx() API and will recycle network
-       sockets between connections.  <code>data</code> waits until data has
-       been transmitted as documented above, and the initial data buffer and
-       network endpoint addresses are all retrieved from the single AcceptEx()
-       invocation.  <code>connect</code> will use the AcceptEx() API, also
-       retrieve the network endpoint addresses, but like <code>none</code>
-       the <code>connect</code> option does not wait for the initial data
-       transmission.</p>
+       API, and does not support http protocol buffering. <code>connect</code>
+       will use the AcceptEx() API, also retrieve the network endpoint
+       addresses, but like <code>none</code> the <code>connect</code> option
+       does not wait for the initial data transmission.</p>
 
     <p>On Windows, <code>none</code> uses accept() rather than AcceptEx()
        and will not recycle sockets between connections.  This is useful for
@@ -106,6 +101,22 @@ AcceptFilter https data
        network providers such as vpn drivers, or spam, virus or spyware
        filters.</p>
 
+    <note type="warning">
+      <title>The <code>data</code> AcceptFilter (Windows)</title>
+
+      <p>For versions 2.4.23 and prior, the Windows <code>data</code> accept
+         filter waited until data had been transmitted and the initial data
+         buffer and network endpoint addresses had been retrieved from the
+         single AcceptEx() invocation. This implementation was subject to a
+         denial of service attack and has been disabled.</p>
+
+      <p>Current releases of httpd default to the <code>connect</code> filter
+         on Windows, and will fall back to <code>connect</code> if
+         <code>data</code> is specified. Users of prior releases are encouraged
+         to add an explicit setting of <code>connect</code> for their
+         AcceptFilter, as shown above.</p>
+    </note>
+
 </usage>
 <seealso><directive module="core">Protocol</directive></seealso>
 </directivesynopsis>

diff --git a/server/core.c b/server/core.c
index 92b2fc4b59dbc5f35747c769f18795a40c59499f..75e9218eeedd52d3cd3ed1290394711f067e6c6d 100644 (file)
--- a/server/core.c
+++ b/server/core.c
@@ -464,6 +464,10 @@ static void *create_core_server_config(apr_pool_t *a, server_rec *s)
 #if APR_HAS_SO_ACCEPTFILTER
         apr_table_setn(conf->accf_map, "http", ACCEPT_FILTER_NAME);
         apr_table_setn(conf->accf_map, "https", "dataready");
+#elif defined(WIN32)
+        /* 'data' is disabled on Windows due to a DoS vuln (PR 59970) */
+        apr_table_setn(conf->accf_map, "http", "connect");
+        apr_table_setn(conf->accf_map, "https", "connect");
 #else
         apr_table_setn(conf->accf_map, "http", "data");
         apr_table_setn(conf->accf_map, "https", "data");

diff --git a/server/mpm/winnt/child.c b/server/mpm/winnt/child.c
index e9a719051609fc2906e0dde235d43c996f23e0d5..321a0240d819c831af33b967887aeafff4197283 100644 (file)
--- a/server/mpm/winnt/child.c
+++ b/server/mpm/winnt/child.c
@@ -323,8 +323,13 @@ static unsigned int __stdcall winnt_accept(void *lr_)
                      "no known accept filter. Using 'none' instead",
                      lr->protocol);
     }
-    else if (strcmp(accf_name, "data") == 0)
-        accf = 2;
+    else if (strcmp(accf_name, "data") == 0) {
+        accf = 1;
+        accf_name = "connect";
+        ap_log_error(APLOG_MARK, APLOG_INFO, 0, ap_server_conf,
+                     APLOGNO(03458) "winnt_accept: 'data' accept filter is no "
+                     "longer supported. Using 'connect' instead");
+    }
     else if (strcmp(accf_name, "connect") == 0)
         accf = 1;
     else if (strcmp(accf_name, "none") == 0)
@@ -350,7 +355,7 @@ static unsigned int __stdcall winnt_accept(void *lr_)
    }
 #endif
 
-    if (accf > 0) /* 'data' or 'connect' */
+    if (accf > 0) /* 'connect' */
     {
         if (WSAIoctl(nlsd, SIO_GET_EXTENSION_FUNCTION_POINTER,
                      &GuidAcceptEx, sizeof GuidAcceptEx, 
@@ -376,7 +381,7 @@ static unsigned int __stdcall winnt_accept(void *lr_)
     }
     else /* accf == 0, 'none' */
     {
-reinit: /* target of data or connect upon too many AcceptEx failures */
+reinit: /* target of connect upon too many AcceptEx failures */
 
         /* last, low priority event is a not yet accepted connection */
         events[0] = exit_event;
@@ -421,9 +426,8 @@ reinit: /* target of data or connect upon too many AcceptEx failures */
             }
         }
 
-        if (accf > 0) /* Either 'connect' or 'data' */
+        if (accf > 0) /* 'connect' */
         {
-            DWORD len;
             char *buf;
 
             /* Create and initialize the accept socket */
@@ -453,20 +457,12 @@ reinit: /* target of data or connect upon too many AcceptEx failures */
                 continue;
             }
 
-            if (accf == 2) { /* 'data' */
-                len = APR_BUCKET_BUFF_SIZE;
-                buf = apr_bucket_alloc(len, context->ba);
-                len -= PADDED_ADDR_SIZE * 2;
-            }
-            else /* (accf == 1) 'connect' */ {
-                len = 0;
-                buf = context->buff;
-            }
+            buf = context->buff;
 
             /* AcceptEx on the completion context. The completion context will be
              * signaled when a connection is accepted.
              */
-            if (!lpfnAcceptEx(nlsd, context->accept_socket, buf, len,
+            if (!lpfnAcceptEx(nlsd, context->accept_socket, buf, 0,
                               PADDED_ADDR_SIZE, PADDED_ADDR_SIZE, &BytesRead,
                               &context->overlapped)) {
                 rv = apr_get_netos_error();
@@ -476,8 +472,6 @@ reinit: /* target of data or connect upon too many AcceptEx failures */
                      * 1) the client disconnects early
                      * 2) handshake was incomplete
                      */
-                    if (accf == 2)
-                        apr_bucket_free(buf);
                     closesocket(context->accept_socket);
                     context->accept_socket = INVALID_SOCKET;
                     continue;
@@ -492,8 +486,6 @@ reinit: /* target of data or connect upon too many AcceptEx failures */
                      * 3) the dynamic address / adapter has changed
                      * Give five chances, then fall back on AcceptFilter 'none'
                      */
-                    if (accf == 2)
-                        apr_bucket_free(buf);
                     closesocket(context->accept_socket);
                     context->accept_socket = INVALID_SOCKET;
                     ++err_count;
@@ -513,8 +505,6 @@ reinit: /* target of data or connect upon too many AcceptEx failures */
                 }
                 else if ((rv != APR_FROM_OS_ERROR(ERROR_IO_PENDING)) &&
                          (rv != APR_FROM_OS_ERROR(WSA_IO_PENDING))) {
-                    if (accf == 2)
-                        apr_bucket_free(buf);
                     closesocket(context->accept_socket);
                     context->accept_socket = INVALID_SOCKET;
                     ++err_count;
@@ -555,14 +545,10 @@ reinit: /* target of data or connect upon too many AcceptEx failures */
                     /* exit_event triggered or event handle was closed */
                     closesocket(context->accept_socket);
                     context->accept_socket = INVALID_SOCKET;
-                    if (accf == 2)
-                        apr_bucket_free(buf);
                     break;
                 }
 
                 if (context->accept_socket == INVALID_SOCKET) {
-                    if (accf == 2)
-                        apr_bucket_free(buf);
                     continue;
                 }
             }
@@ -585,28 +571,11 @@ reinit: /* target of data or connect upon too many AcceptEx failures */
             /* Get the local & remote address
              * TODO; error check
              */
-            lpfnGetAcceptExSockaddrs(buf, len, PADDED_ADDR_SIZE, PADDED_ADDR_SIZE,
+            lpfnGetAcceptExSockaddrs(buf, 0, PADDED_ADDR_SIZE, PADDED_ADDR_SIZE,
                                      &context->sa_server, &context->sa_server_len,
                                      &context->sa_client, &context->sa_client_len);
 
-            /* For 'data', craft a bucket for our data result
-             * and pass to worker_main as context->overlapped.Pointer
-             */
-            if (accf == 2 && BytesRead)
-            {
-                apr_bucket *b;
-                b = apr_bucket_heap_create(buf, APR_BUCKET_BUFF_SIZE,
-                                           apr_bucket_free, context->ba);
-                /* Adjust the bucket to refer to the actual bytes read */
-                b->length = BytesRead;
-                context->overlapped.Pointer = b;
-            }
-            else {
-                if (accf == 2) {
-                    apr_bucket_free(buf);
-                }
-                context->overlapped.Pointer = NULL;
-            }
+            context->overlapped.Pointer = NULL;
         }
         else /* (accf = 0)  e.g. 'none' */
         {