Catch endless headers when reading HTTP responses

Merge branch 'security-fix-http-endless-headers-dos-14-10' into '14-10-stable-ee'

See merge request gitlab-org/security/gitlab!2529

Changelog: security

config/initializers/net_http_response_patch.rb

+# frozen_string_literal: true
+
+module Net
+  class HTTPResponse
+    # rubocop: disable Cop/LineBreakAfterGuardClauses
+    # rubocop: disable Cop/LineBreakAroundConditionalBlock
+    # rubocop: disable Layout/EmptyLineAfterGuardClause
+    # rubocop: disable Style/AndOr
+    # rubocop: disable Style/CharacterLiteral
+    # rubocop: disable Style/InfiniteLoop
+
+    # Original method:
+    # https://github.com/ruby/ruby/blob/v2_7_5/lib/net/http/response.rb#L54-L69
+    #
+    # Our changes:
+    # - Pass along the `start_time` to `Gitlab::BufferedIo`, so we can raise a timeout
+    #   if reading the headers takes too long.
+    # - Limit the regexes to avoid ReDoS attacks.
+    def self.each_response_header(sock)
+      start_time = Process.clock_gettime(Process::CLOCK_MONOTONIC)
+      key = value = nil
+      while true
+        line = sock.is_a?(Gitlab::BufferedIo) ? sock.readuntil("\n", true, start_time) : sock.readuntil("\n", true)
+        line = line.sub(/\s{0,10}\z/, '')
+        break if line.empty?
+        if line[0] == ?\s or line[0] == ?\t and value
+          # :nocov:
+          value << ' ' unless value.empty?
+          value << line.strip
+          # :nocov:
+        else
+          yield key, value if key
+          key, value = line.strip.split(/\s{0,10}:\s{0,10}/, 2)
+          raise Net::HTTPBadResponse, 'wrong header line format' if value.nil?
+        end
+      end
+      yield key, value if key
+    end
+    # rubocop: enable Cop/LineBreakAfterGuardClauses
+    # rubocop: enable Cop/LineBreakAroundConditionalBlock
+    # rubocop: enable Layout/EmptyLineAfterGuardClause
+    # rubocop: enable Style/AndOr
+    # rubocop: enable Style/CharacterLiteral
+    # rubocop: enable Style/InfiniteLoop
+  end
+end

lib/gitlab/buffered_io.rb

@@ -14,6 +14,7 @@ module Gitlab
 
     HEADER_READ_TIMEOUT = 20
 
+    # rubocop: disable Style/RedundantBegin
     # rubocop: disable Style/RedundantReturn
     # rubocop: disable Cop/LineBreakAfterGuardClauses
     # rubocop: disable Layout/EmptyLineAfterGuardClause
@@ -21,9 +22,7 @@ module Gitlab
     # Original method:
     # https://github.com/ruby/ruby/blob/cdb7d699d0641e8f081d590d06d07887ac09961f/lib/net/protocol.rb#L190-L200
     override :readuntil
-    def readuntil(terminator, ignore_eof = false)
-      start_time = Process.clock_gettime(Process::CLOCK_MONOTONIC)
-
+    def readuntil(terminator, ignore_eof = false, start_time = Process.clock_gettime(Process::CLOCK_MONOTONIC))
       begin
         until idx = @rbuf.index(terminator)
           if (elapsed = Process.clock_gettime(Process::CLOCK_MONOTONIC) - start_time) > HEADER_READ_TIMEOUT
@@ -39,6 +38,7 @@ module Gitlab
         return rbuf_consume(@rbuf.size)
       end
     end
+    # rubocop: disable Style/RedundantBegin
     # rubocop: enable Style/RedundantReturn
     # rubocop: enable Cop/LineBreakAfterGuardClauses
     # rubocop: enable Layout/EmptyLineAfterGuardClause

spec/initializers/net_http_response_patch_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Net::HTTPResponse patch header read timeout' do
+  describe '.each_response_header' do
+    let(:server_response) do
+      <<~EOS
+        Content-Type: text/html
+        Header-Two: foo
+
+        Hello World
+      EOS
+    end
+
+    before do
+      stub_const('Gitlab::BufferedIo::HEADER_READ_TIMEOUT', 0.1)
+    end
+
+    subject(:each_response_header) { Net::HTTPResponse.each_response_header(socket) { |k, v| } }
+
+    context 'with Net::BufferedIO' do
+      let(:socket) { Net::BufferedIO.new(StringIO.new(server_response)) }
+
+      it 'does not forward start time to the socket' do
+        allow(socket).to receive(:readuntil).and_call_original
+        expect(socket).to receive(:readuntil).with("\n", true)
+
+        each_response_header
+      end
+
+      context 'when the response contains many consecutive spaces' do
+        before do
+          expect(socket).to receive(:readuntil).and_return(
+            "a: #{' ' * 100_000} b",
+            ''
+          )
+        end
+
+        it 'has no regex backtracking issues' do
+          Timeout.timeout(1) do
+            each_response_header
+          end
+        end
+      end
+    end
+
+    context 'with Gitlab::BufferedIo' do
+      let(:mock_io) { StringIO.new(server_response) }
+      let(:socket) { Gitlab::BufferedIo.new(mock_io) }
+
+      it 'forwards start time to the socket' do
+        allow(socket).to receive(:readuntil).and_call_original
+        expect(socket).to receive(:readuntil).with("\n", true, kind_of(Numeric))
+
+        each_response_header
+      end
+
+      context 'when the response contains an infinite number of headers' do
+        before do
+          read_counter = 0
+
+          allow(mock_io).to receive(:read_nonblock) do
+            read_counter += 1
+            raise 'Test did not raise HeaderReadTimeout' if read_counter > 10
+
+            sleep 0.01
+            +"Yet-Another-Header: foo\n"
+          end
+        end
+
+        it 'raises a timeout error' do
+          expect { each_response_header }.to raise_error(Gitlab::HTTP::HeaderReadTimeout,
+            /Request timed out after reading headers for 0\.[0-9]+ seconds/)
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/buffered_io_spec.rb

-# rubocop:disable Style/FrozenStringLiteralComment
+# frozen_string_literal: true
+
 require 'spec_helper'
 
 RSpec.describe Gitlab::BufferedIo do
   describe '#readuntil' do
-    let(:never_ending_tcp_socket) do
-      Class.new do
-        def initialize(*_)
-          @read_counter = 0
-        end
+    let(:mock_io) { StringIO.new('a') }
+    let(:start_time) { Process.clock_gettime(Process::CLOCK_MONOTONIC) }
 
-        def setsockopt(*_); end
+    before do
+      stub_const('Gitlab::BufferedIo::HEADER_READ_TIMEOUT', 0.1)
+    end
 
-        def closed?
-          false
-        end
+    subject(:readuntil) do
+      Gitlab::BufferedIo.new(mock_io).readuntil('a', false, start_time)
+    end
 
-        def close
-          true
-        end
+    it 'does not raise a timeout error' do
+      expect { readuntil }.not_to raise_error
+    end
 
-        def to_io
-          StringIO.new('Hello World!')
-        end
+    context 'when the response contains infinitely long headers' do
+      before do
+        read_counter = 0
 
-        def write_nonblock(data, *_)
-          data.size
-        end
+        allow(mock_io).to receive(:read_nonblock) do |buffer_size, *_|
+          read_counter += 1
+          raise 'Test did not raise HeaderReadTimeout' if read_counter > 10
 
-        def read_nonblock(buffer_size, *_)
           sleep 0.01
-          @read_counter += 1
-
-          raise 'Test did not raise HeaderReadTimeout' if @read_counter > 10
-
           'H' * buffer_size
         end
       end
-    end
 
-    before do
-      stub_const('Gitlab::BufferedIo::HEADER_READ_TIMEOUT', 0.1)
-    end
+      it 'raises a timeout error' do
+        expect { readuntil }.to raise_error(Gitlab::HTTP::HeaderReadTimeout, /Request timed out after reading headers for 0\.[0-9]+ seconds/)
+      end
 
-    subject(:readuntil) do
-      Gitlab::BufferedIo.new(never_ending_tcp_socket.new).readuntil('a')
-    end
+      context 'when not passing start_time' do
+        subject(:readuntil) do
+          Gitlab::BufferedIo.new(mock_io).readuntil('a', false)
+        end
 
-    it 'raises a timeout error' do
-      expect { readuntil }.to raise_error(Gitlab::HTTP::HeaderReadTimeout, /Request timed out after reading headers for 0\.[0-9]+ seconds/)
+        it 'raises a timeout error' do
+          expect { readuntil }.to raise_error(Gitlab::HTTP::HeaderReadTimeout, /Request timed out after reading headers for 0\.[0-9]+ seconds/)
+        end
+      end
     end
   end
 end
-# rubocop:enable Style/FrozenStringLiteralComment