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
This commit is contained in:
Claire 2023-07-06 15:05:05 +02:00 committed by GitHub
parent c4f2609f7a
commit 0aa0b71f2c
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
8 changed files with 80 additions and 7 deletions

View file

@ -22,15 +22,14 @@ module Attachmentable
included do included do
def self.has_attached_file(name, options = {}) # rubocop:disable Naming/PredicateName def self.has_attached_file(name, options = {}) # rubocop:disable Naming/PredicateName
options = { validate_media_type: false }.merge(options)
super(name, options) super(name, options)
send(:"before_#{name}_post_process") do
send(:"before_#{name}_validate") do
attachment = send(name) attachment = send(name)
check_image_dimension(attachment) check_image_dimension(attachment)
set_file_content_type(attachment) set_file_content_type(attachment)
obfuscate_file_name(attachment) obfuscate_file_name(attachment)
set_file_extension(attachment) set_file_extension(attachment)
Paperclip::Validators::MediaTypeSpoofDetectionValidator.new(attributes: [name]).validate(self)
end end
end end
end end

View file

@ -28,6 +28,7 @@ require_relative '../lib/paperclip/url_generator_extensions'
require_relative '../lib/paperclip/attachment_extensions' require_relative '../lib/paperclip/attachment_extensions'
require_relative '../lib/paperclip/lazy_thumbnail' require_relative '../lib/paperclip/lazy_thumbnail'
require_relative '../lib/paperclip/gif_transcoder' require_relative '../lib/paperclip/gif_transcoder'
require_relative '../lib/paperclip/media_type_spoof_detector_extensions'
require_relative '../lib/paperclip/transcoder' require_relative '../lib/paperclip/transcoder'
require_relative '../lib/paperclip/type_corrector' require_relative '../lib/paperclip/type_corrector'
require_relative '../lib/paperclip/response_with_limit_adapter' require_relative '../lib/paperclip/response_with_limit_adapter'

View file

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

View file

@ -155,3 +155,10 @@ unless defined?(Seahorse)
end end
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

View file

@ -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)

View file

@ -19,10 +19,7 @@ module Paperclip
def make def make
metadata = VideoMetadataExtractor.new(@file.path) metadata = VideoMetadataExtractor.new(@file.path)
unless metadata.valid? raise Paperclip::Error, "Error while transcoding #{@file.path}: unsupported file" unless metadata.valid?
Paperclip.log("Unsupported file #{@file.path}")
return File.open(@file.path)
end
update_attachment_type(metadata) update_attachment_type(metadata)
update_options_from_metadata(metadata) update_options_from_metadata(metadata)

BIN
spec/fixtures/files/boop.mp3 vendored Normal file

Binary file not shown.

View file

@ -150,6 +150,26 @@ RSpec.describe MediaAttachment, type: :model do
end end
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 describe 'jpeg' do
let(:media) { MediaAttachment.create(account: Fabricate(:account), file: attachment_fixture('attachment.jpg')) } let(:media) { MediaAttachment.create(account: Fabricate(:account), file: attachment_fixture('attachment.jpg')) }