From 94e2f00d2abedbea1ef62fc775d031ffda00662c Mon Sep 17 00:00:00 2001
From: Zack <zack.ref@gmail.com>
Date: Fri, 11 Feb 2022 17:26:21 -0500
Subject: [PATCH] Added image transformation validation via configurable
 allow-list.

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

[CVE-2022-21831]
---
 .../app/models/active_storage/variation.rb    | 352 ++++++++++++++++++
 activestorage/lib/active_storage.rb           |   2 +
 activestorage/lib/active_storage/engine.rb    |  16 +
 activestorage/test/models/variant_test.rb     |  74 ++++
 .../test/application/configuration_test.rb    |  29 ++
 5 files changed, 473 insertions(+)

diff --git a/activestorage/app/models/active_storage/variation.rb b/activestorage/app/models/active_storage/variation.rb
index 12e7f9f0b5..2295559572 100644
--- a/activestorage/app/models/active_storage/variation.rb
+++ b/activestorage/app/models/active_storage/variation.rb
@@ -20,6 +20,301 @@
 class ActiveStorage::Variation
   attr_reader :transformations
 
+  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
+
   class << self
     # Returns a Variation instance based on the given variator. If the variator is a Variation, it is
     # returned unmodified. If it is a String, it is passed to ActiveStorage::Variation.decode. Otherwise,
@@ -56,12 +351,15 @@ def initialize(transformations)
   def transform(image)
     ActiveSupport::Notifications.instrument("transform.active_storage") do
       transformations.each do |name, argument_or_subtransformations|
+        validate_transformation(name, argument_or_subtransformations)
         image.mogrify do |command|
           if name.to_s == "combine_options"
             argument_or_subtransformations.each do |subtransformation_name, subtransformation_argument|
+              validate_transformation(subtransformation_name, subtransformation_argument)
               pass_transform_argument(command, subtransformation_name, subtransformation_argument)
             end
           else
+            validate_transformation(name, argument_or_subtransformations)
             pass_transform_argument(command, name, argument_or_subtransformations)
           end
         end
@@ -86,4 +384,58 @@ def pass_transform_argument(command, method, argument)
     def eligible_argument?(argument)
       argument.present? && argument != true
     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
diff --git a/activestorage/lib/active_storage.rb b/activestorage/lib/active_storage.rb
index 81c8c771dc..2b26d0c825 100644
--- a/activestorage/lib/active_storage.rb
+++ b/activestorage/lib/active_storage.rb
@@ -50,4 +50,6 @@ module ActiveStorage
   mattr_accessor :content_types_to_serve_as_binary, default: []
   mattr_accessor :content_types_allowed_inline, default: []
   mattr_accessor :binary_content_type, default: "application/octet-stream"
+  mattr_accessor :supported_image_processing_methods, default: []
+  mattr_accessor :unsupported_image_processing_arguments
 end
diff --git a/activestorage/lib/active_storage/engine.rb b/activestorage/lib/active_storage/engine.rb
index a0a2982c7f..1702e5bc49 100644
--- a/activestorage/lib/active_storage/engine.rb
+++ b/activestorage/lib/active_storage/engine.rb
@@ -51,6 +51,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
@@ -61,6 +75,8 @@ class Engine < Rails::Engine # :nodoc:
         ActiveStorage.analyzers  = app.config.active_storage.analyzers || []
         ActiveStorage.paths      = app.config.active_storage.paths || {}
 
+        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.content_types_allowed_inline = app.config.active_storage.content_types_allowed_inline || []
diff --git a/activestorage/test/models/variant_test.rb b/activestorage/test/models/variant_test.rb
index 1399d85c66..c44a037a06 100644
--- a/activestorage/test/models/variant_test.rb
+++ b/activestorage/test/models/variant_test.rb
@@ -4,6 +4,80 @@
 require "database/setup"
 
 class ActiveStorage::VariantTest < ActiveSupport::TestCase
+  test "variations with unsupported methods raise UnsupportedImageProcessingMethod" do
+    blob = create_file_blob(filename: "racecar.jpg")
+    assert_raise(ActiveStorage::Variation::UnsupportedImageProcessingMethod) do
+      blob.variant(system: "touch /tmp/dangerous").processed
+    end
+  end
+
+  test "variations with unsupported methods in combine_options raise UnsupportedImageProcessingMethod" do
+    blob = create_file_blob(filename: "racecar.jpg")
+
+    assert_raise(ActiveStorage::Variation::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::Variation::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::Variation::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::Variation::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::Variation::UnsupportedImageProcessingArgument) do
+      blob.variant(resize: [123, ["-write", "/tmp/file.erb"], "/tmp/file.erb"]).processed
+    end
+
+    assert_raise(ActiveStorage::Variation::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::Variation::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::Variation::UnsupportedImageProcessingArgument) do
+      blob.variant(saver: {"something": {"-write": "/tmp/file.erb"}}).processed
+    end
+
+    assert_raise(ActiveStorage::Variation::UnsupportedImageProcessingArgument) do
+      blob.variant(saver: {"something": ["-write", "/tmp/file.erb"]}).processed
+    end
+  end
+
   test "resized variation of JPEG blob" do
     blob = create_file_blob(filename: "racecar.jpg")
     variant = blob.variant(resize: "100x100").processed
diff --git a/railties/test/application/configuration_test.rb b/railties/test/application/configuration_test.rb
index 9f7dcb38dc..29963735a8 100644
--- a/railties/test/application/configuration_test.rb
+++ b/railties/test/application/configuration_test.rb
@@ -1964,6 +1964,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
+
     private
       def force_lazy_load_hooks
         yield # Tasty clarifying sugar, homie! We only need to reference a constant to load it.
-- 
2.35.0

