Skip to content

Commit

Permalink
Plug a number of security holes:
Browse files Browse the repository at this point in the history
- EditCSV and ExportCSV altered to include permission checks
- HTTP POST required on actions which alter data
- HTML file uploads served as application/octet-stream
- New item action reject creation of new users
- Item retirement was not being controlled

Additionally include documentation of the changes and modify affected tests.
  • Loading branch information
Richard Jones committed Mar 12, 2009
1 parent 8295039 commit 27ef29f
Show file tree
Hide file tree
Showing 12 changed files with 227 additions and 48 deletions.
12 changes: 12 additions & 0 deletions CHANGES.txt
Original file line number Diff line number Diff line change
@@ -1,6 +1,18 @@
This file contains the changes to the Roundup system over time. The entries
are given with the most recent entry first.

2009-03-?? 1.4.7

Fixes:
- a number of security issues were discovered by Daniel Diniz
- EditCSV and ExportCSV altered to include permission checks
- HTTP POST required on actions which alter data
- HTML file uploads served as application/octet-stream
- New item action reject creation of new users
- Item retirement was not being controlled
- XXX need to include Stefan's changes in here too


2008-09-01 1.4.6
Fixed:
- Fix bug introduced in 1.4.5 in RDBMS full-text indexing
Expand Down
6 changes: 6 additions & 0 deletions doc/customizing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,12 @@ Section **tracker**
LC_MESSAGES, or LANG, in that order of preference.

Section **web**
allow_html_file -- ``no``
Setting this option enables Roundup to serve uploaded HTML
file content *as HTML*. This is a potential security risk
and is therefore disabled by default. Set to 'yes' if you
trust *all* users uploading content to your tracker.

http_auth -- ``yes``
Whether to use HTTP Basic Authentication, if present.
Roundup will use either the REMOTE_USER or HTTP_AUTHORIZATION
Expand Down
65 changes: 65 additions & 0 deletions doc/upgrading.txt
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,71 @@ steps.

.. contents::


Migrating from 1.4.x to 1.4.7
=============================

Several security issues were addressed in this release. Some aspects of your
trackers may no longer function depending on your local customisations. Core
functionality that will need to be modified:

Grant the "retire" permission to users for their queries
--------------------------------------------------------

Users will no longer be able to retire their own queries. To remedy this you
will need to add the following to your tracker's ``schema.py`` just under the
line that grants them permission to edit their own queries::

p = db.security.addPermission(name='Edit', klass='query', check=edit_query,
description="User is allowed to edit their queries")
db.security.addPermissionToRole('User', p)
+ p = db.security.addPermission(name='Retire', klass='query', check=edit_query,
+ description="User is allowed to retire their queries")
+ db.security.addPermissionToRole('User', p)
p = db.security.addPermission(name='Create', klass='query',
description="User is allowed to create queries")
db.security.addPermissionToRole('User', p)

The lines marked "+" should be added, minus the "+" sign.


Fix the "retire" link in the users list for admin users
-------------------------------------------------------

The "retire" link found in the file ``html/users.index.html``::

<td tal:condition="context/is_edit_ok">
<a tal:attributes="href string:user${user/id}?@action=retire&@template=index"
i18n:translate="">retire</a>

Should be replaced with::

<td tal:condition="context/is_retire_ok">
<form style="padding:0"
tal:attributes="action string:user${user/id}">
<input type="hidden" name="@template" value="index">
<input type="hidden" name="@action" value="retire">
<input type="submit" value="retire" i18n:attributes="value">
</form>


Trackers currently allowing HTML file uploading
-----------------------------------------------

Trackers which wish to continue to allow uploading of HTML content against issues
will need to set a new configuration variable in the ``[web]`` section of the
tracker's ``config.ini`` file:

# Setting this option enables Roundup to serve uploaded HTML
# file content *as HTML*. This is a potential security risk
# and is therefore disabled by default. Set to 'yes' if you
# trust *all* users uploading content to your tracker.
# Allowed values: yes, no
# Default: no
allow_html_file = no



Migrating from 1.4.2 to 1.4.3
=============================

