]> git.ipfire.org Git - thirdparty/apache/httpd.git/commitdiff
Merge r1758307, r1758308, r1758309, r1758311 from trunk:
authorJim Jagielski <jim@apache.org>
Tue, 6 Sep 2016 17:38:34 +0000 (17:38 +0000)
committerJim Jagielski <jim@apache.org>
Tue, 6 Sep 2016 17:38:34 +0000 (17:38 +0000)
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

mpm_winnt: remove the AcceptEx data network bucket

Follow-up to the prior commit: without an incoming data buffer, the
custom network bucket code is now orphaned and we can remove it
entirely. This has the added benefit that we are no longer using the
internal OVERLAPPED.Pointer field, which is discouraged by the MSDN
docs.

mpm_winnt: remove duplication of ap_process_connection

Further follow-up to the previous commit: now that we no longer patch a
network bucket into the brigade, we can revert to calling
ap_process_connection() directly instead of duplicating its logic.

docs: rebuild
Submitted by: jchampion
Reviewed/backported by: jim

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1759471 13f79535-47bb-0310-9956-ffa450edef68

CHANGES
docs/manual/mod/core.html.en
docs/manual/mod/core.xml
docs/manual/mod/core.xml.es
docs/manual/mod/core.xml.fr
docs/manual/mod/core.xml.ja
docs/manual/mod/core.xml.tr
server/core.c
server/mpm/winnt/child.c
server/mpm/winnt/mpm_winnt.c
server/mpm/winnt/mpm_winnt.h

diff --git a/CHANGES b/CHANGES
index 8f01560a66c6355ca92ec91d2c79dd737f4afb42..55d4434f0dc4af98ec9c82eb3cc6ce1cba4aa25f 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -2,6 +2,9 @@
 
 Changes with Apache 2.4.24
 
+  *) 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]
index ae769061a5b87c48d609744c9e1a06ce52ba4192..ea2c0a6addfac390f47a5f30a02e29d0e5125160 100644 (file)
@@ -183,20 +183,15 @@ AcceptFilter https data</pre>
        tcp(7)</a> man page.</p>
 
     <p>The default values on Windows are:</p>
-    <pre class="prettyprint lang-config">AcceptFilter http data
-AcceptFilter https data</pre>
+    <pre class="prettyprint lang-config">AcceptFilter http connect
+AcceptFilter https connect</pre>
 
 
     <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
@@ -204,6 +199,22 @@ AcceptFilter https data</pre>
        network providers such as vpn drivers, or spam, virus or spyware
        filters.</p>
 
+    <div class="warning">
+      <h3>The <code>data</code> AcceptFilter (Windows)</h3>
+
+      <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>
+    </div>
+
 
 <h3>See also</h3>
 <ul>
index 946ce7f642ae43bee6fdc6f4f216a52981a46d24..54442305be78b6a1c9bb576765ecdf8424d2ec94 100644 (file)
@@ -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>
index e35de20159d31467abd1754c1cf3420baad0db47..a9cf1530ce1965e4ffc1d65ea0bfdc2a588b53a1 100644 (file)
@@ -1,8 +1,9 @@
 <?xml version="1.0"?>
 <!DOCTYPE modulesynopsis SYSTEM "../style/modulesynopsis.dtd">
-<?xml-stylesheet type="text/xsl" href="../style/manual.en.xsl"?>
-<!-- English Revision: 1040494:1750752 (outdated) -->
-
+<?xml-stylesheet type="text/xsl" href="../style/manual.es.xsl"?>
+<!-- English Revision: 1741251:1758307 (outdated) -->
+<!-- Translated by Luis Gil de Bernabé Pfeiffer lgilbernabe[AT]apache.org -->
+<!-- Reviewed by Sergio Ramos-->
 <!--
  Licensed to the Apache Software Foundation (ASF) under one or more
  contributor license agreements.  See the NOTICE file distributed with
index b55dc302fe8613018026b24653b4596b1eade141..39949a4ed81adb58834d48716081e07039b5502c 100644 (file)
@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="UTF-8" ?>
 <!DOCTYPE modulesynopsis SYSTEM "../style/modulesynopsis.dtd">
 <?xml-stylesheet type="text/xsl" href="../style/manual.fr.xsl"?>
-<!-- English Revision: 1750752 -->
+<!-- English Revision: 1757920:1758307 (outdated) -->
 <!-- French translation : Lucien GENTIS -->
 <!-- Reviewed by : Vincent Deffontaines -->
 
index eb1743a4d5438cdfb7f2caaa599c52c2662c0de6..7277de33be8ec0dd2802e4d24553a92e8f7f3b5d 100644 (file)
@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="UTF-8" ?>
 <!DOCTYPE modulesynopsis SYSTEM "../style/modulesynopsis.dtd">
 <?xml-stylesheet type="text/xsl" href="../style/manual.ja.xsl"?>
-<!-- English Revision: 669847:1750752 (outdated) -->
+<!-- English Revision: 669847:1758307 (outdated) -->
 
 <!--
  Licensed to the Apache Software Foundation (ASF) under one or more
index 1025e2f93e53a54bf77933042cef366e08ebadbb..879fd182fa713704d4641940c52503e38fe91bcd 100644 (file)
@@ -1,7 +1,7 @@
 <?xml version="1.0"?>
 <!DOCTYPE modulesynopsis SYSTEM "../style/modulesynopsis.dtd">
 <?xml-stylesheet type="text/xsl" href="../style/manual.tr.xsl"?>
-<!-- English Revision: 1750752  -->
+<!-- English Revision: 1302855:1758307 (outdated) -->
 <!-- =====================================================
  Translated by: Nilgün Belma Bugüner <nilgun belgeler.gen.tr>
    Reviewed by: Orhan Berent <berent belgeler.gen.tr>
