CVE-2022-23641: Loop with Unreachable Exit Condition ('Infinite Loop') (CWE-835)
value: Discourse is an open source discussion platform. In versions prior to 2.8.1 in the `stable` branch, 2.9.0.beta2 in the `beta` branch, and 2.9.0.beta2 in the `tests-passed` branch, users can trigger a Denial of Service attack by posting a streaming URL. Parsing Oneboxes in the background job trigger an infinite loop, which cause memory leaks. This issue is patched in version 2.8.1 of the `stable` branch, 2.9.0.beta2 of the `beta` branch, and 2.9.0.beta2 of the `tests-passed` branch. As a workaround, disable onebox in admin panel completely or specify allow list of domains that will be oneboxed.
CWE description: The product contains an iteration or loop with an exit condition that cannot be reached, i.e., an infinite loop.
CVSS v3 base score: 6.5
Published: 2022-02-15T21:15Z
Repository: https://github.com/discourse/discourse
Language: Ruby
Files changed: lib/final_destination.rb, spec/components/final_destination_spec.rb
Commit date: 2022-02-14 12:11:09 +1100
Commit message: SECURITY: Onebox response timeout and size limit (#15927)

Validation to ensure that Onebox request is no longer than 10 seconds and response size is not bigger than 1 MB
Diff: diff --git a/lib/final_destination.rb b/lib/final_destination.rb
index 2fbc5711b183..078fd4fd781f 100644
--- a/lib/final_destination.rb
+++ b/lib/final_destination.rb
@@ -6,10 +6,12 @@ require 'excon'
 require 'rate_limiter'
 require 'url_helper'
 
 # Determine the final endpoint for a Web URI, following redirects
 class FinalDestination
+  MAX_REQUEST_TIME_SECONDS = 10
+  MAX_REQUEST_SIZE_BYTES = 1_048_576 # 1024 * 1024
 
   def self.clear_https_cache!(domain)
     key = redis_https_key(domain)
     Discourse.redis.without_namespace.del(key)
   end
@@ -201,16 +203,25 @@ class FinalDestination
 
     headers = request_headers
     middlewares = Excon.defaults[:middlewares]
     middlewares << Excon::Middleware::Decompress if @http_verb == :get
 
+    request_start_time = Time.now
+    response_body = +""
+    request_validator = lambda do |chunk, _remaining_bytes, _total_bytes|
+      response_body << chunk
+      raise Excon::Errors::ExpectationFailed.new("response size too big: #{@uri.to_s}") if response_body.bytesize > MAX_REQUEST_SIZE_BYTES
+      raise Excon::Errors::ExpectationFailed.new("connect timeout reached: #{@uri.to_s}") if Time.now - request_start_time > MAX_REQUEST_TIME_SECONDS
+    end
+
     response = Excon.public_send(@http_verb,
       @uri.to_s,
       read_timeout: timeout,
       connect_timeout: timeout,
       headers: headers,
-      middlewares: middlewares
+      middlewares: middlewares,
+      response_block: request_validator
     )
 
     location = nil
     response_headers = nil
     response_status = response.status.to_i
@@ -218,16 +229,16 @@ class FinalDestination
     case response.status
     when 200
       # Cache body of successful `get` requests
       if @http_verb == :get
         if Oneboxer.cache_response_body?(@uri)
-          Oneboxer.cache_response_body(@uri.to_s, response.body)
+          Oneboxer.cache_response_body(@uri.to_s, response_body)
         end
       end
 
       if @follow_canonical
-        next_url = fetch_canonical_url(response.body)
+        next_url = fetch_canonical_url(response_body)
 
         if next_url.to_s.present? && next_url != @uri
           @follow_canonical = false
           @uri = next_url
           @http_verb = http_verb(@force_get_hosts, @follow_canonical)
diff --git a/spec/components/final_destination_spec.rb b/spec/components/final_destination_spec.rb
index e9548ead0b91..b1ce1a262a0f 100644
--- a/spec/components/final_destination_spec.rb
+++ b/spec/components/final_destination_spec.rb
@@ -47,10 +47,17 @@ describe FinalDestination do
       status: 200,
       headers: { "Content-Type" => "image/jpeg" }
     }
   end
 
