SECURITY: protect upload params, only allow very strict filenames

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

app/models/optimized_image.rb

@@ -98,7 +98,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
@@ -116,6 +133,8 @@ class OptimizedImage < ActiveRecord::Base
   end
 
   def self.resize_instructions_animated(from, to, dimensions, opts={})
+    ensure_safe_paths!(from, to)
+
     %W{
       gifsicle
       --colors=256
@@ -127,6 +146,8 @@ class OptimizedImage < ActiveRecord::Base
   end
 
   def self.crop_instructions(from, to, dimensions, opts={})
+    ensure_safe_paths!(from, to)
+
     %W{
       convert
       #{from}[0]
@@ -142,6 +163,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}
@@ -153,6 +176,8 @@ class OptimizedImage < ActiveRecord::Base
   end
 
   def self.downsize_instructions(from, to, dimensions, opts={})
+    ensure_safe_paths!(from, to)
+
     %W{
       convert
       #{from}[0]

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)
 

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)