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

False Positive "Dangerous Send" when method name is checked against a whitelist #754

Closed
riffraff opened this Issue Oct 30, 2015 · 8 comments

Comments

Projects
None yet
3 participants
@riffraff
Copy link

riffraff commented Oct 30, 2015

This code

  def safeish
    scope_name = params[:scope].presence
    fail ActiveRecord::RecordNotFound unless ['safe', 'also_safe'].include?(scope_name)
    Model.public_send(scope_name)
  end

is safe, AFAICT, as we only allow Model.safe and Model.also_safe to happen (well, as long as .include? and public_send do the normal thing :) ).

I tried a few variations (using raise for fail, using scope_name.in?(array), using a %w[] array literal, removing .presence, adding a default, using if..else..end instead of a guard) but it's still detected.

The only solution seems to be avoiding reflection and listing all options explicitly but that does is not ideal.

This happens within a rails v4.2 project, and brakeman v3.1.2.

@riffraff

This comment has been minimized.

Copy link

riffraff commented Oct 30, 2015

ah I finally found a similar issue, so this is probably a duplicate of #420 and can probably can be closed

@presidentbeef

This comment has been minimized.

Copy link
Owner

presidentbeef commented Oct 30, 2015

Hi Gabriele,

Sorry for the trouble. You should be able to restructure this code so that it's covered by #640 and does not warn. But it isn't working for some reason. Let me look into it.

@riffraff

This comment has been minimized.

Copy link

riffraff commented Oct 30, 2015

Thanks a lot, let me know if I can help.

For what it's worth I also have code using when which looks like somewhat related, and also produces a warning

  def safeish
    scope_name = params[:scope].presence
    case scope_name
    when 'safe', 'also_safe'
      model.send(scope_name)
    when nil
      model.default
    else
      fail 'invalid'
    end
  end
@presidentbeef

This comment has been minimized.

Copy link
Owner

presidentbeef commented Oct 30, 2015

Yes, right now Brakeman is pretty limited in how it detects these guards, but hopefully that will improve over time. Thanks for the example!

@presidentbeef

This comment has been minimized.

Copy link
Owner

presidentbeef commented Nov 19, 2015

Oops, forgot to get back to you. With #757 merged, if you refactor to

def safeish
  scope_name = params[:scope].presence
  if ['safe', 'also_safe'].include?(scope_name)
    Model.public_send(scope_name)
  else
    fail
  end
end

Brakeman should no longer warn about this. Of course, up to you if you really want to make the change just to satisfy Brakeman.

@RST-J

This comment has been minimized.

Copy link

RST-J commented Jun 17, 2016

Here is another example for a safeguard (or do you disagree?):
@relation = Model.send(params[:enum_scope_name]) if Model.enum_scope_names.keys.include?(params[:enum_scope_name])

I see a certain problem with safeguards formed by non-literal arrays. In some other issue with the same background I saw someone "protecting" a dangerous send with model.respond_to?(params[:action]) which most likely is dangerous (destroy?).
The same thing could be achieved with model.methods.include?(params[:action]) and there you go ...
A literal array would at least require you to type 'destroy' there and then its up to you.

But, as Rails is such a widespread framework in the Ruby world, I'd argue, it would be worth to explicitly check/allow such enum based safeguards. What do you think?
Of course this would only work, if Brakeman could be informed, e.g. by annotations, about enums. If you then declare the enum "methods", we'll go ahead and shoot your leg - I got to know Ruby as a language which discourages such things but never hinders you from doing so.
Is that possible? I really thought about adding Brakeman to our build/test chain, but that would only make sense if such things are possible.

@riffraff

This comment has been minimized.

Copy link

riffraff commented Jun 17, 2016

@RST-J you can still integrate brakeman by keeping an ignore file for the false positives, but at the same time, you can just put enum names in a constant in the model and check against that.

Not ideal, but it amounts to the same thing as an annotation :)

@RST-J

This comment has been minimized.

Copy link

RST-J commented Jun 17, 2016

@riffraff Thanks for the hints, I'll look into it. I haven't looked into configuration yet, just plainly used it as is. By intuition I don't like the idea to ignore them, because if someone changes that code and removes the guard or makes a change to it that somehow undermines security, then this won't be detected (at least I assume so, maybe the config allows to be precise about this, I'll see).

presidentbeef added a commit that referenced this issue Sep 3, 2017

Repository owner locked and limited conversation to collaborators Sep 25, 2017

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