From 0de2c5584be47b04af7b389a1812a478a302dbc6 Mon Sep 17 00:00:00 2001 From: richard Date: Thu, 12 Mar 2009 02:25:03 +0000 Subject: [PATCH] Plug a number of security holes: - 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 --- CHANGES.txt | 12 ++ doc/customizing.txt | 6 + doc/upgrading.txt | 65 ++++++++++ roundup/__init__.py | 2 +- roundup/cgi/actions.py | 117 +++++++++++++----- roundup/cgi/client.py | 7 ++ roundup/cgi/templating.py | 26 +++- roundup/configuration.py | 5 + roundup/security.py | 14 +-- .../templates/classic/html/user.index.html | 10 +- share/roundup/templates/classic/schema.py | 3 + test/test_cgi.py | 8 +- 12 files changed, 227 insertions(+), 48 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 4d7db6f..0cf9de6 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -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 diff --git a/doc/customizing.txt b/doc/customizing.txt index 9ab3fbc..aac126b 100644 --- a/doc/customizing.txt +++ b/doc/customizing.txt @@ -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 diff --git a/doc/upgrading.txt b/doc/upgrading.txt index 2636b62..02a0489 100644 --- a/doc/upgrading.txt +++ b/doc/upgrading.txt @@ -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``:: + + + retire + +Should be replaced with:: + + +
+ + + +
+ + +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 91990c1..571e928 100644 --- a/roundup/__init__.py +++ b/roundup/__init__.py @@ -68,6 +68,6 @@ much prettier cake :) ''' __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 4f5a0b0..fc2d5c7 100755 --- a/roundup/cgi/actions.py +++ b/roundup/cgi/actions.py @@ -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' @@ -274,12 +281,19 @@ class EditCSVAction(Action): 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) @@ -293,25 +307,38 @@ class EditCSVAction(Action): 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 @@ -340,15 +367,21 @@ class EditCSVAction(Action): # 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() @@ -493,10 +526,8 @@ class EditCommon(Action): # 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. @@ -554,6 +585,10 @@ class EditItemAction(EditCommon): 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()) @@ -596,6 +631,10 @@ class NewItemAction(EditCommon): 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) @@ -604,6 +643,11 @@ class NewItemAction(EditCommon): % 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 @@ -773,6 +817,10 @@ class RegisterAction(RegoCommon, EditCommon): 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) @@ -887,6 +935,10 @@ class LoginAction(Action): 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')) @@ -986,7 +1038,16 @@ class ExportCSVAction(Action): # 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 dad5c8e..a33a089 100644 --- a/roundup/cgi/client.py +++ b/roundup/cgi/client.py @@ -853,6 +853,13 @@ class Client: 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 diff --git a/roundup/cgi/templating.py b/roundup/cgi/templating.py index b6fef8c..d6e6a2b 100644 --- a/roundup/cgi/templating.py +++ b/roundup/cgi/templating.py @@ -473,6 +473,14 @@ class HTMLPermissions: 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.*) @@ -497,6 +505,12 @@ class HTMLClass(HTMLInputMixin, HTMLPermissions): 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? """ @@ -761,13 +775,19 @@ class _HTMLItem(HTMLInputMixin, HTMLPermissions): 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): @@ -775,7 +795,7 @@ class _HTMLItem(HTMLInputMixin, HTMLPermissions): 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() diff --git a/roundup/configuration.py b/roundup/configuration.py index 8d03f7b..b6571b0 100644 --- a/roundup/configuration.py +++ b/roundup/configuration.py @@ -551,6 +551,11 @@ SETTINGS = ( "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 2a1e520..6138e27 100644 --- a/roundup/security.py +++ b/roundup/security.py @@ -103,15 +103,11 @@ class Security: 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 021e2b8..cdee70c 100644 --- a/share/roundup/templates/classic/html/user.index.html +++ b/share/roundup/templates/classic/html/user.index.html @@ -33,9 +33,13 @@       - - retire + +
+ + + +
diff --git a/share/roundup/templates/classic/schema.py b/share/roundup/templates/classic/schema.py index 85fa495..f756034 100644 --- a/share/roundup/templates/classic/schema.py +++ b/share/roundup/templates/classic/schema.py @@ -128,6 +128,9 @@ db.security.addPermissionToRole('User', p) 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 5929f5b..f09bace 100644 --- a/test/test_cgi.py +++ b/test/test_cgi.py @@ -85,8 +85,8 @@ class FormTestCase(unittest.TestCase): 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',) @@ -615,8 +615,8 @@ class FormTestCase(unittest.TestCase): # # 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 -- 2.30.2