diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index 0f6ccba284a..57f19f151fd 100644 --- a/app/controllers/uploads_controller.rb +++ b/app/controllers/uploads_controller.rb @@ -4,6 +4,9 @@ class UploadsController < ApplicationController def create type = params.require(:type) + + raise Discourse::InvalidAccess.new unless type =~ /^[a-z\-\_]{1,100}$/ + file = params[:file] || params[:files].try(:first) url = params[:url] client_id = params[:client_id] @@ -73,6 +76,7 @@ class UploadsController < ApplicationController # convert pasted images to HQ jpegs if filename == "blob.png" && SiteSetting.convert_pasted_images_to_hq_jpg jpeg_path = "#{File.dirname(tempfile.path)}/blob.jpg" + OptimizedImage.ensure_safe_paths!(tempfile.path, jpeg_path) `convert #{tempfile.path} -quality #{SiteSetting.convert_pasted_images_quality} #{jpeg_path}` # only change the format of the image when JPG is at least 5% smaller if File.size(jpeg_path) < File.size(tempfile.path) * 0.95 diff --git a/app/models/optimized_image.rb b/app/models/optimized_image.rb index 19953098e97..cc4134a43c9 100644 --- a/app/models/optimized_image.rb +++ b/app/models/optimized_image.rb @@ -97,7 +97,24 @@ class OptimizedImage < ActiveRecord::Base !(url =~ /^(https?:)?\/\//) end + def self.safe_path?(path) + # this matches instructions which call #to_s + path = path.to_s + return false if path != File.expand_path(path) + return false if path !~ /\A[_\-a-zA-Z0-9\.\/]+\z/m + true + end + + def self.ensure_safe_paths!(*paths) + paths.each do |path| + raise Discourse::InvalidAccess unless safe_path?(path) + end + end + + def self.resize_instructions(from, to, dimensions, opts={}) + ensure_safe_paths!(from, to) + # NOTE: ORDER is important! %W{ convert @@ -115,6 +132,8 @@ class OptimizedImage < ActiveRecord::Base end def self.resize_instructions_animated(from, to, dimensions, opts={}) + ensure_safe_paths!(from, to) + %W{ gifsicle --colors=256 @@ -126,6 +145,8 @@ class OptimizedImage < ActiveRecord::Base end def self.crop_instructions(from, to, dimensions, opts={}) + ensure_safe_paths!(from, to) + %W{ convert #{from}[0] @@ -141,6 +162,8 @@ class OptimizedImage < ActiveRecord::Base end def self.crop_instructions_animated(from, to, dimensions, opts={}) + ensure_safe_paths!(from, to) + %W{ gifsicle --crop 0,0+#{dimensions} @@ -152,6 +175,8 @@ class OptimizedImage < ActiveRecord::Base end def self.downsize_instructions(from, to, dimensions, opts={}) + ensure_safe_paths!(from, to) + %W{ convert #{from}[0] diff --git a/spec/controllers/uploads_controller_spec.rb b/spec/controllers/uploads_controller_spec.rb index 90c3426b17d..f06bd7fe785 100644 --- a/spec/controllers/uploads_controller_spec.rb +++ b/spec/controllers/uploads_controller_spec.rb @@ -33,6 +33,20 @@ describe UploadsController do }) end + it 'fails if type is invalid' do + xhr :post, :create, file: logo, type: "invalid type cause has space" + expect(response.status).to eq 403 + + xhr :post, :create, file: logo, type: "\\invalid" + expect(response.status).to eq 403 + + xhr :post, :create, file: logo, type: "invalid." + expect(response.status).to eq 403 + + xhr :post, :create, file: logo, type: "toolong"*100 + expect(response.status).to eq 403 + end + it 'is successful with an image' do Jobs.expects(:enqueue).with(:create_thumbnails, anything) diff --git a/spec/models/optimized_image_spec.rb b/spec/models/optimized_image_spec.rb index 40ea15c0d11..ffd51a0b102 100644 --- a/spec/models/optimized_image_spec.rb +++ b/spec/models/optimized_image_spec.rb @@ -5,6 +5,38 @@ describe OptimizedImage do let(:upload) { build(:upload) } before { upload.id = 42 } + describe ".safe_path?" do + + it "correctly detects unsafe paths" do + expect(OptimizedImage.safe_path?("/path/A-AA/22_00.TIFF")).to eq(true) + expect(OptimizedImage.safe_path?("/path/AAA/2200.TIFF")).to eq(true) + expect(OptimizedImage.safe_path?("/tmp/a.png")).to eq(true) + expect(OptimizedImage.safe_path?("../a.png")).to eq(false) + expect(OptimizedImage.safe_path?("/tmp/a.png\\test")).to eq(false) + expect(OptimizedImage.safe_path?("/tmp/a.png\\test")).to eq(false) + expect(OptimizedImage.safe_path?("/path/\u1000.png")).to eq(false) + expect(OptimizedImage.safe_path?("/path/x.png\n")).to eq(false) + expect(OptimizedImage.safe_path?("/path/x.png\ny.png")).to eq(false) + expect(OptimizedImage.safe_path?("/path/x.png y.png")).to eq(false) + expect(OptimizedImage.safe_path?(nil)).to eq(false) + end + + end + + describe "ensure_safe_paths!" do + it "raises nothing on safe paths" do + expect { + OptimizedImage.ensure_safe_paths!("/a.png", "/b.png") + }.not_to raise_error + end + + it "raises nothing on paths" do + expect { + OptimizedImage.ensure_safe_paths!("/a.png", "/b.png", "c.png") + }.to raise_error(Discourse::InvalidAccess) + end + end + describe ".local?" do def local(url)