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

Vulnerability fix: validate return_to param using request.host #3627

Merged

Conversation

nicoayala
Copy link
Contributor

@nicoayala nicoayala commented May 17, 2023

Context

An internal team audits the Maxwell Rails apps occasionally, and in the last run, they discovered a vulnerability. Please take a look at the details bellow:

An arbitrary redirect vulnerability occurs when an application URL redirects users to a 3rd-party, attacker-controlled website. As users recognize the application URL, they are likely to interact with the link. This can be used for phishing attacks against application users.

The rails_admin gem allows a redirect URL to be specified as a GET parameter. The validation of this URL does not properly check the redirect’s host, allowing it to be used as an arbitrary redirect.

Solution

Validate the URL starts with the request.host request.base_url

@coveralls
Copy link

coveralls commented May 17, 2023

Coverage Status

coverage: 96.737%. first build
when pulling 8384683 on himaxwell:3.x-validate-redirect-host
into 5a958e4 on railsadminteam:master.

@nicoayala nicoayala changed the title Validate return_to param using request.host Vulnerability fix: validate return_to param using request.host May 18, 2023
@@ -56,7 +56,7 @@ def respond_to_missing?(sym, include_private)
end

def back_or_index
params[:return_to].presence && params[:return_to].include?(request.host) && (params[:return_to] != request.fullpath) ? params[:return_to] : index_path
params[:return_to].presence && params[:return_to].starts_with?(request.host) && (params[:return_to] != request.fullpath) ? params[:return_to] : index_path
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But request.host is in the form of www.example.com. So this won't allow the valid url https://www.example.com/, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, we need to take the scheme into account. I'll fix it. Thx 🙌 !

.rubocop.yml Outdated Show resolved Hide resolved
@nicoayala nicoayala force-pushed the 3.x-validate-redirect-host branch from 72d1ab8 to 287e207 Compare June 28, 2023 16:06
Copy link
Contributor Author

@nicoayala nicoayala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready for another round.

app/controllers/rails_admin/main_controller.rb Outdated Show resolved Hide resolved
@nicoayala nicoayala requested a review from mshibuya June 28, 2023 22:14
@nicoayala nicoayala requested a review from codealchemy June 29, 2023 16:12
@nicoayala
Copy link
Contributor Author

The failing check is rubocop in:

Offenses:

lib/rails_admin/extensions/paper_trail/auditing_adapter.rb:149:28: C: [Correctable] Style/RedundantSelfAssignmentBranch: Remove the self-assignment branch.
          versions = all ? versions : versions.send(Kaminari.config.page_method_name, current_page).per(per_page)
                           ^^^^^^^^

444 files inspected, 1 offense detected, 1 offense autocorrectable
Error: Process completed with exit code 1.

Is this being addressed? Let me know how should I proceed

@mshibuya mshibuya merged commit da48004 into railsadminteam:master Jul 1, 2023
@mshibuya
Copy link
Member

mshibuya commented Jul 1, 2023

Merged in. Thank you for the fix!

@mshibuya
Copy link
Member

mshibuya commented Jul 1, 2023

The Rubocop failure was already addressed in the master:
f06df8f

jklimke added a commit to 3dcl/rails_admin that referenced this pull request Nov 28, 2023
* 'master' of github.com:railsadminteam/rails_admin:
  Tidy up gemfiles
  Bump jRuby
  Test against Mongoid 8
  Test against Rails 7.1
  Mitigate jRuby build failures
  Upgrade node and vite to fix the vite build
  Un-pin turbo-rails to fix Rails 6.1 builds
  Follow-up for railsadminteam#3555
  Resolved a problem with embedded mongoid documents and boolean selectors (railsadminteam#3555)
  Stop using update_only to decide to show subform on create (railsadminteam#3649)
  Follow-up for railsadminteam#3643
  Basic vite integration attempt (railsadminteam#3643)
  Support client-side dynamic scoping
  Fix Rubocop offense
  Revive the live demo
  Unlock webrick version
  Fix typo: tripple ==> triple (railsadminteam#3637)
  Replace &rdquo; entity with inline <q> element (railsadminteam#3636)
  Vulnerability fix: validate return_to param using request.host (railsadminteam#3627)
  Fix Rubocop offense
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants