CVE-2023-36460: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') (CWE-22)
value: Mastodon is a free, open-source social network server based on ActivityPub. Starting in version 3.5.0 and prior to versions 3.5.9, 4.0.5, and 4.1.3, attackers using carefully crafted media files can cause Mastodon's media processing code to create arbitrary files at any location. This allows attackers to create and overwrite any file Mastodon has access to, allowing Denial of Service and arbitrary Remote Code Execution. Versions 3.5.9, 4.0.5, and 4.1.3 contain a patch for this issue.
CWE description: The product uses external input to construct a pathname that is intended to identify a file or directory that is located underneath a restricted parent directory, but the product does not properly neutralize special elements within the pathname that can cause the pathname to resolve to a location that is outside of the restricted directory.
CVSS v3 base score: 9.9
Published: 2023-07-06T19:15Z
Repository: https://github.com/mastodon/mastodon
Language: Ruby
Files changed: app/models/concerns/attachmentable.rb, config/application.rb, config/imagemagick/policy.xml, config/initializers/paperclip.rb, lib/paperclip/media_type_spoof_detector_extensions.rb, lib/paperclip/transcoder.rb, spec/models/media_attachment_spec.rb
Commit date: 2023-07-06 15:05:05 +0200
Commit message: Merge pull request from GHSA-9928-3cp5-93fm

* Fix attachments getting processed despite failing content-type validation

* Add a restrictive ImageMagick security policy tailored for Mastodon

* Fix misdetection of MP3 files with large cover art

* Reject unprocessable audio/video files instead of keeping them unchanged
Diff: diff --git a/app/models/concerns/attachmentable.rb b/app/models/concerns/attachmentable.rb
index 9cafedc209..f93ee4c919 100644
--- a/app/models/concerns/attachmentable.rb
+++ b/app/models/concerns/attachmentable.rb
@@ -20,19 +20,18 @@ module Attachmentable
     video/webm
   ).freeze
 
   included do
     def self.has_attached_file(name, options = {}) # rubocop:disable Naming/PredicateName
-      options = { validate_media_type: false }.merge(options)
       super(name, options)
-      send(:"before_#{name}_post_process") do
+
+      send(:"before_#{name}_validate") do
         attachment = send(name)
         check_image_dimension(attachment)
         set_file_content_type(attachment)
         obfuscate_file_name(attachment)
         set_file_extension(attachment)
-        Paperclip::Validators::MediaTypeSpoofDetectionValidator.new(attributes: [name]).validate(self)
       end
     end
   end
 
   private
diff --git a/config/application.rb b/config/application.rb
index d3c99baa12..8c4ec27e7f 100644
--- a/config/application.rb
+++ b/config/application.rb
@@ -26,10 +26,11 @@ require_relative '../lib/sanitize_ext/sanitize_config'
 require_relative '../lib/redis/namespace_extensions'
 require_relative '../lib/paperclip/url_generator_extensions'
 require_relative '../lib/paperclip/attachment_extensions'
 require_relative '../lib/paperclip/lazy_thumbnail'
 require_relative '../lib/paperclip/gif_transcoder'
+require_relative '../lib/paperclip/media_type_spoof_detector_extensions'
 require_relative '../lib/paperclip/transcoder'
 require_relative '../lib/paperclip/type_corrector'
 require_relative '../lib/paperclip/response_with_limit_adapter'
 require_relative '../lib/terrapin/multi_pipe_extensions'
 require_relative '../lib/mastodon/snowflake'