index e8c1ef675c019019caf1dd99b60da023af477e21..2c64fdc4d2c75a54e66c34f653aab786a9cecd86 100644 (file)
@@ -453,6 +453,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");
index 16204d4e45e80090d7c04023a64975aaf0686698..94a1438c60769d9449645ee50ca3eff988b08701 100644 (file)
@@ -330,8 +330,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)
@@ -357,7 +362,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, 
@@ -383,7 +388,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;
@@ -428,9 +433,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 */
@@ -460,20 +464,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();
@@ -483,8 +479,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;
@@ -499,8 +493,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;
@@ -520,8 +512,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;
@@ -562,14 +552,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;
                 }
             }
@@ -592,28 +578,9 @@ 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;
-            }
         }
         else /* (accf = 0)  e.g. 'none' */
         {
@@ -687,7 +654,6 @@ reinit: /* target of data or connect upon too many AcceptEx failures */
              * os_sock_make and os_sock_put that it does not query).
              */
             WSAEventSelect(context->accept_socket, 0, 0);
-            context->overlapped.Pointer = NULL;
             err_count = 0;
 
             context->sa_server_len = sizeof(context->buff) / 2;
@@ -792,24 +758,6 @@ static winnt_conn_ctx_t *winnt_get_connection(winnt_conn_ctx_t *context)
     return context;
 }
 
-apr_status_t winnt_insert_network_bucket(conn_rec *c,
-                                         apr_bucket_brigade *bb,
-                                         apr_socket_t *socket)
-{
-    apr_bucket *e;
-    winnt_conn_ctx_t *context = ap_get_module_config(c->conn_config,
-                                                     &mpm_winnt_module);
-    if (context == NULL || (e = context->overlapped.Pointer) == NULL)
-        return AP_DECLINED;
-
-    /* seed the brigade with AcceptEx read heap bucket */
-    APR_BRIGADE_INSERT_HEAD(bb, e);
-    /* also seed the brigade with the client socket. */
-    e = apr_bucket_socket_create(socket, c->bucket_alloc);
-    APR_BRIGADE_INSERT_TAIL(bb, e);
-    return APR_SUCCESS;
-}
-
 /*
  * worker_main()
  * Main entry point for the worker threads. Worker threads block in
@@ -823,8 +771,6 @@ static DWORD __stdcall worker_main(void *thread_num_val)
     winnt_conn_ctx_t *context = NULL;
     int thread_num = (int)thread_num_val;
     ap_sb_handle_t *sbh;
-    apr_bucket *e;
-    int rc;
     conn_rec *c;
     apr_int32_t disconnected;
 
@@ -850,8 +796,6 @@ static DWORD __stdcall worker_main(void *thread_num_val)
             }
         }
 
-        e = context->overlapped.Pointer;
-
         ap_create_sb_handle(&sbh, context->ptrans, 0, thread_num);
         c = ap_run_create_connection(context->ptrans, ap_server_conf,
                                      context->sock, thread_num, sbh,
@@ -860,9 +804,6 @@ static DWORD __stdcall worker_main(void *thread_num_val)
         if (!c) {
             /* ap_run_create_connection closes the socket on failure */
             context->accept_socket = INVALID_SOCKET;
-            if (e) { 
-                apr_bucket_free(e);
-            }
             continue;
         }
 
@@ -870,26 +811,7 @@ static DWORD __stdcall worker_main(void *thread_num_val)
         apr_os_thread_put(&thd, &osthd, context->ptrans);
         c->current_thread = thd;
 
-        /* follow ap_process_connection(c, context->sock) logic
-         * as it left us no chance to reinject our first data bucket.
-         */
-        ap_update_vhost_given_ip(c);
-
-        rc = ap_run_pre_connection(c, context->sock);
-        if (rc != OK && rc != DONE) {
-            c->aborted = 1;
-        }
-
-        if (e && c->aborted) {
-            apr_bucket_free(e);
-        }
-        else {
-            ap_set_module_config(c->conn_config, &mpm_winnt_module, context);
-        }
-
-        if (!c->aborted) {
-            ap_run_process_connection(c);
-        }
+        ap_process_connection(c, context->sock);
 
         apr_socket_opt_get(context->sock, APR_SO_DISCONNECTED, &disconnected);
 
index 8d3a5ff8394b7ab1755da0d5c3e4b3a63f10ec98..2487e45be2bbb90ba072377a6207dc89e9b7b0fa 100644 (file)
@@ -1769,8 +1769,6 @@ static void winnt_hooks(apr_pool_t *p)
     ap_hook_mpm(winnt_run, NULL, NULL, APR_HOOK_MIDDLE);
     ap_hook_mpm_query(winnt_query, NULL, NULL, APR_HOOK_MIDDLE);
     ap_hook_mpm_get_name(winnt_get_name, NULL, NULL, APR_HOOK_MIDDLE);
-    ap_hook_insert_network_bucket(winnt_insert_network_bucket, NULL, NULL,
-                                  APR_HOOK_MIDDLE);
 }
 
 AP_DECLARE_MODULE(mpm_winnt) = {
index 8fb595b423a1254f3ede2b00e0bfe178adbf615b..22ba001407e0ca317900340a291babcdfdc66285 100644 (file)
@@ -91,9 +91,6 @@ void hold_console_open_on_error(void);
 
 /* From child.c: */
 void child_main(apr_pool_t *pconf, DWORD parent_pid);
-apr_status_t winnt_insert_network_bucket(conn_rec *c,
-                                         apr_bucket_brigade *bb,
-                                         apr_socket_t *socket);
 
 #endif /* APACHE_MPM_WINNT_H */
 /** @} */