XSS vulnerability due to window.opener (target="_blank" and window.open())

[Aaron Wells]

The Catalyst security team has pointed out to us that the practice of opening new browser windows via "target" links or the Javascript window.open() command. The problem is that in these cases, the Javascript and HTML standards require that the newly opened window/tab have access to the original window's "Window" object, via "window.opener". This allows the new window to control the navigation of the original window, and possibly access other DOM objects as well, depending on security policies.

The really bad part, though, is that the new window has access to window.opener, and navigation control via it, even if the new window is on a different domain than the original window. And this window.opener object remains there, even if the user goes to a new page or site in the new window, or the old window!

This allows for all kinds of cross-site-scripting attacks. So, we need to prevent this behavior in Mahara.

[Aaron Wells (u-aaronw)]

In Mahara there are three main kinds of links we need to consider, each of which can use a different strategy:

1. User-generated links.

TinyMCE allows users to specify "new window" when creating a link, so this is rather easy to do. We will need to do the following:

1a. Disable the "Target" option in TinyMCE
1b. Add an HTMLPurifier setting to remove "target" attributes in links.
1c. (Maybe) scan the database for links with the "target" attribute, and remove them. This step is probably not necessary, though, because of our use of HTMLPurifier.
1d. Carefully check the code for other sources of user-generated links. For instance, the RSS block, do we run that through HTMLPurifier?

2. Hard-coded links in the Mahara code, directed at external websites.

These are usually "extra information" links on form pages. We want them to open in a new window, so that the user can read them without navigating away from the form. The easiest thing to do here, is to simply remove the target attribute, and rely on our JS functionality that warns you when you're about to navigate away from an incomplete form. When a user sees that, they'll be able to click "cancel" to stay on the page, and then manually open the link in a new window/tab.

3. Hard-coded links in the Mahara code, directed at internal Mahara URLs.

The most obvious example of this is for institution auth configuration. When you're on the institution auth page, and you create or edit the settings for an auth instance, the auth instance config opens in a pop-up window. There may be other places in the admin section where we also do this.

There are two main strategies we might use for these. The simplest, is to simply force the link to open in the same window, and return to that window when the task in the sub-window is complete. This will be functional, but clunky and probably annoying.

The more complicated route, but with a better user interface, would be to open the "pop-up" in an iframe, similar to what we do with block instance configuration. We may be able to use much of the code from the page editor to help with this, but it should be much simpler because it won't need as much flexibility (since it's not dynamically loading up arbitrary blocktypes).

Other ideas:

There is a possible workaround for this issue, which is to use an intermediate redirect page that strips out the window.opener field. So, instead, of creating a direct link to the new window's URL, you create a link to the redirect page. Then you tell the redirect page what URL you want to go to. Then the redirect page and the parent page erase their references to each other, and the redirect page redirects itself to the desired URL. But there are some indications that this approach might not work robustly across all browsers, might be possible to circumvent in some situations, and it may trigger pop-up window protections in some browsers. So it would be more secure to eliminate our use of mandatory pop-up windows altogether.

[Aaron Wells (u-aaronw)]

Patch to remove the "target" field from TinyMCE and update HTMLPurifier to screen out the "target" attribute on links in user-edited text.

https://reviews.mahara.org/6167

[Aaron Wells (u-aaronw)]

Another thing we should do, is add a check for "_blank" in minaccept. This should catch both 'target="_blank"' and "window.open(url, '_blank')" bugs. It won't be perfect (a user could still have the same problem if they do a target with an arbitrary name, and you could assign the window object to another variable), but it should help stop routine addings of target=_blank.

[Aaron Wells (u-aaronw)]

More patches:

https://reviews.mahara.org/6168 : Removes all the simple instances of 'target="_blank"'
https://reviews.mahara.org/6169 : Removes simple instances of window.open() into new window
https://reviews.mahara.org/6170 : Puts a TODO comment next to the remaining ones that aren't so simple

