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

Check link_to false negative when passing only one parameter #1339

Closed
swallenfriedman opened this issue Apr 1, 2019 · 6 comments

Comments

Projects
None yet
3 participants
@swallenfriedman
Copy link

commented Apr 1, 2019

Background

Brakeman version: 4.5.0
Rails version: 5.2.2.1
Ruby version: Using brakeman docker image

Issue

Here's a code snipped that produces an xss vulnerability that isn't getting flagged by brakeman. I believe it's because the brakeman code is only running this check for rails versions '2.0.0' to '2.9.9.' It seems that link_to escaped its first argument in Rails 3, but I don't believe it does so in Rails 4 or 5.

<%= link_to params[:some_param] do %>
  <span>Link Text</span>
<% end %>
@presidentbeef

This comment has been minimized.

Copy link
Owner

commented Apr 1, 2019

Hi @swallenfriedman -

Thank you for reporting this.

I was confused at first, but I see the issue is that you are calling link_to with a block. That version makes the first argument the URL...which as you point out can lead to XSS.

Should be no problem to rectify this.

@presidentbeef

This comment has been minimized.

Copy link
Owner

commented Apr 1, 2019

Just to clarify a little bit...

There are two checks for link_to - LinkTo and LinkToHref. The former is more for older versions of Rails, the latter is for newer versions where link_to HTML escapes its inputs but doesn't prevent the javascript: schema.

What's a little crazy is that I added link_to with a block as a test case in 2012 but... either I thought it was safe then or I forgot to actually test it. I am not going to try to go back to Rails 2.3 and see if that's a true positive or not.

presidentbeef added a commit that referenced this issue Apr 1, 2019

@JacobEvelyn

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

Just a note—I think you can also use link_to without a block and with just a single argument, like:

link_to params[:href]

or

link_to foo_path(arg: params[:arg])

which I'd expect to also be vulnerable here.

presidentbeef added a commit that referenced this issue Apr 2, 2019

@presidentbeef

This comment has been minimized.

Copy link
Owner

commented Apr 2, 2019

@JacobEvelyn I don't think so. If you pass in a single argument to link_to with no block it will treat that argument as the body of the tag and escape the content properly, even if you pass in a hash or array. At least with Rails 5.2/6.0.

@JacobEvelyn

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

Ah yes, @presidentbeef @swallenfriedman you're both correct—link_to with a single argument and no block sets the current location as the href location. Ignore me!

@swallenfriedman

This comment has been minimized.

Copy link
Author

commented Apr 4, 2019

Good to know the single argument escapes properly! @presidentbeef thanks for getting on this so quickly, appreciate both the tool and the quick turnaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.