Bug 16095 - Security issue with the use of target="_blank" or window.open()
Summary: Security issue with the use of target="_blank" or window.open()
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major
Assignee: Owen Leonard
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-03-17 21:47 UTC by Chris Cormack
Modified: 2019-06-27 09:24 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 16095: Remove target="_blank" when a link refer to an external link (22.39 KB, patch)
2016-03-18 08:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16095: Remove target="_blank" when a link refer to an external link (22.45 KB, patch)
2016-03-20 19:51 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 16095: Remove target="_blank" when a link refer to an external link (22.50 KB, patch)
2016-03-21 20:30 UTC, Jesse Weaver
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2016-03-17 21:47:02 UTC
If you click on a link that opens a new tab/window to another site, that tab has access to the original window through JavaScript. The browsing context is related, even if the domains are totally different.

The tab retains access to the original window's object via window.opener, even if you navigate to another page or domain, in the new or original window. Access to the Window object means the new window can use Window.location to open a different URL in the original window, perfect for phishing attacks.

Depending on the site's Same-Origin Policy settings, the new window may have access to other parts of the original window's DOM as well. 

 Every graphical web browser is likely to be affected. Text browser, such as Lynx are probably safe



Any  'A HREF' that contains a target of of '_blank' or '_new' or a fixed name is vulnerable. Previous security best practice often suggested creating a random fixed name for an unpredictable namespace - that won't help with this problem! 
Targets of '_self' and '_parent' are safe. 

You can check a potentially problematic _blank link in the wild by using WebDev tools to change the link href to https://mathiasbynens.be/demo/opener and seeing what happens to the original tab when you open the link. 

We use _blank about 59 times, we need to change this
Comment 1 Chris Cormack 2016-03-17 21:53:09 UTC
How can we fix it?

1/ Remove all _blank links

Or

2/ Less good is <a href="somewhere" rel="noreferrer" target="_blank">

It is only supported by
- WebKit-based browsers (Chrome/Safari/Opera...) - added support in 2009 [4]
- Firefox completed support in version 37 (March 2015)
- Microsoft added support to Edge in Windows 10, but no version of Internet Explorer 

3/ <a href="somewhere" rel="noopener" target="_blank">

Only supported by chrome 49+

4/ <meta name="referrer" content="origin">

This is supported in Firefox+WebKit browsers, not in IE, and partially in Edge. 



I think we should do 1/ every where we can, 2/ everywhere we can't and add 4/ into the doc-head-open.inc just for good measure
Comment 2 Chris Cormack 2016-03-17 23:00:12 UTC
Galen, probably one for Evergreen too, in case it hasnt been filed yet
Comment 3 Jonathan Druart 2016-03-18 08:09:17 UTC
git grep _blank **/*.tt **/*.inc|wc -l
45

But some are referring an internal url:
git grep _blank **/*.tt **/*.inc|grep '/cgi-bin/koha'|wc -l
15