diff --git a/config/imagemagick/policy.xml b/config/imagemagick/policy.xml
new file mode 100644
index 0000000000..1052476b31
--- /dev/null
+++ b/config/imagemagick/policy.xml
@@ -0,0 +1,27 @@
+<policymap>
+  <!-- Set some basic system resource limits -->
+  <policy domain="resource" name="time" value="60" />
+
+  <policy domain="module" rights="none" pattern="URL" />
+
+  <policy domain="filter" rights="none" pattern="*" />
+
+  <!--
+    Ideally, we would restrict ImageMagick to only accessing its own
+    disk-backed pixel cache as well as Mastodon-created Tempfiles.
+
+    However, those paths depend on the operating system and environment
+    variables, so they can only be known at runtime.
+
+    Furthermore, those paths are not necessarily shared across Mastodon
+    processes, so even creating a policy.xml at runtime is impractical.
+
+    For the time being, only disable indirect reads.
+  -->
+  <policy domain="path" rights="none" pattern="@*" />
+
+  <!-- Disallow any coder by default, and only enable ones required by Mastodon -->
+  <policy domain="coder" rights="none" pattern="*" />
+  <policy domain="coder" rights="read | write" pattern="{PNG,JPEG,GIF,HEIC,WEBP}" />
+  <policy domain="coder" rights="write" pattern="{HISTOGRAM,RGB,INFO}" />
+</policymap>
diff --git a/config/initializers/paperclip.rb b/config/initializers/paperclip.rb
index 093d2ba9ae..f2da410dbe 100644
--- a/config/initializers/paperclip.rb
+++ b/config/initializers/paperclip.rb
@@ -151,5 +151,12 @@ unless defined?(Seahorse)
     module Client
       class NetworkingError < StandardError; end
     end
   end
 end
+
+# Set our ImageMagick security policy, but allow admins to override it
+ENV['MAGICK_CONFIGURE_PATH'] = begin
+  imagemagick_config_paths = ENV.fetch('MAGICK_CONFIGURE_PATH', '').split(File::PATH_SEPARATOR)
+  imagemagick_config_paths << Rails.root.join('config', 'imagemagick').expand_path.to_s
+  imagemagick_config_paths.join(File::PATH_SEPARATOR)
+end
diff --git a/lib/paperclip/media_type_spoof_detector_extensions.rb b/lib/paperclip/media_type_spoof_detector_extensions.rb
new file mode 100644
index 0000000000..a406ef312f
--- /dev/null
+++ b/lib/paperclip/media_type_spoof_detector_extensions.rb
@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+module Paperclip
+  module MediaTypeSpoofDetectorExtensions
+    def calculated_content_type
+      return @calculated_content_type if defined?(@calculated_content_type)
+
+      @calculated_content_type = type_from_file_command.chomp
+
+      # The `file` command fails to recognize some MP3 files as such
+      @calculated_content_type = type_from_marcel if @calculated_content_type == 'application/octet-stream' && type_from_marcel == 'audio/mpeg'
+      @calculated_content_type
+    end
+
+    def type_from_marcel
+      @type_from_marcel ||= Marcel::MimeType.for Pathname.new(@file.path),
+                                                 name: @file.path
+    end
+  end
+end
+
+Paperclip::MediaTypeSpoofDetector.prepend(Paperclip::MediaTypeSpoofDetectorExtensions)
diff --git a/lib/paperclip/transcoder.rb b/lib/paperclip/transcoder.rb
index afd9f58ff6..be40b49241 100644
--- a/lib/paperclip/transcoder.rb
+++ b/lib/paperclip/transcoder.rb
@@ -17,14 +17,11 @@ module Paperclip
     end
 
     def make
       metadata = VideoMetadataExtractor.new(@file.path)
 
-      unless metadata.valid?
-        Paperclip.log("Unsupported file #{@file.path}")
-        return File.open(@file.path)
-      end
+      raise Paperclip::Error, "Error while transcoding #{@file.path}: unsupported file" unless metadata.valid?
 
       update_attachment_type(metadata)
       update_options_from_metadata(metadata)
 
       destination = Tempfile.new([@basename, @format ? ".#{@format}" : ''])
diff --git a/spec/fixtures/files/boop.mp3 b/spec/fixtures/files/boop.mp3
new file mode 100644
index 0000000000..ba106a3a32
Binary files /dev/null and b/spec/fixtures/files/boop.mp3 differ
diff --git a/spec/models/media_attachment_spec.rb b/spec/models/media_attachment_spec.rb
index 2dfc6cf925..90e4f2f47b 100644
--- a/spec/models/media_attachment_spec.rb
+++ b/spec/models/media_attachment_spec.rb
@@ -150,10 +150,30 @@ RSpec.describe MediaAttachment, paperclip_processing: true do
     it 'gives the file a random name' do
       expect(media.file_file_name).to_not eq 'boop.ogg'
     end
   end
 
