Merge pull request #13813 from lukesarnacki/log-deep-munge

Log for which keys values were set to nil in deep_munge

actionpack/CHANGELOG.md

@@ -11,6 +11,14 @@
 
     *Maurizio De Santis*
 
+*   Log which keys were affected by deep munge.
+
+    Deep munge solves CVE-2013-0155 security vulnerability, but its
+    behaviour is definately confusing, so now at least information
+    about for which keys values were set to nil is visible in logs.
+
+    *Łukasz Sarnacki*
+
 *   Automatically convert dashes to underscores for shorthand routes, e.g:
 
         get '/our-work/latest'

actionpack/lib/action_controller/log_subscriber.rb

@@ -53,6 +53,15 @@ def unpermitted_parameters(event)
       debug("Unpermitted parameters: #{unpermitted_keys.join(", ")}")
     end
 
+    def deep_munge(event)
+      message = "Value for params[:#{event.payload[:keys].join('][:')}] was set"\
+                "to nil, because it was one of [], [null] or [null, null, ...]."\
+                "Go to http://guides.rubyonrails.org/security.html#unsafe-query-generation"\
+                "for more information."\
+
+      debug(message)
+    end
+
     %w(write_fragment read_fragment exist_fragment?
        expire_fragment expire_page write_page).each do |method|
       class_eval <<-METHOD, __FILE__, __LINE__ + 1

actionpack/lib/action_dispatch/request/utils.rb

@@ -7,18 +7,23 @@ class Utils # :nodoc:
 
       class << self
         # Remove nils from the params hash
-        def deep_munge(hash)
+        def deep_munge(hash, keys = [])
           return hash unless perform_deep_munge
 
           hash.each do |k, v|
+            keys << k
             case v
             when Array
-              v.grep(Hash) { |x| deep_munge(x) }
+              v.grep(Hash) { |x| deep_munge(x, keys) }
               v.compact!
-              hash[k] = nil if v.empty?
+              if v.empty?
+                hash[k] = nil
+                ActiveSupport::Notifications.instrument("deep_munge.action_controller", keys: keys)
+              end
             when Hash
-              deep_munge(v)
+              deep_munge(v, keys)
             end
+            keys.pop
           end
 
           hash

guides/source/action_controller_overview.md

@@ -112,6 +112,10 @@ NOTE: The actual URL in this example will be encoded as "/clients?ids%5b%5d=1&id
 
 The value of `params[:ids]` will now be `["1", "2", "3"]`. Note that parameter values are always strings; Rails makes no attempt to guess or cast the type.
 
+NOTE: Values such as `[]`, `[nil]` or `[nil, nil, ...]` in `params` are replaced
+with `nil` for security reasons by default. See [Security Guide](security.html#unsafe-query-generation)
+for more information.
+
 To send a hash you include the key name inside the brackets:
 
 ```html

guides/source/configuring.md

@@ -352,6 +352,10 @@ value. Defaults to `'encrypted cookie'`.
 * `config.action_dispatch.encrypted_signed_cookie_salt` sets the signed
 encrypted cookies salt value. Defaults to `'signed encrypted cookie'`.
 
+* `config.action_dispatch.perform_deep_munge` configures whether `deep_munge`
+  method should be performed on the parameters. See [Security Guide](security.html#unsafe-query-generation)
+  for more information. It defaults to true.
+
 * `ActionDispatch::Callbacks.before` takes a block of code to run before the request.
 
 * `ActionDispatch::Callbacks.to_prepare` takes a block to run after `ActionDispatch::Callbacks.before`, but before the request. Runs for every request in `development` mode, but only once for `production` or environments with `cache_classes` set to `true`.

guides/source/security.md

@@ -915,6 +915,49 @@ Content-Type: text/html
 
 Under certain circumstances this would present the malicious HTML to the victim. However, this only seems to work with Keep-Alive connections (and many browsers are using one-time connections). But you can't rely on this. _In any case this is a serious bug, and you should update your Rails to version 2.0.5 or 2.1.2 to eliminate Header Injection (and thus response splitting) risks._
 
+Unsafe Query Generation
+-----------------------
+
+Due to the way Active Record interprets parameters in combination with the way
+that Rack parses query parameters it was possible to issue unexpected database
+queries with `IS NULL` where clauses. As a response to that security issue
+([CVE-2012-2660](https://groups.google.com/forum/#!searchin/rubyonrails-security/deep_munge/rubyonrails-security/8SA-M3as7A8/Mr9fi9X4kNgJ),
+[CVE-2012-2694](https://groups.google.com/forum/#!searchin/rubyonrails-security/deep_munge/rubyonrails-security/jILZ34tAHF4/7x0hLH-o0-IJ)
+and [CVE-2013-0155](https://groups.google.com/forum/#!searchin/rubyonrails-security/CVE-2012-2660/rubyonrails-security/c7jT-EeN9eI/L0u4e87zYGMJ))
+`deep_munge` method was introduced as a solution to keep Rails secure by default.
+
+Example of vulnerable code that could be used by attacker, if `deep_munge`
+wasn't performed is:
+
+```ruby
+unless params[:token].nil?
+  user = User.find_by_token(params[:token])
+  user.reset_password!
+end
+```
+
+When `params[:token]` is one of: `[]`, `[nil]`, `[nil, nil, ...]` or
+`['foo', nil]` it will bypass the test for `nil`, but `IS NULL` or
+`IN ('foo', NULL)` where clauses still will be added to the SQL query.
+
+To keep rails secure by default, `deep_munge` replaces some of the values with
+`nil`. Below table shows what the parameters look like based on `JSON` sent in
+request:
+
+| JSON                              | Parameters               |
+|-----------------------------------|--------------------------|
+| `{ "person": null }`              | `{ :person => nil }`     |
+| `{ "person": [] }`                | `{ :person => nil }`     |
+| `{ "person": [null] }`            | `{ :person => nil }`     |
+| `{ "person": [null, null, ...] }` | `{ :person => nil }`     |
+| `{ "person": ["foo", null] }`     | `{ :person => ["foo"] }` |
+
+It is possible to return to old behaviour and disable `deep_munge` configuring
+your application if you are aware of the risk and know how to handle it:
+
+```ruby
+config.action_dispatch.perform_deep_munge = false
+```
 
 Default Headers
 ---------------