Re: Patch for denial of service vulnerability in WEBrick

Hi,

In message <[e-mail]>,
 `Dominique Brezinski <[e-mail]>' wrote:
> If
> connections are terminated during the SSL handsake, an exception is
> raised that is not properly handled in webrick/server.rb. The result
> is that there is a leak in the token queue used to limit the number of
> threads created to handle connections.

Thanks.

> Here is a patch for 1.8.2 that fixes this and a related leak:

I want to simplify operations about @tokens as soon as
possible. How do you think about the following patch?
-- 
gotoyuzo

--- lib/webrick/server.rb	18 Jan 2005 06:03:43 -0000	1.5.2.4
+++ lib/webrick/server.rb	5 Mar 2005 20:01:17 -0000
@@ -88,17 +88,15 @@     def start(&block)
             if svrs = IO.select(@listeners, nil, nil, 2.0)
               svrs[0].each{|svr|
                 @tokens.pop          # blocks while no token is there.
-                sock = svr.accept
-                sock.sync = true
-                Utils::set_close_on_exec(sock)
-                th = start_thread(sock, &block)
-                th[:WEBrickThread] = true
-                thgroup.add(th)
+                if sock = accept_client(svr)
+                  th = start_thread(sock, &block)
+                  th[:WEBrickThread] = true
+                  thgroup.add(th)
+                else
+                  @tokens.push(nil)
+                end
               }
             end
-          rescue Errno::ECONNRESET, Errno::ECONNABORTED, Errno::EPROTO => ex
-            # TCP connection was established but RST segment was sent
-            # from peer before calling TCPServer#accept.
           rescue Errno::EBADF, IOError => ex
             # if the listening socket was closed in GenericServer#shutdown,
             # IO::select raise it.
@@ -140,6 +138,22 @@     def run(sock)
 
     private
 
+    def accept_client(svr)
+      sock = nil
+      begin
+        sock = svr.accept
+        sock.sync = true
+        Utils::set_close_on_exec(sock)
+      rescue Errno::ECONNRESET, Errno::ECONNABORTED, Errno::EPROTO => ex
+        # TCP connection was established but RST segment was sent
+        # from peer before calling TCPServer#accept.
+      rescue Exception => ex
+        msg = "#{ex.class}: #{ex.message}\n\t#{ex.backtrace[0]}"
+        @logger.error msg
+      end
+      return sock
+    end
+
     def start_thread(sock, &block)
       Thread.start{
         begin
@@ -161,6 +175,7 @@     def start_thread(sock, &block)
         rescue Exception => ex
           @logger.error ex
         ensure
+          @tokens.push(nil)
           Thread.current[:WEBrickSocket] = nil
           if addr
             @logger.debug "close: #{addr[3]}:#{addr[1]}"
@@ -169,7 +184,6 @@     def start_thread(sock, &block)
           end
           sock.close
         end
-        @tokens.push(nil)
       }
     end