Strong parameters potential vulnerability

[pyromaniac]

Steps to reproduce

Hello everyone. Just stumbled upon a flaky spec. It happens rarely, once in 100-500 cases:

[22] pry(#<Api::V3::BookingsController>)> params
=> #<ActionController::Parameters {"bookings"=>[{"notes"=>"Changed!", "canceled_at"=>"2021-11-21 16:08:03 UTC"}], "id"=>"982468349", "controller"=>"api/v3/bookings", "action"=>"update"} permitted: false>
[23] pry(#<Api::V3::BookingsController>)> params.fetch('bookings')
=> [{"notes"=>"Changed!", "canceled_at"=>"2021-11-21 16:08:03 UTC"}]

As you can see here, the value returned is not wrapped with ActionController::Parameters

I dug deeper and found that this line is the reason:

rails/actionpack/lib/action_controller/metal/strong_parameters.rb

Line 941 in 90357af

return value if converted_arrays.member?(value)

And the funniest thing is:

[19] pry(#<Api::V3::BookingsController>)> params.converted_arrays.instance_variable_get(:@hash).key?([{"notes"=>"Changed!", "canceled_at"=>"2021-11-21 16:08:03 UTC"}])
=> true
[20] pry(#<Api::V3::BookingsController>)> params.converted_arrays.instance_variable_get(:@hash)
=> {[#<ActionController::Parameters {"notes"=>"Changed!", "canceled_at"=>"2021-11-21 16:08:03 UTC"} permitted: false>]=>true}

I know that hash lookup utilizes #equal? along with the #hash value but they are not #equal?. They are at most #==.

So I frankly have no idea why does it happen but the bottom line is that in some circumstances strong params can return a raw value which can cause exception if handled and appears to be a potential security risk if not handled.

System configuration

[24] pry(#<Api::V3::BookingsController>)> Rails.version
=> "6.1.4.1"
[25] pry(#<Api::V3::BookingsController>)> RUBY_VERSION
=> "2.7.4"

[rafaelfranca]

Can you see if you reproduce it running the instantiation of parameters in a loop? The only way I can see this happening is if the hash of an array of Hashes is the same as an array of ActionController::Parameters created using the same Hashes, but that should be coved but the hash implementation in ActionController::Parameters which append the @permitted which in your case is false.

[rafaelfranca]

Also, since it is flaky spec, maybe it is some monkey patch/mocking in your application?

[pyromaniac]

@rafaelfranca I checked hashes of both objects. They are different. Also, they even have different % 11. As far as I get, hash has 11 bins. So the objects are even in different bins, I checked this.

I don't think we have ActionController::Parameters patched. At least, pry reveals unpatched sources of all the methods involved.

I will try to reproduce it in a loop, great point.

[rafaelfranca]

Yeah, I didn't expect you having monkey patches, but the only way I can see this failing is if we have a hash collision, which is very unlikely to happen, but not impossible

[pyromaniac]

Checked it, can't reproduce in a loop. Can't reproduce in a bash loop with rails runner as well. I bet it has something to do with either Kernel.srand or the number of objects in memory that RSpec generates. Tried to set Kernel.srand manually - no luck as well.

What can be the reason? It indeed might be in the app code but I'm unable to debug it to the C code. Cause the root cause is clearly there.

Can anyone at least suggest, what might be the reason? It can be an incorrect pointer in memory, for example, right? Some leak in the C implementation.

[p8]

@pyromaniac Equality of elements in a Set is determined according to Object#eql? and Object#hash.
eql? is aliased to ==.

rails/actionpack/lib/action_controller/metal/strong_parameters.rb

Line 294 in 90357af

alias eql? ==

Are you using threads somewhere that could cause a race condition?

[pyromaniac]

@p8 I don't think so. Set uses a Hash instance internally. Hash uses #hash and #equal?, not #eql?. If what you said would be true - it would return an unwrapped hash every time.

No, no threads in the surrounding code.

[p8]

@pyromaniac I copied that definition straight from the Set documentation:
https://ruby-doc.org/stdlib-2.7.4/libdoc/set/rdoc/Set.html
I should have mentioned that, sorry 🙂

[pyromaniac]

Hm, you are right. Then why doesn't it fail in all the cases? Going to check this. Thanks for pointing this out.

[pyromaniac]

Ok, what I have found so far is:

1. Still something odd regarding the Set#member? method. In some cases it returns the inside object if it is eql? and in some - not. And I have no idea why yet.
2. There is a potential bug in strong parameters: #[] method calls #convert_hashes_to_parameters internally which updates the value of @parameters internally (

   rails/actionpack/lib/action_controller/metal/strong_parameters.rb

   Line 957 in a797083

   @parameters[key] = converted unless converted.equal?(value)

   ) but #fetch calls only #convert_value_to_parameters which does not do this. I'm pretty sure the intention on caching should be similar for both. And this will solve the first issue as well. Well, sweep it under carpet at least.

[rafaelfranca]

It seems our test suite started reproducing this issue as well https://buildkite.com/rails/rails/builds/83354#5113ba6e-529c-4b73-a639-428fcfc97923/1015-1026

[tenderlove]

I think I've figured this out though I'm not sure how to fix. Applying this patch will make the error from our CI reproduce consistently:

diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb
index 9909832ab6..75257c9476 100644
--- a/actionpack/lib/action_controller/metal/strong_parameters.rb
+++ b/actionpack/lib/action_controller/metal/strong_parameters.rb
@@ -961,6 +961,8 @@ def convert_hashes_to_parameters(key, value)
       def convert_value_to_parameters(value)
         case value
         when Array
+          converted_arrays.reset
+
           return value if converted_arrays.member?(value)
           converted = value.map { |_| convert_value_to_parameters(_) }
           converted_arrays << converted

The cause is that we're "converting an array" then inserting that array in to a set. The set is just backed by a hash. When there's a hash miss we "convert the array" (looping through everything and converting it). When there's a hit we don't do that.

I think what's happening is that most of the time we're missing the hash lookup, and most of the time we'll just loop through the array and convert the params (meaning that most of the time it just works).

However, since we're letting people mutate the underlying array that's stored in the set, it's possible that we get a cache hit and we don't try reconverting anything in the array.

Here is kind of a demo:

set = {}
key = [Object.new]
set[key] = 1

# mutate the key
key << 1

p set.key?(key) # false cache miss
p set.keys.first == key # true it's the same array

We've cached "converted" things in the set, but we let people mutate the cache keys, which poisons the cache.

[tenderlove]

Btw, I think the reason that we're seeing this "sometimes" is that hashes will automatically rehash themselves when the underlying hash table needs to resize. So if you insert enough data, eventually you'll see the failure (which I demo'd in the test)

[p8]

That's some mindblowing debugging @tenderlove !

[pyromaniac]

@tenderlove thanks for diving into this! Hope there will be no issue anymore. But what of the point number 2 in my previous comment? There is still inconsistency between #fetch and #[] behaviors since the latter doesn't update the @parameters hash with cached value.