[OSSA 2014-023] XSS in Horizon Heat template - resource name (CVE-2014-3473)

What additional information would you like? Are you unable to reproduce? I'm running DevStack on Ubuntu 12.04.4 LTS. Steps to reproduce:

Upload the attached template file in Orchestration->Stacks and create a new instance. Click on the Stack Name and go to the Resources or Events tab. The HTML output is:

<a href="my_instance"><img src="zz" onerror="alert(1)">" class=""&gt;my_instance"&gt;&lt;img src=zz onerror=alert(1)&gt;</a>

This is due to the resource name in the attached yaml file, which I will paste here:

======

heat_template_version: 2013-05-23

description: Simple template to deploy a single compute instance

resources:
  my_instance"><img src=zz onerror=alert(1)>:
    type: OS::Nova::Server
    properties:
      key_name: my_key
      image: cirros-0.3.1-x86_64-uec
      flavor: m1.nano

======

Please let me know if you need further information.

Could Horizon folks confirm that they repoduced it ? Looks valid to me.

Yes, the templates in this entire module appear to be improperly written.

As explained at the top of the Django Security docs:
https://docs.djangoproject.com/en/dev/topics/security/

The automatic HTML escaping does NOT allow you to assign values into properties in templates. The fix for this (and the rest of the latent XSS issues in this module) is to go through all of the templates, eliminating any unescaped assignment to properties.

Furthermore, the url construction needs to be refactored to properly use Django's url reversing mechanisms, preferably inside the templates. As it currently stands, things like this:
https://github.com/openstack/horizon/blob/master/openstack_dashboard/dashboards/project/stacks/templates/stacks/_stack_info.html#L14
are likely to be vulnerabilities. I'll take a look through the templates and catalog a detailed list of things that need to be cleaned up.

Fixing this and related bugs is going to be a relatively large cleanup task.

It's probably worth auditing the rest of the Horizon templates for this kind of behavior, since clearly existing code reviewers didn't know to look for this pattern of abuse.

A quick look at just the stacks dashboard shows:

This is probably the reported vulnerability:

https://github.com/openstack/horizon/blob/56addb790a40da1feae360f3f413dc1f539ed01a/openstack_dashboard/dashboards/project/stacks/templates/stacks/_detail_topology.html#L7

This is likely to be a vulnerability:
https://github.com/openstack/horizon/blob/56addb790a40da1feae360f3f413dc1f539ed01a/openstack_dashboard/dashboards/project/stacks/templates/stacks/_resource_overview.html#L15

This looks user-controlled, so also a problem:
https://github.com/openstack/horizon/blob/56addb790a40da1feae360f3f413dc1f539ed01a/openstack_dashboard/dashboards/project/stacks/templates/stacks/_stack_info.html#L14

Obviously I'm not going to open a new ticket about these separate issues that weren't in the original report, but as you can see, there's some fundamental problems. I didn't examine the rest of Horizon, someone should do that.

Hi Paul, thanks for taking a look. I went through the templates above and I believe that this is the one containing the vulnerability: https://github.com/openstack/horizon/blob/56addb790a40da1feae360f3f413dc1f539ed01a/openstack_dashboard/dashboards/project/stacks/templates/stacks/_stack_info.html#L14

The other ones look interesting as well but I do not believe they are vulnerable since they don't seem to take content from the user:

Contains the stack UUID, so not vulnerable
https://github.com/openstack/horizon/blob/56addb790a40da1feae360f3f413dc1f539ed01a/openstack_dashboard/dashboards/project/stacks/templates/stacks/_detail_topology.html#L7

I believe this too is a URL containing the UUID of a physical resource in a nested stack, so I don't believe that one is vulnerable either.
https://github.com/openstack/horizon/blob/56addb790a40da1feae360f3f413dc1f539ed01a/openstack_dashboard/dashboards/project/stacks/templates/stacks/_resource_overview.html#L15

Even if they are not directly exploitable, it would be good to fix the others in the same patch.

Very much agreed it would be best to fix the templates since they could become vulnerable in the future.

I'm not familiar enough with the codebase to conclusively say that the second two instances don't pull the UUID or any part of physical resource url from a user-provided value. We've had problems in the past where arbitrary unicode was allowed in things like project or usernames, but then caused problems in urls like these.

Additionally, it's not uncommon throughout openstack to generate a fully qualified url from the current http request host, which can result in various kinds of stored XSS. See, for example, this similar vulnerability: https://www.djangoproject.com/weblog/2013/feb/19/security/#s-issue-host-header-poisoning

Here is impact description #1
Note: I used 8914ed95 to track down affected versions.

Title: XSS in Horizon-Orchestration through resource name
Reporter: Jason Hullinger (HP)
Products: Horizon
Versions: 2013.2 to 2013.2.3, and 2014.1

Description:
Jason Hullinger from Hewlett Packard reported a vulnerability in Horizon Orchestration dashboard. By tricking an Horizon user, a malicious templates owner/catalog, may trigger an XSS once a corrupted template is used in the Orchestration/Stack section of Horizon, resulting in potential assets stealing (horizon user/admin access credentials (session cookies/CSRF tokens), VMs/Network configuration/management, tenants confidential informartion, etc.). Only setups using Heat and Horizon are affected.

I would say "...a malicious templates owner/catalog may conduct an XSS attack once..." (also removing the comma after catalog). Would also remove the inner parenthesis "(session cookies/CSRF tokens)" and s/informartion/information/. Finally I would say "setups using Heat together with Horizon" so that it's clear it's the combo that is affected.

Thanks!
Here is impact description #2

Title: XSS in Horizon-Orchestration through resource name
Reporter: Jason Hullinger (HP)
Products: Horizon
Versions: 2013.2 to 2013.2.3, and 2014.1

Description:
Jason Hullinger from Hewlett Packard reported a vulnerability in Horizon Orchestration dashboard. By tricking an Horizon user, a malicious templates owner/catalog may conduct an XSS once a corrupted template is used in the Orchestration/Stack section of Horizon, resulting in potential assets stealing (horizon user/admin access credentials, VMs/Network configuration/management, tenants confidential information, etc.). Only setups using Heat together with Horizon are affected.

+1

Here is the combined impact description #3 for #1308727, #1322197 and #1320235:

Title: Multiple XSS vulnerabilities in Horizon
Reporter: Jason Hullinger (HP), Craig Lorentzen (Cisco), Michael Xin
(Rackspace)
Products: Horizon
Versions: 2013.2 to 2013.2.3, and 2014.1

Description:
Jason Hullinger from Hewlett Packard, Craig Lorentzen from Cisco and
Michael Xin from Rackspace reported 3 XSS vulnerabilities in Horizon. A
malicious Orchestration templates owner/catalog may conduct an XSS once
a corrupted template is used in the Orchestration/Stack section of
Horizon. A malicious Horizon user may store an XSS by creating a network
with a corrupted name. A malicious Horizon administrator may store an
XSS by creating a user with a corrupted email address. Once executed in
a legitimate context those XSS may result in potential assets stealing
(horizon user/admin access credentials, VMs/Network
configuration/management, tenants confidential information, etc.). All
Horizon setups are affected.

It may help to initially expand the XSS acronym in the description for improved clarity and searchability. Here is a proposed alternative with some additional English grammar fixes:

Jason Hullinger from Hewlett Packard, Craig Lorentzen from Cisco and Michael Xin from Rackspace reported 3 cross-site scripting (XSS) vulnerabilities in Horizon. A malicious Orchestration template owner or catalog may conduct an XSS attack once a corrupted template is used in the Orchestration/Stack section of Horizon. A malicious Horizon user may store an XSS attack by creating a network with a corrupted name. A malicious Horizon administrator may store an XSS attack by creating a user with a corrupted email address. Once executed in a legitimate context these attacks may result in potential asset stealing (horizon user/admin access credentials, VMs/Network configuration/management, tenants' confidential information, etc.). All Horizon setups are affected.

Also worth confirming, the "Versions" header implies that no release prior to Havana (2013.2) is affected. Is this true? If prior unsupported releases may also be affected then this could more accurately be stated as "Versions: up to 2013.2.3, and 2014.1" (we simply won't release official backports to versions prior to 2013.2 but that doesn't mean they're unaffected by these vulnerabilities).

Many thanks fungi!

About affected versions, it is not confirmed indeed. Yet #1320235 seemed to be introduced by e410ee9ff, which date back to Folsom.
So this is impact description #4:

Title: Multiple XSS vulnerabilities in Horizon
Reporter: Jason Hullinger (HP), Craig Lorentzen (Cisco), Michael Xin
(Rackspace)
Products: Horizon
Versions: up to 2013.2.3, and 2014.1

Description:
Jason Hullinger from Hewlett Packard, Craig Lorentzen from Cisco and Michael Xin from Rackspace reported 3 cross-site scripting (XSS) vulnerabilities in Horizon. A malicious Orchestration template owner or catalog may conduct an XSS attack once a corrupted template is used in the Orchestration/Stack section of Horizon. A malicious Horizon user may store an XSS attack by creating a network with a corrupted name. A malicious Horizon administrator may store an XSS attack by creating a user with a corrupted email address. Once executed in a legitimate context these attacks may result in potential asset stealing (horizon user/admin access credentials, VMs/Network configuration/management, tenants' confidential information, etc.). All Horizon setups are affected.

Impact desc +1

As discussed with the vulnerability team, I'm attaching a single patch to fix the 3 XSS issues currently open against Horizon.

About the other clean-up tasks mentioned in this bug, I tried to exploit the potential vulnerability in _stack_info.html by adding things like this to the template:

outputs:
  WebsiteURL:
    description: A URL
    value: http://"><img src=zz onerror=alert(2)>

However the strings were escaped properly and I wasn't able to. I believe this is already safe thanks to django's auto-escaping (see https://docs.djangoproject.com/en/1.4/topics/templates/#automatic-html-escaping ).

I'd appreciate additional eyes from folks more familiar with Javascript on the network names fix, as I found the .text() behaviour fairly unintuitive. It parses HTML entities like &lt; into their text equivalent <, making safely escaped strings vulnerable again. I haven't had a chance to investigate other places where we use text(). I'll provide the Havana version of the patch shortly.

The revised impact description in comment #16 looks good to me. Thanks!

The master patch applies cleanly on Icehouse, attaching the Havana version too, both tested.

Horizon coresec, please review and approve proposed patches (by saying you like them on this bug as a comment)

I think the the patch to instances.js, while correct, is indicative of an underlying security design problem (we need to re-separate our html generation from our core code logic), but this ticket is not the place for that discussion.

Both patches look correct to me, I've asked Gabriel Hurley to take a look at the javascript issue.

The javascript change for horizon.instances.js is a very roundabout way that's probably still exploitable although I don't have an immediate example.

the .text() method will take the text representation of the content of an element, hence resulting in certain HTML entities being unescaped...

by contrast the .html() method takes the HTML content of the element, reading HTML entities as their original entity form.

the problem here lies in what happens when you re-insert the value you pulled out into a new element on this line: https://github.com/openstack/horizon/blob/master/horizon/static/horizon/js/horizon.instances.js#L73

because that uses the .html() method, any malicious code gets re-inserted as HTML.

In the short term, I think a better solution is to continue to use .text() for the read, and to change the code where the new element is constructed to either construct the safe (non-user-input) HTML parts and then fill in the user bits with the .text() method (meaning that the read and the write both use .text() thus ensuring text stays text; or otherwise to sanitize the read values before inserting them back into the DOM. There are plenty of examples of how to do XSS santization in javascript on Stack Overflow, or pretty much any javascript templating library has it built in.

The patch as it stands should probably be reworked to address this; I don't think it should be too hard to fix.

I would recommend reviewing the rest of the javascript codebase for places where elements are being constructed by hand using the .html() method. Those are always red flags where you need to be sure that any variables are safe.

Forgot to add the "long-term" part of that comment... long term we need to move away form reading these values out of the DOM entirely. JSON should stay JSON and should be loaded as such via AJAX or websockets. Then a proper client-side templating library can be used to construct these elements and sanitizing can be done in a standardized fashion.

Hi Julie, thanks for the patch.

One further issue we need to consider with this bug in particular is javascript: URLs. If you change the resource name to "javascript:alert(1)" you'll see that this code is still executed. We should probably be using 'reverse' to generate the URL to the resource?

Thank you for the feedback everyone, and sorry for the delay in replying. I'm attaching a new patch that should resolve the Javascript issue Gabriel brought up. This very probably should be refactored into a function later on but I don't see any obvious place where it should live at the moment, and it seems to me the refactoring discussion would be better handled in a regular gerrit review afterwards.

With regard to your comment Kieran, I can indeed reproduce when calling a resource with only a "javascript:" name (then the alert happens when clicking on the link rather than when loading the page). Using reverse to generate URLs is likely safer but appears to be more awkward because the resource URL requires also the stack_id, which isn't accessible from the resource object itself. It is however visible in the "links" dictionary that contains direct heat URLs so playing a bit with regexes we could manage to generate the URL correctly for both the Events and Resources tables:

def generate_resource_url(obj):
    for link in getattr(obj, 'links', []):
        if link.get('rel', None) == "stack":
            url = link.get('href', '')
            m = re.search("/stacks/\w+/([\w-]+)", url)
            if m is not None:
                stack_id = m.group(1)
                return urlresolvers.reverse('horizon:project:stacks:resource',
                                            args=(stack_id, obj.resource_name))
    else:
        return None

It feels a bit awkward though. Unfortunately I don't have a lot of time to spend on this at the moment, if someone else wants to look into it please feel free to pick it up. If there's feedback in the next couple of days I'll do my best to address it, otherwise it'll have to wait until next week as I'll be out of office later on. Thanks again for the feedback.

Attaching a patch that also includes the previous regex solution to generate urls using the safe 'reverse' mechanism, in case that's useful. I'll be out of office until Monday, thanks everyone.

@horizon-coresec: please review latest patch and comment here !

@ttx: I attached a small patch to https://bugs.launchpad.net/horizon/+bug/1322197

which addresses the raised concerns there. I don't see anything comparable here. So, I think, both patches should be merged.

Hey Matthias. I don't think we should try to sanitise/change names on input, since users can always use the CLI to create networks with "forbidden" characters. Whenever we disallow in Horizon things that users can do on the CLI it tends to lead to confusion and bug reports. (Also we don't know if we'll need to display the name in HTML, Javascript or clear log messages so we can't guess at what format to escape and store it in at input time.)

The other part of the patch is interesting but doesn't seem to work for me, e.g. in the Launch Instance screen the network named "<script>" becomes "\u003Cscript\u003E" which granted doesn't trigger the XSS anymore but doesn't read very user-friendly. Am I looking in the wrong place?

Thank you for the input! If you have time to provide feedback on the other aspects of the patch I would really appreciate it too.

@horizon-coresec, could you please review the patch Julie submitted in comment #28 ?
According to https://www.owasp.org/index.php/XSS_%28Cross_Site_Scripting%29_Prevention_Cheat_Sheet#A_Positive_XSS_Prevention_Model, the javascript cleaning looks good, and python filters are reasonable as well.

Please test the patch for regression!

@mrunge: I agree with Julie, we should not cleaning the input data as it is inconsistent with the input validation from CLI that may cause confusion.

@jpich: patch looks good except for the hack on the Events and Resources tables.

Can we just inject the stack_id in the events and resources data so that we don't have to parse the links and extracting the stack_id?

Like adding this
https://github.com/openstack/horizon/blob/master/openstack_dashboard/dashboards/project/stacks/tabs.py#L101

for event in events:
    event.stack_id = stack.id

https://github.com/openstack/horizon/blob/master/openstack_dashboard/dashboards/project/stacks/tabs.py#L126

for resource in resources:
    resource.stack_id = stack.id

I think this approach might be better since the code in tabs.py is responsible for the data preparation for the view anyway. And it is more obvious for someone else reading the code what's actually going on.

This approach is also less prone to external issue, because it doesn't have data format dependency of having the links attributes that comes from heat.

Thanks for the helpful feedback lcheng! The suggestion you suggest is much more elegant and solid indeed, I've updated the patch to use it instead.

Thanks jpich!

Patch looks good to me!

#34 looks good to me as well. Thanks Julie!

Many thanks for the quick update Julie!
I'm running test against master branch and icehouse, sadly the patch doesn't apply cleanly on havana, could you attach a backport for it as well ?

Also, it might be related to my ubuntu based devstack, but I had to manually install httplib2 in .venv to get tests success.

If it's possible, I think we should add httplib2 to test-requirements.txt

Thanks for the feedback folks, I'll add the havana version of the patch tomorrow (unless someone wants to beat me to it!).

Tristan, httplib2 doesn't seem to be a testing requirement for any Horizon version, I'm not sure this is related here? I'll check tomorrow as well.

The httplib2 issue is likely due to https://github.com/openstack/horizon/commit/f712ed04ccb49e832142944d579e89fb7c14d394 . I believe the Havana / Icehouse gate jobs will still run on 12.04 so the problem wouldn't happen on the gate, but as Sean mentions in the commit message since it's an explicit dependency Horizon should be explicit about it... I can backport his patch (separately from the current discussion), though I'd have preferred too if it were added to test-requirements.txt. Not sure if that can be done without updating global-requirements.txt first though, since it's not listed as a possible test requirement there.

Here's the Havana version.

The havana patch in comment #41 is passing unit-tests here.

@horizon-coresec, can you please have a quick look at that last patch?

#41 havana patch looks good to me!

We are going to send the stakeholders advance notification with a disclosure date set to:
2014-07-08, 1500UTC

Master has changed and the patch in #34 does not apply cleanly anymore (it is still ok for Icehouse). The link classes attribute has changed extensively and I don't have time to test the new interaction so I removed the escaping on that one. I think it's ok because it should already be safe as these attributes can only defined programmatically.

Fix proposed to branch: master
Review: https://review.openstack.org/105476

Fix proposed to branch: stable/icehouse
Review: https://review.openstack.org/105477

Fix proposed to branch: stable/havana
Review: https://review.openstack.org/105478

Reviewed: https://review.openstack.org/105476
Committed: https://git.openstack.org/cgit/openstack/horizon/commit/?id=de4466d88b816437fb29eff5ab23b9b964cd3985
Submitter: Jenkins
Branch: master

commit de4466d88b816437fb29eff5ab23b9b964cd3985
Author: Julie Pichon <email address hidden>
Date: Thu May 22 16:45:03 2014 +0100

    Fix multiple Cross-Site Scripting (XSS) vulnerabilities.

     * Ensure user emails are properly escaped

    User emails in the Users and Groups panel are being passed through the
    urlize filter to transform them into clickable links. However, urlize
    expects input to be already escaped and safe. We should make sure to
    escape the strings first as email addresses are not validated and can
    contain any type of string.

    Closes-Bug: #1320235

     * Ensure network names are properly escaped in the Launch Instance menu

    Closes-Bug: #1322197

     * Escape the URLs generated for the Horizon tables

    When generating the Horizon tables, there was an assumption that only
    the anchor text needed to be escaped. However some URLs are generated
    based on user-provided data and should be escaped as well.

     * Use 'reverse' to generate the Resource URLs in the stacks tables

    Closes-Bug: #1308727

    Change-Id: Ic8a92e69f66c2d265a802f350e30f091181aa42e

Reviewed: https://review.openstack.org/105477
Committed: https://git.openstack.org/cgit/openstack/horizon/commit/?id=32a7b713468161282f2ea01d5e2faff980d924cd
Submitter: Jenkins
Branch: stable/icehouse

commit 32a7b713468161282f2ea01d5e2faff980d924cd
Author: Julie Pichon <email address hidden>
Date: Thu May 22 16:45:03 2014 +0100

    Fix multiple Cross-Site Scripting (XSS) vulnerabilities.

     * Ensure user emails are properly escaped

    User emails in the Users and Groups panel are being passed through the
    urlize filter to transform them into clickable links. However, urlize
    expects input to be already escaped and safe. We should make sure to
    escape the strings first as email addresses are not validated and can
    contain any type of string.

    Closes-Bug: #1320235

     * Ensure network names are properly escaped in the Launch Instance menu

    Closes-Bug: #1322197

     * Escape the URLs generated for the Horizon tables

    When generating the Horizon tables, there was an assumption that only
    the anchor text needed to be escaped. However some URLs are generated
    based on user-provided data and should be escaped as well. Also escape
    the link attributes for good measure.

     * Use 'reverse' to generate the Resource URLs in the stacks tables

    Closes-Bug: #1308727

    Change-Id: Ic8a92e69f66c2d265a802f350e30f091181aa42e

Reviewed: https://review.openstack.org/105478
Committed: https://git.openstack.org/cgit/openstack/horizon/commit/?id=c844bd692894353c60b320005b804970605e910f
Submitter: Jenkins
Branch: stable/havana

commit c844bd692894353c60b320005b804970605e910f
Author: Julie Pichon <email address hidden>
Date: Thu May 22 16:45:03 2014 +0100

    Fix multiple Cross-Site Scripting (XSS) vulnerabilities

     * Ensure user emails are properly escaped

    User emails in the Users and Groups panel are being passed through the
    urlize filter to transform them into clickable links. However, urlize
    expects input to be already escaped and safe. We should make sure to
    escape the strings first as email addresses are not validated and can
    contain any type of string.

    Closes-Bug: #1320235

     * Ensure network names are properly escaped in the Launch Instance menu

    Closes-Bug: #1322197

     * Escape the URLs generated for the Horizon tables

    When generating the Horizon tables, there was an assumption that only
    the anchor text needed to be escaped. However some URLs are generated
    based on user-provided data and should be escaped as well. Also escape
    the link attributes for good measure.

     * Use 'reverse' to generate the Resource URLs in the stacks tables

    Closes-Bug: #1308727

    Conflicts:
     horizon/tables/base.py
     openstack_dashboard/dashboards/admin/users/tables.py

    Change-Id: Ic8a92e69f66c2d265a802f350e30f091181aa42e