[prev in list] [next in list] [prev in thread] [next in thread] 

List:       oss-security
Subject:    [oss-security] [CVE-2015-7577] Nested attributes rejection proc bypass in Active Record.
From:       Aaron Patterson <tenderlove () ruby-lang ! org>
Date:       2016-01-25 19:33:08
Message-ID: 20160125193308.GC14069 () TC ! local
[Download RAW message or body]

[Attachment #2 (multipart/mixed)]


Nested attributes rejection proc bypass in Active Record.

There is a vulnerability in how the nested attributes feature in Active Record
handles updates in combination with destroy flags when destroying records is
disabled. This vulnerability has been assigned the CVE identifier CVE-2015-7577.

Versions Affected:  3.1.0 and newer
Not affected:       3.0.x and older
Fixed Versions:     5.0.0.beta1.1, 4.2.5.1, 4.1.14.1, 3.2.22.1

Impact
------
When using the nested attributes feature in Active Record you can prevent the
destruction of associated records by passing the `allow_destroy: false` option
to the `accepts_nested_attributes_for` method. However due to a change in the
commit [a9b4b5d][1] the `_destroy` flag prevents the `:reject_if` proc from
being called because it assumes that the record will be destroyed anyway.

However this isn't true if `:allow_destroy` is false so this leads to changes
that would have been rejected being applied to the record. Attackers could use
this do things like set attributes to invalid values and to clear all of the
attributes amongst other things. The severity will be dependent on how the
application has used this feature.

All users running an affected release should either upgrade or use one of
the workarounds immediately.

Releases
--------
The FIXED releases are available at the normal locations.

Workarounds
-----------
If you can't upgrade, please use the following monkey patch in an initializer
that is loaded before your application:

```
$ cat config/initializers/nested_attributes_bypass_fix.rb
module ActiveRecord
  module NestedAttributes
    private

    def reject_new_record?(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 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)
      when Proc
        callback.call(attributes)
      end
    end

    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
  end
end
```

Patches
-------
To aid users who aren't able to upgrade immediately we have provided patches for
the two supported release series. They are in git-am format and consist of a
single changeset.

* 3-2-nested-attributes-reject-if-bypass.patch - Patch for 3.2 series
* 4-1-nested-attributes-reject-if-bypass.patch - Patch for 4.1 series
* 4-2-nested-attributes-reject-if-bypass.patch - Patch for 4.2 series
* 5-0-nested-attributes-reject-if-bypass.patch - Patch for 5.0 series

Please note that only the 4.1.x and 4.2.x series are supported at present. Users
of earlier unsupported releases are advised to upgrade as soon as possible as we
cannot guarantee the continued availability of security fixes for unsupported
releases.

Credits
-------
Thank you to Justin Coyne for reporting the problem and working with us to fix it.

[1]: https://github.com/rails/rails/commit/a9b4b5da7c216e4464eeb9dbd0a39ea258d64325

-- 
Aaron Patterson
http://tenderlovemaking.com/

["3-2-nested-attributes-reject-if-bypass.patch" (text/plain)]

From 9ce690ea4a6403a9d575ad381da2685f6ef4fcda Mon Sep 17 00:00:00 2001
From: Andrew White <andyw@pixeltrix.co.uk>
Date: Fri, 27 Nov 2015 13:46:46 +0000
Subject: [PATCH] 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 41b62f6..a0e98c3 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 <tt>:reject_if</tt> 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 <tt>:allow_destroy</tt> 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 85b9d3c..f5f0517 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')
-- 
2.4.9 (Apple Git-60)


["4-1-nested-attributes-reject-if-bypass.patch" (text/plain)]

From 5dc869dc73bcbe0b3dd415f257cf175015c4d014 Mon Sep 17 00:00:00 2001
From: Andrew White <andyw@pixeltrix.co.uk>
Date: Fri, 27 Nov 2015 13:46:46 +0000
Subject: [PATCH] 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 6df01b7..03a4009 100644
--- a/activerecord/lib/active_record/nested_attributes.rb
+++ b/activerecord/lib/active_record/nested_attributes.rb
@@ -523,7 +523,7 @@ module ActiveRecord
     # has_destroy_flag? or if a <tt>:reject_if</tt> 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
 
     # Determines if a record with the particular +attributes+ should be
@@ -532,7 +532,8 @@ module ActiveRecord
     #
     # Returns false if there is a +destroy_flag+ on the attributes.
     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)
