Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log for which keys values were set to nil in deep_munge #13813

Merged
merged 1 commit into from Jan 29, 2014

Conversation

@lukesarnacki
Copy link
Contributor

@lukesarnacki lukesarnacki commented Jan 23, 2014

I think we all want deep_munge to be gone soon, but I am afraid that we will have to wait little bit longer. There are many developers which are not aware of this behaviour so I thought that it would be nice of RoR if there was some kind of hint of what actually happened, so I added log message when value is set to nil in deep_munge.

I hope that with #13420 we will quickly make code from this PR go away, but in case it will take bit longer...

Any suggestions for better log message would be awesome.

@rafaelfranca
rafaelfranca reviewed Jan 23, 2014
View changes
actionpack/CHANGELOG.md Outdated
@@ -1,3 +1,11 @@
* Log which keys were affected by deep munge

This comment has been minimized.

@rafaelfranca
rafaelfranca reviewed Jan 23, 2014
View changes
actionpack/CHANGELOG.md Outdated
* Log which keys were affected by deep munge

Deep munge solves CVE-2013-0155 security vulnerability, but its
behaviour is definately confuisng, so now at least information

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 23, 2014
Member

confuisng => confusing

@rafaelfranca
rafaelfranca reviewed Jan 23, 2014
View changes
actionpack/CHANGELOG.md Outdated
behaviour is definately confuisng, so now at least information
about for which keys values were set to nil is visible in logs.

Lukasz Sarnacki

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 23, 2014
Member

*Lukasz Sarnacki*

@rafaelfranca
rafaelfranca reviewed Jan 23, 2014
View changes
actionpack/lib/action_dispatch/request/utils.rb Outdated
hash[k] = nil if v.empty?
if v.empty?
hash[k] = nil
name = "deep_munge.action_controller"

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 23, 2014
Member

I think we can avoid this local variable

@rafaelfranca
rafaelfranca reviewed Jan 23, 2014
View changes
actionpack/lib/action_controller/log_subscriber.rb Outdated
@@ -53,6 +53,10 @@ def unpermitted_parameters(event)
debug("Unpermitted parameters: #{unpermitted_keys.join(", ")}")
end

def deep_munge(event)
debug("Value for key :#{event.payload[:key]} in params was set to nil, because it was one of [], [null] or [null, null, ...]. ")

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 23, 2014
Member

When we say params are we talking about the method or the parameters in general sense?

This comment has been minimized.

@chancancode

chancancode Jan 23, 2014
Member

Would it be too verbose if we (very briefly) what actions to take if you don't want this? And/or (very briefly) explain why we do this?

I'm just thinking it might be even more confusing when I see this because you didn't tell me what I can do about it.

At the very least, maybe we can say "Refer to the documentation of ActionDispatch::Request#deep_munge for details." And update that piece of documentation to include the details. Or perhaps put that in a guide.

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 23, 2014
Member

Linking to documentation seems a good plan

This comment has been minimized.

@lukesarnacki

lukesarnacki Jan 23, 2014
Author Contributor

Actually that is second problem with deep_munge - the only resources are discussions in issues, I will fix those typos and will add mention in documentation. Thanks for quick reply! :)

This comment has been minimized.

@egilburg

egilburg Jan 24, 2014
Contributor

+1, or at least prefix message with deep_munge: so it's easier to trace

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jan 23, 2014

Seems a good plan to me.

@chancancode
Copy link
Member

@chancancode chancancode commented Jan 23, 2014

👍

@lukesarnacki
Copy link
Contributor Author

@lukesarnacki lukesarnacki commented Jan 23, 2014

@rafaelfranca @chancancode I fixed typos and made logs a little bit smarter, so i.e. params[:article][:tags] is printed instead of just :tags. I will add mention about deep_munge to guides and will link to it in log message later today.

@lukesarnacki
Copy link
Contributor Author

@lukesarnacki lukesarnacki commented Jan 24, 2014

