From 92f64fec3136baabbebac97073c5213ea055dc53 Mon Sep 17 00:00:00 2001
From: Zack <zack.ref@gmail.com>
Date: Tue, 15 Feb 2022 10:00:57 -0500
Subject: [PATCH] Added image transformation validation via configurable
 allow-list.

ImageProcessingTransformer now offers a configurable allow-list for
transformation methods in addition to a configurable deny-list for arguments.

[CVE-2022-21831]
---
 activestorage/lib/active_storage.rb           |  10 +-
 activestorage/lib/active_storage/engine.rb    |  16 +
 .../image_processing_transformer.rb           | 358 ++++++++++++++++++
 activestorage/test/models/variant_test.rb     |  74 ++++
 .../test/application/configuration_test.rb    |  29 ++
 5 files changed, 483 insertions(+), 4 deletions(-)

diff --git a/activestorage/lib/active_storage.rb b/activestorage/lib/active_storage.rb
index 75eb63f0b4..fcd53c85ac 100644
--- a/activestorage/lib/active_storage.rb
+++ b/activestorage/lib/active_storage.rb
@@ -52,10 +52,12 @@ module ActiveStorage
 
   mattr_accessor :paths, default: {}
 
-  mattr_accessor :variable_content_types,           default: []
-  mattr_accessor :binary_content_type,              default: "application/octet-stream"
-  mattr_accessor :content_types_to_serve_as_binary, default: []
-  mattr_accessor :content_types_allowed_inline,     default: []
+  mattr_accessor :variable_content_types,             default: []
+  mattr_accessor :binary_content_type,                default: "application/octet-stream"
+  mattr_accessor :content_types_to_serve_as_binary,   default: []
+  mattr_accessor :content_types_allowed_inline,       default: []
+  mattr_accessor :supported_image_processing_methods, default: []
+  mattr_accessor :unsupported_image_processing_arguments
 
   mattr_accessor :service_urls_expire_in, default: 5.minutes
 
diff --git a/activestorage/lib/active_storage/engine.rb b/activestorage/lib/active_storage/engine.rb
index b665c4983f..feb9cd2858 100644
--- a/activestorage/lib/active_storage/engine.rb
+++ b/activestorage/lib/active_storage/engine.rb
@@ -63,6 +63,20 @@ class Engine < Rails::Engine # :nodoc:
       application/pdf
     )
 
+    default_unsupported_image_processing_arguments = %w(
+      -debug
+      -display
+      -distribute-cache
+      -help
+      -path
+      -print
+      -set
+      -verbose
+      -version
+      -write
+      -write-mask
+    )
+
     config.eager_load_namespaces << ActiveStorage
 
     initializer "active_storage.configs" do
@@ -74,6 +88,8 @@ class Engine < Rails::Engine # :nodoc:
         ActiveStorage.paths             = app.config.active_storage.paths || {}
         ActiveStorage.routes_prefix     = app.config.active_storage.routes_prefix || "/rails/active_storage"
 
+        ActiveStorage.supported_image_processing_methods = app.config.active_storage.supported_image_processing_methods || []
+        ActiveStorage.unsupported_image_processing_arguments = app.config.active_storage.unsupported_image_processing_arguments || default_unsupported_image_processing_arguments
         ActiveStorage.variable_content_types = app.config.active_storage.variable_content_types || []
         ActiveStorage.content_types_to_serve_as_binary = app.config.active_storage.content_types_to_serve_as_binary || []
         ActiveStorage.service_urls_expire_in = app.config.active_storage.service_urls_expire_in || 5.minutes
diff --git a/activestorage/lib/active_storage/transformers/image_processing_transformer.rb b/activestorage/lib/active_storage/transformers/image_processing_transformer.rb
index 506150576c..60d38d51b2 100644
--- a/activestorage/lib/active_storage/transformers/image_processing_transformer.rb
+++ b/activestorage/lib/active_storage/transformers/image_processing_transformer.rb
@@ -6,6 +6,300 @@ module ActiveStorage
   module Transformers
     class ImageProcessingTransformer < Transformer
       private
