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
This commit is contained in:
parent
c1a3607d29
commit
fca19a1b0d
@ -12,7 +12,6 @@
|
|||||||
# License for the specific language governing permissions and limitations
|
# License for the specific language governing permissions and limitations
|
||||||
# under the License.
|
# under the License.
|
||||||
|
|
||||||
from django.utils.safestring import mark_safe
|
|
||||||
from django.utils.translation import ugettext_lazy as _
|
from django.utils.translation import ugettext_lazy as _
|
||||||
|
|
||||||
from horizon import exceptions
|
from horizon import exceptions
|
||||||
@ -25,6 +24,7 @@ from manila_ui.api import manila
|
|||||||
from manila_ui.api import network
|
from manila_ui.api import network
|
||||||
from manila_ui.dashboards.admin.shares import tables
|
from manila_ui.dashboards.admin.shares import tables
|
||||||
from manila_ui.dashboards.admin.shares import utils
|
from manila_ui.dashboards.admin.shares import utils
|
||||||
|
from manila_ui.dashboards import utils as common_utils
|
||||||
|
|
||||||
|
|
||||||
class SnapshotsTab(tabs.TableTab):
|
class SnapshotsTab(tabs.TableTab):
|
||||||
@ -104,10 +104,8 @@ class ShareTypesTab(tabs.TableTab):
|
|||||||
_("Unable to retrieve share types"))
|
_("Unable to retrieve share types"))
|
||||||
# Convert dict with extra specs to friendly view
|
# Convert dict with extra specs to friendly view
|
||||||
for st in share_types:
|
for st in share_types:
|
||||||
es_str = ""
|
st.extra_specs = common_utils.metadata_to_str(
|
||||||
for k, v in st.extra_specs.iteritems():
|
st.extra_specs, 8, 45)
|
||||||
es_str += "%s=%s\r\n<br />" % (k, v)
|
|
||||||
st.extra_specs = mark_safe(es_str)
|
|
||||||
return share_types
|
return share_types
|
||||||
|
|
||||||
|
|
||||||
|
@ -15,7 +15,6 @@
|
|||||||
from django.core.urlresolvers import NoReverseMatch # noqa
|
from django.core.urlresolvers import NoReverseMatch # noqa
|
||||||
from django.core.urlresolvers import reverse
|
from django.core.urlresolvers import reverse
|
||||||
from django.template.defaultfilters import title # noqa
|
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 string_concat, ugettext_lazy # noqa
|
||||||
from django.utils.translation import pgettext_lazy
|
from django.utils.translation import pgettext_lazy
|
||||||
from django.utils.translation import ugettext_lazy as _
|
from django.utils.translation import ugettext_lazy as _
|
||||||
@ -149,8 +148,7 @@ class UpdateRow(tables.Row):
|
|||||||
share.share_network = share_net.name or share_net.id
|
share.share_network = share_net.name or share_net.id
|
||||||
else:
|
else:
|
||||||
share.share_network = None
|
share.share_network = None
|
||||||
meta_str = utils.metadata_to_str(share.metadata)
|
share.metadata = utils.metadata_to_str(share.metadata)
|
||||||
share.metadata = mark_safe(meta_str)
|
|
||||||
|
|
||||||
return share
|
return share
|
||||||
|
|
||||||
|
@ -10,7 +10,6 @@
|
|||||||
# License for the specific language governing permissions and limitations
|
# License for the specific language governing permissions and limitations
|
||||||
# under the License.
|
# under the License.
|
||||||
|
|
||||||
from django.utils.safestring import mark_safe
|
|
||||||
from django.utils.translation import ugettext_lazy as _
|
from django.utils.translation import ugettext_lazy as _
|
||||||
|
|
||||||
from horizon import exceptions
|
from horizon import exceptions
|
||||||
@ -42,11 +41,10 @@ class SharesTab(tabs.TableTab):
|
|||||||
try:
|
try:
|
||||||
shares = manila.share_list(self.request)
|
shares = manila.share_list(self.request)
|
||||||
for share in shares:
|
for share in shares:
|
||||||
share.share_network = \
|
share.share_network = (
|
||||||
share_nets_names.get(share.share_network_id) or \
|
share_nets_names.get(share.share_network_id) or
|
||||||
share.share_network_id
|
share.share_network_id)
|
||||||
meta_str = utils.metadata_to_str(share.metadata)
|
share.metadata = utils.metadata_to_str(share.metadata)
|
||||||
share.metadata = mark_safe(meta_str)
|
|
||||||
|
|
||||||
snapshots = manila.share_snapshot_list(self.request, detailed=True)
|
snapshots = manila.share_snapshot_list(self.request, detailed=True)
|
||||||
share_ids_with_snapshots = []
|
share_ids_with_snapshots = []
|
||||||
|
@ -13,9 +13,23 @@
|
|||||||
# under the License.
|
# under the License.
|
||||||
|
|
||||||
from django.forms import ValidationError # noqa
|
from django.forms import ValidationError # noqa
|
||||||
|
from django.utils.safestring import mark_safe
|
||||||
from django.utils.translation import ugettext_lazy as _
|
from django.utils.translation import ugettext_lazy as _
|
||||||
|
|
||||||
|
|
||||||
|
html_escape_table = {
|
||||||
|
"&": "&",
|
||||||
|
'"': """,
|
||||||
|
"'": "'",
|
||||||
|
">": ">",
|
||||||
|
"<": "<",
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
|
def html_escape(text):
|
||||||
|
return ''.join(html_escape_table.get(s, s) for s in text)
|
||||||
|
|
||||||
|
|
||||||
def parse_str_meta(meta_s):
|
def parse_str_meta(meta_s):
|
||||||
"""Parse multiline string with data from form.
|
"""Parse multiline string with data from form.
|
||||||
|
|
||||||
@ -58,14 +72,12 @@ def parse_str_meta(meta_s):
|
|||||||
return set_dict, unset_list
|
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
|
# Only convert dictionaries
|
||||||
if not hasattr(metadata, 'keys'):
|
if not hasattr(metadata, 'keys'):
|
||||||
return metadata
|
return metadata
|
||||||
|
|
||||||
meta_visible_limit = 4
|
|
||||||
text_length_limit = 25
|
|
||||||
meta = []
|
meta = []
|
||||||
meta_keys = metadata.keys()
|
meta_keys = metadata.keys()
|
||||||
meta_keys.sort()
|
meta_keys.sort()
|
||||||
@ -77,11 +89,11 @@ def metadata_to_str(metadata):
|
|||||||
v = metadata[k]
|
v = metadata[k]
|
||||||
if len(v) > text_length_limit:
|
if len(v) > text_length_limit:
|
||||||
v = 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)
|
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 += '...'
|
meta_str += '...'
|
||||||
return meta_str
|
return mark_safe(meta_str)
|
||||||
|
|
||||||
|
|
||||||
def get_nice_security_service_type(security_service):
|
def get_nice_security_service_type(security_service):
|
||||||
|
@ -62,6 +62,26 @@ class ManilaDashboardsUtilsTests(base.TestCase):
|
|||||||
def test_parse_str_meta_validation_error(self, input_data):
|
def test_parse_str_meta_validation_error(self, input_data):
|
||||||
self.assertRaises(ValidationError, utils.parse_str_meta, input_data)
|
self.assertRaises(ValidationError, utils.parse_str_meta, input_data)
|
||||||
|
|
||||||
|
@ddt.data(
|
||||||
|
(({"a": "<script>alert('A')/*", "b": "*/</script>"}, ),
|
||||||
|
"a = <script>alert('A')/*<br/>b = */</script>"),
|
||||||
|
(({"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(
|
@ddt.data(
|
||||||
("ldap", "LDAP"),
|
("ldap", "LDAP"),
|
||||||
("active_directory", "Active Directory"),
|
("active_directory", "Active Directory"),
|
||||||
|
Loading…
Reference in New Issue
Block a user