@@ -541,6 +542,15 @@ module ActiveRecord
       end
     end
 
+    # Only take into account the destroy flag if <tt>:allow_destroy</tt> 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 c87a837..e421600 100644
--- a/activerecord/test/cases/nested_attributes_test.rb
+++ b/activerecord/test/cases/nested_attributes_test.rb
@@ -161,6 +161,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')
-- 
2.4.9 (Apple Git-60)


["4-2-nested-attributes-reject-if-bypass.patch" (text/plain)]

From 5aa44e2ffacf85e6efc0e8eb706bc1a3b6492aec Mon Sep 17 00:00:00 2001
From: Andrew White <andyw@pixeltrix.co.uk>
Date: Fri, 27 Nov 2015 13:46:46 +0000
Subject: [PATCH] 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 04b6182..a8ee082 100644
--- a/activerecord/lib/active_record/nested_attributes.rb
+++ b/activerecord/lib/active_record/nested_attributes.rb
@@ -523,7 +523,7 @@ module ActiveRecord
     # has_destroy_flag? or if a <tt>:reject_if</tt> 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
 
     # Determines if a record with the particular +attributes+ should be
@@ -532,7 +532,8 @@ module ActiveRecord
     #
     # Returns false if there is a +destroy_flag+ on the attributes.
     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)
@@ -541,6 +542,15 @@ module ActiveRecord
       end
     end
 
+    # Only take into account the destroy flag if <tt>:allow_destroy</tt> 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 9b5ea95..fc203cf 100644
--- a/activerecord/test/cases/nested_attributes_test.rb
+++ b/activerecord/test/cases/nested_attributes_test.rb
@@ -146,6 +146,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')
-- 
2.4.9 (Apple Git-60)


["5-0-nested-attributes-reject-if-bypass.patch" (text/plain)]

From 20dadd422bb074fd46e23aa0cc5f7abe64a93f58 Mon Sep 17 00:00:00 2001
From: Andrew White <andyw@pixeltrix.co.uk>
Date: Fri, 27 Nov 2015 13:46:46 +0000
Subject: [PATCH] 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 c5a1488..0d5a8e6 100644
--- a/activerecord/lib/active_record/nested_attributes.rb
+++ b/activerecord/lib/active_record/nested_attributes.rb
@@ -542,7 +542,7 @@ module ActiveRecord
     # has_destroy_flag? or if a <tt>:reject_if</tt> 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
 
     # Determines if a record with the particular +attributes+ should be
@@ -551,7 +551,8 @@ module ActiveRecord
     #
     # Returns false if there is a +destroy_flag+ on the attributes.
     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)
@@ -560,6 +561,15 @@ module ActiveRecord
       end
     end
 
+    # Only take into account the destroy flag if <tt>:allow_destroy</tt> 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)
       model = self.class._reflect_on_association(association_name).klass.name
       raise RecordNotFound.new("Couldn't find #{model} with ID=#{record_id} for \
                #{self.class.name} with ID=#{id}",
diff --git a/activerecord/test/cases/nested_attributes_test.rb \
b/activerecord/test/cases/nested_attributes_test.rb index 0b700af..6fbc619 100644
--- a/activerecord/test/cases/nested_attributes_test.rb
+++ b/activerecord/test/cases/nested_attributes_test.rb
@@ -146,6 +146,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')
-- 
2.2.1


[Attachment #9 (application/pgp-signature)]

[prev in list] [next in list] [prev in thread] [next in thread]