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

Add reverse tabnabbing check #1367

Closed

Conversation

@linosgian
Copy link

commented Jun 19, 2019

Description

Anchor HTML tags have an attribute that describes how the linked document should be opened, named target. Some of the accepted values are the following:

  • _blank tells the browser to open the linked document in a new tab/window
  • _self (the default value) opens the document on the same tab.

Reverse tabnabbing is a security vulnerability where if an adversary can create anchor tags with target: _blank, they can redirect the victim to any domain they want. This is possible because the newly opened tab is able to communication with the parent tab through window.opener. The most common exploitation route here is to redirect the victim to a phishing using the following PoC:
window.opener.location = "https://attacker.co";.

Let's consider the following scenario:
We have a forum-type of website where users' comments can contain URLs. These URLs get automatically converted to <a> tags with the target: _blank element so that users original tab stays intact (clicked document opens in a new tab). In this case, a malicious user can submit a link that redirects the victim to their own malicious website and present them with our forum's (fake) login page. The user submits their credentials to the adversary's website.

Fix

The fix for this is quite simple: add the noopener value to the rel attribute as such:
<a href="//attacker.co" target="_blank" rel="noopener">

Proposed check

This PR adds a new optional check that looks for such cases. More specifically, we look for link_to calls where target: "_blank" is passed to html_opts and either:

  • the rel attribute is not supplied at all
  • the noopener is not supplied.

I've tried to follow the projects conventions but there will be room for improvement.

Awaiting your feedback! :)

Linos Giannopoulos added some commits Jun 19, 2019

Linos Giannopoulos
Add reverse tabnabbing optional check
Reverse tabnabbing refers to a vulnerability where an anchor tag with
target "_blank" (aka opens linked page in a new tab) is able to control
the parent tab's location through `window.opener.location`. This can
greatly assist phishing attacks when malicious users are able to create
such anchor tags.
@presidentbeef

This comment has been minimized.

Copy link
Owner

commented Jun 19, 2019

Okay, weird...I literally worked on this same check yesterday.

end

def process_result result
return unless original? result and result[:call].third_arg

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Jun 19, 2019

Owner

This assumes only one form of link_to. There are several: https://api.rubyonrails.org/classes/ActionView/Helpers/UrlHelper.html#method-i-link_to

You can check result[:block] to see if it's a block form and result[:call].last_arg can be used to get the options.

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Jun 19, 2019

Owner

Actually, here are my test cases:

<%= link_to '', 'something_static', target: '_blank' %> No warning
<%= link_to '', some_url, target: '_blank' %> Warn
<%= link_to some_url, target: '_blank' do -%>
  Warn
<% end %>
<%= link_to '', some_url, target: '_blank', rel: 'noopener' %> Weak warning
<%= link_to '', some_url, target: '_blank', rel: 'noreferrer' %> Weak warning
<%= link_to '', some_url, target: '_blank', rel: 'noopener noreferrer' %> No warning
<%= link_to some_url, target: '_blank', rel: 'noopener noreferrer' do -%>
  No Warning
<% end %>

This comment has been minimized.

Copy link
@linosgian

linosgian Jun 20, 2019

Author

I am not sure ruling out static strings as false positives would be the right choice here. A statically provided URL (e.g. https://example.com) still leaves the victim vulnerable to that specific website. This might or might not be exploitable, depending on the security of the referred website. We essentially trust that this website won't have a security issue that let's the attacker add javascript to the resource we provide a link to, on our application.

I think that these cases have to be included in the results.

Also, I've tested the case for noreferrer only and both Chrome and Firefox seem to forbid the access to window.opener.location from the new tab. Thus the issue is effectively solved. This seems weird, since noreferrer is supposed to only drop the Referrer: header when issuing a request to the newly opened tab.

What do you think about the first case?

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Jun 24, 2019

Owner

We essentially trust that this website won't have a security issue that let's the attacker add javascript to the resource we provide a link to, on our application.

I hear you and this is a valid concern, but one that is a bit below the threshold for many organizations to worry about.

I think that these cases have to be included in the results.

However, I disagree. It creates too many false positives. I see them in my test corpus.

Examples:

https://github.com/18F/myusa/blob/devel/app/views/devise/sessions/new.html.erb#L9
https://github.com/alphagov/whitehall/blob/master/app/views/admin/edition_workflow/confirm_unpublish.html.erb#L16
https://github.com/amahi/platform/blob/master/app/views/layouts/_footer.html.slim#L6-L25

lib/brakeman/checks/check_reverse_tabnabbing.rb Outdated Show resolved Hide resolved
lib/brakeman/checks/check_reverse_tabnabbing.rb Outdated Show resolved Hide resolved
# already has the ability to redirect the victim user anywhere.
if target_url[0] == :call then
func_s = target_url.method.to_s
return unless !func_s.end_with?("url") && !func_s.end_with?("path")

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Jun 19, 2019

Owner

Not sure I agree with checking for url. That could easily be an attacker-controllable value on a model.

In my version I did url.method.match?(/^url_for$|_path$/) (although I haven't checked for false positives yet).

This comment has been minimized.

Copy link
@linosgian

linosgian Jun 20, 2019

Author

You are right, and I've also tested this in conjuction with this. And links that use the _url helper are vulnerable. If the the _url helper method is used (the same is true for url_for calls), the X-Forwarded-Host header can be used to poison the cache and so control the domain of the resulting <a> tags for as long as the cache holds the cache entry.

So you are specifically looking for url_for/_path or you are ruling those out?

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Jun 24, 2019

Owner

I think url_for/_path should be ignored. The issue with X-Forwarded-Host aside, these are most often going to be used for internal links.

lib/brakeman/checks/check_reverse_tabnabbing.rb Outdated Show resolved Hide resolved
test/tests/rails5.rb Show resolved Hide resolved
require 'brakeman/checks/base_check'

class Brakeman::CheckReverseTabnabbing < Brakeman::BaseCheck
Brakeman::Checks.add_optional self

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Jun 19, 2019

Owner

I'm not sure it needs to be an optional. Depends on false positive rate, which I will look into.

This comment has been minimized.

Copy link
@linosgian

linosgian Jun 20, 2019

Author

Alright. I am not sure there is a legitimate use of the window.opener communication, a case where a user actually wants the newly opened tab to affect the parent tab.. I think that a suggestion to add the noopener everywhere wouldn't hurt, and in fact improve performance.

The noreferrer might have negative side-effects in cases where the developer actually wants the Referer header to be included in the request sent to the new tab, for analytics and so on..

@linosgian

This comment has been minimized.

Copy link
Author

commented Jun 20, 2019

I've pushed multiple fixups addressing the issues that you mentioned

<%= link_to some_url, target: '_blank', rel: 'noopener noreferrer' do -%>
No Warning
<% end %>
<%= link_to '', target: '_blank' %> No warning

This comment has been minimized.

Copy link
@linosgian

linosgian Jun 27, 2019

Author

The URL could be absent entirely. This covers that case

# already has the ability to redirect the victim user anywhere.
# Also statically provided URLs (interpolated or otherwise) are also
# ignored as they produce many false positives.
return if !call?(target_url) || target_url.method.match(/^url_for$|_path$/)

This comment has been minimized.

Copy link
@linosgian

linosgian Jun 27, 2019

Author

I figured that variables are represented by :calls as they could be no-argument function calls.
I think the !call?(target_url) covers all the cases we need

@presidentbeef

This comment has been minimized.

Copy link
Owner

commented Jul 22, 2019

Sorry for the delay!

I did some squashing and adjusting, which ended up making it look like this was not merged. But it is! ❇️

Thank you for your contribution and patience!

@linosgian

This comment has been minimized.

Copy link
Author

commented Jul 23, 2019

No worries, cheers! 👍

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.