Skip to content

Commit

Permalink
Fix XSS in issue2550817
Browse files Browse the repository at this point in the history
Note that the code that triggers that particular bug is no longer in
roundup core. But the change to the templates we suggest is a *lot*
safer as it always escapes the error and ok messages now.

If you are upgrading: you *MUST* read doc/upgrading.txt and do the
necessary changes to your templates, the escaping now happens in the
template and not in the roundup code. So if you don't make the necessary
changes *you are vulnerable*.
  • Loading branch information
schlatterbeck committed Dec 20, 2013
1 parent ed6ead0 commit fd2d2d9
Show file tree
Hide file tree
Showing 10 changed files with 88 additions and 54 deletions.
10 changes: 9 additions & 1 deletion CHANGES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ Fixed:
discovery) (Ralf Schlatterbeck)
- Pythons cgi form code can return a TypeError, we now guard for this
condition. (Ralf Schlatterbeck)
- Small bug-fix in SQL backends: An query (e.g. in a html menu) with a
- Small bug-fix in SQL backends: A query (e.g. in a html menu) with a
where-clause that always evaluates to false now will not raise a
traceback. (Ralf Schlatterbeck)
- Remove Python 2.3 compatibility code for i18n (anatoly techtonik)
Expand Down Expand Up @@ -46,6 +46,14 @@ Fixed:
- Fix subtle bug when sorting by a Link that contains a Multilink from
which we also search for an attribute. In that case the LEFT OUTER
JOIN clause was missing in generated SQL. (Ralf Schlatterbeck)
- Fix another XSS issue2550817. Note that the code that triggers that
particular bug is no longer in roundup core. But the change to the
templates we suggest is a *lot* safer as it always escapes the error
and ok messages now.
If you are upgrading: you *MUST* read doc/upgrading.txt and do the
necessary changes to your templates, the escaping now happens in the
template and not in the roundup code. So if you don't make the
necessary changes *you are vulnerable*. (Ralf Schlatterbeck)


2013-07-06: 1.5.0
Expand Down
41 changes: 41 additions & 0 deletions doc/upgrading.txt
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,47 @@ steps.

.. contents::

Migrating from 1.5.0 to 1.5.1
=============================

*Important*:
There was a security bug fixed in the html templates (an XSS
vulnerability). So if you have a running tracker you will have to fix
the file ``html/page.html`` in your tracker directory. You need to
*twice* remove the ``structure`` element in the template and modify the
'tal:content' attribute, you need to replace the section::

<td>
<p tal:condition="options/error_message | nothing" class="error-message"
tal:repeat="m options/error_message"
tal:content="structure string:$m <br/ > " />
<p tal:condition="options/ok_message | nothing" class="ok-message">
<span tal:repeat="m options/ok_message"
tal:content="structure string:$m <br/ > " />
<a class="form-small" tal:attributes="href request/current_url"
i18n:translate="">clear this message</a>
</p>
</td>

with::

<td>
<p tal:condition="options/error_message | nothing" class="error-message"
tal:repeat="m options/error_message" tal:content="m" />
<p tal:condition="options/ok_message | nothing" class="ok-message">
<span tal:repeat="m options/ok_message" tal:content="m" />
<a class="form-small" tal:attributes="href request/current_url"
i18n:translate="">clear this message</a>
</p>
</td>

if you are using the new *jinja2* base templates, we are now iterating
over the error- and ok-messages and creating a paragraph for each
message. In addition ``autoescape`` is turned on for the section (which
is the critical security change).
See ``templates/jinja2/html/layout/page.html`` for details.


Migrating from 1.4.20 to 1.4.21
===============================

Expand Down
11 changes: 8 additions & 3 deletions roundup/cgi/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,12 @@ def initialiseSecurity(security):
security.addPermissionToRole('Admin', p)

def clean_message(msg):
return cgi.escape (msg).replace ('\n', '<br />\n')
""" A multi-line message is now split at line boundaries.
The templates will do the right thing to format this message.
Note that we no longer need to escape the message as this is now
taken care of by the template.
"""
return msg.split('\n')

