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

Unvalidated redirect false negatives #1398

Open
swallenfriedman opened this issue Sep 17, 2019 · 1 comment
Open

Unvalidated redirect false negatives #1398

swallenfriedman opened this issue Sep 17, 2019 · 1 comment

Comments

@swallenfriedman
Copy link

@swallenfriedman swallenfriedman commented Sep 17, 2019

Background

Brakeman version: 4.6.1
Rails version: 5.2.3
Ruby version: Using brakeman docker image

Issue

I noticed a few places where brakeman doesn't flag certain instances of unvalidated redirect vulnerabilities. Here's some example contrived controller code:

# Brakeman output for this file:
# == Warnings ==
# Confidence: High
# Category: Redirect
# Check: Redirect
# Message: Possible unprotected redirect
# Code: redirect_to(params)
# File: app/controllers/example_controller.rb
# Line: 22

# Confidence: Medium
# Category: Mass Assignment
# Check: MassAssignment
# Message: Parameters should be whitelisted for mass assignment
# Code: params.permit!
# File: app/controllers/example_controller.rb
# Line: 32

class AppplicationController < ActionController::Base
  # vulnerable to www.example.com/?host=http://www.evilexample.com
  def example_redirect_to_params
    redirect_to params
  end

  # vulnerable to www.example.com/?host=http://www.evilexample.com
  def example_redirect_to_request_params
    redirect_to request.params
  end

  # vulnerable to www.example.com/?host=http://www.evilexample.com
  def example_redirect_to_url
    redirect_to url_for(params.permit!)
  end

  # vulnerable to www.example.com/?host=http://www.evilexample.com
  def example_redirect_to_url_with_other
    redirect_to url_for(params.permit!.merge(other_param: value))
  end
end

Two of these 4 examples get correctly flagged, but all are vulnerable to open redirects (unless manually adding only_path: true as an additional parameter).

Thanks!

@presidentbeef

This comment has been minimized.

Copy link
Owner

@presidentbeef presidentbeef commented Sep 18, 2019

Hi @swallenfriedman - thank you for reporting this! I suspect this is at least a little related to #1034 which I have not gone back to yet.

request.params is easy - looks like request.parameters and request.path_parameters are covered, just not request.params.

In any case - I will take a deeper look. Thanks!

presidentbeef added a commit that referenced this issue Oct 31, 2019
partial fix for #1398
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.