Revert "Strip nils from collections on JSON and XML posts dealing with empty hashes" #8870

Closed
wants to merge 1 commit into
from

Projects

None yet

6 participants

@imanel
Contributor
imanel commented Jan 10, 2013

This change was considered as fix for CVE-2013-0155 but is completely wrong solution for this problem.

Rationale:

  • #8831 - incorrect delivering valid JSON that contains nils in array(a lot of APIs are using that)
  • #8832 - error in accepts_nested_attributes_for with valid attributes
  • #8845 - regression with non-object JSON content
  • incorrect handling of serialize method(waiting for bug report)

This is not partially bypassing bug like #8862, but the only valid and working solution - allow nils in arrays as JSON specification is saying.

We should work to fix design flaw in AR instead of breaking working standards.

@imanel imanel Revert "* Strip nils from collections on JSON and XML posts. [CVE-201…
…3-0155] * dealing with empty hashes. Thanks Damien Mathieu"

This reverts commit 8e577fe.
8d3cd95
@mikebaldry

I agree with this. Breaking the JSON specification is not the way to go.

I can't currently say what the right way is though.

@mikebaldry

Problem with serialize is not a problem - related to the initial issue.

@lawrencepit
Contributor

+1 on reverting it completely.

My take on this is that there wasn't a bug in the first place. The workaround in the bug report is the actual solution.

I.e. it's the responsibility of the developer (and not rails) to make sure parameters are sanitised.

@rafaelfranca
Member

Guys, before revert it completely we need to address the original issue. I'm not seeing the fix for the original issue anywhere.

@imanel
Contributor
imanel commented Jan 10, 2013

@rafaelfranca I'm not sure about that. If you are checking twitter then you should already know that there is huge amount of developers that are not updating Rails version because of this issue. That was little unfortunate to address really serious bug(CVE-2013-0156) in the same release that is breaking a lot of applications(not to mention JSON specification) So from my point of view it's much more important to deliver working version and patch more important bug.

After we finish doing that then we should start thinking about how to fix CVE-2013-0155.

@rafaelfranca
Member

Agree, but since we will not do another release without fixing CVE-2013-0155 too, I think you can use your fork now.

@rafaelfranca
Member

BTW, I really don't recommend to use in your application this revert, unless you did the workaround. We are working to fix this issue.

@imanel
Contributor
imanel commented Jan 10, 2013

Good to know that it will not be released - little unfortunate but at least I know on what should I focus now :)

Let's then go back to provided example:

unless params[:token].nil?
  user = User.find_by_token(params[:token])
  user.reset_password!
end

IMHO this code have bug in first line, as in this situation we should use blank? instead of nil?. Correct code:

unless params[:token].blank?
  user = User.find_by_token(params[:token])
  user.reset_password!
end

This would allow revert at least part about changing empty array and empty hash to nil(probably biggest problem and cause of #8832)

The only remaining problem is [nil] structure and this still need to be handled by JSON, as it is perfectly valid one. The problem is not with allowing such structure pass JSON decoding, but combination of passing through blank? and converting to NULL in ActiveRecord. Let's split problem to 2 parts:

  1. ActiveRecord will convert User.find_by_token([nil]) to WHERE token IN (NULL), which is valid - we are using it and preventing it would be confusing
  2. [nil].blank? returns false. This probably need to remain unchanged, as this array surely is not nil? or empty?

So using just this two cases we can't resolve this bug. This would probably require adding another method that would return true if only value would be nil:

def example_blank?
  self.compact.blank?
end

This would fix passing [nil] to mentioned example, but there would be one another problem: [nil, "test"].

In given example the only valid method would be always compacting array and then doing all checks on it. Unfortunately we can't do it by default, as nils are still valid values. What we need then is option to compact(or otherwise strip nils) passed parameters. Such method might be for example to_s(at least for given example), which would resolve all problems:

token = params[:token].to_s

unless token.blank?
  user = User.find_by_token(token)
  user.reset_password!
end

What is funny this would mark whole CVE-2013-0155 as invalid because it's programmer bug and not framework bug.

We could be as secure as we want, but unfortunately sometimes we need to believe in programmer that is producing valid code. In this case we should propagate good practices and add notes to documentation, but there is no solution to such problem except of knowledge about framework behavior.

@lawrencepit
Contributor
What is funny this would mark whole CVE-2013-0155 as invalid 
because it's programmer bug and not framework bug.

@imanel exactly! CVE-2013-0155 isn't a bug. It's an example of a programmer writing bad code. All programmers should know that ANY incoming parameters cannot be trusted and always need to be sanitised. No framework can do that for you.

@rafaelfranca
Member

As I said, we are working to fix this issue, so please be patient.

@tehgeekmeister
Contributor

@imanel @lawrencepit https://lkml.org/lkml/2012/12/23/75 <== I think Linus got the bias right in this case. Frameworks should make it hard to mess things up on accident. But, what's more, frameworks shouldn't blame users wherever they can avoid it.

@rafaelfranca and all the others putting time into this, thank you. Has to be a pretty emotionally charged thing for some of you.

@jeremy
Member
jeremy commented Jan 11, 2013

We agree it's an app error. However, we should be secure by default. The combination of params parsing and Active Record API makes it easy to inadvertently open a vulnerability. This is a door that should take serious effort to open, not an accident or oversight. Hang in there.

In the meantime, you can work around this by swapping out the ParamsParser middleware with your own:

config.middleware.swap ActionDispatch::ParamsParser,
  ActionDispatch::ParamsParser,
  Mime::JSON => lambda { |body| Yajl.load(body).with_indifferent_access }
@jeremy jeremy closed this Jan 11, 2013
@lawrencepit
Contributor

@jeremy Ooc, how does that work? With that workaround rake middleware reports that there isn't any ActionDispatch::ParamsParser middleware anymore. Afaics the lambda block is never called.

@imanel imanel referenced this pull request Dec 4, 2013
Closed

Remove deep munge #13157

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment