From 393b63329b67aba61f420082f06eb7ec9c4ffe73 Mon Sep 17 00:00:00 2001
From: Rodrigo Souto <rodrigo@colivre.coop.br>
Date: Thu, 9 Jun 2011 13:37:18 -0300
Subject: [PATCH] Avoiding malicious uploaded file

---
 app/models/environment.rb       |  7 +++++++
 app/models/image.rb             |  2 ++
 app/models/thumbnail.rb         |  2 ++
 app/models/uploaded_file.rb     |  1 +
 test/unit/environment_test.rb   | 17 +++++++++++++++++
 test/unit/image_test.rb         |  5 +++++
 test/unit/thumbnail_test.rb     |  5 +++++
 test/unit/uploaded_file_test.rb |  5 +++++
 8 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/app/models/environment.rb b/app/models/environment.rb
index c82774d..ec1adf0 100644
--- a/app/models/environment.rb
+++ b/app/models/environment.rb
@@ -9,6 +9,13 @@ class Environment < ActiveRecord::Base
 
   has_many :tasks, :dependent => :destroy, :as => 'target'
 
+  IDENTIFY_SCRIPTS = /(?:php[0-9s]?(\..*)?|[sp]htm[l]?(\..*)?|pl|py|cgi|rb)/
+
+  def self.verify_filename(filename)
+    filename += '.txt' if filename =~ IDENTIFY_SCRIPTS
+    return filename
+  end
+
   PERMISSIONS['Environment'] = {
     'view_environment_admin_panel' => N_('View environment admin panel'),
     'edit_environment_features' => N_('Edit environment features'),
diff --git a/app/models/image.rb b/app/models/image.rb
index 2fa4420..2b22672 100644
--- a/app/models/image.rb
+++ b/app/models/image.rb
@@ -4,6 +4,8 @@ class Image < ActiveRecord::Base
     Image.attachment_options[:max_size]
   end
 
+  before_create { |file| file.filename = Environment.verify_filename(file.filename) }
+
   has_attachment :content_type => :image, 
                  :storage => :file_system, 
                  :path_prefix => 'public/image_uploads',
diff --git a/app/models/thumbnail.rb b/app/models/thumbnail.rb
index 2b1f523..26e9f40 100644
--- a/app/models/thumbnail.rb
+++ b/app/models/thumbnail.rb
@@ -3,5 +3,7 @@ class Thumbnail < ActiveRecord::Base
     :content_type => :image, :max_size => 5.megabytes
   validates_as_attachment
 
+  before_create { |file| file.filename = Environment.verify_filename(file.filename) }
+
   postgresql_attachment_fu
 end
diff --git a/app/models/uploaded_file.rb b/app/models/uploaded_file.rb
index 8b1111f..af9572c 100644
--- a/app/models/uploaded_file.rb
+++ b/app/models/uploaded_file.rb
@@ -20,6 +20,7 @@ class UploadedFile < Article
 
   before_create do |uploaded_file|
     uploaded_file.is_image = true if uploaded_file.image?
+    uploaded_file.filename = Environment.verify_filename(uploaded_file.filename)
   end
 
   def thumbnail_path
diff --git a/test/unit/environment_test.rb b/test/unit/environment_test.rb
index 7347b0b..85ef56e 100644
--- a/test/unit/environment_test.rb
+++ b/test/unit/environment_test.rb
@@ -1125,4 +1125,21 @@ class EnvironmentTest < Test::Unit::TestCase
     assert_equal 'localhost:9999', env.default_hostname
   end
 
+  should 'identify scripts with regex' do
+    scripts_extensions = %w[php php1 php4 phps php.bli cgi shtm phtm shtml phtml pl py rb]
+    name = 'uploaded_file'
+    scripts_extensions.each do |extension|
+      assert_not_nil name+'.'+extension =~ Environment::IDENTIFY_SCRIPTS
+    end
+  end
+
+  should 'verify filename and append .txt if script' do
+    scripts_extensions = %w[php php1 php4 phps php.bli cgi shtm phtm shtml phtml pl py rb]
+    name = 'uploaded_file'
+    scripts_extensions.each do |extension|
+      filename = name+'.'+extension
+      assert_equal filename+'.txt', Environment.verify_filename(filename)
+    end
+  end
+
 end
diff --git a/test/unit/image_test.rb b/test/unit/image_test.rb
index 17cee4e..4ae66da 100644
--- a/test/unit/image_test.rb
+++ b/test/unit/image_test.rb
@@ -118,4 +118,9 @@ class ImageTest < Test::Unit::TestCase
     file.destroy
   end
 
+  should 'not allow script files to be uploaded without append .txt in the end' do
+    file = Image.create!(:uploaded_data => fixture_file_upload('files/hello_world.php', 'image/png'))
+    assert_equal 'hello_world.php.txt', file.filename
+  end
+
 end
diff --git a/test/unit/thumbnail_test.rb b/test/unit/thumbnail_test.rb
index 86a6c60..51016d4 100644
--- a/test/unit/thumbnail_test.rb
+++ b/test/unit/thumbnail_test.rb
@@ -9,5 +9,10 @@ class ThumbnailTest < Test::Unit::TestCase
       assert_match 'image/', item 
     end
   end
+
+  should 'not allow script files to be uploaded without append .txt in the end' do
+    file = Thumbnail.create!(:uploaded_data => fixture_file_upload('files/hello_world.php', 'image/png'))
+    assert_equal 'hello_world.php.txt', file.filename
+  end
   
 end
diff --git a/test/unit/uploaded_file_test.rb b/test/unit/uploaded_file_test.rb
index b92517d..a96d1e1 100644
--- a/test/unit/uploaded_file_test.rb
+++ b/test/unit/uploaded_file_test.rb
@@ -325,4 +325,9 @@ class UploadedFileTest < Test::Unit::TestCase
     uses_sqlite
   end
 
+  should 'not allow script files to be uploaded without append .txt in the end' do
+    file = UploadedFile.create!(:uploaded_data => fixture_file_upload('files/hello_world.php', 'application/x-php'), :profile => @profile)
+    assert_equal 'hello_world.php.txt', file.filename
+  end
+
 end
--
libgit2 0.21.2