Browse files

Log which keys were set to nil in deep_munge

deep_munge solves CVE-2013-0155 security vulnerability, but its
behaviour is definately confuisng. This commit adds logging to deep_munge.
It logs keys for which values were set to nil.

Also mentions in guides were added.
  • Loading branch information...
1 parent b9cd5a2 commit 69ab91ae9396f0101afd13871f179a7f779d3178 @lukesarnacki lukesarnacki committed Jan 23, 2014
@@ -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'
@@ -53,6 +53,15 @@ def unpermitted_parameters(event)
debug("Unpermitted parameters: #{unpermitted_keys.join(", ")}")
+ 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"\
+ "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
@@ -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) }
- 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)
+ keys.pop
@@ -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:
@@ -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`.
@@ -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
+and [CVE-2013-0155](!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:
+unless params[:token].nil?
+ user = User.find_by_token(params[:token])
+ user.reset_password!
+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
+| 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:
+config.action_dispatch.perform_deep_munge = false
Default Headers

0 comments on commit 69ab91a

Please sign in to comment.