There may be other instances that I didn't find in my search, so feel free to take a look, everyone else. :)

In cases where it wouldn't break any functionality, I removed 'target="_blank"' from links wherever I found it, and I added "_self" for the target param in window.open() wherever I found it configured to open into a new window, or where it had the default window target set up.

The remaining cases are ones which require a bit more thought, because allowing the link to open in the same window may cause problems. These are:

1. htdocs/artefact/interna/blocktype/textbox/lib.php: The link from the "Note" block to the "Manage notes" screen. Although I suppose the "unsaved form" notice might make this one not such a big deal?

2. htdocs/theme/raw/templates/form/authlist.tpl: The institution auth config popup

3. htdocs/webservice/oauthv1.php: I haven't looked into it yet, but I think target="_blank" may be required for OAuth.

[Kristina Hoeppner (kris-hoeppner)]

"The more complicated route, but with a better user interface, would be to open the "pop-up" in an iframe, similar to what we do with block instance configuration."

What do you mean with that, Aaron? Do you mean the docked modal?

[Aaron Wells (u-aaronw)]

Kristina,

That's right, I meant that for auth instance config, we could put the config into a docked modal. Like the ones we use for block instance configuration.

Thinking about it this morning, I can think of three main approaches:

1. The modal window I mentioned before.

2. Similar to Profile -> Social Media, we could move the list of the institution's auth instances onto a separate page from the rest of the institution's configuration form. And then, from that list page, have buttons to navigate to the add/edit form and back again.

3. Dynamically insert the auth instance config into the same page as the institution form, using Javascript. This would be most similar to how we used to the "add file attachment" buttons for Resume -> Goals & Skills, prior to 15.10. But I think this would probably require more code change, because currently the processing of the auth instance config forms is handled by a separate PHP script than the rest of the institution config.

4. Just leave this one there, and rely on admins to be smart enough to close it when they're done with it, and not use it for general browsing. In favor of this approach: the window does close itself when you submit the form. Against this approach: it would not be surprising for some auth instance config forms to have links to external pages in their help fields.

I think the modal window (#1) is probably the one that will require the least code change and be the best user interface. However, because our modal window code changed so much from 15.04 to 15.10, it may be challenging to backport that. If that's then maybe we should do the modal window (idea #1) for 15.10+, and the separate list page (idea #2) for 15.04 and earlier.

[Kristina Hoeppner (kris-hoeppner)]

Thanks for confirming that it's the docked modal you were talking about, Aaron. I'm in favor of that for 15.10+ because it is similar to the block configuration and still shows you that you stay on the page and can return to it. It's probably even going to work better than the separate window as you'll not have to look for the config elsewhere esp. on multiple monitors and the text in the modal will be well displayed.

[Aaron Wells (u-aaronw)]

Had a quick discussion with Kristina, she pointed out that, on my idea #2, rather than having an intermediary "list" page, it would be better to have you do one quick to navigate from the institution page to the auth instance config page. Like this:

1. The institution config page would look basically the same as it currently does, showing the list of configured auth instances (each one a link), a "delete" button next to each auth instance, and a dropdown with the available auth types and an "Add" button.

2. Clicking on the link for one of the auth instances would navigate you away from the institution config page, and to that auth instance's config page. When you submit the form, you would navigate back to the institution config page, which would now show that new auth instance.

3. Clicking the "new" button would navigate you away to a new page to create a new auth instance. When you submit that form, it navigates you back.

4. The "delete" buttons require no change at all to their current operation. They work by changing a hidden form variable in the institution config form, and the change is not actually saved until you submit the institution config form. And they already hook into the "unsaved form" API.

[Aaron Wells (u-aaronw)]

One possible fix for this is to add rel="noreferrer" to all "target" links. But reports say this doesn't work in Internet Explorer, so it's not an option for Mahara, which still supports IE11.