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?
-        logo = Current.account.logo.variant(logo_variant).processed
-        send_png_file ActiveStorage::Blob.service.path_for(logo.key)
+      if (logo_variant = Current.account&.logo_variant(logo_size))
+        send_png_file ActiveStorage::Blob.service.path_for(logo_variant.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
-      small_logo? ? SMALL_SQUARE_PNG_VARIANT : LARGE_SQUARE_PNG_VARIANT
+    def logo_size
+      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?
-        avatar_variant = @user.avatar.variant(SQUARE_WEBP_VARIANT).processed
+      if (avatar_variant = @user.avatar_variant)
         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 ]

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/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/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