CVE-2023-47120: Allocation of Resources Without Limits or Throttling (CWE-770)
value: Discourse is an open source platform for community discussion. In versions 3.1.0 through 3.1.2 of the `stable` branch and versions 3.1.0,beta6 through 3.2.0.beta2 of the `beta` and `tests-passed` branches, Redis memory can be depleted by crafting a site with an abnormally long favicon URL and drafting multiple posts which Onebox it. The issue is patched in version 3.1.3 of the `stable` branch and version 3.2.0.beta3 of the `beta` and `tests-passed` branches. There are no known workarounds.
CWE description: The product allocates a reusable resource or group of resources on behalf of an actor without imposing any restrictions on the size or number of resources that can be allocated, in violation of the intended security policy for that actor.
CVSS v3 base score: 7.5
Published: 2023-11-10T16:15Z
Repository: https://github.com/discourse/discourse
Language: Ruby
Files changed: lib/onebox.rb, lib/onebox/engine/standard_embed.rb, lib/url_helper.rb, spec/lib/onebox/engine/standard_embed_spec.rb, spec/lib/url_helper_spec.rb
Commit date: 2023-11-09 13:39:18 +1100
Commit message: SECURITY: Prevent Onebox cache overflow by limiting downloads and URL lengths

Diff: diff --git a/lib/onebox.rb b/lib/onebox.rb
index feee7c8e6dad..d5d1b9ae6725 100644
--- a/lib/onebox.rb
+++ b/lib/onebox.rb
@@ -14,11 +14,11 @@ require_relative "onebox/sanitize_config"
 
 module Onebox
   DEFAULTS = {
     connect_timeout: 5,
     timeout: 10,
-    max_download_kb: (10 * 1024), # 10MB
+    max_download_kb: 2048, # 2MB
     load_paths: [File.join(Rails.root, "lib/onebox/templates")],
     allowed_ports: [80, 443],
     allowed_schemes: %w[http https],
     sanitize_config: SanitizeConfig::ONEBOX,
     redirect_limit: 5,
diff --git a/lib/onebox/engine/standard_embed.rb b/lib/onebox/engine/standard_embed.rb
index 2473dfe58291..90bd59094a7c 100644
--- a/lib/onebox/engine/standard_embed.rb
+++ b/lib/onebox/engine/standard_embed.rb
@@ -96,11 +96,17 @@ module Onebox
           html_doc.css(
             'link[rel="shortcut icon"], link[rel="icon shortcut"], link[rel="shortcut"], link[rel="icon"]',
           ).first
         favicon = favicon.nil? ? nil : (favicon["href"].nil? ? nil : favicon["href"].strip)
 
-        Onebox::Helpers.get_absolute_image_url(favicon, url)
+        return nil if favicon.blank?
+
+        absolute_url = Onebox::Helpers.get_absolute_image_url(favicon, url)
+
+        return nil if absolute_url.length > UrlHelper::MAX_URL_LENGTH
+
+        absolute_url
       end
 
       def get_description
         return nil unless html_doc
 
diff --git a/lib/url_helper.rb b/lib/url_helper.rb
index eb973777d911..e299a8e32324 100644
--- a/lib/url_helper.rb
+++ b/lib/url_helper.rb
@@ -1,9 +1,9 @@
 # frozen_string_literal: true
 
 class UrlHelper
-  MAX_URL_LENGTH = 100_000
+  MAX_URL_LENGTH = 2_000
 
   # At the moment this handles invalid URLs that browser address bar accepts
   # where second # is not encoded
   #
   # Longer term we can add support of simpleidn and encode unicode domains
diff --git a/spec/lib/onebox/engine/standard_embed_spec.rb b/spec/lib/onebox/engine/standard_embed_spec.rb
index 6c62ae5276e7..55cffc4a9beb 100644
--- a/spec/lib/onebox/engine/standard_embed_spec.rb
+++ b/spec/lib/onebox/engine/standard_embed_spec.rb
@@ -64,10 +64,38 @@ RSpec.describe Onebox::Engine::StandardEmbed do
       Onebox::Helpers.stubs(fetch_html_doc: html_doc)
 
       expect(instance.raw).to eq({ title: "do not override me" })
     end
 
+    it "sets favicon URL" do
+      html_doc =
+        mocked_html_doc(
+          twitter_data: {
+            "name" => "twitter:url",
+            "content" => "cool.url",
+          },
+          favicon_url: "https://favicon.co/default.ico",
+        )
+      Onebox::Helpers.stubs(fetch_html_doc: html_doc)
+
+      expect(instance.raw).to eq({ url: "cool.url", favicon: "https://favicon.co/default.ico" })
+    end
+
+    it "ignores suspiciously long favicon URLs" do
+      html_doc =
+        mocked_html_doc(
+          twitter_data: {
+            "name" => "twitter:url",
+            "content" => "cool.url",
+          },
+          favicon_url: "https://favicon.co/#{"a" * 2_000}.ico",
+        )
+      Onebox::Helpers.stubs(fetch_html_doc: html_doc)
+
+      expect(instance.raw).to eq({ url: "cool.url" })
+    end
+
     it "sets oembed data" do
       Onebox::Helpers.stubs(fetch_html_doc: nil)
       Onebox::Oembed.any_instance.stubs(:data).returns({ description: "description" })
 
       expect(instance.raw).to eq({ description: "description" })
@@ -82,30 +110,30 @@ RSpec.describe Onebox::Engine::StandardEmbed do
     end
   end
 
   private
 
-  def mocked_html_doc(twitter_data: nil)
+  def mocked_html_doc(twitter_data: nil, favicon_url: nil)
     html_doc = mock
     html_doc.stubs(at_css: nil, at: nil)
     stub_twitter(html_doc, twitter_data)
-    stub_favicon(html_doc)
+    stub_favicon(html_doc, favicon_url)
     stub_json_ld
     html_doc
   end
 
   def stub_twitter(html_doc, twitter_data = [])
     html_doc.expects(:css).with("meta").at_least_once.returns([twitter_data])
   end
 
-  def stub_favicon(html_doc)
+  def stub_favicon(html_doc, favicon_url = nil)
     html_doc
       .stubs(:css)
       .with(
         'link[rel="shortcut icon"], link[rel="icon shortcut"], link[rel="shortcut"], link[rel="icon"]',
       )
-      .returns([])
+      .returns([{ "href" => favicon_url }.compact])
   end
 
   def stub_json_ld
     normalizer = mock
     normalizer.stubs(:data).returns([])
diff --git a/spec/lib/url_helper_spec.rb b/spec/lib/url_helper_spec.rb
index e98d5ac829d7..7bfad9825940 100644
--- a/spec/lib/url_helper_spec.rb
+++ b/spec/lib/url_helper_spec.rb
@@ -164,11 +164,11 @@ RSpec.describe UrlHelper do
         "https://test.com/original/3X/b/5/575bcc2886bf7a39684b57ca90be85f7d399bbc7.png?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AK8888999977/20200130/us-west-1/s3/aws4_request&X-Amz-Date=20200130T064355Z&X-Amz-Expires=15&X-Amz-SignedHeaders=host&X-Amz-Security-Token=blahblah+blahblah/blah//////////wEQA==&X-Amz-Signature=test"
       expect(UrlHelper.normalized_encode(presigned_url)).not_to eq(encoded_presigned_url)
     end
 
     it "raises error if too long" do
-      expect do UrlHelper.normalized_encode("https://#{"a" * 100_000}.com") end.to raise_error(
+      expect do UrlHelper.normalized_encode("https://#{"a" * 2_000}.com") end.to raise_error(
         ArgumentError,
       )
     end
   end
 

Vulnerable code:     max_download_kb: (10 * 1024), # 10MB
        Onebox::Helpers.get_absolute_image_url(favicon, url)
  MAX_URL_LENGTH = 100_000
  def mocked_html_doc(twitter_data: nil)
    stub_favicon(html_doc)
  def stub_favicon(html_doc)
      .returns([])
      expect do UrlHelper.normalized_encode("https://#{"a" * 100_000}.com") end.to raise_error(
Fixed code:     max_download_kb: 2048, # 2MB
        return nil if favicon.blank?

        absolute_url = Onebox::Helpers.get_absolute_image_url(favicon, url)

        return nil if absolute_url.length > UrlHelper::MAX_URL_LENGTH

        absolute_url
  MAX_URL_LENGTH = 2_000
    it "sets favicon URL" do
      html_doc =
        mocked_html_doc(
          twitter_data: {
            "name" => "twitter:url",
            "content" => "cool.url",
          },
          favicon_url: "https://favicon.co/default.ico",
        )
      Onebox::Helpers.stubs(fetch_html_doc: html_doc)

      expect(instance.raw).to eq({ url: "cool.url", favicon: "https://favicon.co/default.ico" })
    end

    it "ignores suspiciously long favicon URLs" do
      html_doc =
        mocked_html_doc(
          twitter_data: {
            "name" => "twitter:url",
            "content" => "cool.url",
          },
          favicon_url: "https://favicon.co/#{"a" * 2_000}.ico",
        )
      Onebox::Helpers.stubs(fetch_html_doc: html_doc)

      expect(instance.raw).to eq({ url: "cool.url" })
    end

  def mocked_html_doc(twitter_data: nil, favicon_url: nil)
    stub_favicon(html_doc, favicon_url)
  def stub_favicon(html_doc, favicon_url = nil)
      .returns([{ "href" => favicon_url }.compact])
      expect do UrlHelper.normalized_encode("https://#{"a" * 2_000}.com") end.to raise_error(