+        class UnsupportedImageProcessingMethod < StandardError; end
+        class UnsupportedImageProcessingArgument < StandardError; end
+        SUPPORTED_IMAGE_PROCESSING_METHODS = [
+          "adaptive_blur",
+          "adaptive_resize",
+          "adaptive_sharpen",
+          "adjoin",
+          "affine",
+          "alpha",
+          "annotate",
+          "antialias",
+          "append",
+          "apply",
+          "attenuate",
+          "authenticate",
+          "auto_gamma",
+          "auto_level",
+          "auto_orient",
+          "auto_threshold",
+          "backdrop",
+          "background",
+          "bench",
+          "bias",
+          "bilateral_blur",
+          "black_point_compensation",
+          "black_threshold",
+          "blend",
+          "blue_primary",
+          "blue_shift",
+          "blur",
+          "border",
+          "bordercolor",
+          "borderwidth",
+          "brightness_contrast",
+          "cache",
+          "canny",
+          "caption",
+          "channel",
+          "channel_fx",
+          "charcoal",
+          "chop",
+          "clahe",
+          "clamp",
+          "clip",
+          "clip_path",
+          "clone",
+          "clut",
+          "coalesce",
+          "colorize",
+          "colormap",
+          "color_matrix",
+          "colors",
+          "colorspace",
+          "colourspace",
+          "color_threshold",
+          "combine",
+          "combine_options",
+          "comment",
+          "compare",
+          "complex",
+          "compose",
+          "composite",
+          "compress",
+          "connected_components",
+          "contrast",
+          "contrast_stretch",
+          "convert",
+          "convolve",
+          "copy",
+          "crop",
+          "cycle",
+          "deconstruct",
+          "define",
+          "delay",
+          "delete",
+          "density",
+          "depth",
+          "descend",
+          "deskew",
+          "despeckle",
+          "direction",
+          "displace",
+          "dispose",
+          "dissimilarity_threshold",
+          "dissolve",
+          "distort",
+          "dither",
+          "draw",
+          "duplicate",
+          "edge",
+          "emboss",
+          "encoding",
+          "endian",
+          "enhance",
+          "equalize",
+          "evaluate",
+          "evaluate_sequence",
+          "extent",
+          "extract",
+          "family",
+          "features",
+          "fft",
+          "fill",
+          "filter",
+          "flatten",
+          "flip",
+          "floodfill",
+          "flop",
+          "font",
+          "foreground",
+          "format",
+          "frame",
+          "function",
+          "fuzz",
+          "fx",
+          "gamma",
+          "gaussian_blur",
+          "geometry",
+          "gravity",
+          "grayscale",
+          "green_primary",
+          "hald_clut",
+          "highlight_color",
+          "hough_lines",
+          "iconGeometry",
+          "iconic",
+          "identify",
+          "ift",
+          "illuminant",
+          "immutable",
+          "implode",
+          "insert",
+          "intensity",
+          "intent",
+          "interlace",
+          "interline_spacing",
+          "interpolate",
+          "interpolative_resize",
+          "interword_spacing",
+          "kerning",
+          "kmeans",
+          "kuwahara",
+          "label",
+          "lat",
+          "layers",
+          "level",
+          "level_colors",
+          "limit",
+          "limits",
+          "linear_stretch",
+          "linewidth",
+          "liquid_rescale",
+          "list",
+          "loader",
+          "log",
+          "loop",
+          "lowlight_color",
+          "magnify",
+          "map",
+          "mattecolor",
+          "median",
+          "mean_shift",
+          "metric",
+          "mode",
+          "modulate",
+          "moments",
+          "monitor",
+          "monochrome",
+          "morph",
+          "morphology",
+          "mosaic",
+          "motion_blur",
+          "name",
+          "negate",
+          "noise",
+          "normalize",
+          "opaque",
+          "ordered_dither",
+          "orient",
+          "page",
+          "paint",
+          "pause",
+          "perceptible",
+          "ping",
+          "pointsize",
+          "polaroid",
+          "poly",
+          "posterize",
+          "precision",
+          "preview",
+          "process",
+          "quality",
+          "quantize",
+          "quiet",
+          "radial_blur",
+          "raise",
+          "random_threshold",
+          "range_threshold",
+          "red_primary",
+          "regard_warnings",
+          "region",
+          "remote",
+          "render",
+          "repage",
+          "resample",
+          "resize",
+          "resize_to_fill",
+          "resize_to_fit",
+          "resize_to_limit",
+          "resize_and_pad",
+          "respect_parentheses",
+          "reverse",
+          "roll",
+          "rotate",
+          "sample",
+          "sampling_factor",
+          "saver",
+          "scale",
+          "scene",
+          "screen",
+          "seed",
+          "segment",
+          "selective_blur",
+          "separate",
+          "sepia_tone",
+          "shade",
+          "shadow",
+          "shared_memory",
+          "sharpen",
+          "shave",
+          "shear",
+          "sigmoidal_contrast",
+          "silent",
+          "similarity_threshold",
+          "size",
+          "sketch",
+          "smush",
+          "snaps",
+          "solarize",
+          "sort_pixels",
+          "sparse_color",
+          "splice",
+          "spread",
+          "statistic",
+          "stegano",
+          "stereo",
+          "storage_type",
+          "stretch",
+          "strip",
+          "stroke",
+          "strokewidth",
+          "style",
+          "subimage_search",
+          "swap",
+          "swirl",
+          "synchronize",
+          "taint",
+          "text_font",
+          "threshold",
+          "thumbnail",
+          "tile_offset",
+          "tint",
+          "title",
+          "transform",
+          "transparent",
+          "transparent_color",
+          "transpose",
+          "transverse",
+          "treedepth",
+          "trim",
+          "type",
+          "undercolor",
+          "unique_colors",
+          "units",
+          "unsharp",
+          "update",
+          "valid_image",
+          "view",
+          "vignette",
+          "virtual_pixel",
+          "visual",
+          "watermark",
+          "wave",
+          "wavelet_denoise",
+          "weight",
+          "white_balance",
+          "white_point",
+          "white_threshold",
+          "window",
+          "window_group"
+        ].concat(ActiveStorage.supported_image_processing_methods)
+
+        UNSUPPORTED_IMAGE_PROCESSING_ARGUMENTS = ActiveStorage.unsupported_image_processing_arguments
+
         def process(file, format:)
           processor.
             source(file).
