backported XSS message cleaning fix [SF#757128]

Author: Richard Jones

CHANGES.txt

@@ -1,6 +1,9 @@
 This file contains the changes to the Roundup system over time. The entries
 are given with the most recent entry first.
 
+2003-07-?? 0.5.9
+- backported XSS message cleaning fix (sf bug 757128)
+
 
 2003-06-19 0.5.8
 - plugged cross-site-scripting hole (thanks Jeff Epler)

roundup/cgi/client.py

@@ -1,4 +1,4 @@
-# $Id: client.py,v 1.65.2.9 2003-06-19 23:02:32 richard Exp $
+# $Id: client.py,v 1.65.2.10 2003-06-24 03:33:56 richard Exp $
 
 __doc__ = """
 WWW request handler (also used in the stand-alone server).
@@ -47,10 +47,16 @@ def initialiseSecurity(security):
         description="User may manipulate user Roles through the web")
     security.addPermissionToRole('Admin', p)
 
-def clean_message(match, ok={'a':1,'i':1,'b':1,'br':1}):
+# used to clean messages passed through CGI variables - HTML-escape any tag
+# that isn't <a href="">, <i>, <b> and <br> (including XHTML variants) so
+# that people can't pass through nasties like <script>, <iframe>, ...
+CLEAN_MESSAGE_RE = r'(<(/?(.*?)(\s*href="[^"]")?\s*/?)>)'
+def clean_message(message, mc=re.compile(CLEAN_MESSAGE_RE, re.I)):
+    return mc.sub(clean_message_callback, message)
+def clean_message_callback(match, ok={'a':1,'i':1,'b':1,'br':1}):
     ''' Strip all non <a>,<i>,<b> and <br> tags from a string
     '''
-    if ok.has_key(match.group(2)):
+    if ok.has_key(match.group(3).lower()):
         return match.group(1)
     return '&lt;%s&gt;'%match.group(2)
 
@@ -256,8 +262,7 @@ def determine_user(self):
         # reopen the database as the correct user
         self.opendb(self.user)
 
-    def determine_context(self, dre=re.compile(r'([^\d]+)(\d+)'),
-            mc=re.compile(r'(</?(.*?)>)')):
+    def determine_context(self, dre=re.compile(r'([^\d]+)(\d+)')):
         ''' Determine the context of this page from the URL:
 
             The URL path after the instance identifier is examined. The path
@@ -339,10 +344,10 @@ def determine_context(self, dre=re.compile(r'([^\d]+)(\d+)'),
 
         # see if we were passed in a message
         if self.form.has_key(':ok_message'):
-            msg = mc.sub(clean_message, self.form[':ok_message'].value)
+            msg = clean_message(self.form[':ok_message'].value)
             self.ok_message.append(msg)
         if self.form.has_key(':error_message'):
-            msg = mc.sub(clean_message, self.form[':error_message'].value)
+            msg = clean_message(self.form[':error_message'].value)
             self.error_message.append(msg)
 
     def serve_file(self, designator, dre=re.compile(r'([^\d]+)(\d+)')):

test/test_cgi.py

@@ -8,7 +8,7 @@
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
 #
-# $Id: test_cgi.py,v 1.4.2.2 2003-03-21 21:43:04 richard Exp $
+# $Id: test_cgi.py,v 1.4.2.3 2003-06-24 03:33:56 richard Exp $
 
 import unittest, os, shutil, errno, sys, difflib, cgi
 
@@ -24,6 +24,26 @@ def makeForm(args):
             form.list.append(cgi.MiniFieldStorage(k, v))
     return form
 
+cm = client.clean_message
+class MessageTestCase(unittest.TestCase):
+    def testCleanMessageOK(self):
+        self.assertEqual(cm('<br>x<br />'), '<br>x<br />')
+        self.assertEqual(cm('<i>x</i>'), '<i>x</i>')
+        self.assertEqual(cm('<b>x</b>'), '<b>x</b>')
+        self.assertEqual(cm('<a href="y">x</a>'),
+            '<a href="y">x</a>')
+        self.assertEqual(cm('<BR>x<BR />'), '<BR>x<BR />')
+        self.assertEqual(cm('<I>x</I>'), '<I>x</I>')
+        self.assertEqual(cm('<B>x</B>'), '<B>x</B>')
+        self.assertEqual(cm('<A HREF="y">x</A>'),
+            '<A HREF="y">x</A>')
+
+    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;')
+
 class FormTestCase(unittest.TestCase):
     def setUp(self):
         self.dirname = '_test_cgi_form'
@@ -277,7 +297,9 @@ def testEmptyPasswordNOTSet(self):
 
 
 def suite():
-    l = [unittest.makeSuite(FormTestCase),
+    l = [
+        unittest.makeSuite(FormTestCase),
+        unittest.makeSuite(MessageTestCase),
     ]
     return unittest.TestSuite(l)