+  describe 'mp3 with large cover art' do
+    let(:media) { described_class.create(account: Fabricate(:account), file: attachment_fixture('boop.mp3')) }
+
+    it 'detects it as an audio file' do
+      expect(media.type).to eq 'audio'
+    end
+
+    it 'sets meta for the duration' do
+      expect(media.file.meta['original']['duration']).to be_within(0.05).of(0.235102)
+    end
+
+    it 'extracts thumbnail' do
+      expect(media.thumbnail.present?).to be true
+    end
+
+    it 'gives the file a random name' do
+      expect(media.file_file_name).to_not eq 'boop.mp3'
+    end
+  end
+
   describe 'jpeg' do
     let(:media) { described_class.create(account: Fabricate(:account), file: attachment_fixture('attachment.jpg')) }
 
     it 'sets meta for different style' do
       expect(media.file.meta['original']['width']).to eq 600

Vulnerable code:       options = { validate_media_type: false }.merge(options)
      send(:"before_#{name}_post_process") do
        Paperclip::Validators::MediaTypeSpoofDetectionValidator.new(attributes: [name]).validate(self)
      unless metadata.valid?
        Paperclip.log("Unsupported file #{@file.path}")
        return File.open(@file.path)
      end
Fixed code: 
      send(:"before_#{name}_validate") do
require_relative '../lib/paperclip/media_type_spoof_detector_extensions'
<policymap>
  <!-- Set some basic system resource limits -->
  <policy domain="resource" name="time" value="60" />

  <policy domain="module" rights="none" pattern="URL" />

  <policy domain="filter" rights="none" pattern="*" />

  <!--
    Ideally, we would restrict ImageMagick to only accessing its own
    disk-backed pixel cache as well as Mastodon-created Tempfiles.

    However, those paths depend on the operating system and environment
    variables, so they can only be known at runtime.

    Furthermore, those paths are not necessarily shared across Mastodon
    processes, so even creating a policy.xml at runtime is impractical.

    For the time being, only disable indirect reads.
  -->
  <policy domain="path" rights="none" pattern="@*" />

  <!-- Disallow any coder by default, and only enable ones required by Mastodon -->
  <policy domain="coder" rights="none" pattern="*" />
  <policy domain="coder" rights="read | write" pattern="{PNG,JPEG,GIF,HEIC,WEBP}" />
  <policy domain="coder" rights="write" pattern="{HISTOGRAM,RGB,INFO}" />
</policymap>

# Set our ImageMagick security policy, but allow admins to override it
ENV['MAGICK_CONFIGURE_PATH'] = begin
  imagemagick_config_paths = ENV.fetch('MAGICK_CONFIGURE_PATH', '').split(File::PATH_SEPARATOR)
  imagemagick_config_paths << Rails.root.join('config', 'imagemagick').expand_path.to_s
  imagemagick_config_paths.join(File::PATH_SEPARATOR)
end
# frozen_string_literal: true

module Paperclip
  module MediaTypeSpoofDetectorExtensions
    def calculated_content_type
      return @calculated_content_type if defined?(@calculated_content_type)

      @calculated_content_type = type_from_file_command.chomp

      # The `file` command fails to recognize some MP3 files as such
      @calculated_content_type = type_from_marcel if @calculated_content_type == 'application/octet-stream' && type_from_marcel == 'audio/mpeg'
      @calculated_content_type
    end

    def type_from_marcel
      @type_from_marcel ||= Marcel::MimeType.for Pathname.new(@file.path),
                                                 name: @file.path
    end
  end
end

Paperclip::MediaTypeSpoofDetector.prepend(Paperclip::MediaTypeSpoofDetectorExtensions)
      raise Paperclip::Error, "Error while transcoding #{@file.path}: unsupported file" unless metadata.valid?
  describe 'mp3 with large cover art' do
    let(:media) { described_class.create(account: Fabricate(:account), file: attachment_fixture('boop.mp3')) }

    it 'detects it as an audio file' do
      expect(media.type).to eq 'audio'
    end

    it 'sets meta for the duration' do
      expect(media.file.meta['original']['duration']).to be_within(0.05).of(0.235102)
    end

    it 'extracts thumbnail' do
      expect(media.thumbnail.present?).to be true
    end

    it 'gives the file a random name' do
      expect(media.file_file_name).to_not eq 'boop.mp3'
    end
  end