@@ -21,6 +315,16 @@ def processor
 
         def operations
           transformations.each_with_object([]) do |(name, argument), list|
+            if ActiveStorage.variant_processor == :mini_magick
+              if name.to_s == "combine_options"
+                argument.each do |subtransformation_name, subtransformation_argument|
+                  validate_transformation(subtransformation_name, subtransformation_argument)
+                end
+              else
+                validate_transformation(name, argument)
+              end
+            end
+
             if name.to_s == "combine_options"
               ActiveSupport::Deprecation.warn <<~WARNING.squish
                 Active Storage's ImageProcessing transformer doesn't support :combine_options,
@@ -34,6 +338,60 @@ def operations
             end
           end
         end
+
+        def validate_transformation(name, argument)
+          method_name = name.to_s.gsub("-","_")
+
+          unless SUPPORTED_IMAGE_PROCESSING_METHODS.any? { |method| method_name == method }
+            raise UnsupportedImageProcessingMethod, <<~ERROR.squish
+              One or more of the provided transformation methods is not supported.
+            ERROR
+          end
+
+          if argument.present?
+            if argument.is_a?(String) || argument.is_a?(Symbol)
+              validate_arg_string(argument)
+            elsif argument.is_a?(Array)
+              validate_arg_array(argument)
+            elsif argument.is_a?(Hash)
+              validate_arg_hash(argument)
+            end
+          end
+        end
+
+        def validate_arg_string(argument)
+          if UNSUPPORTED_IMAGE_PROCESSING_ARGUMENTS.any? { |bad_arg| argument.to_s.downcase.include?(bad_arg) }; raise UnsupportedImageProcessingArgument end
+        end
+
+        def validate_arg_array(argument)
+          argument.each do |arg|
+            if arg.is_a?(Integer) || arg.is_a?(Float)
+              next
+            elsif arg.is_a?(String) || arg.is_a?(Symbol)
+              validate_arg_string(arg)
+            elsif arg.is_a?(Array)
+              validate_arg_array(arg)
+            elsif arg.is_a?(Hash)
+              validate_arg_hash(arg)
+            end
+          end
+        end
+
+        def validate_arg_hash(argument)
+          argument.each do |key, value|
+            validate_arg_string(key)
+
+            if value.is_a?(Integer) || value.is_a?(Float)
+              next
+            elsif value.is_a?(String) || value.is_a?(Symbol)
+              validate_arg_string(value)
+            elsif value.is_a?(Array)
+              validate_arg_array(value)
+            elsif value.is_a?(Hash)
+              validate_arg_hash(value)
+            end
+          end
+        end
     end
   end
 end
diff --git a/activestorage/test/models/variant_test.rb b/activestorage/test/models/variant_test.rb
index 1d462b2294..717c437762 100644
--- a/activestorage/test/models/variant_test.rb
+++ b/activestorage/test/models/variant_test.rb
@@ -196,4 +196,78 @@ class ActiveStorage::VariantTest < ActiveSupport::TestCase
   ensure
     ActiveStorage.variant_processor = :mini_magick
   end
