CVE-2019-16779: Concurrent Execution using Shared Resource with Improper Synchronization ('Race Condition') (CWE-362)
value: In RubyGem excon before 0.71.0, there was a race condition around persistent connections, where a connection which is interrupted (such as by a timeout) would leave data on the socket. Subsequent requests would then read this data, returning content from the previous response. The race condition window appears to be short, and it would be difficult to purposefully exploit this.
CWE description: The product contains a code sequence that can run concurrently with other code, and the code sequence requires temporary, exclusive access to a shared resource, but a timing window exists in which the shared resource can be modified by another code sequence that is operating concurrently.
CVSS v3 base score: 5.9
Published: 2019-12-16T20:15Z
Repository: https://github.com/excon/excon
Language: Ruby
Files changed: lib/excon/connection.rb, tests/connection_tests.rb, tests/rackups/basic.rb
Commit date: 2019-12-12 16:45:12 -0600
Commit message: fix for leftover data with interrupted persistent connections

Thanks to @pje for disclosure, initial patch, and input

Diff: diff --git a/lib/excon/connection.rb b/lib/excon/connection.rb
index 86368ef..275bb77 100644
--- a/lib/excon/connection.rb
+++ b/lib/excon/connection.rb
@@ -260,19 +260,26 @@ module Excon
         datum[:response_block] = block
       end
 
       datum[:connection] = self
 
+      # cleanup data left behind on persistent connection after interrupt
+      if datum[:persistent] && !@persistent_socket_reusable
+        reset
+      end
+
       datum[:stack] = datum[:middlewares].map do |middleware|
         lambda {|stack| middleware.new(stack)}
       end.reverse.inject(self) do |middlewares, middleware|
         middleware.call(middlewares)
       end
       datum = datum[:stack].request_call(datum)
 
       unless datum[:pipeline]
+        @persistent_socket_reusable = false
         datum = response(datum)
+        @persistent_socket_reusable = true
 
         if datum[:persistent]
           if key = datum[:response][:headers].keys.detect {|k| k.casecmp('Connection') == 0 }
             if datum[:response][:headers][key].casecmp('close') == 0
               reset
@@ -342,10 +349,11 @@ module Excon
 
     def reset
       if old_socket = sockets.delete(@socket_key)
         old_socket.close rescue nil
       end
+      @persistent_socket_reusable = true
     end
 
     # Generate HTTP request verb methods
     Excon::HTTP_VERBS.each do |method|
       class_eval <<-DEF, __FILE__, __LINE__ + 1
diff --git a/tests/connection_tests.rb b/tests/connection_tests.rb
index e98291b..6d663cc 100644
--- a/tests/connection_tests.rb
+++ b/tests/connection_tests.rb
@@ -12,10 +12,33 @@ Shindo.tests('Excon Connection') do
         :ssl_verify_peer  => false
       )
       connection.send(:socket) # creates/connects socket
       connection.data[:remote_ip]
     end
+
+    tests("persistent connections") do
+      connection = Excon.new('http://127.0.0.1:9292', persistent: true)
+
+      response_body = connection.request(path: '/foo', method: 'get').body
+      test("successful uninterrupted request") do
+        connection.request(path: '/foo', method: 'get').body == 'foo'
+      end
+
+      begin
+        # simulate an interrupted connection which leaves data behind
+        Timeout::timeout(0.0000000001) do
+          connection.request(path: '/foo', method: 'get')
+        end
+      rescue Timeout::Error
+        nil
+      end
+
+      test("resets connection after interrupt") do
+        response = connection.request(path: '/bar', method: 'get')
+        response.body == 'bar'
+      end
+    end
   end
 
   tests("inspect redaction") do
     cases = [
       ['user & pass', 'http://user1:pass1@foo.com/', 'Basic dXNlcjE6cGFzczE='],
diff --git a/tests/rackups/basic.rb b/tests/rackups/basic.rb
index 0bddd9a..af19954 100644
--- a/tests/rackups/basic.rb
+++ b/tests/rackups/basic.rb
@@ -30,10 +30,18 @@ class Basic < Sinatra::Base
 
   get('/echo dirty') do
     echo
   end
 
+  get('/foo') do
+    'foo'
+  end
+
+  get('/bar') do
+    'bar'
+  end
+
   private
 
   def echo
     request.body.read
   end

Fixed code:       # cleanup data left behind on persistent connection after interrupt
      if datum[:persistent] && !@persistent_socket_reusable
        reset
      end

        @persistent_socket_reusable = false
        @persistent_socket_reusable = true
      @persistent_socket_reusable = true

    tests("persistent connections") do
      connection = Excon.new('http://127.0.0.1:9292', persistent: true)

      response_body = connection.request(path: '/foo', method: 'get').body
      test("successful uninterrupted request") do
        connection.request(path: '/foo', method: 'get').body == 'foo'
      end

      begin
        # simulate an interrupted connection which leaves data behind
        Timeout::timeout(0.0000000001) do
          connection.request(path: '/foo', method: 'get')
        end
      rescue Timeout::Error
        nil
      end

      test("resets connection after interrupt") do
        response = connection.request(path: '/bar', method: 'get')
        response.body == 'bar'
      end
    end
  get('/foo') do
    'foo'
  end

  get('/bar') do
    'bar'
  end