+  let(:body_response) do
+    {
+      status: 200,
+      body: "<body>test</body>"
+    }
+  end
+
   def canonical_follow(from, dest)
     stub_request(:get, from).to_return(
       status: 200,
       body: "<head><link rel=\"canonical\" href=\"#{dest}\"></head>"
     )
@@ -180,10 +187,24 @@ describe FinalDestination do
         expect(final.redirected?).to eq(false)
         expect(final.status).to eq(:resolved)
       end
     end
 
+    it 'raises error when response is too big' do
+      stub_const(described_class, "MAX_REQUEST_SIZE_BYTES", 1) do
+        stub_request(:get, "https://codinghorror.com/blog").to_return(body_response)
+        final = FinalDestination.new('https://codinghorror.com/blog', opts.merge(follow_canonical: true))
+        expect { final.resolve }.to raise_error(Excon::Errors::ExpectationFailed, "response size too big: https://codinghorror.com/blog")
+      end
+    end
+
+    it 'raises error when response is too slow' do
+      stub_request(:get, "https://codinghorror.com/blog").to_return(lambda { |request| freeze_time(11.seconds.from_now) ; body_response })
+      final = FinalDestination.new('https://codinghorror.com/blog', opts.merge(follow_canonical: true))
+      expect { final.resolve }.to raise_error(Excon::Errors::ExpectationFailed, "connect timeout reached: https://codinghorror.com/blog")
+    end
+
     context 'follows canonical links' do
       it 'resolves the canonical link as the final destination' do
         canonical_follow("https://eviltrout.com", "https://codinghorror.com/blog")
         stub_request(:head, "https://codinghorror.com/blog").to_return(doc_response)
 

Vulnerable code:       middlewares: middlewares
          Oneboxer.cache_response_body(@uri.to_s, response.body)
        next_url = fetch_canonical_url(response.body)
Fixed code:   MAX_REQUEST_TIME_SECONDS = 10
  MAX_REQUEST_SIZE_BYTES = 1_048_576 # 1024 * 1024
    request_start_time = Time.now
    response_body = +""
    request_validator = lambda do |chunk, _remaining_bytes, _total_bytes|
      response_body << chunk
      raise Excon::Errors::ExpectationFailed.new("response size too big: #{@uri.to_s}") if response_body.bytesize > MAX_REQUEST_SIZE_BYTES
      raise Excon::Errors::ExpectationFailed.new("connect timeout reached: #{@uri.to_s}") if Time.now - request_start_time > MAX_REQUEST_TIME_SECONDS
    end

      middlewares: middlewares,
      response_block: request_validator
          Oneboxer.cache_response_body(@uri.to_s, response_body)
        next_url = fetch_canonical_url(response_body)
  let(:body_response) do
    {
      status: 200,
      body: "<body>test</body>"
    }
  end

    it 'raises error when response is too big' do
      stub_const(described_class, "MAX_REQUEST_SIZE_BYTES", 1) do
        stub_request(:get, "https://codinghorror.com/blog").to_return(body_response)
        final = FinalDestination.new('https://codinghorror.com/blog', opts.merge(follow_canonical: true))
        expect { final.resolve }.to raise_error(Excon::Errors::ExpectationFailed, "response size too big: https://codinghorror.com/blog")
      end
    end

    it 'raises error when response is too slow' do
      stub_request(:get, "https://codinghorror.com/blog").to_return(lambda { |request| freeze_time(11.seconds.from_now) ; body_response })
      final = FinalDestination.new('https://codinghorror.com/blog', opts.merge(follow_canonical: true))
      expect { final.resolve }.to raise_error(Excon::Errors::ExpectationFailed, "connect timeout reached: https://codinghorror.com/blog")
    end