@rafaelfranca @chancancode I added deep_munge description in guides. One mention is in action controller overview in "Hash and array parameters" section, second one is in security guide as "Unsafe query generation" chapter. I think it covers this well, I am not sure if this is in proper place when it comes to security guide.

Output of command will look like:

Value for params[:article][:tags] 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.

(new line here is made for better readibility)

@rafaelfranca
rafaelfranca reviewed Jan 24, 2014
View changes
guides/source/action_controller_overview.md Outdated
@@ -112,6 +112,8 @@ 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: Since `3.2.8` version of rails, 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.

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 24, 2014
Member

Should be "in params" since we are talking about the method. If we are talking about the parameters, it should be "in parameters"

This comment has been minimized.

@robin850

robin850 Jan 24, 2014
Member

There is an extra space between the penultimate period and "See". Could you also wrap new additions around 80 chars please ? 😃

@rafaelfranca
rafaelfranca reviewed Jan 24, 2014
View changes
guides/source/security.md Outdated
Unsafe Query Generation
-----------------------

Due to the way `ActiveRecord` 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.

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 24, 2014
Member

It should be "Due to the way Active Record" since we are talking about the framework.

@rafaelfranca
rafaelfranca reviewed Jan 24, 2014
View changes
guides/source/security.md Outdated
| `{ "person": [null, null, ...] }` | `{ :person => nil }` |
| `{ "person": ["foo", null] }` | `{ :person => ["foo"] }` |

It is possible to return to old behaviour and disable `deep_munge` in config if you are aware of the risk and know how to handle it:

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 24, 2014
Member

Maybe:

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:
@rafaelfranca
rafaelfranca reviewed Jan 24, 2014
View changes
guides/source/security.md Outdated
It is possible to return to old behaviour and disable `deep_munge` in config if you are aware of the risk and know how to handle it:

```ruby
config.perform_deep_munge = false

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 24, 2014
Member

We need to put this config in the configuration.md too.

@robin850
robin850 reviewed Jan 24, 2014
View changes
actionpack/lib/action_controller/log_subscriber.rb Outdated
@@ -53,6 +53,10 @@ def unpermitted_parameters(event)
debug("Unpermitted parameters: #{unpermitted_keys.join(", ")}")
end

def deep_munge(event)
debug("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.")

This comment has been minimized.

@robin850

robin850 Jan 24, 2014
Member

What do you think about using heredoc here or simply move the message to a variable and call debug(message) ? I find files hard to read when you have to scroll but this is just a proposal.

@lukesarnacki
Copy link
Contributor Author

@lukesarnacki lukesarnacki commented Jan 27, 2014

@rafaelfranca I corrected mistakes you pointed out and added mention in configuration guide.

@robin850 Yeah, that line started to be very long ;) I changed it to multiline string.

@robin850
robin850 reviewed Jan 28, 2014
View changes
guides/source/action_controller_overview.md Outdated
@@ -112,6 +112,8 @@ 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.

This comment has been minimized.

@robin850

robin850 Jan 28, 2014
Member

Could you wrap your additions around 80 chars please ? The other guides aren't wrapped too. 😄

Also, this will need to be rebased. Thanks for the great work here so far! 👍

This comment has been minimized.

@lukesarnacki

lukesarnacki Jan 28, 2014
Author Contributor

Hey, thanks for heads up, is it ok now? :)

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.
rafaelfranca added a commit that referenced this pull request Jan 29, 2014
Log for which keys values were set to nil in deep_munge
@rafaelfranca rafaelfranca merged commit 0fcbc65 into rails:master Jan 29, 2014
1 check passed
1 check passed
default The Travis CI build passed
Details
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jan 29, 2014

Thank @lukesarnacki. Very good PR as always ❤️

@lukesarnacki
Copy link
Contributor Author

@lukesarnacki lukesarnacki commented Jan 29, 2014

Thanks @rafaelfranca :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.