Expand Down
2 changes: 1 addition & 1 deletion roundup/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,6 @@
'''
__docformat__ = 'restructuredtext'

__version__ = '1.4.6'
__version__ = '1.4.7'

# vim: set filetype=python ts=4 sw=4 et si
117 changes: 89 additions & 28 deletions roundup/cgi/actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,30 +103,37 @@ class RetireAction(Action):

def handle(self):
"""Retire the context item."""
# if we want to view the index template now, then unset the nodeid
# ensure modification comes via POST
if self.client.env['REQUEST_METHOD'] != 'POST':
self.client.error_message.append(self._('Invalid request'))

# if we want to view the index template now, then unset the itemid
# context info (a special-case for retire actions on the index page)
nodeid = self.nodeid
itemid = self.nodeid
if self.template == 'index':
self.client.nodeid = None

# make sure we don't try to retire admin or anonymous
if self.classname == 'user' and \
self.db.user.get(nodeid, 'username') in ('admin', 'anonymous'):
self.db.user.get(itemid, 'username') in ('admin', 'anonymous'):
raise ValueError, self._(
'You may not retire the admin or anonymous user')

# check permission
if not self.hasPermission('Retire', classname=self.classname,
itemid=itemid):
raise exceptions.Unauthorised, self._(
'You do not have permission to retire %(class)s'
) % {'class': self.classname}

# do the retire
self.db.getclass(self.classname).retire(nodeid)
self.db.getclass(self.classname).retire(itemid)
self.db.commit()

self.client.ok_message.append(
self._('%(classname)s %(itemid)s has been retired')%{
'classname': self.classname.capitalize(), 'itemid': nodeid})
'classname': self.classname.capitalize(), 'itemid': itemid})

def hasPermission(self, permission, classname=Action._marker, itemid=None):
if itemid is None:
itemid = self.nodeid
return Action.hasPermission(self, permission, classname, itemid)

class SearchAction(Action):
name = 'search'
Expand Down Expand Up @@ -274,12 +281,19 @@ def handle(self):
The "rows" CGI var defines the CSV-formatted entries for the class. New
nodes are identified by the ID 'X' (or any other non-existent ID) and
removed lines are retired.
"""
# ensure modification comes via POST
if self.client.env['REQUEST_METHOD'] != 'POST':
self.client.error_message.append(self._('Invalid request'))

# figure the properties list for the class
cl = self.db.classes[self.classname]
idlessprops = cl.getprops(protected=0).keys()
idlessprops.sort()
props = ['id'] + idlessprops
props_without_id = cl.getprops(protected=0).keys()

# the incoming CSV data will always have the properties in colums
# sorted and starting with the "id" column
props_without_id.sort()
props = ['id'] + props_without_id

# do the edit
rows = StringIO.StringIO(self.form['rows'].value)
Expand All @@ -293,25 +307,38 @@ def handle(self):
if values == props:
continue

# extract the nodeid
nodeid, values = values[0], values[1:]
found[nodeid] = 1
# extract the itemid
itemid, values = values[0], values[1:]
found[itemid] = 1

# see if the node exists
if nodeid in ('x', 'X') or not cl.hasnode(nodeid):
if itemid in ('x', 'X') or not cl.hasnode(itemid):
exists = 0

# check permission to create this item
if not self.hasPermission('Create', classname=self.classname):
raise exceptions.Unauthorised, self._(
'You do not have permission to create %(class)s'
) % {'class': self.classname}
else:
exists = 1

# confirm correct weight
if len(idlessprops) != len(values):
if len(props_without_id) != len(values):
self.client.error_message.append(
self._('Not enough values on line %(line)s')%{'line':line})
return

# extract the new values
d = {}
for name, value in zip(idlessprops, values):
for name, value in zip(props_without_id, values):
# check permission to edit this property on this item
if exists and not self.hasPermission('Edit', itemid=itemid,
classname=self.classname, property=name):
raise exceptions.Unauthorised, self._(
'You do not have permission to edit %(class)s'
) % {'class': self.classname}

prop = cl.properties[name]
value = value.strip()
# only add the property if it has a value
Expand Down Expand Up @@ -340,15 +367,21 @@ def handle(self):
# perform the edit
if exists:
# edit existing
cl.set(nodeid, **d)
cl.set(itemid, **d)
else:
# new node
found[cl.create(**d)] = 1

# retire the removed entries
for nodeid in cl.list():
if not found.has_key(nodeid):
cl.retire(nodeid)
for itemid in cl.list():
if not found.has_key(itemid):
# check permission to retire this item
if not self.hasPermission('Retire', itemid=itemid,
classname=self.classname):
raise exceptions.Unauthorised, self._(
'You do not have permission to retire %(class)s'
) % {'class': self.classname}
cl.retire(itemid)

# all OK
self.db.commit()
Expand Down Expand Up @@ -493,10 +526,8 @@ def editItemPermission(self, props, classname=_cn_marker, itemid=None):
# The user must have permission to edit each of the properties
# being changed.
for p in props:
if not self.hasPermission('Edit',
itemid=itemid,
classname=classname,
property=p):
if not self.hasPermission('Edit', itemid=itemid,
classname=classname, property=p):
return 0
# Since the user has permission to edit all of the properties,
# the edit is OK.
Expand Down Expand Up @@ -554,6 +585,10 @@ def handle(self):
See parsePropsFromForm and _editnodes for special variables.
"""
# ensure modification comes via POST
if self.client.env['REQUEST_METHOD'] != 'POST':
self.client.error_message.append(self._('Invalid request'))

user_activity = self.lastUserActivity()
if user_activity:
props = self.detectCollision(user_activity, self.lastNodeActivity())
Expand Down Expand Up @@ -596,6 +631,10 @@ def handle(self):
This follows the same form as the EditItemAction, with the same
special form values.
'''
# ensure modification comes via POST
if self.client.env['REQUEST_METHOD'] != 'POST':
self.client.error_message.append(self._('Invalid request'))