30 remains.
Comment 4 Jonathan Druart 2016-03-18 08:42:31 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2016-03-18 08:46:13 UTC
(Sorry, that won't be very readable...)

git grep _blank **/*.js **/*.tt **/*.inc # ignore if we are doing weird stuffs in pl
|grep -v '/cgi-bin/koha' # ignore internal url
|grep -v noreferrer # ignore _blank with noreferrer set
|grep -v koha-tmpl/intranet-tmpl/lib # We won't modify external lib
|grep _blank # just for the color

koha-tmpl/intranet-tmpl/js/browser.js:                    var newwindow = window.open( $(this).attr('href') + '&searchid=' + me.searchid, '_blank' )
koha-tmpl/intranet-tmpl/js/browser.js:                    $('a[href*="biblionumber="]').not('a[target="_blank"]').click(function (ev) {
=> Refer to internal links (ok)

koha-tmpl/intranet-tmpl/prog/en/includes/cateditor-ui.inc:            $('#title').append(' <a target="_blank" href="' + link.href.replace( '{ID}', parts[1] ) + '">(' + link.title + ')</a>' );
koha-tmpl/intranet-tmpl/prog/en/includes/cateditor-ui.inc:                    $('#status-tag-info').append( '<a href="' + getFieldHelpURL( field.tag ) + '" 
target="_blank" class="show-field-help" title="' + _("Show help for this tag") + '">[?]</a> '  + taginfo.lib );
=> I don't know, please help (?)

koha-tmpl/intranet-tmpl/prog/en/js/cataloging.js:    newin=window.open("../authorities/auth_finder.pl?source="+source+"&authtypecode="+authtype+"&index="+tagsubfieldid+"&value_mainstr="+encodeURI(mainmainstring)+"&value_main="+encodeURI(mainstring), "_blank",'width=700,height=550,toolbar=false,scrollbars=yes');
=> Refer to internal links (ok)

koha-tmpl/intranet-tmpl/prog/en/js/xmlControlfield.js:                                url = ((nodePos.getAttributeNode("url") || nodePos.hasAttribute("url")) && nodePos.getAttribute("url") != "" && nodePos.getElementsByTagName('urltext')[0].textContent != "")?"&nbsp;<a href='" + nodePos.attributes.getNamedItem("url").nodeValue + "' target='_blank'>" + nodePos.getElementsByTagName('urltext')[0].textContent + "</a>":"";
=> Does somebody know and understand this file? (?)

koha-tmpl/intranet-tmpl/prog/en/modules/admin/biblio_framework.tt:              <form action="import_export_framework.pl" name="form_defaul" method="get" target="_blank"  class="form_export">
koha-tmpl/intranet-tmpl/prog/en/modules/admin/biblio_framework.tt:                  <form action="import_export_framework.pl" name="form_[% loo.frameworkcode %]" method="get" target="_blank"  class="form_export">
koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt:          <a target="_blank" href="printfeercpt.pl?action=print&amp;accountlines_id=[% account.accountlines_id %]&amp;borrowernumber=[% account.borrowernumber %]">Print</a>
koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt:          <a target="_blank" href="printinvoice.pl?action=print&amp;accountlines_id=[% account.accountlines_id %]&amp;borrowernumber=[% account.borrowernumber %]">Print</a>
koha-tmpl/intranet-tmpl/prog/en/modules/tools/manage-marc-import.tt:                    '<a target="_blank" href="' + record_details_url
koha-tmpl/intranet-tmpl/prog/en/modules/tools/manage-marc-import.tt:                '<a target="_blank" href="' + record_details_url
=> Refer to internal links (ok)
Comment 6 Jesse Weaver 2016-03-18 16:46:23 UTC
(In reply to Jonathan Druart from comment #5)
> koha-tmpl/intranet-tmpl/prog/en/includes/cateditor-ui.inc:           
> $('#title').append(' <a target="_blank" href="' + link.href.replace( '{ID}',
> parts[1] ) + '">(' + link.title + ')</a>' );
> koha-tmpl/intranet-tmpl/prog/en/includes/cateditor-ui.inc:                  
> $('#status-tag-info').append( '<a href="' + getFieldHelpURL( field.tag ) +
> '" 
> target="_blank" class="show-field-help" title="' + _("Show help for this
> tag") + '">[?]</a> '  + taginfo.lib );
> => I don't know, please help (?)
> 

These are the MARC help links in the statusbar, and should have rel="noreferrer". These are likely not the largest security risks in this list, as the URLs are restricted to the loc.gov and archive.ifla.org domains.

One thing that helps with the lack of IE support for noreferrer is that window.opener does not seem to be accessible cross-domain in IE anyway: http://stackoverflow.com/questions/18624351/how-do-i-detect-window-opener-in-ie/18624735#18624735
Comment 7 Chris Cormack 2016-03-20 19:51:51 UTC Comment hidden (obsolete)
Comment 8 Chris Cormack 2016-03-20 19:54:36 UTC
I have signed the patch off, are we waiting for another patch? Or shall I switch to signed off? It would be great to get this into the upcoming maintenance releases. If possible.
Comment 9 Jonathan Druart 2016-03-21 14:42:51 UTC
(In reply to Chris Cormack from comment #1)
> 4/ <meta name="referrer" content="origin">
> 
> This is supported in Firefox+WebKit browsers, not in IE, and partially in
> Edge. 

We could add a patch for that point, but I don't know how to test it.
Comment 10 Chris Cormack 2016-03-21 19:22:30 UTC
Let's get this patch in, and then we can do more if we wish.
Comment 11 Brendan Gallagher 2016-03-21 19:34:43 UTC
(In reply to Chris Cormack from comment #10)
> Let's get this patch in, and then we can do more if we wish.

I've asked piano to QA this.
Comment 12 Jesse Weaver 2016-03-21 20:30:41 UTC
Created attachment 49405 [details] [review]
Bug 16095: Remove target="_blank" when a link refer to an external link

If you click on a link that opens a new tab/window to another site, that tab
has access to the original window through JavaScript. The browsing context is
related, even if the domains are totally different.

The tab retains access to the original window's object via window.opener, even
if you navigate to another page or domain, in the new or original window.
Access to the Window object means the new window can use Window.location to
open a different URL in the original window, perfect for phishing attacks.

Depending on the site's Same-Origin Policy settings, the new window may have
access to other parts of the original window's DOM as well.

Any  'A HREF' that contains a target of of '_blank' or '_new' or a fixed name
is vulnerable. Previous security best practice often suggested creating a random
fixed name for an unpredictable namespace - that won't help with this problem!
Targets of '_self' and '_parent' are safe.

We do not use _new (at first glance) but several _blank. Some are used
to refer internal url, we do not need to update or remove them. Others
are used to satisfy OPACURLOpenInNewWindow, in these case, we should add
the rel="noreferrer" attribute to the a tags.
In other cases, we can simply remove them and let the users discover
that a mouse has more than one button (we are in 2016, they can do it!)

Signed-off-by: Chris <chrisc@catalyst.net.nz>

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 13 Brendan Gallagher 2016-03-21 20:45:33 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks!
Comment 14 Julian Maurice 2016-03-23 07:19:08 UTC
Patch pushed to 3.22.x, will be in 3.22.5
Comment 15 Frédéric Demians 2016-03-23 12:51:53 UTC
Pushed to 3.20.x, will be in 3.20.10