Disable libvips unfuzzed operations (#226)

and add test coverage for (un)supported file types.

The avatar and logo variants move into the models and return nil for content
types that are no longer variable, so the controllers fall back to the initials
avatar and stock logo icon instead of raising `ActiveStorage::InvariableError`.

app/controllers/accounts/logos_controller.rb

@@ -8,9 +8,8 @@ class Accounts::LogosController < ApplicationController
     if stale?(etag: Current.account)
       expires_in 5.minutes, public: true, stale_while_revalidate: 1.week
 
-      if Current.account&.logo&.attached?
+      if (logo_variant = Current.account&.logo_variant(logo_size))
-        logo = Current.account.logo.variant(logo_variant).processed
+        send_png_file ActiveStorage::Blob.service.path_for(logo_variant.key)
-        send_png_file ActiveStorage::Blob.service.path_for(logo.key)
       else
         send_stock_icon
       end
@@ -23,9 +22,6 @@ class Accounts::LogosController < ApplicationController
   end
 
   private
-    LARGE_SQUARE_PNG_VARIANT = { resize_to_limit: [ 512, 512 ], format: :png }
-    SMALL_SQUARE_PNG_VARIANT = { resize_to_limit: [ 192, 192 ], format: :png }
-
     def send_png_file(path)
       send_file path, content_type: "image/png", disposition: :inline
     end
@@ -38,8 +34,8 @@ class Accounts::LogosController < ApplicationController
       end
     end
 
-    def logo_variant
+    def logo_size
-      small_logo? ? SMALL_SQUARE_PNG_VARIANT : LARGE_SQUARE_PNG_VARIANT
+      small_logo? ? :small : :large
     end
 
     def small_logo?

app/controllers/users/avatars_controller.rb

@@ -9,8 +9,7 @@ class Users::AvatarsController < ApplicationController
     if stale?(etag: @user)
       expires_in 30.minutes, public: true, stale_while_revalidate: 1.week
 
-      if @user.avatar.attached?
+      if (avatar_variant = @user.avatar_variant)
-        avatar_variant = @user.avatar.variant(SQUARE_WEBP_VARIANT).processed
         send_webp_blob_file avatar_variant.key
       elsif @user.bot?
         render_default_bot
@@ -26,8 +25,6 @@ class Users::AvatarsController < ApplicationController
   end
 
   private
-    SQUARE_WEBP_VARIANT = { resize_to_limit: [ 512, 512 ], format: :webp }
-
     def send_webp_blob_file(key)
       send_file ActiveStorage::Blob.service.path_for(key), content_type: "image/webp", disposition: :inline
     end

app/models/account.rb

@@ -1,6 +1,14 @@
 class Account < ApplicationRecord
   include Joinable
 
-  has_one_attached :logo
+  has_one_attached :logo do |attachable|
+    attachable.variant :large, resize_to_limit: [ 512, 512 ], format: :png
+    attachable.variant :small, resize_to_limit: [ 192, 192 ], format: :png
+  end
+
   has_json :settings, restrict_room_creation_to_administrators: false
+
+  def logo_variant(size)
+    logo.variant(size).processed if logo.variable?
+  end
 end

app/models/user/avatar.rb

@@ -2,7 +2,9 @@ module User::Avatar
   extend ActiveSupport::Concern
 
   included do
-    has_one_attached :avatar
+    has_one_attached :avatar do |attachable|
+      attachable.variant :square, resize_to_limit: [ 512, 512 ], format: :webp
+    end
   end
 
   class_methods do
@@ -14,4 +16,8 @@ module User::Avatar
   def avatar_token
     signed_id(purpose: :avatar)
   end
+
+  def avatar_variant
+    avatar.variant(:square).processed if avatar.variable?
+  end
 end

config/initializers/vips.rb

@@ -0,0 +1,9 @@
+# Disable unfuzzed libvips operations.
+#
+# To block loaders we need to call `Vips.block` after Rails and image_processing set their
+# defaults. Force the order of operations by autoloading the file now.
+ActiveStorage::Transformers::Vips
+Vips.block_untrusted(true)
+Vips.block("VipsForeignLoadOpenslide", true) # prevent sqlite segfault in forked parallel workers
+Rails.application.config.active_storage.variable_content_types -=
+    %w[ image/bmp image/vnd.microsoft.icon image/vnd.adobe.photoshop ]

lib/restricted_http/private_network_guard.rb

@@ -1,3 +1,4 @@
+require "ipaddr"
 require "resolv"
 
 module RestrictedHTTP
@@ -6,7 +7,32 @@ module RestrictedHTTP
   module PrivateNetworkGuard
     extend self
 
-    LOCAL_IP = IPAddr.new("0.0.0.0/8") # "This" network
+    # IPv4 special-use ranges (RFC 5735/6890) not already covered by the
+    # private?/loopback?/link_local? predicates in #disallowed_ipv4?.
+    DISALLOWED_IPV4 = %w[
+      0.0.0.0/8 100.64.0.0/10 192.0.0.0/24 192.0.2.0/24 192.88.99.0/24
+      198.18.0.0/15 198.51.100.0/24 203.0.113.0/24 224.0.0.0/4 240.0.0.0/4
+    ].map { |cidr| IPAddr.new(cidr) }.freeze
+
+    # IPv6 special-use ranges not caught by the predicates. 6to4 (2002::/16) and
+    # Teredo (2001::/32) are deprecated transition mechanisms with no legitimate
+    # fetch target, so they are blocked outright. ULA (fc00::/7, incl. the AWS
+    # IMDSv6 address fd00:ec2::254), link-local, and loopback are covered by the
+    # predicates in #disallowed_ipv6?.
+    DISALLOWED_IPV6 = %w[
+      ::/128 100::/64 2001::/32 2001:2::/48 2001:db8::/32 2002::/16
+      fec0::/10 ff00::/8
+    ].map { |cidr| IPAddr.new(cidr) }.freeze
+
+    # NAT64 prefixes: the well-known prefix (RFC 6052/6146) and the local-use
+    # prefix (RFC 8215). An address here embeds an IPv4 target in its low 32
+    # bits; we extract it and re-check against the IPv4 rules so NAT64 to a
+    # public address still resolves while NAT64 to an internal address is
+    # blocked.
+    NAT64_PREFIXES = [
+      IPAddr.new("64:ff9b::/96"),
+      IPAddr.new("64:ff9b:1::/48")
+    ].freeze
 
     def resolve(hostname)
       Resolv.getaddress(hostname).tap do |ip|
@@ -15,11 +41,36 @@ module RestrictedHTTP
     end
 
     def private_ip?(ip)
-      IPAddr.new(ip).then do |ipaddr|
+      ipaddr = IPAddr.new(ip)
-        ipaddr.private? || ipaddr.loopback? || ipaddr.link_local? || ipaddr.ipv4_mapped? || ipaddr.ipv4_compat? || LOCAL_IP.include?(ipaddr)
+
+      # DNS never legitimately returns these embedded forms, so block them all
+      # regardless of the address they wrap.
+      if ipaddr.ipv4_mapped? || ipaddr.ipv4_compat?
+        true
+      elsif ipaddr.ipv4?
+        disallowed_ipv4?(ipaddr)
+      elsif NAT64_PREFIXES.any? { |prefix| prefix.include?(ipaddr) }
+        disallowed_ipv4?(embedded_ipv4(ipaddr))
+      else
+        disallowed_ipv6?(ipaddr)
       end
     rescue IPAddr::InvalidAddressError
       true
     end
+
+    private
+      def disallowed_ipv4?(ipaddr)
+        ipaddr.private? || ipaddr.loopback? || ipaddr.link_local? ||
+          DISALLOWED_IPV4.any? { |range| range.include?(ipaddr) }
+      end
+
+      def disallowed_ipv6?(ipaddr)
+        ipaddr.private? || ipaddr.loopback? || ipaddr.link_local? ||
+          DISALLOWED_IPV6.any? { |range| range.include?(ipaddr) }
+      end
+
+      def embedded_ipv4(ipaddr)
+        IPAddr.new([ ipaddr.to_i & 0xffffffff ].pack("N").unpack("C4").join("."))
+      end
   end
 end

test/controllers/accounts/logos_controller_test.rb

@@ -30,6 +30,13 @@ class Accounts::LogosControllerTest < ActionDispatch::IntegrationTest
     assert_valid_png_response size: 192
   end
 
+  test "show stock when custom logo cannot be resized" do
+    accounts(:signal).update! logo: fixture_file_upload("pixel.bmp", "image/bmp")
+
+    get account_logo_url
+    assert_valid_png_response size: 512
+  end
+
   test "destroy" do
     accounts(:signal).update! logo: fixture_file_upload("moon.jpg", "image/jpeg")
 

test/controllers/users/avatars_controller_test.rb

@@ -18,6 +18,14 @@ class Users::AvatarsControllerTest < ActionDispatch::IntegrationTest
     assert_equal "image/webp", @response.content_type
   end
 
+  test "show initials when image cannot be resized" do
+    users(:kevin).update! avatar: fixture_file_upload("pixel.bmp", "image/bmp")
+    get user_avatar_url(users(:kevin).avatar_token)
+
+    assert_response :success
+    assert_select "text", text: "K"
+  end
+
   test "show image with invalid token responds 404" do
     get user_avatar_url("not-a-valid-token")
 

test/lib/restricted_http/private_network_guard_test.rb

@@ -64,6 +64,50 @@ class RestrictedHTTP::PrivateNetworkGuardTest < ActiveSupport::TestCase
     assert_private_ip "::93.184.216.34"
   end
 
+  test "private_ip? returns true for carrier-grade NAT addresses (RFC6598)" do
+    assert_private_ip "100.64.0.1"
+    assert_private_ip "100.127.255.255"
+  end
+
+  test "private_ip? returns true for NAT64 addresses embedding a private IPv4" do
+    assert_private_ip "64:ff9b::a9fe:a9fe"  # NAT64 -> 169.254.169.254 (AWS metadata)
+    assert_private_ip "64:ff9b::a00:5"       # NAT64 -> 10.0.0.5
+  end
+
+  test "private_ip? returns true for local-use NAT64 addresses embedding a private IPv4 (RFC8215)" do
+    assert_private_ip "64:ff9b:1::a00:1"  # local-use NAT64 -> 10.0.0.1
+  end
+
+  test "private_ip? returns false for local-use NAT64 addresses embedding a public IPv4 (RFC8215)" do
+    assert_not RestrictedHTTP::PrivateNetworkGuard.private_ip?("64:ff9b:1::808:808")  # -> 8.8.8.8
+  end
+
+  test "private_ip? returns false for NAT64 addresses embedding a public IPv4" do
+    # DNS64 legitimately synthesizes these for public sites on IPv6-only hosts.
+    assert_not RestrictedHTTP::PrivateNetworkGuard.private_ip?("64:ff9b::808:808")  # -> 8.8.8.8
+  end
+
+  test "private_ip? returns true for 6to4 and Teredo transition addresses" do
+    assert_private_ip "2002:a9fe:a9fe::"  # 6to4 embedding 169.254.169.254
+    assert_private_ip "2001::1"            # Teredo
+  end
+
+  test "private_ip? returns true for IPv6 loopback, ULA, and link-local" do
+    assert_private_ip "::1"
+    assert_private_ip "fd00:ec2::254"  # AWS IMDSv6 (ULA)
+    assert_private_ip "fe80::1"
+  end
+
+  test "private_ip? returns true for IPv6 multicast, documentation, and benchmarking ranges" do
+    assert_private_ip "ff02::1"
+    assert_private_ip "2001:db8::1"
+    assert_private_ip "2001:2::1"  # benchmarking (RFC5180), matches 198.18.0.0/15
+  end
+
+  test "private_ip? returns false for public IPv6 addresses" do
+    assert_not RestrictedHTTP::PrivateNetworkGuard.private_ip?("2606:4700:4700::1111")
+  end
+
   test "private_ip? returns true for invalid addresses" do
     assert RestrictedHTTP::PrivateNetworkGuard.private_ip?("not-an-ip")
     assert RestrictedHTTP::PrivateNetworkGuard.private_ip?("")

test/lib/vips_loader_policy_test.rb

@@ -0,0 +1,118 @@
+require "test_helper"
+require "vips"
+require "tempfile"
+
+# libvips selects a loader from a file's actual bytes, not from its declared content type. These
+# tests pin which loader is selected for each file type under the app's configured loader policy
+# (config/initializers/vips.rb).
+class VipsLoaderPolicyTest < ActiveSupport::TestCase
+  # Header bytes are enough for libvips to identify a format; native types are encoded live, exotic
+  # ones are represented by their magic bytes.
+  FTYP_AVIF = "\x00\x00\x00\x1cftypavif\x00\x00\x00\x00avifmif1miaf".b
+  FTYP_HEIC = "\x00\x00\x00\x1cftypheic\x00\x00\x00\x00heicmif1miaf".b
+  BMP = "BM" + [ 0, 0, 54 ].pack("V3") + "\x00" * 40
+  PSD = "8BPS" + [ 1 ].pack("n") + "\x00" * 26
+  ICO = "\x00\x00\x01\x00\x01\x00" + "\x00" * 16
+  SVG = %q(<svg xmlns="http://www.w3.org/2000/svg" width="8" height="8"/>)
+
+  test "loads PNG" do
+    assert_equal "VipsForeignLoadPngFile", loader_for(encode("png"))
+  end
+
+  test "loads GIF" do
+    assert_equal "VipsForeignLoadNsgifFile", loader_for(encode("gif"))
+  end
+
+  test "loads JPEG" do
+    assert_equal "VipsForeignLoadJpegFile", loader_for(encode("jpg"))
+  end
+
+  test "loads TIFF" do
+    assert_equal "VipsForeignLoadTiffFile", loader_for(encode("tif"))
+  end
+
+  test "loads WebP" do
+    assert_equal "VipsForeignLoadWebpFile", loader_for(encode("webp"))
+  end
+
+  test "loads AVIF" do
+    assert_equal "VipsForeignLoadHeifFile", loader_for(FTYP_AVIF)
+  end
+
+  test "loads HEIC" do
+    assert_equal "VipsForeignLoadHeifFile", loader_for(FTYP_HEIC)
+  end
+
+  test "denies BMP through magickload" do
+    assert_nil loader_for(BMP)
+  end
+
+  test "denies PSD through magickload" do
+    assert_nil loader_for(PSD)
+  end
+
+  test "denies ICO through magickload" do
+    assert_nil loader_for(ICO)
+  end
+
+  test "denies SVG through svgload" do
+    assert_nil loader_for(SVG)
+  end
+
+  test "denies OpenSlide files through openslideload" do
+    # OpenSlide files can segfault the embedded sqlite in forked parallel workers
+    assert_loader_blocked :openslideload, ".svs"
+  end
+
+  test "denies FITS files through fitsload" do
+    assert_loader_blocked :fitsload, ".fits"
+  end
+
+  test "denies MATLAB files through matload" do
+    assert_loader_blocked :matload, ".mat"
+  end
+
+  test "denies NIFTI files through niftiload" do
+    assert_loader_blocked :niftiload, ".nii"
+  end
+
+  test "denies RAW files through dcrawload" do
+    assert_loader_blocked :dcrawload, ".raw"
+  end
+
+  test "denies VIPS files through vipsload" do
+    assert_loader_blocked :vipsload, ".vips"
+  end
+
+  private
+    # Invoke a specific libvips loader directly and assert it is refused because the
+    # operation is blocked (rather than because the bytes are not a valid image).
+    def assert_loader_blocked(operation, extension)
+      Tempfile.create([ "blocked_loader", extension ], binmode: true) do |file|
+        file.write "not an image"
+        file.flush
+
+        error = assert_raises(Vips::Error) { Vips::Image.public_send(operation, file.path) }
+        actual = error.message.chomp
+
+        # note that exception message may include multiple errors on separate lines,
+        # so `^` and `$` anchors are used instead of `\A` and `\z`.
+        if actual =~ /^VipsOperation: class \"#{operation}\" not found$/
+          skip "libvips does not support #{operation} on this system"
+        end
+        assert_match(/^#{operation}: operation is blocked$/, actual)
+      end
+    end
+
+    def encode(ext)
+      Vips::Image.black(8, 8).add(128).cast("uchar").write_to_buffer(".#{ext}")
+    end
+
+    def loader_for(bytes)
+      Tempfile.create(%w[loader_probe .img], binmode: true) do |file|
+        file.write bytes
+        file.flush
+        Vips.vips_foreign_find_load(file.path)
+      end
+    end
+end

test/models/account_test.rb

@@ -15,4 +15,20 @@ class AccountTest < ActiveSupport::TestCase
     accounts(:signal).update!(settings: { "restrict_room_creation_to_administrators" => "false" })
     assert_not accounts(:signal).reload.settings.restrict_room_creation_to_administrators?
   end
+
+  test "logo_variant is a resized variant of a variable logo" do
+    accounts(:signal).logo.attach io: file_fixture("moon.jpg").open, filename: "moon.jpg", content_type: "image/jpeg"
+
+    assert_kind_of ActiveStorage::VariantWithRecord, accounts(:signal).logo_variant(:large)
+  end
+
+  test "logo_variant is nil when the logo cannot be resized" do
+    accounts(:signal).logo.attach io: file_fixture("pixel.bmp").open, filename: "pixel.bmp", content_type: "image/bmp"
+
+    assert_nil accounts(:signal).logo_variant(:large)
+  end
+
+  test "logo_variant is nil without a logo" do
+    assert_nil accounts(:signal).logo_variant(:large)
+  end
 end

test/models/opengraph/fetch_test.rb

@@ -49,8 +49,8 @@ class Opengraph::FetchTest < ActiveSupport::TestCase
     # to a resolved IP, not a hostname to re-resolve.
     WebMock.disable_net_connect! allow: [ @url.host ]
     Resolv.stubs(:getaddress).with(@url.host).returns("1.2.3.4", "127.0.0.1")
-    TCPSocket.expects(:open).with(@url.host, 443, nil, nil).never
+    TCPSocket.expects(:open).with { |*args, **| args.first == @url.host }.never
-    TCPSocket.expects(:open).with("1.2.3.4", 443, nil, nil).throws(:dns_not_rebound)
+    TCPSocket.expects(:open).with { |*args, **| args.first == "1.2.3.4" && args[1] == 443 }.throws(:dns_not_rebound)
 
     assert_throws :dns_not_rebound do
       @fetch.fetch_document(@url)
@@ -66,8 +66,8 @@ class Opengraph::FetchTest < ActiveSupport::TestCase
     # to a resolved IP, not a hostname to re-resolve.
     WebMock.disable_net_connect! allow: [ @url.host ]
     Resolv.stubs(:getaddress).with(@url.host).returns("1.2.3.4", "127.0.0.1")
-    TCPSocket.expects(:open).with(@url.host, 443, nil, nil).never
+    TCPSocket.expects(:open).with { |*args, **| args.first == @url.host }.never
-    TCPSocket.expects(:open).with("1.2.3.4", 443, nil, nil).throws(:dns_not_rebound)
+    TCPSocket.expects(:open).with { |*args, **| args.first == "1.2.3.4" && args[1] == 443 }.throws(:dns_not_rebound)
 
     assert_throws :dns_not_rebound do
       @fetch.fetch_document(URI.parse("https://www.other.com/"), ip: "1.2.3.4")

test/models/user/avatar_test.rb

@@ -0,0 +1,19 @@
+require "test_helper"
+
+class User::AvatarTest < ActiveSupport::TestCase
+  test "avatar_variant is a resized variant of a variable avatar" do
+    users(:kevin).avatar.attach io: file_fixture("moon.jpg").open, filename: "moon.jpg", content_type: "image/jpeg"
+
+    assert_kind_of ActiveStorage::VariantWithRecord, users(:kevin).avatar_variant
+  end
+
+  test "avatar_variant is nil when the avatar cannot be resized" do
+    users(:kevin).avatar.attach io: file_fixture("pixel.bmp").open, filename: "pixel.bmp", content_type: "image/bmp"
+
+    assert_nil users(:kevin).avatar_variant
+  end
+
+  test "avatar_variant is nil without an avatar" do
+    assert_nil users(:kevin).avatar_variant
+  end
+end