# parse the props from the form
try:
props, links = self.client.parsePropsFromForm(create=1)
Expand All @@ -604,6 +643,11 @@ def handle(self):
% str(message))
return

# guard against new user creation that would bypass security checks
for key in props:
if 'user' in key:
return

# handle the props - edit or create
try:
# when it hits the None element, it'll set self.nodeid
Expand Down Expand Up @@ -773,6 +817,10 @@ def handle(self):
Return 1 on successful login.
"""
# ensure modification comes via POST
if self.client.env['REQUEST_METHOD'] != 'POST':
self.client.error_message.append(self._('Invalid request'))

# parse the props from the form
try:
props, links = self.client.parsePropsFromForm(create=1)
Expand Down Expand Up @@ -887,6 +935,10 @@ def handle(self):
Sets up a session for the user which contains the login credentials.
"""
# ensure modification comes via POST
if self.client.env['REQUEST_METHOD'] != 'POST':
self.client.error_message.append(self._('Invalid request'))

# we need the username at a minimum
if not self.form.has_key('__login_name'):
self.client.error_message.append(self._('Username required'))
Expand Down Expand Up @@ -986,7 +1038,16 @@ def handle(self):

# and search
for itemid in klass.filter(matches, filterspec, sort, group):
self.client._socket_op(writer.writerow, [str(klass.get(itemid, col)) for col in columns])
row = []
for name in columns:
# check permission to view this property on this item
if exists and not self.hasPermission('View', itemid=itemid,
classname=request.classname, property=name):
raise exceptions.Unauthorised, self._(
'You do not have permission to view %(class)s'
) % {'class': request.classname}
row.append(str(klass.get(itemid, name)))
self.client._socket_op(writer.writerow, row)

return '\n'

Expand Down
7 changes: 7 additions & 0 deletions roundup/cgi/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -853,6 +853,13 @@ def serve_file(self, designator, dre=re.compile(r'([^\d]+)(\d+)')):

mime_type = klass.get(nodeid, 'type')

# if the mime_type is HTML-ish then make sure we're allowed to serve up
# HTML-ish content
if mime_type in ('text/html', 'text/x-html'):
if not self.instance.config['WEB_ALLOW_HTML_FILE']:
# do NOT serve the content up as HTML
mime_type = 'application/octet-stream'

# If this object is a file (i.e., an instance of FileClass),
# see if we can find it in the filesystem. If so, we may be
# able to use the more-efficient request.sendfile method of
Expand Down
Loading

0 comments on commit 27ef29f

Please sign in to comment.