Possibilities of SQL injection and cross-site scripting

[Jun Omae]

DB-API with passing parameters and tag builder should be used. See ​trac:TracDev/DatabaseApi#Parameterpassing and ​trac:TracDev/PortingFromGenshiToJinja#tag.

Locations:

• tracchecklistmacro/0.1/checklist/macros.py@18464:69-72,190-192,268#L46​
• tracchecklistmacro/0.1/checklist/macros.py@18464:128,142#L128​

[Ralph Ewig]

Good catch, thank you for reporting this (and the documentation link).

[Ralph Ewig]

Fixed in version 0.1.6: tracchecklistmacro?rev=18503​

FIXED updated all sql to follow TracDev Guidelines
DONE used tag constructors (in some places)

The query changes might help with cross-db compatibility, but I've only tested using MariaDB/MySQL as the backend.

The tag constructors require generating html from the "inside-out", rather than "top-down" which was awkward in places. Since their use is not security related (as I understand it), I only used them were they made the code more convenient.

[Jun Omae]

It seem that XSS attack is able via name parameter at source:/tracchecklistmacro/0.1/checklist/macros.py@18503#L131​

-        head  = "<form class='checklist' method='post' action='"+dest+"'> \n"
-        head += "   <div class='source'>                                  \n"
-        head += ("       TracChecklist: <a href='" + formatter.req.href("/wiki/" + name) +
-                 "'>" + name + "</a>\n")
-        head += "   </div>"
+        head = tag.form(class_='checklist', method='post', action=dest)
+        head.append(tag.div('TracChecklist: ',
+                            tag.a(name, href=formatter.req.href('wiki', name)),
+                            class_='source'))
+        head = unicode(head)
         # template footer / form
         backpath   = formatter.req.href(formatter.req.path_info)
         form_token = formatter.req.form_token
         realm      = str(formatter.resource.realm)
         resource   = str(formatter.resource.id   )
         checklist  = str(content                 )
-        foot  = Markup(Element('input', type='hidden', name='__backpath__', value=backpath         ))
-        foot += Markup(Element('input', type='hidden', name='__FORM_TOKEN', value=form_token       ))
-        foot += Markup(Element('input', type='hidden', name='realm'       , value=realm            ))
-        foot += Markup(Element('input', type='hidden', name='resource'    , value=resource         ))
-        foot += Markup(Element('input', type='hidden', name='checklist'   , value=checklist        ))
-        foot += Markup(Element('input', type='submit', class_='button'    , value='Save Checklist' ))
-        foot += "</form>"
+        foot = unicode(tag(
+            tag.input(type='hidden', name='__backpath__', value=backpath         ),
+            tag.input(type='hidden', name='__FORM_TOKEN', value=form_token       ),
+            tag.input(type='hidden', name='realm'       , value=realm            ),
+            tag.input(type='hidden', name='resource'    , value=resource         ),
+            tag.input(type='hidden', name='checklist'   , value=checklist        ),
+            tag.input(type='submit', class_='button'    , value='Save Checklist' ),
+        ))

[Ralph Ewig]

How would you exploit that though? Not that I don't believe you, just trying to understand. From what I can tell, both methods (using the tag constructor vs. string coding) result in the same source for the generated page, so I'm not sure how one is vulnerable but not the other?

[Jun Omae]

1. Create wiki page TracChecklist/<a href="javascript:alert(42)">XSS</a> with [[Checklist(<a href="javascript:alert(42)">XSS</a>)]].
   1. Visit SandBox page, add [wiki:'TracChecklist/<a href="javascript:alert(42)">XSS</a>'] to the page, and save it.
   2. Click the added link to TracChecklist/<a href="javascript:alert(42)">XSS</a> page.
   3. Click "Create this page" button
   4. Store [[Checklist(<a href="javascript:alert(42)">XSS</a>)]] in the page.
2. Click XSS

(Edit: The page content is revised)

[Jun Omae]

Another thing, Option usage in the plugin is wrong.

checklist/macros.py

@@ -38 +38 @@
     _description = "Inserts a checklist sourced from a wiki page template."
     implements(ITemplateProvider)
 
+    template_root = Option(
+        'checklist', 'template_root', default='TracChecklist',
+        doc="wiki page root for checklist templates")
+
     def get_htdocs_dirs(self):
         return [('checklist', resource_filename(__name__, 'htdocs'))]
 
@@ -84 +88 @@
                 check_dict = json.loads(row[0])
 
         # get the template wiki page (create if needed)
-        root = Option('checklist', 'template_root', default='TracChecklist',
-                    doc="wiki page root for checklist templates")
-
-        root = self.config.get('checklist', 'template_root')
+        root = self.template_root
         name = str(root) + '/' + content
         name = name.replace('//','/')
         page = WikiPage(self.env, name)
@@ -262 +263 @@
 
             # list of available checklist templates (pages) under root wiki page
             templates = []
-            root  = Option('checklist', 'template_root', default='TracChecklist',
-                            doc="wiki page root for checklist templates")
-            root  = self.config.get('checklist', 'template_root')
+            root  = self.template_root
             query = "SELECT name FROM wiki WHERE version = 1 AND name LIKE %s"
             param = ['%'+root+'%']
 

[Ralph Ewig]

Super helpful - thank you. The tag.append() method is great, but I'm unsure how to append the raw html from the converted wiki page? Right now I have:

html = format_to_html(self.env, formatter.context, wiki)
html = ... (replace '[x]' with '<input type=checkbox> ...'

head = ... (some html)
foot = ... (some html)

out  = unescape(head + html + foot)

So, I tried to construct the form instead using:

html = format_to_html(self.env, formatter.context, wiki)
html = ... (replace '[x]' with '<input type=checkbox> ...'

form = tag.form (...
head = tag.div(...
foot = tag(...

form.append(head)
form.append(html)
form.append(foot)
out = unicode(form)

But this strips the checkbox functionality from the converted wiki page?

[Jun Omae]

But this strips the checkbox functionality from the converted wiki page?

I'm afraid what do "this strips the checkbox functionality" mean? It seems that work like the following. Please explain it.

>>> from trac.test import EnvironmentStub
>>> from trac.wiki.formatter import format_to_html
>>> from trac.web.chrome import web_context
>>> from trac.test import MockRequest
>>> from trac.util.html import tag
>>> env = EnvironmentStub()
>>> req = MockRequest(env)
>>> context = web_context(req)
>>> rv = format_to_html(env, context, '[x] text')
>>> rv
Markup(u'<p>\n[x] text\n</p>\n')
>>> rv = rv.replace('[x]', tag.input(type='checkbox'))
>>> rv
Markup(u'<p>\n<input type="checkbox" /> text\n</p>\n')
>>> form = tag.form()
>>> form.append(rv)
>>> unicode(form)
u'<form><p>\n<input type="checkbox" /> text\n</p>\n</form>'

[Ralph Ewig]

The way the plugin works, a user first creates a checklist template as a wiki page ("edit mode") and that template is then reused to create the clickable checklist in many tickets ("live mode"). In the wiki page (template), a "step" to be checked off later is identified with a pattern of [x] some description \\.

To generate the page for "live mode", I do the following steps:

1. load the wiki page (template) from the database
2. convert page to html using format_to_html(self.env, formatter.context, wiki)
3. replace string pattern [x] some description \\ with checkbox code <input ...>
4. wrap the result in a form, and insert it in the ticket display

When I use tag constructors for step 4, the part I'm struggling with is how to keep the html formatting (step 2) and check boxes (step 3), but I think your example above can do that (I'll try it).

[Jun Omae]

The procedure which replaces [x] markers in html with checkboxes is buggy. If forgetting \\ at end of line, it is busy loop and eats huge memory (as the results, oom-killer is called).

It could be simplified using re.sub().

checklist/macros.py

@@ -3 +3 @@
 # Macro that pulls a checklist template from a wiki page, and renders it as
 # a checklist in a ticket description.
 #
-import json, time
+import json, re, time
 import pkg_resources
 from pkg_resources import resource_filename
 
@@ -96 +96 @@
             return("CHECKLIST ERROR: \n template '" + name + "' does not exist.")
 
         # strip wiki page header & convert to html
-        wiki = page.text[page.text.find('----')+4:]
+        wiki = page.text
+        sep = wiki.find('----')
+        if sep != -1:
+            wiki = wiki[sep + 4:]
         html = format_to_html(self.env, formatter.context, wiki)
 
         # parse template for steps
-        # steps start with [x] and end with \\, which converts to <br />
-        check_index = 0
-        check_start = html.find('[x]')
+        cbox_re = re.compile(r'\s*\[x\]([^<]+)(<br\s*/?>)?', re.DOTALL)
+        cbox_idx = [0]
 
-        while check_start > -1:
-            check_end   = html.find('<br />',check_start)+6
-            check_step  = 'step_' + str(check_index)
-            check_label = html[check_start+3:check_end]
+        def cbox_repl(match):
+            label = match.group(1)
+            cbox_name = 'step_%d' % cbox_idx[0]
+            cbox_idx[0] += 1
+            br = match.group(2)
+            return (u'<label><input type="checkbox" name="{}" value="done" />'
+                    u'{}</label>{}').format(cbox_name, label, br)
 
-            check_box   = ("<label class='container'>" + check_label +
-                           "<input type='checkbox' " +
-                           "name='step_" + str(check_index) + "' " +
-                           "value='done' ")
+        html = cbox_re.sub(cbox_repl, html)
 
-            if check_step in check_dict:
-                check_box += "checked"
-
-            check_box += "><span class='checkmark'></span></label>"
-            html = html[0:check_start] + check_box + html[check_end:]
-
-            check_index +=1
-            check_start = html.find('[x]')
-
         # template header info + form
         dest  = formatter.req.href('/checklist/update')
         head  = "<form class='checklist' method='post' action='"+dest+"'> \n"

[Ralph Ewig]

I think I fixed it in v0.1.7. Also improved the conversion of the wiki checklist template into the html checklist instance to make it more robust. It uploaded without error to PyPi and svn, but doesn't show yet in trac-hacks for some reason.

[Jun Omae]

Replying to Ralph Ewig:

It uploaded without error to PyPi and svn, but doesn't show yet in trac-hacks for some reason.

The issue has been fixed by Ryan.

[Ralph Ewig]

Great! I just uploaded v0.1.8 which should have everything linted/formatted/spellchecked and a couple more small tweaks to make the code more robust. I'll close this as fixed, but please let me know if anything else needs attention.