From 15b5fddd49e8fc5a783fa3a4e7f3c7f648e5ca8b Mon Sep 17 00:00:00 2001
From: Sam <sam.saffron@gmail.com>
Date: Mon, 19 Dec 2016 10:16:18 +1100
Subject: [PATCH] SECURITY: protect upload params, only allow very strict
filenames
---
app/controllers/uploads_controller.rb | 4 +++
app/models/optimized_image.rb | 25 ++++++++++++++++
spec/controllers/uploads_controller_spec.rb | 14 +++++++++
spec/models/optimized_image_spec.rb | 32 +++++++++++++++++++++
4 files changed, 75 insertions(+)
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)