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

Log for which keys values were set to nil in deep_munge

3 files changed, 51 insertions, 0 deletions

diff --git a/guides/source/action_controller_overview.md b/guides/source/action_controller_overview.md
index f394daa6aa..c55637eb0a 100644
--- a/guides/source/action_controller_overview.md
+++ b/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
diff --git a/guides/source/configuring.md b/guides/source/configuring.md
index 38f7162fcf..5e0010725e 100644
--- a/guides/source/configuring.md
+++ b/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`.
diff --git a/guides/source/security.md b/guides/source/security.md
index cffe7c85f1..70fb066b64 100644
--- a/guides/source/security.md
+++ b/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
 ---------------