+
+  test "variations with unsupported methods in combine_options raise UnsupportedImageProcessingMethod" do
+    blob = create_file_blob(filename: "racecar.jpg")
+
+    assert_raise(ActiveStorage::Transformers::ImageProcessingTransformer::UnsupportedImageProcessingMethod) do
+      blob.variant(combine_options: {
+        gravity: "center",
+        write: "/tmp/danger",
+        crop: "100x10000",
+      }).processed
+    end
+  end
+
+  test "variations with dangerous argument in combine_options raise UnsupportedImageProcessingArgument" do
+    blob = create_file_blob(filename: "racecar.jpg")
+
+    assert_raise(ActiveStorage::Transformers::ImageProcessingTransformer::UnsupportedImageProcessingArgument) do
+      blob.variant(combine_options: {
+        gravity: "center",
+        resize: "-write /tmp/danger",
+        crop: "100x10000",
+      }).processed
+    end
+  end
+
+  test "variations with dangerous argument string raise UnsupportedImageProcessingArgument" do
+    blob = create_file_blob(filename: "racecar.jpg")
+    assert_raise(ActiveStorage::Transformers::ImageProcessingTransformer::UnsupportedImageProcessingArgument) do
+      blob.variant(resize: "-PaTh /tmp/file.erb").processed
+    end
+  end
+
+  test "variations with dangerous argument array raise UnsupportedImageProcessingArgument" do
+    blob = create_file_blob(filename: "racecar.jpg")
+    assert_raise(ActiveStorage::Transformers::ImageProcessingTransformer::UnsupportedImageProcessingArgument) do
+      blob.variant(resize: [123, "-write", "/tmp/file.erb"]).processed
+    end
+  end
+
+  test "variations with dangerous argument in a nested array raise UnsupportedImageProcessingArgument" do
+    blob = create_file_blob(filename: "racecar.jpg")
+    assert_raise(ActiveStorage::Transformers::ImageProcessingTransformer::UnsupportedImageProcessingArgument) do
+      blob.variant(resize: [123, ["-write", "/tmp/file.erb"], "/tmp/file.erb"]).processed
+    end
+
+    assert_raise(ActiveStorage::Transformers::ImageProcessingTransformer::UnsupportedImageProcessingArgument) do
+      blob.variant(resize: [123, {"-write /tmp/file.erb": "something"}, "/tmp/file.erb"]).processed
+    end
+  end
+
+  test "variations with dangerous argument hash raise UnsupportedImageProcessingArgument" do
+    blob = create_file_blob(filename: "racecar.jpg")
+    assert_raise(ActiveStorage::Transformers::ImageProcessingTransformer::UnsupportedImageProcessingArgument) do
+      blob.variant(saver: {"-write": "/tmp/file.erb"}).processed
+    end
+  end
+
+  test "variations with dangerous argument in a nested hash raise UnsupportedImageProcessingArgument" do
+    blob = create_file_blob(filename: "racecar.jpg")
+    assert_raise(ActiveStorage::Transformers::ImageProcessingTransformer::UnsupportedImageProcessingArgument) do
+      blob.variant(saver: {"something": {"-write": "/tmp/file.erb"}}).processed
+    end
+
+    assert_raise(ActiveStorage::Transformers::ImageProcessingTransformer::UnsupportedImageProcessingArgument) do
+      blob.variant(saver: {"something": ["-write", "/tmp/file.erb"]}).processed
+    end
+  end
+
+  test "variations with unsupported methods raise UnsupportedImageProcessingMethod" do
+    blob = create_file_blob(filename: "racecar.jpg")
+    assert_raise(ActiveStorage::Transformers::ImageProcessingTransformer::UnsupportedImageProcessingMethod) do
+      blob.variant(system: "touch /tmp/dangerous").processed
+    end
+  end
 end
diff --git a/railties/test/application/configuration_test.rb b/railties/test/application/configuration_test.rb
index 8de87c0c70..24836737a3 100644
--- a/railties/test/application/configuration_test.rb
+++ b/railties/test/application/configuration_test.rb
@@ -2355,6 +2355,35 @@ def index
       assert_equal Digest::SHA1, ActiveSupport::Digest.hash_digest_class
     end
 
+    test "ActiveStorage.supported_image_processing_methods can be configured via config.active_storage.supported_image_processing_methods" do
+      remove_from_config '.*config\.load_defaults.*\n'
+
+      app_file "config/initializers/add_image_processing_methods.rb", <<-RUBY
+        Rails.application.config.active_storage.supported_image_processing_methods = ["write", "set"]
+      RUBY
+
+      app "development"
+
+      assert ActiveStorage.supported_image_processing_methods.include?("write")
+      assert ActiveStorage.supported_image_processing_methods.include?("set")
+    end
+
+    test "ActiveStorage.unsupported_image_processing_arguments can be configured via config.active_storage.unsupported_image_processing_arguments" do
+      remove_from_config '.*config\.load_defaults.*\n'
+
+      app_file "config/initializers/add_image_processing_arguments.rb", <<-RUBY
+      Rails.application.config.active_storage.unsupported_image_processing_arguments = %w(
+        -write
+        -danger
+      )
+      RUBY
+
+      app "development"
+
+      assert ActiveStorage.unsupported_image_processing_arguments.include?("-danger")
+      refute ActiveStorage.unsupported_image_processing_arguments.include?("-set")
+    end
+
     test "custom serializers should be able to set via config.active_job.custom_serializers in an initializer" do
       class ::DummySerializer < ActiveJob::Serializers::ObjectSerializer; end
 
-- 
2.35.0

