From a6fa3960c3a149e83eb2ff057be4472a82958e3d Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 29 Oct 2015 10:42:44 -0700 Subject: use secure string comparisons for basic auth username / password this will avoid timing attacks against applications that use basic auth. Conflicts: activesupport/lib/active_support/security_utils.rb Conflicts: actionpack/lib/action_controller/metal/http_authentication.rb CVE-2015-7576 --- .../action_controller/metal/http_authentication.rb | 7 +++++- activesupport/lib/active_support/security_utils.rb | 27 ++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) create mode 100644 activesupport/lib/active_support/security_utils.rb diff --git a/actionpack/lib/action_controller/metal/http_authentication.rb b/actionpack/lib/action_controller/metal/http_authentication.rb index fe4ab65bba..2ae516097a 100644 --- a/actionpack/lib/action_controller/metal/http_authentication.rb +++ b/actionpack/lib/action_controller/metal/http_authentication.rb @@ -1,5 +1,6 @@ require 'active_support/base64' require 'active_support/core_ext/object/blank' +require 'active_support/security_utils' module ActionController module HttpAuthentication @@ -111,7 +112,11 @@ module ActionController def http_basic_authenticate_with(options = {}) before_filter(options.except(:name, :password, :realm)) do authenticate_or_request_with_http_basic(options[:realm] || "Application") do |name, password| - name == options[:name] && password == options[:password] + # This comparison uses & so that it doesn't short circuit and + # uses `variable_size_secure_compare` so that length information + # isn't leaked. + ActiveSupport::SecurityUtils.variable_size_secure_compare(name, options[:name]) & + ActiveSupport::SecurityUtils.variable_size_secure_compare(password, options[:password]) end end end diff --git a/activesupport/lib/active_support/security_utils.rb b/activesupport/lib/active_support/security_utils.rb new file mode 100644 index 0000000000..9be8613ada --- /dev/null +++ b/activesupport/lib/active_support/security_utils.rb @@ -0,0 +1,27 @@ +require 'digest' + +module ActiveSupport + module SecurityUtils + # Constant time string comparison. + # + # The values compared should be of fixed length, such as strings + # that have already been processed by HMAC. This should not be used + # on variable length plaintext strings because it could leak length info + # via timing attacks. + def secure_compare(a, b) + return false unless a.bytesize == b.bytesize + + l = a.unpack "C#{a.bytesize}" + + res = 0 + b.each_byte { |byte| res |= byte ^ l.shift } + res == 0 + end + module_function :secure_compare + + def variable_size_secure_compare(a, b) # :nodoc: + secure_compare(::Digest::SHA256.hexdigest(a), ::Digest::SHA256.hexdigest(b)) + end + module_function :variable_size_secure_compare + end +end -- cgit v1.2.3 From 127967b735813cd4f263df7a50426d74e7e9cc17 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Mon, 11 Jan 2016 14:36:49 -0800 Subject: stop caching mime types globally Unknown mime types should not be cached globally. This global cache leads to a memory leak and a denial of service vulnerability. CVE-2016-0751 --- actionpack/lib/action_dispatch/http/mime_type.rb | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/actionpack/lib/action_dispatch/http/mime_type.rb b/actionpack/lib/action_dispatch/http/mime_type.rb index 2152351703..be0088b562 100644 --- a/actionpack/lib/action_dispatch/http/mime_type.rb +++ b/actionpack/lib/action_dispatch/http/mime_type.rb @@ -22,7 +22,7 @@ module Mime SET = Mimes.new EXTENSION_LOOKUP = {} - LOOKUP = Hash.new { |h, k| h[k] = Type.new(k) unless k.blank? } + LOOKUP = {} def self.[](type) return type if type.is_a?(Type) @@ -85,7 +85,7 @@ module Mime Q_SEPARATOR_REGEXP = /;\s*q=/ def lookup(string) - LOOKUP[string] + LOOKUP[string] || Type.new(string) end def lookup_by_extension(extension) @@ -204,9 +204,12 @@ module Mime end end + attr_reader :hash + def initialize(string, symbol = nil, synonyms = []) @symbol, @synonyms = symbol, synonyms @string = string + @hash = [@string, @synonyms, @symbol].hash end def to_s @@ -240,6 +243,13 @@ module Mime end end + def eql?(other) + super || (self.class == other.class && + @string == other.string && + @synonyms == other.synonyms && + @symbol == other.symbol) + end + def =~(mime_type) return false if mime_type.blank? regexp = Regexp.new(Regexp.quote(mime_type.to_s)) @@ -262,6 +272,10 @@ module Mime super || method.to_s =~ /(\w+)\?$/ end + protected + + attr_reader :string, :synonyms + private def method_missing(method, *args) if method.to_s =~ /(\w+)\?$/ -- cgit v1.2.3 From cdabc95608336dbea7b6a3a3e925de5bbd5313ba Mon Sep 17 00:00:00 2001 From: Andrew White Date: Fri, 27 Nov 2015 13:46:46 +0000 Subject: Don't short-circuit reject_if proc When updating an associated record via nested attribute hashes the reject_if proc could be bypassed if the _destroy flag was set in the attribute hash and allow_destroy was set to false. The fix is to only short-circuit if the _destroy flag is set and the option allow_destroy is set to true. It also fixes an issue where a new record wasn't created if _destroy was set and the option allow_destroy was set to false. CVE-2015-7577 --- activerecord/lib/active_record/nested_attributes.rb | 14 ++++++++++++-- activerecord/test/cases/nested_attributes_test.rb | 13 +++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb index 41b62f6037..a0e98c37ae 100644 --- a/activerecord/lib/active_record/nested_attributes.rb +++ b/activerecord/lib/active_record/nested_attributes.rb @@ -470,11 +470,12 @@ module ActiveRecord # has_destroy_flag? or if a :reject_if proc exists for this # association and evaluates to +true+. def reject_new_record?(association_name, attributes) - has_destroy_flag?(attributes) || call_reject_if(association_name, attributes) + will_be_destroyed?(association_name, attributes) || call_reject_if(association_name, attributes) end def call_reject_if(association_name, attributes) - return false if has_destroy_flag?(attributes) + return false if will_be_destroyed?(association_name, attributes) + case callback = self.nested_attributes_options[association_name][:reject_if] when Symbol method(callback).arity == 0 ? send(callback) : send(callback, attributes) @@ -483,6 +484,15 @@ module ActiveRecord end end + # Only take into account the destroy flag if :allow_destroy is true + def will_be_destroyed?(association_name, attributes) + allow_destroy?(association_name) && has_destroy_flag?(attributes) + end + + def allow_destroy?(association_name) + self.nested_attributes_options[association_name][:allow_destroy] + end + def raise_nested_attributes_record_not_found(association_name, record_id) raise RecordNotFound, "Couldn't find #{self.class.reflect_on_association(association_name).klass.name} with ID=#{record_id} for #{self.class.name} with ID=#{id}" end diff --git a/activerecord/test/cases/nested_attributes_test.rb b/activerecord/test/cases/nested_attributes_test.rb index 85b9d3c1a1..f5f0517681 100644 --- a/activerecord/test/cases/nested_attributes_test.rb +++ b/activerecord/test/cases/nested_attributes_test.rb @@ -147,6 +147,19 @@ class TestNestedAttributesInGeneral < ActiveRecord::TestCase assert man.reload.interests.empty? end + def test_reject_if_is_not_short_circuited_if_allow_destroy_is_false + Pirate.accepts_nested_attributes_for :ship, reject_if: ->(a) { a[:name] == "The Golden Hind" }, allow_destroy: false + + pirate = Pirate.create!(catchphrase: "Stop wastin' me time", ship_attributes: { name: "White Pearl", _destroy: "1" }) + assert_equal "White Pearl", pirate.reload.ship.name + + pirate.update!(ship_attributes: { id: pirate.ship.id, name: "The Golden Hind", _destroy: "1" }) + assert_equal "White Pearl", pirate.reload.ship.name + + pirate.update!(ship_attributes: { id: pirate.ship.id, name: "Black Pearl", _destroy: "1" }) + assert_equal "Black Pearl", pirate.reload.ship.name + end + def test_has_many_association_updating_a_single_record Man.accepts_nested_attributes_for(:interests) man = Man.create(:name => 'John') -- cgit v1.2.3 From 18269d250fa58001ce7d8318571546aa90412975 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 20 Jan 2016 10:39:19 -0800 Subject: allow :file to be outside rails root, but anything else must be inside the rails view directory Conflicts: actionpack/test/controller/render_test.rb actionview/lib/action_view/template/resolver.rb CVE-2016-0752 --- actionpack/lib/action_view/template/resolver.rb | 17 ++++++++++++ .../test/controller/new_base/render_file_test.rb | 18 ++++++++++--- actionpack/test/controller/render_test.rb | 31 ++++++++++++++++++++++ actionpack/test/template/render_test.rb | 7 +++++ 4 files changed, 69 insertions(+), 4 deletions(-) diff --git a/actionpack/lib/action_view/template/resolver.rb b/actionpack/lib/action_view/template/resolver.rb index 47ea8a3c9b..c6db6685e4 100644 --- a/actionpack/lib/action_view/template/resolver.rb +++ b/actionpack/lib/action_view/template/resolver.rb @@ -110,6 +110,9 @@ module ActionView super() end + cattr_accessor :allow_external_files, instance_reader: false, instance_writer: false + self.allow_external_files = false + private def find_templates(name, prefix, partial, details) @@ -122,6 +125,10 @@ module ActionView template_paths = find_template_paths query + unless self.class.allow_external_files + template_paths = reject_files_external_to_app(template_paths) + end + template_paths.map { |template| handler, format = extract_handler_and_format(template, formats) contents = File.binread template @@ -133,6 +140,10 @@ module ActionView } end + def reject_files_external_to_app(files) + files.reject { |filename| !inside_path?(@path, filename) } + end + if RUBY_VERSION >= '2.2.0' def find_template_paths(query) Dir[query].reject { |filename| @@ -153,6 +164,12 @@ module ActionView end end + def inside_path?(path, filename) + filename = File.expand_path(filename) + path = File.join(path, '') + filename.start_with?(path) + end + # Helper for building query glob string based on resolver's pattern. def build_query(path, details) query = @pattern.dup diff --git a/actionpack/test/controller/new_base/render_file_test.rb b/actionpack/test/controller/new_base/render_file_test.rb index a961cbf849..c0e23db457 100644 --- a/actionpack/test/controller/new_base/render_file_test.rb +++ b/actionpack/test/controller/new_base/render_file_test.rb @@ -72,13 +72,23 @@ module RenderFile end test "rendering a relative path" do - get :relative_path - assert_response "The secret is in the sauce\n" + begin + ActionView::PathResolver.allow_external_files = true + get :relative_path + assert_response "The secret is in the sauce\n" + ensure + ActionView::PathResolver.allow_external_files = false + end end test "rendering a relative path with dot" do - get :relative_path_with_dot - assert_response "The secret is in the sauce\n" + begin + ActionView::PathResolver.allow_external_files = true + get :relative_path_with_dot + assert_response "The secret is in the sauce\n" + ensure + ActionView::PathResolver.allow_external_files = false + end end test "rendering a Pathname" do diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb index 3964540def..6210524cef 100644 --- a/actionpack/test/controller/render_test.rb +++ b/actionpack/test/controller/render_test.rb @@ -60,6 +60,16 @@ class TestController < ActionController::Base end end + def dynamic_render + render params[:id] # => String, Hash + end + + def dynamic_render_with_file + # This is extremely bad, but should be possible to do. + file = params[:id] # => String, Hash + render file: file + end + def conditional_hello_with_public_header if stale?(:last_modified => Time.now.utc.beginning_of_day, :etag => [:foo, 123], :public => true) render :action => 'hello_world' @@ -235,6 +245,27 @@ class TestController < ActionController::Base render :inline => "<%= action_name %>" end + def test_dynamic_render_with_file + # This is extremely bad, but should be possible to do. + assert File.exist?(File.join(File.dirname(__FILE__), '../../test/abstract_unit.rb')) + response = get :dynamic_render_with_file, { id: '../\\../test/abstract_unit.rb' } + assert_equal File.read(File.join(File.dirname(__FILE__), '../../test/abstract_unit.rb')), + response.body + end + + def test_dynamic_render + assert File.exist?(File.join(File.dirname(__FILE__), '../../test/abstract_unit.rb')) + assert_raises ActionView::MissingTemplate do + get :dynamic_render, { id: '../\\../test/abstract_unit.rb' } + end + end + + def test_dynamic_render_file_hash + assert_raises ArgumentError do + get :dynamic_render, { id: { file: '../\\../test/abstract_unit.rb' } } + end + end + def accessing_controller_name_in_template render :inline => "<%= controller_name %>" end diff --git a/actionpack/test/template/render_test.rb b/actionpack/test/template/render_test.rb index 03f3a34681..f207ad731f 100644 --- a/actionpack/test/template/render_test.rb +++ b/actionpack/test/template/render_test.rb @@ -126,6 +126,13 @@ module RenderTestCases assert_equal "only partial", @view.render("test/partial_only") end + def test_render_outside_path + assert File.exist?(File.join(File.dirname(__FILE__), '../../test/abstract_unit.rb')) + assert_raises ActionView::MissingTemplate do + @view.render(:template => "../\\../test/abstract_unit.rb") + end + end + def test_render_partial assert_equal "only partial", @view.render(:partial => "test/partial_only") end -- cgit v1.2.3 From 8d86637fb64ae8ae81ab71a286ddba02cc3144a4 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Mon, 25 Jan 2016 10:27:00 -0800 Subject: bumping version --- RAILS_VERSION | 2 +- actionmailer/lib/action_mailer/version.rb | 2 +- actionpack/lib/action_pack/version.rb | 2 +- activemodel/lib/active_model/version.rb | 2 +- activerecord/lib/active_record/version.rb | 2 +- activeresource/lib/active_resource/version.rb | 2 +- activesupport/lib/active_support/version.rb | 2 +- railties/lib/rails/version.rb | 2 +- version.rb | 2 +- 9 files changed, 9 insertions(+), 9 deletions(-) diff --git a/RAILS_VERSION b/RAILS_VERSION index 0698331f6a..94f93ee70c 100644 --- a/RAILS_VERSION +++ b/RAILS_VERSION @@ -1 +1 @@ -3.2.22 +3.2.22.1 diff --git a/actionmailer/lib/action_mailer/version.rb b/actionmailer/lib/action_mailer/version.rb index ef12355cd7..a8850dc834 100644 --- a/actionmailer/lib/action_mailer/version.rb +++ b/actionmailer/lib/action_mailer/version.rb @@ -3,7 +3,7 @@ module ActionMailer MAJOR = 3 MINOR = 2 TINY = 22 - PRE = nil + PRE = "1" STRING = [MAJOR, MINOR, TINY, PRE].compact.join('.') end diff --git a/actionpack/lib/action_pack/version.rb b/actionpack/lib/action_pack/version.rb index f608225e63..f9cbb645c5 100644 --- a/actionpack/lib/action_pack/version.rb +++ b/actionpack/lib/action_pack/version.rb @@ -3,7 +3,7 @@ module ActionPack MAJOR = 3 MINOR = 2 TINY = 22 - PRE = nil + PRE = "1" STRING = [MAJOR, MINOR, TINY, PRE].compact.join('.') end diff --git a/activemodel/lib/active_model/version.rb b/activemodel/lib/active_model/version.rb index a44ecbdb41..e0cc92f609 100644 --- a/activemodel/lib/active_model/version.rb +++ b/activemodel/lib/active_model/version.rb @@ -3,7 +3,7 @@ module ActiveModel MAJOR = 3 MINOR = 2 TINY = 22 - PRE = nil + PRE = "1" STRING = [MAJOR, MINOR, TINY, PRE].compact.join('.') end diff --git a/activerecord/lib/active_record/version.rb b/activerecord/lib/active_record/version.rb index f964aef280..14f87999ce 100644 --- a/activerecord/lib/active_record/version.rb +++ b/activerecord/lib/active_record/version.rb @@ -3,7 +3,7 @@ module ActiveRecord MAJOR = 3 MINOR = 2 TINY = 22 - PRE = nil + PRE = "1" STRING = [MAJOR, MINOR, TINY, PRE].compact.join('.') end diff --git a/activeresource/lib/active_resource/version.rb b/activeresource/lib/active_resource/version.rb index aa9c06cad2..33db975653 100644 --- a/activeresource/lib/active_resource/version.rb +++ b/activeresource/lib/active_resource/version.rb @@ -3,7 +3,7 @@ module ActiveResource MAJOR = 3 MINOR = 2 TINY = 22 - PRE = nil + PRE = "1" STRING = [MAJOR, MINOR, TINY, PRE].compact.join('.') end diff --git a/activesupport/lib/active_support/version.rb b/activesupport/lib/active_support/version.rb index 4ff3b521a5..18dbb49146 100644 --- a/activesupport/lib/active_support/version.rb +++ b/activesupport/lib/active_support/version.rb @@ -3,7 +3,7 @@ module ActiveSupport MAJOR = 3 MINOR = 2 TINY = 22 - PRE = nil + PRE = "1" STRING = [MAJOR, MINOR, TINY, PRE].compact.join('.') end diff --git a/railties/lib/rails/version.rb b/railties/lib/rails/version.rb index 9a42f80024..1147248f81 100644 --- a/railties/lib/rails/version.rb +++ b/railties/lib/rails/version.rb @@ -3,7 +3,7 @@ module Rails MAJOR = 3 MINOR = 2 TINY = 22 - PRE = nil + PRE = "1" STRING = [MAJOR, MINOR, TINY, PRE].compact.join('.') end diff --git a/version.rb b/version.rb index 9a42f80024..1147248f81 100644 --- a/version.rb +++ b/version.rb @@ -3,7 +3,7 @@ module Rails MAJOR = 3 MINOR = 2 TINY = 22 - PRE = nil + PRE = "1" STRING = [MAJOR, MINOR, TINY, PRE].compact.join('.') end -- cgit v1.2.3