error_message = ''"""<html><head><title>An error has occurred</title></head>
<body><h1>An error has occurred</h1>
Expand Down Expand Up @@ -877,9 +882,9 @@ def determine_context(self, dre=re.compile(r'([^\d]+)0*(\d+)')):

# see if we were passed in a message
if ok_message:
self.ok_message.append(ok_message)
self.ok_message.extend(ok_message)
if error_message:
self.error_message.append(error_message)
self.error_message.extend(error_message)

# determine the classname and possibly nodeid
path = self.path.split('/')
Expand Down
6 changes: 2 additions & 4 deletions share/roundup/templates/classic/html/page.html
Original file line number Diff line number Diff line change
Expand Up @@ -170,11 +170,9 @@ <h2><span metal:define-slot="body_title">body title</span></h2>
</td>
<td>
<p tal:condition="options/error_message | nothing" class="error-message"
tal:repeat="m options/error_message"
tal:content="structure string:$m <br/ > " />
tal:repeat="m options/error_message" tal:content="m" />
<p tal:condition="options/ok_message | nothing" class="ok-message">
<span tal:repeat="m options/ok_message"
tal:content="structure string:$m <br/ > " />
<span tal:repeat="m options/ok_message" tal:content="m" />
<a class="form-small" tal:attributes="href request/current_url"
i18n:translate="">clear this message</a>
</p>
Expand Down
6 changes: 2 additions & 4 deletions share/roundup/templates/devel/html/page.html
Original file line number Diff line number Diff line change
Expand Up @@ -234,11 +234,9 @@ <h1><a href="/">Roundup Demo Tracker</a></h1>
<div class="content">
<h1 id="breadcrumb"><span metal:define-slot="body_title">body title</span></h1>
<p tal:condition="options/error_message | nothing" class="error-message"
tal:repeat="m options/error_message"
tal:content="structure string:$m <br/ > " />
tal:repeat="m options/error_message" tal:content="m" />
<p tal:condition="options/ok_message | nothing" class="ok-message">
<span tal:repeat="m options/ok_message"
tal:content="structure string:$m <br/ > " />
<span tal:repeat="m options/ok_message" tal:content="m" />
<a class="form-small" tal:attributes="href request/current_url"
i18n:translate="">clear this message</a>
</p>
Expand Down
22 changes: 14 additions & 8 deletions share/roundup/templates/jinja2/html/layout/page.html
Original file line number Diff line number Diff line change
Expand Up @@ -25,25 +25,31 @@
{% endblock %}
</div>

{% autoescape true %}
<div class='span9'>
<div class='page-header'>
<h1> {% block page_header %} {% endblock %} </h1>
{% if options.error_message %}
<p class='alert alert-block alert-error fade in'>
{{ options.error_message }}
<button type='button' class='close' data-dismiss='alert'>X</button>
</p>
{% for m in options.error_message %}
<p class='alert alert-block alert-error fade in'>
{{ m }}
<button type='button' class='close' data-dismiss='alert'>X</button>
</p>
{% endfor %}
{% endif %}
{% if options.ok_message %}
<p class='alert alert-block alert-success fade in'>
{{ options.ok_message }}
<button type='button' class='close' data-dismiss='alert'>X</button>
</p>
{% for m in options.ok_message %}
<p class='alert alert-block alert-success fade in'>
{{ m }}
<button type='button' class='close' data-dismiss='alert'>X</button>
</p>
{% endfor %}
{% endif %}
</div>
{% block content %}
{% endblock %}
</div>
{% endautoescape %}
</div>
</div>
{% endblock %}
Expand Down
6 changes: 2 additions & 4 deletions share/roundup/templates/minimal/html/page.html
Original file line number Diff line number Diff line change
Expand Up @@ -157,11 +157,9 @@ <h2><span metal:define-slot="body_title">body title</span></h2>
</td>
<td>
<p tal:condition="options/error_message | nothing" class="error-message"
tal:repeat="m options/error_message"
tal:content="structure string:$m <br/ > " />
tal:repeat="m options/error_message" tal:content="m" />
<p tal:condition="options/ok_message | nothing" class="ok-message">
<span tal:repeat="m options/ok_message"
tal:content="structure string:$m <br/ > " />
<span tal:repeat="m options/ok_message" tal:content="m" />
<a class="form-small" tal:attributes="href request/current_url"
i18n:translate="">clear this message</a>
</p>
Expand Down
6 changes: 2 additions & 4 deletions share/roundup/templates/responsive/html/page.html
Original file line number Diff line number Diff line change
Expand Up @@ -250,11 +250,9 @@
<div class="span8">
<h1><span metal:define-slot="body_title">body title</span></h1>
<p tal:condition="options/error_message | nothing" class="alert alert-error"
tal:repeat="m options/error_message"
tal:content="structure string:$m <br/ > " />
tal:repeat="m options/error_message" tal:content="m" />
<p tal:condition="options/ok_message | nothing" class="alert alert-success">
<span tal:repeat="m options/ok_message"
tal:content="structure string:$m <br/ > " />
<span tal:repeat="m options/ok_message" tal:content="m" />
<a class="form-small" tal:attributes="href request/current_url"
i18n:translate="">clear this message</a>
</p>
Expand Down
28 changes: 6 additions & 22 deletions test/test_cgi.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,29 +43,13 @@ def makeForm(args):
cm = client.clean_message
class MessageTestCase(unittest.TestCase):
# Note: We used to allow some html tags in error message. Now *only*
# newlines are allowed which are translated to <br />.
# All other tags are escaped.
# newlines are allowed and messages are split at newlines.
# Note that tags are no longer escaped, see doc/upgrading.txt for
# the changes needed in the templates (Migrating from 1.5.0 to 1.5.1)
def testCleanMessageOK(self):
self.assertEqual(cm('a\nb'), 'a<br />\nb')
self.assertEqual(cm('a\nb\nc\n'), 'a<br />\nb<br />\nc<br />\n')

def testCleanMessageBAD(self):
self.assertEqual(cm('<script>x</script>'),
'&lt;script&gt;x&lt;/script&gt;')
self.assertEqual(cm('<iframe>x</iframe>'),
'&lt;iframe&gt;x&lt;/iframe&gt;')
self.assertEqual(cm('<<script >>alert(42);5<</script >>'),
'&lt;&lt;script &gt;&gt;alert(42);5&lt;&lt;/script &gt;&gt;')
self.assertEqual(cm('<a href="y">x</a>'),
'&lt;a href="y"&gt;x&lt;/a&gt;')
self.assertEqual(cm('<A HREF="y">x</A>'),
'&lt;A HREF="y"&gt;x&lt;/A&gt;')
self.assertEqual(cm('<br>x<br />'), '&lt;br&gt;x&lt;br /&gt;')
self.assertEqual(cm('<i>x</i>'), '&lt;i&gt;x&lt;/i&gt;')
self.assertEqual(cm('<b>x</b>'), '&lt;b&gt;x&lt;/b&gt;')
self.assertEqual(cm('<BR>x<BR />'), '&lt;BR&gt;x&lt;BR /&gt;')
self.assertEqual(cm('<I>x</I>'), '&lt;I&gt;x&lt;/I&gt;')
self.assertEqual(cm('<B>x</B>'), '&lt;B&gt;x&lt;/B&gt;')
self.assertEqual(cm('a'), ['a'])
self.assertEqual(cm('a\nb'), ['a','b'])
self.assertEqual(cm('a\nb\nc\n'), ['a','b','c',''])

class FormTestCase(unittest.TestCase):
def setUp(self):
Expand Down
6 changes: 2 additions & 4 deletions website/issues/html/page.html
Original file line number Diff line number Diff line change
Expand Up @@ -185,11 +185,9 @@ <h1><a href="/">Roundup Tracker - Issues</a></h1>
<div class="content">
<h1 id="breadcrumb"><span metal:define-slot="body_title">body title</span></h1>
<p tal:condition="options/error_message | nothing" class="error-message"
tal:repeat="m options/error_message"
tal:content="structure string:$m <br/ > " />
tal:repeat="m options/error_message" tal:content="m" />
<p tal:condition="options/ok_message | nothing" class="ok-message">
<span tal:repeat="m options/ok_message"
tal:content="structure string:$m <br/ > " />
<span tal:repeat="m options/ok_message" tal:content="m" />
<a class="form-small" tal:attributes="href request/current_url"
i18n:translate="">clear this message</a>
</p>
Expand Down

0 comments on commit fd2d2d9

Please sign in to comment.