Let's fix deep_munge <3 <3 <3 #13420

Closed
chancancode opened this Issue Dec 20, 2013 · 15 comments

Projects

None yet

10 participants

@chancancode
Member

As a response to a security issue (CVE-2012-2660, CVE-2012-2694 and CVE-2013-0155), deep_munge was introduced as a solution to keep Rails secure by default.

Some of you feel strongly about this and are not satisfied with the solution. The good news is, #13188 has been merged to provide a temporary escape valve for those who know what they are doing. This will be available in Rails 4.1, so there's yet another reason to upgrade for those of you who are affected by this! (I encourage you to go through the advisories and make sure you really understand the issue before you fiddle with this.)

However, many of you are probably still dissatisfied with the current state of affairs. Personally, I admire your desire to push the framework forward and make it better for everyone.

Unfortunately, most of the solutions proposed so far do not meet the criteria @jeremy laid out in #13157. I'll reproduce the comment here:

Thanks for filing the PR, but for this to move forward, we need a much
stronger answer to the security vulnerability than just education.

We choose to be secure by default, and relying on education about
proper API use is begging for accidents.

So, this will not fly as-is. Please do keep pushing on this. We'd love
to have a compelling alternative that's secure by default.

* * *

To reiterate, fixes are welcome, so long as they address the security
vulnerability. Those are the constraints at play. Choosing to see that
as a plan to keep params parsing broken intentionally mischaracterizes
the issue and ensures zero progress on it. I encourage you to go deeper
than that! :grin:

For example, we could use strong_params to make a stronger statement
about the params we expect. We could declare what request content-types
our actions accept so the params parser would reject JSON requests
to actions that expect query string or www-urlencoded params. Etc.

In summary, "security by default" is a key cornerstone of Rails as a framework, and the core team is willing to aggressively defend this goal. Over the years, we have made some great progress in this area with mass assignment white-listing, strong parameters, automatic html escaping in erb, CSRF tokens, etc. I believe there is a similar opportunity here for us to push the envelope again as a community.
#13188 is going to buy us some time to explore different solutions to the problem. I encourage all the great minds in the community to gather here and start proposing solutions that meets the requirements @jeremy laid out. #13215 seems like a promising start, and I encourage you to help review the proposal and express your concerns.

Please don't simply respond with +1s and "me too"s. Those comments are only going to create noise for everyone and slow down the progress. We heard you loud and clear, now it's time to get to work!

Thank you in advance for your contribution 💛 💙 💜 ❤️ 💚

@christhekeele

Hey! I'm representing pull request #12251.

I'm a huge fan of security by default. Education is indeed insufficient: web applications have such a large surface area that it's impossible for a single developer to keep educated on all vectors of attack, historical and emergent. Having a group of very intelligent people working hard to watch my back is the most invaluable part of using Rails, and I'm very thankful for the security team's efforts to protect us against CVE-2012-2660, CVE-2012-2694 and CVE-2013-0155. The only reason to remove deep_munge would be to replace it with a better mechanism.

@imanel provides a table of the "bugs" that deep_munge introduces:

This table shows what kind of behavior will we get when passing JSON:

JSON Hash
{"person":null} {'person' => nil}
{"person":[]} {'person' => nil}
{"person":[null]} {'person' => nil}
{"person":[null, null, ...]} {'person' => nil}
{"person":["foo", null]} {'person' => ["foo"]}

It's obvious that current behavior prevents all nils in params. A lot of API's allows them, so braking JSON spec is probably not a solution. It also makes confusion by changing type of passed argument from Array to NilClass - that prevents strong params to work in some cases.

I can see how this might be considered a bug from an API designer's perspective. An API that relies on passing back nils in arrays is kind of screwed. On the other hand, an API that relys on that and passes those params into AR is subtly and deeply screwed on an insidious level, so deep_munge should stay.

I would argue that most of the content of his table of complaints can be categorized as a bug, though, except for the first and last columns. The security vulnerability is very clearly stated in CVE-2012-2660, CVE-2012-2694 and CVE-2013-0155:

  • The [nil] value will bypass the test for nil, but will still add an "IS NULL" clause to the SQL query.
  • The [nil] value will bypass the test for nil, but will still add an "IN ('xyz', NULL)" clause to the SQL query.
  • An empty hash will eliminate the WHERE clause of the query, but can bypass the nil? check.

The vulnerability lies in Arrays with nils, and empty hashes. API designers relying on these mechanisms are out of luck, but this is a necessary evil. On the other hand, there's no associated vulnerability with empty arrays (that I can see). This is another useful tool in an API designers toolbelt, even one encouraged by Rails patterns: returning an empty array of ids should clear out a one-to-many association. If that empty array becomes nil, though, it gets scrubbed out and ignored when being passed to a relationship ids array, and we lose the ability for one-to-many go to one-to-none.

Pull request #12251 fixes the buggy unexpected behavior of deep_merge, converting empty arrays to nil, while keeping the security by default that protects us all from CVE-2012-2660, CVE-2012-2694 and CVE-2013-0155. I believe it can help fix deep_merge and re-enable one of the missing tools in the API designer's toolbelt.

The new table looks like, and should have always looked like:

JSON Hash
{"person":null} {'person' => nil}
{"person":[]} {'person' => []}
{"person":[null]} {'person' => []}
{"person":[null, null, ...]} {'person' => []}
{"person":["foo", null]} {'person' => ["foo"]}
{"person":{}} {'person' => nil}

As for the expected behaviour of deep_merge, disabling APIs from using empty hashes and nils in arrays: I'm glad we're protected from security vulnerabilities, but...

Maybe #13215 is a better place to put paranoid secure default behaviour, but as long as deep_merge is around and serves an important purpose, it'd be nice to see #12251 accepted and improve it.

@acapilleri
Contributor

👎

@chancancode
Member

@christhekeele thanks for the detailed argument (@acapilleri too, if I understand correctly both of your PR #11044 does the same thing). It's definitely possible that I mistakenly closed them while trying to funnel the related discussions here. (And maybe I should appologize in advance 😅) I'll double check both your PRs, and respond over there if I have anything to report I'll comment on your PRs directly!

@imanel
Contributor
imanel commented Dec 20, 2013

@chancancode I reviewed both #11044 and #12251 - you can find my comments at the bottom. Hope this will help.

@chancancode
Member

@imamel thank you ❤️

@lukesarnacki
Contributor

I haven't paid much attention to workarounds in security reports before, but while writing some documentation for deep_munge I noticed that some of the workarounds won't work for ruby 1.9.3 and higher.

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

In ruby 1.8.7:

[nil].to_s #=> ""

In ruby > 1.8.7:

[nil].to_s #=> "[nil]"

/cc @tenderlove

@enocom enocom added a commit to enocom/json-api that referenced this issue Feb 23, 2014
@enocom enocom Turn off deep munge
rails/rails#13420

tl;dr JSON posts with `{"foo": []}` become {"foo" => nil} otherwise.
0978df9
@daviddavis daviddavis referenced this issue in rails/strong_parameters Mar 3, 2014
Closed

Strong params issue with empty arrays in JSON requests #192

@chancancode chancancode added JRuby and removed JRuby labels Jun 26, 2014
@dmcinnes dmcinnes added a commit to att-cloud/daylight that referenced this issue Aug 28, 2014
@dmcinnes dmcinnes update collection reset example
cannot set association to [], because rails converts that to nil and we
ignore nils when updating a collection.
See rails/rails#13420
67a6bd5
@dmcinnes dmcinnes referenced this issue in att-cloud/daylight Aug 29, 2014
Closed

Support resetting has_many associations with [] #48

@chancancode chancancode added this to the 5.0.0 milestone Sep 15, 2014
@juliusv juliusv added a commit to prometheus/promdash that referenced this issue Nov 7, 2014
@juliusv juliusv Support for uploading dashboard definitions.
This enables PUT-ing and GET-ing of full dashboard definitions through
/<slug>.json. In this API, the 'dashboards_json' database field is
converted from a JSON string to a native JSON suboject of the Rails
Dashboard model JSON. The full externally exposed JSON looks like this:

{
  "name": "Test Dashboard",
  "created_at": "2014-11-07T00:25:57.000Z",
  "updated_at": "2014-11-07T00:38:23.000Z",
  "dashboard_json": {
    "globalConfig": {
      // ...global configuration...
    },
    "widgets": [
      // ...widget definitions...
    ]
  }
}

Upon PUT, the 'dashboards_json' field is validated against the
JSON-schema definition in the file 'dashboard_schema.json' and then
saved again as a string in a single database field.

Since Rails converts JSON empty arrays to "nil" values by default, this
also upgrades PromDash to Rails 4.1.1, which supports an option to turn
off this automatic conversion. See the following issues about this
regarding security implications (which for now we shouldn't need to
worry about in PromDash):

rails/rails#8832
rails/rails#13420

Further, as we're exposing the dashboard_json as an external API for the
first time, this also includes a lot of naming cleanups in the JSON
fields and the related code.
86c09b3
@juliusv juliusv added a commit to prometheus/promdash that referenced this issue Nov 7, 2014
@juliusv juliusv Support for uploading dashboard definitions.
This enables JSON-schema-validated PUT-ing and GET-ing of full dashboard
definitions through /<slug>.json. In the external part of this API, the
'dashboards_json' database field is converted from a JSON-encoded string
to a native JSON subobject of the Rails Dashboard model JSON
representation. The full externally exposed JSON for a dashboard looks
like this:

{
  "name": "Test Dashboard",
  "created_at": "2014-11-07T00:25:57.000Z",
  "updated_at": "2014-11-07T00:38:23.000Z",
  "dashboard_json": {
    "globalConfig": {
      // ...global configuration...
    },
    "widgets": [
      // ...widget definitions...
    ]
  }
}

Upon PUT, the 'dashboards_json' field is validated against the
JSON-schema definition in the file 'dashboard_schema.json' and then
saved again as a serialized string in a single database field.

Since Rails converts empty JSON arrays to "nil" values by default, this
also upgrades PromDash to Rails 4.1.1, which supports an option to turn
off this automatic conversion. See the following issues about this
regarding security implications (which for now we shouldn't need to
worry about in PromDash):

rails/rails#8832
rails/rails#13420

Further, as we're exposing the dashboard_json as an external API for the
first time, this also includes a lot of naming cleanups in the JSON
fields and the related code.
b71abf8
@juliusv juliusv referenced this issue in prometheus/promdash Nov 7, 2014
Merged

Support for uploading dashboard definitions. #229

@juliusv juliusv added a commit to prometheus/promdash that referenced this issue Nov 7, 2014
@juliusv juliusv Support for uploading dashboard definitions.
This enables JSON-schema-validated PUT-ing and GET-ing of full dashboard
definitions through /<slug>.json. In the external part of this API, the
'dashboards_json' database field is converted from a JSON-encoded string
to a native JSON subobject of the Rails Dashboard model JSON
representation. The full externally exposed JSON for a dashboard looks
like this:

{
  "name": "Test Dashboard",
  "created_at": "2014-11-07T00:25:57.000Z",
  "updated_at": "2014-11-07T00:38:23.000Z",
  "dashboard_json": {
    "globalConfig": {
      // ...global configuration...
    },
    "widgets": [
      // ...widget definitions...
    ]
  }
}

Upon PUT, the 'dashboards_json' field is validated against the
JSON-schema definition in the file 'dashboard_schema.json' and then
saved again as a serialized string in a single database field.

Since Rails converts empty JSON arrays to "nil" values by default, this
also upgrades PromDash to Rails 4.1.1, which supports an option to turn
off this automatic conversion. See the following issues about this
regarding security implications (which for now we shouldn't need to
worry about in PromDash):

rails/rails#8832
rails/rails#13420

Further, as we're exposing the dashboard_json as an external API for the
first time, this also includes a lot of naming cleanups in the JSON
fields and the related code.
be9933f
@chancancode
Member

Just an update: #16924 was just merged into master (for Rails 5.0, not 4.2), so we are one step closer to closing this ticket 😄

@chancancode
Member

@christhekeele can you review the new table? It seems identical to the one you wanted 😄

@christhekeele

That looks dreamy! 👍 😸

@imanel
Contributor
imanel commented Dec 16, 2014

Yay - it's much better. I'm still trying to figure out how to avoid removing nils (see #13157) as it's REALLY confusing when you are expecting one, but it looks like #16924 at least stops breaking JSON array type :)

@vuntz vuntz added a commit to vuntz/barclamp-crowbar that referenced this issue Jan 8, 2015
@vuntz vuntz Do not use deep munge
This is breaking the JSON parsing for empty arrays, which we use with
the magic crowbar-deep-merge-template feature when creating the initial
crowbar proposal.

With deep munge enabled, we cannot remove the nagios proposal, which
results in failures when nagios barclamp is not available.

See:
rails/rails#8832
rails/rails#13420

We can revert this once switching to rails 5 (see comments in second
issue).
e77d543
@vuntz vuntz referenced this issue in crowbar/barclamp-crowbar Jan 8, 2015
Merged

Do not use deep munge #1170

@vuntz vuntz added a commit to vuntz/barclamp-crowbar that referenced this issue Jan 8, 2015
@vuntz vuntz Do not use deep munge
This is breaking the JSON parsing for empty arrays, which we use with
the magic crowbar-deep-merge-template feature when creating the initial
crowbar proposal.

With deep munge enabled, we cannot remove the nagios proposal, which
results in failures when nagios barclamp is not available.

See:
rails/rails#8832
rails/rails#13420

We can revert this once switching to rails 5 (see comments in second
issue).
762e128
@vuntz vuntz added a commit to vuntz/barclamp-crowbar that referenced this issue Jan 9, 2015
@vuntz vuntz Do not use deep munge
This is breaking the JSON parsing for empty arrays, which we use with
the magic crowbar-deep-merge-template feature when creating the initial
crowbar proposal.

With deep munge enabled, we cannot remove the nagios proposal, which
results in failures when nagios barclamp is not available.

See:
rails/rails#8832
rails/rails#13420

We can revert this once switching to rails 5 (see comments in second
issue).
9905eef
@vuntz vuntz added a commit to vuntz/barclamp-crowbar that referenced this issue Jan 9, 2015
@vuntz vuntz Do not use deep munge
This is breaking the JSON parsing for empty arrays, which we use with
the magic crowbar-deep-merge-template feature when creating the initial
crowbar proposal.

With deep munge enabled, we cannot remove the nagios proposal, which
results in failures when nagios barclamp is not available.

See:
rails/rails#8832
rails/rails#13420

We can revert this once switching to rails 5 (see comments in second
issue).
01d6eda
@jeremy
Member
jeremy commented Feb 2, 2015

Discussed with @sgrif and @chancancode. A Rails 5 path to eliminate params munging is to drop Active Record support for foo: [1, 2, nil] => foo IN (1, 2) OR foo IS NULL.

Instead, drop the special nil handling and include NULL in the IN list: foo IN (1, 2, NULL). Then databases do what we expect and we needn't munge params.

We lose a convenient, but very niche, feature that can be worked around (and deprecated) by explicitly requesting where(foo: [1, 2]).or(where(foo: nil))

@sgrif sgrif added a commit to sgrif/rails that referenced this issue Feb 2, 2015
@sgrif sgrif [Discussion] Remove special casing nil in arrays passed to where
Opened in order to give a place to discuss the proposed path forward,
proposed in
rails#13420 (comment)
cf46d39
@sgrif
Member
sgrif commented Feb 2, 2015

Opened #18790 so we can discuss that potential path forward there.

@christhekeele

🎉

@kaspth
Member
kaspth commented Sep 17, 2015

Fixed by 52cf1a7 :trollface:

@maclover7
Member

Looks ready to be closed to me.

@sgrif sgrif closed this Dec 19, 2015
@rafaelfranca rafaelfranca modified the milestone: 5.0.0 [temp], 5.0.0 Dec 30, 2015
@dsander dsander referenced this issue in cantino/huginn Oct 5, 2016
Merged

Update to Rails 5 #1688

9 of 14 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment