About default searchable attributes and associations

[wonda-tea-coffee]

Currently, if you don't restrict the searchable attributes and associations, you can search all ranges.
https://github.com/activerecord-hackery/ransack#authorization-whitelistingblacklisting

I feel that this is very dangerous.
My opinion is exactly the same as the following blog post.
https://younes.codes/posts/how-to-hack-with-ransack

I would like to make this gem secure by default.
Can you please tell me if there is a reason for the current way it is built? Also, I am willing to help if modifications are needed.

Many thanks.

[deivid-rodriguez]

I'm not sure why this default was left like that, I would be happy to change it to use more secure defaults, but it'd be nice to dig into issue/PR/commit history to figure out the reason (if any) first.

[wonda-tea-coffee]

I looked into it as best I could, but could not find a clear reason for not making the default secure.

Although ransackable_attributes have been around since the very beginning, I don't think there has been any discussion since then towards making the default safe.
77e570c

[deivid-rodriguez]

I didn't look closely but are you sure 77e570c is related?

[wonda-tea-coffee]

My apologies. That was not an answer.

I looked in issue/PR/commit to see why the default is the current behavior, but could not find it.
I wanted to tell you that the basis for authorization was established in 77e570c and no specific reason was given as to why this was done.

[deivid-rodriguez]

I see. In 77e570c, when ransackable_attributes was first added, a comment was added on top of it reading # TODO: Let's actually do some authorization. Whitelist-only..

However the idea was somehow dropped at f151d62, leaving it completely to the application to provide its own secure defaults.

I think we can add some configuration that applies to attributes of all models, and give it some reasonably safe default value, like

Ransack.configure do |c|
  c.non_searchable_attributes = [:encrypted_password, :password]
end

[wonda-tea-coffee]

I understand.
I'll have to look at the values I'm setting, but it looks good.

[lukas-eu]

I share OP's concern about the insecure default settings here.
I am a security researcher who recently came across a vulnerable Ransack configuration in a client project, and have looked into other projects since, also planning the release of a blog post related to this topic soon.
My estimate is that there are several hundred applications on the internet which are vulnerable to a full compromise or major data leak via insecure Ransack configurations. I have proven and reported such issues for a handful of different projects in the past few weeks.

Proposals/Request for comments @deivid-rodriguez

1. Add a warning with high visibility in the documentation
   Currently, there is no mention of this potential issue (and its mitigation via ransackable_[attributes/associations/etc.] properties) in Getting Started -> Simple mode/Advanced Mode, even though both of these pages suggest passing unfiltered user input to the ransack method.
2. Implement restrictive defaults (potentially allowing an explicit bypass)
   I understand that easy usability and rich out-of-the-box functionality is likely essential to this library's appeal to its users. Nevertheless I'd like to propose making the authorization properties ransackable_[attributes/associations/etc.] empty sets by default, forcing the developer to explicitly define whitelists for their use case.
   To soften the usability blow, a new ransack_unsafe(params[:q]) or ransack_explicit(params[:q], ransackable_attributes='*', ransackable_associations=(:post, :comment)) method could be introduced to offer developers a shorthand to bypass or override the whitelists for specific queries (after they've had to read a warning about why these methods can be dangerous).
3. Implement less restrictive defaults
   If the suggestion in 2. seems too drastic, a blacklist for attributes like mentioned in a comment above coupled with an empty ransackable_associations whitelist (or another blacklist like ['user', 'users', 'owner', 'author', ...]) might help prevent serious exploitation in about half of the vulnerable examples I have seen so far.

[deivid-rodriguez]

Hei @lukas-eu, sorry for not replying earlier and not giving this issue the priority it deserves. And thanks for your great investigation of this issue!

I essentially agree with your suggestions after reading about this problem a bit more. In particular I think we can go with some version of "2." above.

I wrote https://github.com/activerecord-hackery/ransack/tree/improve-defaults that I think should satisfy both having secure defaults but also easily overriding those to relax them as wanted or even going back to the current behavior.

The idea is that you need to explicitly provide explicit ransackable_associations and ransackable_attributles for all models, or otherwise an error will be raised. However, as long as you explicitly define the proper methods in your models or even ApplicationRecord, then you get back the current behavior in the parent method of returning the full list of attributes/associations. For people already authorizing the list by delegating to super and then denylisting some attributes/associations from it, that means no breaking change from this change in defaults. And also you can easily get the current behavior for toy apps, apps without sensitive data, or whatever, by defining ransackable_attributes and ransackable_associations methods in your ApplicationRecord that just delegate to super.

This would need proper documentation, and not sure if the implementation is ideal or will work properly in all cases, by do you think the idea makes sense?

[lukas-eu]

This looks great, I like it!

Usually I would not be a fan of the easy override via a simple super call because the resulting behavior is not immediately obvious when reading through the code that contains the super call.
Given the compatibility argument with existing code though, it looks like a smart workaround and a very adequate compromise to make, and I like that the error message comes pre-filled with the list of associations instead of referring to the super hack.

Two small suggestions:

1. Since you cannot expect all members of all_searchable_(attributes/associations) to actually be searchable, maybe they could be renamed something like all_record_(attributes/associations)
2. To keep in line with not encouraging the use of super too much, maybe the attribute and association definitions in the test app could be changed to dynamic all_searchable_(attributes/associations)(or insert new name here) calls since they have a more meaningful name

I'll add a short note to our blog post when a new version with this behavior is released.

[hakusaro]

I'm here from @lukas-eu's post, which is slowly filtering through social media. I strongly suggest going with option 2 in the proposed solution list: "Implement restrictive defaults (potentially allowing an explicit bypass)" and I think that this needs to be prioritized extremely highly. There are a lot of applications that are vulnerable here. The information is now public knowledge. In the default use-case a lot of people are likely sitting ducks. I would probably advocate for getting a CVE, issuing an update that changes the default, and providing detailed suggestions for people to fix their implementations.

[deivid-rodriguez]

Thanks @lukas-eu. Sorry I didn't reply earlier today, too many things!

Usually I would not be a fan of the easy override via a simple super call because the resulting behavior is not immediately obvious when reading through the code that contains the super call.

Me neither, this is what I dislike the most about the current approach. That just overwriting the method and delegating to super changes the behavior. I wonder if we should in addition deprecate delegating to super at all.

Since you cannot expect all members of all_searchable_(attributes/associations) to actually be searchable, maybe they could be renamed something like all_record_(attributes/associations)

Fine with updating this, but maybe we should use something that suggest that it comes from ransack? I would think of all_record_attributes/associations as something that comes from Rails. Just looking to prevent any future collisions.

To keep in line with not encouraging the use of super too much, maybe the attribute and association definitions in the test app could be changed to dynamic all_searchable_(attributes/associations)(or insert new name here) calls since they have a more meaningful name

Makes sense I can give it a try.

@hakusaro I'll release this ASAP as Ransack 3.0.

[lukas-eu]

Fine with updating this, but maybe we should use something that suggest that it comes from ransack? I would think of all_record_attributes/associations as something that comes from Rails. Just looking to prevent any future collisions.

That makes sense as well, collision safety is of course more important than aesthetics. I can't think of the perfect suggestion right now, maybe adding a ransack or rs somewhere in there?
I don't wanna blow up the discussion too much either to avoid bike-shedding and potentially delaying other progress, your original naming definitely seems reasonable enough if there are no better ideas.

[hakusaro]

I'll release this ASAP as Ransack 3.0.

@deivid-rodriguez do you mean 4.0?

[deivid-rodriguez]

Yes, sorry!

[hakusaro]

If you need any help with a github security advisory, I'd be more than happy to help you, too. If you go through github security advisories, you can get a GHSA ID and a CVE assigned, as well, which will expedite notification of potentially vulnerable people through scanning tools.

[deivid-rodriguez]

@lukas-eu I think I mostly addressed the concerns at #1400:

• Using super still works as proposed, but as we noticed above it's not really great so I deprecated that in favor of using an explicit method that returns the full list of "unauthorized" attributes. I used unauthorized_ransackable_attributes (and unauthorized_ransackable_associations), but I'm not fully convinced it's the best naming.
• I also removed usage of super from our specs as per the previous point.

Could you have a look at #1400 and let me know?

[Narnach]

Thanks everyone! @wonda-tea-coffee for raising the issue, @lukas-eu for digging in to this and making the problem visible via the blog post that went viral, and @deivid-rodriguez for your work on the gem and on providing a very neat solution.

I especially like the new message that gives a copy/paste-able solution that makes visible what you expose and allows you to audit it easily. After reading the blog post last week I manually did a similar thing via the console, so I appreciate this solution being available as part of the gem. A lot of devs will appreciate its guidance when doing the upgrade.

[lukas-eu]

I left a review on #1400 :

• Small typo fix
• Soft suggestion to change unauthorized to authorizable

[deivid-rodriguez]

Hei! As you may have noticed, I finally got around to releasing Ransack 4.0.0! Also we received an interesting report at #1403. It seems like a good idea to me, feel free to share your thoughts there!

[finist]

Great feature! But how about specific setting for disable it? I use strong parameters for filtering searching params and separate method in model looks like overthinking for me.

[deivid-rodriguez]

I think you're aiming for #1403? I think it's a good idea, just needs someone to take a stab at it!

[farooqch11]

for existing apps, what to do? Do we have to mention for every model? if we wanna only block passwords, nothing else. whats the best way ?

[runephilosof-karnovgroup]

I would be nice with a CVE. It does not seem like Dependabot is aware of the need to upgrade.