![[tokkee]](http://tokkee.org/images/avatar.png){kind=avatar}
summary | shortlog | log | commit | commitdiff | tree
raw | patch | inline | side by side (parent: a7e6e07)
raw | patch | inline | side by side (parent: a7e6e07)
| author | richard <richard@57a73879-2fb5-44c3-a270-3262357dd7e2> | |
| Thu, 12 Mar 2009 02:25:03 +0000 (02:25 +0000) | ||
| committer | richard <richard@57a73879-2fb5-44c3-a270-3262357dd7e2> | |
| Thu, 12 Mar 2009 02:25:03 +0000 (02:25 +0000) |
- 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.
git-svn-id: http://svn.roundup-tracker.org/svnroot/roundup/roundup/trunk@4180 57a73879-2fb5-44c3-a270-3262357dd7e2
- 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.
git-svn-id: http://svn.roundup-tracker.org/svnroot/roundup/roundup/trunk@4180 57a73879-2fb5-44c3-a270-3262357dd7e2
12 files changed:
diff --git a/CHANGES.txt b/CHANGES.txt
index 4d7db6f08f802fb26350a5f0a00923eb314c1945..0cf9de674a8f9bcd04a370cdf4b2c02649845ca1 100644 (file)
--- a/CHANGES.txt
+++ b/CHANGES.txt
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
diff --git a/doc/customizing.txt b/doc/customizing.txt
index 9ab3fbc6b0dd50e8323a6ecc77cc1988302540c8..aac126b31328a5461248733df991f7eea5471e39 100644 (file)
--- a/doc/customizing.txt
+++ b/doc/customizing.txt
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
diff --git a/doc/upgrading.txt b/doc/upgrading.txt
index 2636b6297b31dfe6d67722531dd315220b94a812..02a0489a37d015566e092eab9caa039ee072bd5f 100644 (file)
--- a/doc/upgrading.txt
+++ b/doc/upgrading.txt
.. 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
=============================
diff --git a/roundup/__init__.py b/roundup/__init__.py
index 91990c1d9a58ffb4d3116e885f893f12be7601dd..571e928938a0c9368d896cece2ef9ab9f188aa17 100644 (file)
--- a/roundup/__init__.py
+++ b/roundup/__init__.py
'''
__docformat__ = 'restructuredtext'
-__version__ = '1.4.6'
+__version__ = '1.4.7'
# vim: set filetype=python ts=4 sw=4 et si
diff --git a/roundup/cgi/actions.py b/roundup/cgi/actions.py
index 4f5a0b00c9dec3f7d41efccc67844f0efc2f60a5..fc2d5c7d236cc26fcf8c94f47a974d4d89e9bade 100755 (executable)
--- a/roundup/cgi/actions.py
+++ b/roundup/cgi/actions.py
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'
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)
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
# 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()
# 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.
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())
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)
% 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
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)
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'))
# 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'
diff --git a/roundup/cgi/client.py b/roundup/cgi/client.py
index dad5c8ecda0bb3aff80fe1d98dabb6eb4b5d195d..a33a08930178cfa7050f0b3ae920aa20c344dae5 100644 (file)
--- a/roundup/cgi/client.py
+++ b/roundup/cgi/client.py
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
index b6fef8c6639f6ddc4ae86cd2545e65d94ca4d171..d6e6a2b6aa1d5eb3022b1368edb3cc0452aaef15 100644 (file)
raise Unauthorised("edit", self._classname,
translator=self._client.translator)
+ def retire_check(self):
+ """ Raise the Unauthorised exception if the user's not permitted to
+ retire items of this class.
+ """
+ if not self.is_retire_ok():
+ raise Unauthorised("retire", self._classname,
+ translator=self._client.translator)
+
class HTMLClass(HTMLInputMixin, HTMLPermissions):
""" Accesses through a class (either through *class* or *db.<classname>*)
return self._db.security.hasPermission('Create', self._client.userid,
self._classname)
+ def is_retire_ok(self):
+ """ Is the user allowed to retire items of the current class?
+ """
+ return self._db.security.hasPermission('Retire', self._client.userid,
+ self._classname)
+
def is_view_ok(self):
""" Is the user allowed to View the current class?
"""
HTMLInputMixin.__init__(self)
def is_edit_ok(self):
- """ Is the user allowed to Edit the current class?
+ """ Is the user allowed to Edit this item?
"""
return self._db.security.hasPermission('Edit', self._client.userid,
self._classname, itemid=self._nodeid)
+ def is_retire_ok(self):
+ """ Is the user allowed to Reture this item?
+ """
+ return self._db.security.hasPermission('Retire', self._client.userid,
+ self._classname, itemid=self._nodeid)
+
def is_view_ok(self):
- """ Is the user allowed to View the current class?
+ """ Is the user allowed to View this item?
"""
if self._db.security.hasPermission('View', self._client.userid,
self._classname, itemid=self._nodeid):
return self.is_edit_ok()
def is_only_view_ok(self):
- """ Is the user only allowed to View (ie. not Edit) the current class?
+ """ Is the user only allowed to View (ie. not Edit) this item?
"""
return self.is_view_ok() and not self.is_edit_ok()
index 8d03f7bb3e299752864a3c37e96f57ee99287130..b6571b01449fc8ac1ae5da635a5b9faf0bbd88e4 100644 (file)
--- a/roundup/configuration.py
+++ b/roundup/configuration.py
"or LANG, in that order of preference."),
)),
("web", (
+ (BooleanOption, "allow_html_file", "no",
+ "Setting this option enables Roundup to serve uploaded HTML\n"
+ "file content *as HTML*. This is a potential security risk\n"
+ "and is therefore disabled by default. Set to 'yes' if you\n"
+ "trust *all* users uploading content to your tracker."),
(BooleanOption, 'http_auth', "yes",
"Whether to use HTTP Basic Authentication, if present.\n"
"Roundup will use either the REMOTE_USER or HTTP_AUTHORIZATION\n"
diff --git a/roundup/security.py b/roundup/security.py
index 2a1e520caede8f84eaa53494afc8246bfba43102..6138e271a1ab430ddb9f01d7bf084ada4c3d75b4 100644 (file)
--- a/roundup/security.py
+++ b/roundup/security.py
self.addRole(name="Admin", description="An admin user, full privs")
self.addRole(name="Anonymous", description="An anonymous user")
- ce = self.addPermission(name="Create",
- description="User may create everthing")
- self.addPermissionToRole('Admin', ce)
- ee = self.addPermission(name="Edit",
- description="User may edit everthing")
- self.addPermissionToRole('Admin', ee)
- ae = self.addPermission(name="View",
- description="User may access everything")
- self.addPermissionToRole('Admin', ae)
+ # default permissions - Admin may do anything
+ for p in 'create edit retire view'.split():
+ p = self.addPermission(name=p.title(),
+ description="User may %s everthing"%p)
+ self.addPermissionToRole('Admin', p)
# initialise the permissions and roles needed for the UIs
from roundup.cgi import client
diff --git a/share/roundup/templates/classic/html/user.index.html b/share/roundup/templates/classic/html/user.index.html
index 021e2b808a9f31423a9cb1a3e0a014bf044ffd9b..cdee70cc59914b8b44bc2bf15f0afa0b9a15dd6d 100644 (file)
<td tal:content="python:user.organisation.plain() or default"> </td>
<td tal:content="python:user.address.email() or default"> </td>
<td tal:content="python:user.phone.plain() or default"> </td>
- <td tal:condition="context/is_edit_ok">
- <a tal:attributes="href string:user${user/id}?@action=retire&@template=index"
- i18n:translate="">retire</a>
+ <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>
</td>
</tr>
</tal:block>
index 85fa49526167cc5a4a225e2920c76e5baceaa3e2..f7560348aed052183f6248ec69c979460c9d9df4 100644 (file)
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)
diff --git a/test/test_cgi.py b/test/test_cgi.py
index 5929f5b323c470683918d34ad32c3cf7f00a2da9..f09bace2da5e87c566051196b7243a36b05d8aae 100644 (file)
--- a/test/test_cgi.py
+++ b/test/test_cgi.py
re.VERBOSE)
def parseForm(self, form, classname='test', nodeid=None):
- cl = client.Client(self.instance, None, {'PATH_INFO':'/'},
- makeForm(form))
+ cl = client.Client(self.instance, None, {'PATH_INFO':'/',
+ 'REQUEST_METHOD':'POST'}, makeForm(form))
cl.classname = classname
cl.nodeid = nodeid
cl.language = ('en',)
#
# XXX test all default permissions
def _make_client(self, form, classname='user', nodeid='2', userid='2'):
- cl = client.Client(self.instance, None, {'PATH_INFO':'/'},
- makeForm(form))
+ cl = client.Client(self.instance, None, {'PATH_INFO':'/',
+ 'REQUEST_METHOD':'POST'}, makeForm(form))
cl.classname = 'user'
cl.nodeid = '1'
cl.db = self.db