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
(clean cherry pick of commit fca19a1b0d)
(cherry picked from commit 89593686ef)
This commit is contained in:
Valeriy Ponomaryov 2016-06-30 20:19:22 +03:00 committed by vponomaryov
parent b99604db58
commit 009913d725
5 changed files with 46 additions and 20 deletions

View File

@ -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

View File

@ -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

View File

@ -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 = []

View File

@ -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)

View File

@ -61,3 +61,23 @@ 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)