Different approach to fix XSS in issue2550817

Encapsulate the error/ok message append method as add_ok_message and
add_error_message. The new approach escapes the messages when appending
-- at a point in the code where we still know where the message comes
from. Escaping is the default but can bei turned off. This also fixes
issue2550836 where certain messages may contain links.

Another advantage of the new fix is that users don't need to change
installed trackers and are secure by default.

Author: schlatterbeck

CHANGES.txt

@@ -11,10 +11,11 @@ Entries without name were done by Richard Jones.
 
 Pay attention:
 
-  This release includes *important change affecting security*. Since
-  this version escaping now happens in the template and not in the
-  roundup code. Please read doc/upgrading.txt on how to change your
-  templates. Without this you are vulnerable. (Ralf Schlatterbeck)
+  If you have installed an intermediate version from our version control
+  system and have modified your tracker instance to escape OK and
+  error-messages in the HTML templates you need to revert this change.
+  If you're upgrading from a previous roundup release version
+  you should look into ``doc/upgrading.txt``.  (Ralf Schlatterbeck)
 
 Features:
 
@@ -72,18 +73,18 @@ Fixed:
   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. Thanks to Thibault Fevry for the original
+  templates we suggest is a *lot* safer as it by default escapes the
+  error and ok messages now. Thanks to Thibault Fevry for the original
   bug-report.
 - issue2117897: Fixed two more places in date.py where seconds can be
   rounded to 60.0 and causing exceptions. Change them to 59.999 as was
   done in the fix for issue2550802. (Thomas Arendsen Hein)
 - Fix batch.propchanged for transitive id properties (would result in a
   backtrace when trying to group by property.id)
-- Fix issue2550835 which tests for date-range queries with an interval
-  that depends on the local time. Put the queried date a little later to
-  avoid a race condition where the queried interval doesn't match the
-  date because the clock has advanced. (Ralf Schlatterbeck)
+- Fix issue2550835, the test checks for date-range queries with an
+  interval that depends on the local time. Put the queried date a little
+  later to avoid a race condition where the queried interval doesn't
+  match the date because the clock has advanced. (Ralf Schlatterbeck)
 
 Minor:
 - demo.py usage message improved: explains "nuke" now. (Bernhard Reiter)
@@ -335,10 +336,10 @@ Fixed:
   issue2550689, but is untested if this really works in browsers.
   Thanks to Joseph Myers for reporting. (Ralf)
 - Fix another XSS with the ok- and error message, see issue2550724. We
-  solve this differently from the proposals in the bug-report by not
-  allowing *any* html-tags in ok/error messages anymore. Thanks to
-  David Benjamin for the bug-report and to Ezio Melotti for several
-  proposed fixes. (Ralf)
+  now escape messages when added to the list so we can decide whether to
+  escape a message individually for each message. The default is to
+  escape. Thanks to David Benjamin for the bug-report and to Ezio
+  Melotti for several proposed fixes. (Ralf)
 
 
 2011-07-15: 1.4.19

doc/customizing.txt

@@ -2958,7 +2958,9 @@ See the docstring of that class for details of what it can do.
 The method will typically check the ``self.form`` variable's contents.
 It may then:
 
-- add information to ``self.client.ok_message`` or ``self.client.error_message``
+- add information to ``self.client._ok_message``
+  or ``self.client._error_message`` (by using ``self.client.add_ok_message``
+  or ``self.client.add_error_message``, respectively)
 - change the ``self.client.template`` variable to alter what the user will see
   next
 - raise Unauthorised, SendStaticFile, SendFile, NotFound or Redirect
@@ -4993,7 +4995,7 @@ Setting up a "wizard" (or "druid") for controlled adding of issues
             '''
             category = self.form['category'].value
             if category == '-1':
-                self.client.error_message.append('You must select a category of report')
+                self.client.add_error_message('You must select a category of report')
                 return
             # everything's ok, move on to the next page
             self.client.template = 'add_page2'

doc/upgrading.txt

@@ -16,43 +16,27 @@ steps.
 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.
+If you have defined your own cgi actions in your tracker instance
+(e.g. in a custom ``extensions/spambayes.py`` file) you need to modify
+all cases where client.error_message or client.ok_message are modified
+directly. Instead of::
 
+  self.client.ok_message.append(...)
+
+you need to call::
+
+  self.client.add_ok_message(...)
+
+and the same for::
+
+  self.client.error_message.append(...)
+
+vs.::
+
+  self.client.add_error_message(...)
+
+The new calls escape the passed string by default and avoid XSS security
+issues.
 
 Migrating from 1.4.20 to 1.4.21
 ===============================

roundup/cgi/actions.py

@@ -131,7 +131,7 @@ def handle(self):
         self.db.getclass(self.classname).retire(itemid)
         self.db.commit()
 
-        self.client.ok_message.append(
+        self.client.add_ok_message(
             self._('%(classname)s %(itemid)s has been retired')%{
                 'classname': self.classname.capitalize(), 'itemid': itemid})
 
@@ -331,7 +331,7 @@ def handle(self):
 
             # confirm correct weight
             if len(props_without_id) != len(values):
-                self.client.error_message.append(
+                self.client.add_error_message(
                     self._('Not enough values on line %(line)s')%{'line':line})
                 return
 
@@ -392,7 +392,7 @@ def handle(self):
         # all OK
         self.db.commit()
 
-        self.client.ok_message.append(self._('Items edited OK'))
+        self.client.add_ok_message(self._('Items edited OK'))
 
 class EditCommon(Action):
     '''Utility methods for editing.'''
@@ -593,7 +593,7 @@ def handleCollision(self, props):
             'View <a target="new" href="%s%s">their changes</a> '
             'in a new window.')%(self.classname, ', '.join(props),
             self.classname, self.nodeid)
-        self.client.error_message.append(message)
+        self.client.add_error_message(message, escape=False)
         return
 
     def handle(self):
@@ -620,7 +620,7 @@ def handle(self):
             message = self._editnodes(props, links)
         except (ValueError, KeyError, IndexError,
                 roundup.exceptions.Reject), message:
-            self.client.error_message.append(
+            self.client.add_error_message(
                 self._('Edit Error: %s') % str(message))
             return
 
@@ -656,7 +656,7 @@ def handle(self):
         try:
             props, links = self.client.parsePropsFromForm(create=1)
         except (ValueError, KeyError), message:
-            self.client.error_message.append(self._('Error: %s')
+            self.client.add_error_message(self._('Error: %s')
                 % str(message))
             return
 
@@ -667,7 +667,7 @@ def handle(self):
         except (ValueError, KeyError, IndexError,
                 roundup.exceptions.Reject), message:
             # these errors might just be indicative of user dumbness
-            self.client.error_message.append(_('Error: %s') % str(message))
+            self.client.add_error_message(_('Error: %s') % str(message))
             return
 
         # commit now that all the tricky stuff is done
@@ -692,7 +692,7 @@ def handle(self):
             otk = self.form['otk'].value
             uid = otks.get(otk, 'uid', default=None)
             if uid is None:
-                self.client.error_message.append(
+                self.client.add_error_message(
                     self._("Invalid One Time Key!\n"
                         "(a Mozilla bug may cause this message "
                         "to show up erroneously, please check your email)"))
@@ -716,7 +716,7 @@ def handle(self):
                 otks.destroy(otk)
                 self.db.commit()
             except (ValueError, KeyError), message:
-                self.client.error_message.append(str(message))
+                self.client.add_error_message(str(message))
                 return
 
             # user info
@@ -734,7 +734,7 @@ def handle(self):
             if not self.client.standard_message([address], subject, body):
                 return
 
-            self.client.ok_message.append(
+            self.client.add_ok_message(
                 self._('Password reset and email sent to %s') % address)
             return
 
@@ -744,19 +744,19 @@ def handle(self):
             try:
                 uid = self.db.user.lookup(name)
             except KeyError:
-                self.client.error_message.append(self._('Unknown username'))
+                self.client.add_error_message(self._('Unknown username'))
                 return
             address = self.db.user.get(uid, 'address')
         elif 'address' in self.form:
             address = self.form['address'].value
             uid = uidFromAddress(self.db, ('', address), create=0)
             if not uid:
-                self.client.error_message.append(
+                self.client.add_error_message(
                     self._('Unknown email address'))
                 return
             name = self.db.user.get(uid, 'username')
         else:
-            self.client.error_message.append(
+            self.client.add_error_message(
                 self._('You need to specify a username or address'))
             return
 
@@ -782,7 +782,7 @@ def handle(self):
         if not self.client.standard_message([address], subject, body):
             return
 
-        self.client.ok_message.append(self._('Email sent to %s') % address)
+        self.client.add_ok_message(self._('Email sent to %s') % address)
 
 class RegoCommon(Action):
     def finishRego(self):
@@ -815,7 +815,7 @@ def handle(self):
             # pull the rego information out of the otk database
             self.userid = self.db.confirm_registration(self.form['otk'].value)
         except (ValueError, KeyError), message:
-            self.client.error_message.append(str(message))
+            self.client.add_error_message(str(message))
             return
         return self.finishRego()
 
@@ -837,7 +837,7 @@ def handle(self):
         try:
             props, links = self.client.parsePropsFromForm(create=1)
         except (ValueError, KeyError), message:
-            self.client.error_message.append(self._('Error: %s')
+            self.client.add_error_message(self._('Error: %s')
                 % str(message))
             return
 
@@ -850,7 +850,7 @@ def handle(self):
             except (ValueError, KeyError, IndexError,
                     roundup.exceptions.Reject), message:
                 # these errors might just be indicative of user dumbness
-                self.client.error_message.append(_('Error: %s') % str(message))
+                self.client.add_error_message(_('Error: %s') % str(message))
                 return
 
             # fix up the initial roles
@@ -938,7 +938,7 @@ def handle(self):
         self.client.session_api.destroy()
 
         # Let the user know what's going on
-        self.client.ok_message.append(self._('You are logged out'))
+        self.client.add_ok_message(self._('You are logged out'))
 
         # reset client context to render tracker home page
         # instead of last viewed page (may be inaccessibe for anonymous)
@@ -959,7 +959,7 @@ def handle(self):
 
         # we need the username at a minimum
         if '__login_name' not in self.form:
-            self.client.error_message.append(self._('Username required'))
+            self.client.add_error_message(self._('Username required'))
             return
 
         # get the login info
@@ -973,7 +973,8 @@ def handle(self):
             self.verifyLogin(self.client.user, password)
         except exceptions.LoginError, err:
             self.client.make_user_anonymous()
-            self.client.error_message.extend(list(err.args))
+            for arg in err.args:
+                self.client.add_error_message(arg)
             return
 
         # now we're OK, re-open the database for real, using the user