>From 9c1c1e4a096d5480444e025757692309be26a14b Mon Sep 17 00:00:00 2001
From: Valeriy Ponomaryov <vponomaryov@mirantis.com>
Date: Thu, 30 Jun 2016 20:19:22 +0300
Subject: [PATCH] Fix metadata_to_str function code injection vulnerability

It is possible to inject HTML/JavaScript code into shares table
member page setting metadata to shares and share types table admin page
setting extra specs. So, escape HTML-specific symbols in output
string of 'metadata_to_str' function to make it interpreted
as string and not as code.

Change-Id: Ied567e06d91941e9aaac7d3117e03cd1770fb75e
Security-Fix
Closes-Bug: #1597738
---
 manila_ui/dashboards/admin/shares/tabs.py          |  8 ++---
 .../dashboards/project/shares/shares/tables.py     |  4 +--
 manila_ui/dashboards/project/shares/shares/tabs.py | 10 +++----
 manila_ui/dashboards/utils.py                      | 24 +++++++++++----
 manila_ui/test/dashboards/test_utils.py            | 34 ++++++++++++++++++++++
 5 files changed, 60 insertions(+), 20 deletions(-)

diff --git a/manila_ui/dashboards/admin/shares/tabs.py b/manila_ui/dashboards/admin/shares/tabs.py
index 4544286..8aded92 100644
--- a/manila_ui/dashboards/admin/shares/tabs.py
+++ b/manila_ui/dashboards/admin/shares/tabs.py
@@ -12,7 +12,6 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-from django.utils.safestring import mark_safe
 from django.utils.translation import ugettext_lazy as _
 
 from horizon import exceptions
@@ -25,6 +24,7 @@ from manila_ui.api import manila
 from manila_ui.api import network
 from manila_ui.dashboards.admin.shares import tables
 from manila_ui.dashboards.admin.shares import utils
+from manila_ui.dashboards import utils as common_utils
 
 
 class SnapshotsTab(tabs.TableTab):
@@ -104,10 +104,8 @@ class ShareTypesTab(tabs.TableTab):
                               _("Unable to retrieve share types"))
         # Convert dict with extra specs to friendly view
         for st in share_types:
-            es_str = ""
-            for k, v in st.extra_specs.iteritems():
-                es_str += "%s=%s\r\n<br />" % (k, v)
-            st.extra_specs = mark_safe(es_str)
+            st.extra_specs = common_utils.metadata_to_str(
+                st.extra_specs, 8, 45)
         return share_types
 
 
diff --git a/manila_ui/dashboards/project/shares/shares/tables.py b/manila_ui/dashboards/project/shares/shares/tables.py
index f529ae9..09f7f96 100644
--- a/manila_ui/dashboards/project/shares/shares/tables.py
+++ b/manila_ui/dashboards/project/shares/shares/tables.py
@@ -17,7 +17,6 @@
 from django.core.urlresolvers import NoReverseMatch  # noqa
 from django.core.urlresolvers import reverse
 from django.template.defaultfilters import title  # noqa
-from django.utils.safestring import mark_safe
 from django.utils.translation import string_concat, ugettext_lazy  # noqa
 from django.utils.translation import pgettext_lazy
 from django.utils.translation import ugettext_lazy as _
@@ -150,8 +149,7 @@ class UpdateRow(tables.Row):
             share.share_network = share_net.name or share_net.id
         else:
             share.share_network = None
-        meta_str = utils.metadata_to_str(share.metadata)
-        share.metadata = mark_safe(meta_str)
+        share.metadata = utils.metadata_to_str(share.metadata)
 
         return share
 
diff --git a/manila_ui/dashboards/project/shares/shares/tabs.py b/manila_ui/dashboards/project/shares/shares/tabs.py
index cc5c7f1..7a126d8 100644
--- a/manila_ui/dashboards/project/shares/shares/tabs.py
+++ b/manila_ui/dashboards/project/shares/shares/tabs.py
@@ -10,7 +10,6 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-from django.utils.safestring import mark_safe
 from django.utils.translation import ugettext_lazy as _
 
 from horizon import exceptions
@@ -42,11 +41,10 @@ class SharesTab(tabs.TableTab):
         try:
             shares = manila.share_list(self.request)
             for share in shares:
-                share.share_network = \
-                    share_nets_names.get(share.share_network_id) or \
-                    share.share_network_id
-                meta_str = utils.metadata_to_str(share.metadata)
-                share.metadata = mark_safe(meta_str)
+                share.share_network = (
+                    share_nets_names.get(share.share_network_id) or
+                    share.share_network_id)
+                share.metadata = utils.metadata_to_str(share.metadata)
 
             snapshots = manila.share_snapshot_list(self.request, detailed=True)
             share_ids_with_snapshots = []
diff --git a/manila_ui/dashboards/utils.py b/manila_ui/dashboards/utils.py
index f622084..9912e18 100644
--- a/manila_ui/dashboards/utils.py
+++ b/manila_ui/dashboards/utils.py
@@ -13,9 +13,23 @@
 #    under the License.
 
 from django.forms import ValidationError  # noqa
+from django.utils.safestring import mark_safe
 from django.utils.translation import ugettext_lazy as _
 
 
+html_escape_table = {
+    "&": "&amp;",
+    '"': "&quot;",
+    "'": "&apos;",
+    ">": "&gt;",
+    "<": "&lt;",
+}
+
+
+def html_escape(text):
+    return ''.join(html_escape_table.get(s, s) for s in text)
+
+
 def parse_str_meta(meta_s):
     """Parse multiline string with data from form.
 
@@ -58,14 +72,12 @@ def parse_str_meta(meta_s):
     return set_dict, unset_list
 
 
-def metadata_to_str(metadata):
+def metadata_to_str(metadata, meta_visible_limit=4, text_length_limit=25):
 
     # Only convert dictionaries
     if not hasattr(metadata, 'keys'):
         return metadata
 
-    meta_visible_limit = 4
-    text_length_limit = 25
     meta = []
     meta_keys = metadata.keys()
     meta_keys.sort()
@@ -77,8 +89,8 @@ def metadata_to_str(metadata):
         v = metadata[k]
         if len(v) > text_length_limit:
             v = v[:text_length_limit] + '...'
-        meta.append("%s = %s" % (k_shortenned, v))
+        meta.append("%s = %s" % (html_escape(k_shortenned), html_escape(v)))
     meta_str = "<br/>".join(meta)
-    if len(metadata.keys()) > meta_visible_limit:
+    if len(metadata.keys()) > meta_visible_limit and meta_str[-3:] != "...":
         meta_str += '...'
-    return meta_str
+    return mark_safe(meta_str)
diff --git a/manila_ui/test/dashboards/test_utils.py b/manila_ui/test/dashboards/test_utils.py
index dcde6c9..60f2afa 100644
--- a/manila_ui/test/dashboards/test_utils.py
+++ b/manila_ui/test/dashboards/test_utils.py
@@ -61,3 +61,37 @@ class ManilaDashboardsUtilsTests(base.TestCase):
     )
     def test_parse_str_meta_validation_error(self, input_data):
         self.assertRaises(ValidationError, utils.parse_str_meta, input_data)
+
+    @ddt.data(
+        (({"a": "<script>alert('A')/*", "b": "*/</script>"}, ),
+         "a = &lt;script&gt;alert(&apos;A&apos;)/*<br/>b = */&lt;/script&gt;"),
+        (({"fookey": "foovalue", "barkey": "barvalue"}, ),
+         "barkey = barvalue<br/>fookey = foovalue"),
+        (({"foo": "barquuz"}, 1, 2), "fo... = ba..."),
+        (({"foo": "barquuz", "zfoo": "zbarquuz"}, 1, 3), "foo = bar..."),
+        (({"foo": "barquuz", "zfoo": "zbarquuz"}, 2, 3),
+         "foo = bar...<br/>zfo... = zba..."),
+        (({"foo": "barquuz", "zfoo": "zbarquuz"}, 3, 3),
+         "foo = bar...<br/>zfo... = zba..."),
+        (({"foo": "barquuz", "zfoo": "zbarquuz"}, 3, 8),
+         "foo = barquuz<br/>zfoo = zbarquuz"),
+    )
+    @ddt.unpack
+    def test_metadata_to_str(self, input_args, expected_output):
+        result = utils.metadata_to_str(*input_args)
+
+        self.assertEqual(expected_output, result)
+
+    @ddt.data(
+        ("ldap", "LDAP"),
+        ("active_directory", "Active Directory"),
+        ("kerberos", "Kerberos"),
+        ("FaKe", "FaKe"),
+    )
+    @ddt.unpack
+    def test_get_nice_security_service_type(self, input_value, expected_value):
+        security_service = type("FakeSS", (object, ), {"type": input_value})()
+
+        result = utils.get_nice_security_service_type(security_service)
+
+        self.assertEqual(expected_value, result)
-- 
1.9.1