From e5fbd34eac28264991ec804114531df73519f81a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 10 Nov 2021 22:43:46 +0100 Subject: [PATCH 1/3] Extract method to check for a filesystem storage We'll use this method to write a test dealing with remote storages. --- app/models/concerns/attachable.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/models/concerns/attachable.rb b/app/models/concerns/attachable.rb index 1fbe15512..8aec7ec65 100644 --- a/app/models/concerns/attachable.rb +++ b/app/models/concerns/attachable.rb @@ -26,7 +26,7 @@ module Attachable end def set_cached_attachment_from_attachment - self.cached_attachment = if Paperclip::Attachment.default_options[:storage] == :filesystem + self.cached_attachment = if filesystem_storage? attachment.path else attachment.url @@ -34,7 +34,7 @@ module Attachable end def set_attachment_from_cached_attachment - if Paperclip::Attachment.default_options[:storage] == :filesystem + if filesystem_storage? File.open(cached_attachment) { |file| self.attachment = file } else self.attachment = URI.open(cached_attachment) @@ -51,6 +51,10 @@ module Attachable private + def filesystem_storage? + Paperclip::Attachment.default_options[:storage] == :filesystem + end + def validate_attachment_size if association_class && attachment_file_size > max_file_size.megabytes errors.add(:attachment, I18n.t("#{model_name.plural}.errors.messages.in_between", From 5519518cfb41c6dce4ce206ab6d90723289b537b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 10 Nov 2021 23:18:37 +0100 Subject: [PATCH 2/3] Parse cached attachment URLs with remote storages In commit 5a4921a1a we replaced `URI.parse` with `URI.open` due to some issues during our tests with S3. However, there are some security issues with `URI.open` [1], since it might allow some users to execute code on the server. So we're using `URI.parse#open` instead. [1] https://docs.rubocop.org/rubocop/cops_security.html#securityopen --- app/models/concerns/attachable.rb | 2 +- lib/tasks/active_storage.rake | 2 +- spec/controllers/proposals_controller_spec.rb | 34 +++++++++++++++++++ 3 files changed, 36 insertions(+), 2 deletions(-) diff --git a/app/models/concerns/attachable.rb b/app/models/concerns/attachable.rb index 8aec7ec65..9fb8c7a80 100644 --- a/app/models/concerns/attachable.rb +++ b/app/models/concerns/attachable.rb @@ -37,7 +37,7 @@ module Attachable if filesystem_storage? File.open(cached_attachment) { |file| self.attachment = file } else - self.attachment = URI.open(cached_attachment) + self.attachment = URI.parse(cached_attachment).open end end diff --git a/lib/tasks/active_storage.rake b/lib/tasks/active_storage.rake index d548ab21d..548d5cfc9 100644 --- a/lib/tasks/active_storage.rake +++ b/lib/tasks/active_storage.rake @@ -88,7 +88,7 @@ namespace :active_storage do source_file = if paperclip_storage == :filesystem paperclip_attachment.path else - URI.open(paperclip_attachment.url, &:read) + URI.parse(paperclip_attachment.url).open.read end logger.info "Copying #{paperclip_attachment.url} to active storage" diff --git a/spec/controllers/proposals_controller_spec.rb b/spec/controllers/proposals_controller_spec.rb index 05e1cc279..ae586123b 100644 --- a/spec/controllers/proposals_controller_spec.rb +++ b/spec/controllers/proposals_controller_spec.rb @@ -8,4 +8,38 @@ describe ProposalsController do expect { get :index }.to raise_exception(FeatureFlags::FeatureDisabled) end end + + describe "PUT update" do + let(:file) { Rails.root.join("spec/hacked") } + + before do + File.delete(file) if File.exist?(file) + InvisibleCaptcha.timestamp_enabled = false + end + + after { InvisibleCaptcha.timestamp_enabled = true } + + it "ignores malicious cached attachments with remote storages" do + allow_any_instance_of(Image).to receive(:filesystem_storage?).and_return(false) + user = create(:user) + proposal = create(:proposal, author: user) + sign_in user + + begin + put :update, params: { + id: proposal, + proposal: { + image_attributes: { + title: "Hacked!", + user_id: user.id, + cached_attachment: "| touch #{file}" + } + } + } + rescue StandardError + ensure + expect(file).not_to exist + end + end + end end From dc87f9d69a65cc64aa49defb0fedfe94f96b59b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 10 Nov 2021 23:25:48 +0100 Subject: [PATCH 3/3] Add Security/Open rubocop rule The `open` method can be used to open files or URLs and it's deprecated in Ruby 2.7. In this case, it's clear we're dealing with a URL, so we can use `URI.parse`. The code was a bit strange, since it returned a value and had a side effect: opening the URL. I'm not sure about the intention of the code; my best guess is we wanted to test the URL exists and was accessible before returning it (and, if that's the case, IMHO the code should be a bit more explicit in order to show the intention behind it), but it could also be an unintended side effect which was there by accident. Now the URL is no longer opened; if the URL isn't accessible, we'll find out when trying to connect to it with the Savon client. --- .rubocop.yml | 3 +++ lib/sms_api.rb | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/.rubocop.yml b/.rubocop.yml index 483b38bbe..147fbb070 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -446,6 +446,9 @@ Security/Eval: Security/JSONLoad: Enabled: true +Security/Open: + Enabled: true + Security/YAMLLoad: Enabled: true diff --git a/lib/sms_api.rb b/lib/sms_api.rb index 64a25e7e0..e3187b710 100644 --- a/lib/sms_api.rb +++ b/lib/sms_api.rb @@ -9,7 +9,7 @@ class SMSApi def url return "" unless end_point_available? - open(Rails.application.secrets.sms_end_point).base_uri.to_s + URI.parse(Rails.application.secrets.sms_end_point).to_s end def authorization