Improvements for Zope2's security
Hey guys! In the past few months I fiddled around with Zope2's security and access control code. I analysied my own code and code from other developers to search for common errors. Also I tried to think of ways to make the security system easier and more verbose on coding errors I have not yet implemented all my ideas but Zope 2.10 is on the door steps. Here is my first set of improvements. Issue 1 ======= Zope's security declarations have to be called with a method *name* AS STRING. Developers are human beeings and human beeings tend to make small errors like typos. Or they forget to change the security declaration when they rename a method. Zope doesn't raise an error when a developer adds a security declaration for a non existing method. Have a look at the following example. It contains a tiny but devastating typo:: security.declarePrivate('chooseProtocol') def chooseProtocols(self, request): ... These kinds or errors are extremly hard to find and may lead to big security holes. By the way this example was taken from a well known and well tested Zope addon! Solution -------- The solution was very easy to implement. I created a small helper function checkClassHasMethod() and called it in the apply() method of AccessControl.SecurityInfo.ClassSecurityInfo. The apply() method is called at startup. The code doesn't slow down requests. Issue 2 ======= Another way to introduce security breaches is to forget the InitializeClass() call. The call is responsable for applying the informations from a ClassSecurityInfo. Without the call all security settings made through security.declare* are useless. The good news is: Even if a developer forgets to call the method explictly in his code the function is called implictly. The implicit call was hard to find for me at first. I struggled with the code in OFS.Image because there is an import of InitializeClass but it is never called. Never the less the security informations are somehow merged automagically. After some digging I found the code that is responsible for the magic. It's ExtensionClass' __class_init__ magic and OFS.ObjectManager.ObjectManager.__class_init__ Now the bad news: The magic doesn't work under some conditions. For example if you class doesn't subclass ObjectManager or if you monkey patch a class with security info object you have to call InitializeClass explictly. Solution -------- Not yet finished. I've created a function checkObjectHasSecurityInfo() and added a call to ZPublisher.BaseRequest.DefaultPublishTraverse but it is untested. Issue 3 ======= Developers are lazy and they like to make typos. No one likes to type security.declarePrivate('chooseProtocol') so we are using copy & paste which may cause even more typos. Wouldn't it be cool to get rid of security. and typing the name of the method twice? Let's use decorators! Here is the doc test example from my patch: Solution -------- Security decorators are an alternative syntax to define security declarations on classes.
from ExtensionClass import Base from AccessControl import ClassSecurityInfo from AccessControl.decorator import declarePublic from AccessControl.decorator import declarePrivate from AccessControl.decorator import declareProtected from AccessControl.Permissions import view as View from Globals import InitializeClass
class DecoratorExample(Base): ... '''decorator example''' ... ... security = ClassSecurityInfo() ... ... @declarePublic ... def publicMethod(self): ... "public method" ... ... @declarePrivate ... def privateMethod(self): ... "private method" ... ... @declareProtected(View) ... def protectedByView(self): ... "method protected by View" ... InitializeClass(DecoratorExample)
With the new syntax you have to type only 15 letters instead of 41! Issue 4 ======= Some methods shouldn't be callable from certain types of request methods. For example there is no need that the webdav DELETE and PROP* methods should be callable from an ordinary HTTP GET request. I don't want to go into details. Some people know why :) Solution -------- Only a small subset is implemented. There are two methods in my patch to either whitelist or blacklist a request method. An older version of my patch contained even code to distinguish between request types (ftp, http, ftp, xml-rpc) but Jim told me in a private mail it's kind of YAGNI. At the moment blacklistRequestMethod() and whitelistRequestMethod() have to be called explictly inside a method. There is no way to protect methods via security.blacklistRequestMethod() or @blacklistRequestMethod. I have two ideas to implement such security declarations but both ways are complicated and hard to implement. An ordinary decorator doesn't work because it messes up with ZPublisher.mapply. The following code does NOT work with POST requests because mapply is using introspection to get the names and default values of a method. The decorator has a different signatur. def blacklistRequestMethod(*dargs): """Blacklists the available request methods """ __security_decorator__ = True def wrapper(method): name = _getFunctionName(method) def decorator(self, *oargs, **okwargs): _blacklistMethod(self, blacklist=dargs) return method(self, *oargs, **okwargs) decorator.__doc__ = method.__doc__ decorator.func_name = name return decorator return wrapper The problem could be solved by either creating a decorator with a correct signatur using eval/compile/exec (ugly!) or altering the mapply magic. My first approach stored some informations in the class similar to methodname__roles__ and some explicit checks in the request broker and DefaultPublishTraverse. Comments? :) The attached patch is a svn diff against the 2.10 tree. Christian Index: lib/python/AccessControl/__init__.py =================================================================== --- lib/python/AccessControl/__init__.py (revision 70214) +++ lib/python/AccessControl/__init__.py (working copy) @@ -26,6 +26,8 @@ from ZopeGuards import full_write_guard, safe_builtins ModuleSecurityInfo('AccessControl').declarePublic('getSecurityManager') +ModuleSecurityInfo('AccessControl.requestsecurity').declarePublic('blacklistRequestMethod') +ModuleSecurityInfo('AccessControl.requestsecurity').declarePublic('whitelistRequestMethod') import DTML del DTML Index: lib/python/AccessControl/checker.py =================================================================== --- lib/python/AccessControl/checker.py (revision 0) +++ lib/python/AccessControl/checker.py (revision 0) @@ -0,0 +1,164 @@ +############################################################################## +# +# Copyright (c) 2006 Zope Corporation and Contributors. +# All Rights Reserved. +# +# This software is subject to the provisions of the Zope Public License, +# Version 2.1 (ZPL). A copy of the ZPL should accompany this distribution. +# THIS SOFTWARE IS PROVIDED "AS IS" AND ANY AND ALL EXPRESS OR IMPLIED +# WARRANTIES ARE DISCLAIMED, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED +# WARRANTIES OF TITLE, MERCHANTABILITY, AGAINST INFRINGEMENT, AND FITNESS +# FOR A PARTICULAR PURPOSE. +# +############################################################################## +"""Misc security related check functions + +$Id: $ +""" +from logging import getLogger + +from Acquisition import aq_base +from ExtensionClass import Base as ExtensionClassBase +from AccessControl.decorator import _getFunctionName + +LOG = getLogger('SecurityCheck') + +def checkClassHasMethod(classobj, name='', log=True): + """Check if a class has a given method + + This checker is used to find security declarations for methods that don't + exist. Per definition an empty name always exists since an empty name + has a special meaning. + + >>> from ExtensionClass import Base + >>> class Foo(Base): + ... def method(self): + ... pass + + >>> checkClassHasMethod(Foo, '', log=False) + True + >>> checkClassHasMethod(Foo, 'method', log=False) + True + >>> checkClassHasMethod(Foo, 'nomethod', log=False) + False + """ + if not name or hasattr(classobj, name): + return True + + if log: + LOG.warn("Class '%s' has a security setting for a non " + "existing method '%s'" % (_dottedName(classobj), name)) + return False + + +def checkObjectHasSecurityInfo(obj, log=True, paranoid=False): + """Check if the class of an object has a security info object + + obj can be an instance, class or a bound method. + + Under some rare circumstances a class can still have a security info + object. In most cases InitializeClass() is called either explictly by the + programmer or implictly by the ExtensionClass's __class_init__() magic if + the class is a subclass of OFS.ObjectManager. + Edge cases are for example monkey patching with a security info object or + classes that don't subclass from ObjectManager. + + >>> from ExtensionClass import Base + >>> class FakeSecurityInfo(Base): + ... __security_info__ = 1 + ... + + Test class with no security info + >>> class NoSecurity(Base): + ... def method(self): + ... pass + >>> nosecurity = NoSecurity() + >>> checkObjectHasSecurityInfo(NoSecurity, log=False) + False + >>> checkObjectHasSecurityInfo(NoSecurity.method, log=False) + False + >>> checkObjectHasSecurityInfo(nosecurity, log=False) + False + >>> checkObjectHasSecurityInfo(nosecurity.method, log=False) + False + + Test class with a security info object + >>> class WithSecurity(NoSecurity): + ... security = FakeSecurityInfo() + >>> withsecurity = WithSecurity() + >>> checkObjectHasSecurityInfo(WithSecurity, log=False) + True + >>> checkObjectHasSecurityInfo(WithSecurity.method, log=False) + True + >>> checkObjectHasSecurityInfo(withsecurity, log=False) + True + >>> checkObjectHasSecurityInfo(withsecurity.method, log=False) + True + + Test class with a security info object not named security + >>> class HiddenSecurity(NoSecurity): + ... hidden = FakeSecurityInfo() + >>> hiddensecurity = HiddenSecurity() + >>> checkObjectHasSecurityInfo(hiddensecurity, log=False) + False + >>> checkObjectHasSecurityInfo(hiddensecurity.method, log=False) + False + >>> checkObjectHasSecurityInfo(hiddensecurity, log=False, paranoid=True) + True + >>> checkObjectHasSecurityInfo(hiddensecurity.method, log=False, paranoid=True) + True + + checkObjectHasSecurityInfo() shouldn't bark and fail with other object + >>> checkObjectHasSecurityInfo(None, log=False, paranoid=True) + False + >>> checkObjectHasSecurityInfo(1, log=False, paranoid=True) + False + >>> checkObjectHasSecurityInfo('1', log=False, paranoid=True) + False + >>> checkObjectHasSecurityInfo(object(), log=False, paranoid=True) + False + """ + target = aq_base(obj) + if hasattr(target, 'im_class'): + target = target.im_class + name = _dottedName(target) + '.' + _getFunctionName(obj) + elif isinstance(target, ExtensionClassBase): + target = target.__class__ + name = _dottedName(target) + else: + name = _dottedName(target) + + found = [] + if paranoid: + if not hasattr(target, '__dict__'): + return False + for key, value in target.__dict__.items(): + if hasattr(value, '__security_info__'): + found.append(key) + found = ','.join(found) + else: + security = getattr(target, 'security', None) + if security is not None and hasattr(security, '__security_info__'): + found = 'security' + + if not found: + return False + + if log: + LOG.warn("Object %s has a security info object %s. This problem " + "is probably caused by a missing InitializeClass() call and may " + "introduce severe security breaches!" % (name, found)) + return True + +# internal helper functions +def _dottedName(obj, method=None): + """Get the dotted name of an object + """ + modname = getattr(obj, '__module__', 'UNKNOWN') + try: + clsname = getattr(obj, '__name__', None) + if clsname is None: + clsname = obj.__class__.__name__ + except AttributeError: + clsname = 'UNKNOWN' + return modname + '.' + clsname Index: lib/python/AccessControl/tests/test_checker.py =================================================================== --- lib/python/AccessControl/tests/test_checker.py (revision 0) +++ lib/python/AccessControl/tests/test_checker.py (revision 0) @@ -0,0 +1,25 @@ +############################################################################## +# +# Copyright (c) 2002 Zope Corporation and Contributors. All Rights Reserved. +# +# This software is subject to the provisions of the Zope Public License, +# Version 2.1 (ZPL). A copy of the ZPL should accompany this distribution. +# THIS SOFTWARE IS PROVIDED "AS IS" AND ANY AND ALL EXPRESS OR IMPLIED +# WARRANTIES ARE DISCLAIMED, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED +# WARRANTIES OF TITLE, MERCHANTABILITY, AGAINST INFRINGEMENT, AND FITNESS +# FOR A PARTICULAR PURPOSE +# +############################################################################## +"""Unit tests for AccessControl.checker +""" + +import unittest +from zope.testing.doctest import DocTestSuite + +def test_suite(): + suite = unittest.TestSuite() + suite.addTest(DocTestSuite('AccessControl.checker')) + return suite + +if __name__ == '__main__': + unittest.main(defaultTest='test_suite') Index: lib/python/AccessControl/tests/testClassSecurityInfo.py =================================================================== --- lib/python/AccessControl/tests/testClassSecurityInfo.py (revision 70214) +++ lib/python/AccessControl/tests/testClassSecurityInfo.py (working copy) @@ -10,66 +10,79 @@ # FOR A PARTICULAR PURPOSE # ############################################################################## -""" Unit tests for ClassSecurityInfo. +"""Unit tests for ClassSecurityInfo and some decorator stuff """ import unittest +import Globals +from App.class_init import default__class_init__ +from ExtensionClass import Base +from AccessControl.SecurityInfo import ClassSecurityInfo +from AccessControl.SecurityInfo import ACCESS_PRIVATE +from AccessControl.SecurityInfo import ACCESS_PUBLIC +from AccessControl.decorator import declareProtected +from AccessControl.decorator import declarePrivate +from AccessControl.decorator import declarePublic -class ClassSecurityInfoTests(unittest.TestCase): +# Setup a test class with default role -> permission decls. +class Test(Base): + """Test class + """ + __ac_roles__ = ('Role A', 'Role B', 'Role C') + meta_type = "Test" - def _getTargetClass(self): + security = ClassSecurityInfo() - from AccessControl.SecurityInfo import ClassSecurityInfo - return ClassSecurityInfo + security.setPermissionDefault('Make food', ('Chef',)) - def test_SetPermissionDefault(self): + security.setPermissionDefault( + 'Test permission', + ('Manager', 'Role A', 'Role B', 'Role C') + ) - # Test setting default roles for permissions. + security.declareProtected('Test permission', 'foo') + def foo(self, REQUEST=None): + """ """ + pass + + @declareProtected('Test permission') + def protected(self, REQUEST=None): + """ """ - import Globals # XXX: avoiding import cycle - from App.class_init import default__class_init__ - from ExtensionClass import Base + @declarePrivate + def private(self, REQUEST=None): + """ """ - ClassSecurityInfo = self._getTargetClass() + @declarePublic + def public(self, REQUEST=None): + """ """ - # Setup a test class with default role -> permission decls. - class Test(Base): - """Test class - """ - __ac_roles__ = ('Role A', 'Role B', 'Role C') +# Do class initialization. +default__class_init__(Test) - meta_type = "Test" +class ClassSecurityInfoTests(unittest.TestCase): - security = ClassSecurityInfo() + + def _checkPerm(self, func__roles__): + imPermissionRole = [r for r in func__roles__ + if not r.endswith('_Permission')] + self.failUnless(len(imPermissionRole) == 4) - security.setPermissionDefault('Make food', ('Chef',)) + for item in ('Manager', 'Role A', 'Role B', 'Role C'): + self.failUnless(item in imPermissionRole) - security.setPermissionDefault( - 'Test permission', - ('Manager', 'Role A', 'Role B', 'Role C') - ) - - security.declareProtected('Test permission', 'foo') - def foo(self, REQUEST=None): - """ """ - pass - - # Do class initialization. - default__class_init__(Test) - + def test_SetPermissionDefault(self): # Now check the resulting class to see if the mapping was made # correctly. Note that this uses carnal knowledge of the internal # structures used to store this information! object = Test() - imPermissionRole = [r for r in object.foo__roles__ - if not r.endswith('_Permission')] - self.failUnless(len(imPermissionRole) == 4) - - for item in ('Manager', 'Role A', 'Role B', 'Role C'): - self.failUnless(item in imPermissionRole) - + self._checkPerm(object.foo__roles__) + self._checkPerm(object.protected__roles__) + self.failUnless(object.private__roles__ == ACCESS_PRIVATE) + self.failUnless(object.public__roles__ == ACCESS_PUBLIC) + # Make sure that a permission defined without accompanying method # is still reflected in __ac_permissions__ self.assertEquals([t for t in Test.__ac_permissions__ if not t[1]], Index: lib/python/AccessControl/tests/test_requestsecurity.py =================================================================== --- lib/python/AccessControl/tests/test_requestsecurity.py (revision 0) +++ lib/python/AccessControl/tests/test_requestsecurity.py (revision 0) @@ -0,0 +1,25 @@ +############################################################################## +# +# Copyright (c) 2002 Zope Corporation and Contributors. All Rights Reserved. +# +# This software is subject to the provisions of the Zope Public License, +# Version 2.1 (ZPL). A copy of the ZPL should accompany this distribution. +# THIS SOFTWARE IS PROVIDED "AS IS" AND ANY AND ALL EXPRESS OR IMPLIED +# WARRANTIES ARE DISCLAIMED, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED +# WARRANTIES OF TITLE, MERCHANTABILITY, AGAINST INFRINGEMENT, AND FITNESS +# FOR A PARTICULAR PURPOSE +# +############################################################################## +"""Unit tests for AccessControl.requestsecurity +""" + +import unittest +from zope.testing.doctest import DocTestSuite + +def test_suite(): + suite = unittest.TestSuite() + suite.addTest(DocTestSuite('AccessControl.requestsecurity')) + return suite + +if __name__ == '__main__': + unittest.main(defaultTest='test_suite') Index: lib/python/AccessControl/requestsecurity.py =================================================================== --- lib/python/AccessControl/requestsecurity.py (revision 0) +++ lib/python/AccessControl/requestsecurity.py (revision 0) @@ -0,0 +1,141 @@ +############################################################################## +# +# Copyright (c) 2006 Zope Corporation and Contributors. +# All Rights Reserved. +# +# This software is subject to the provisions of the Zope Public License, +# Version 2.1 (ZPL). A copy of the ZPL should accompany this distribution. +# THIS SOFTWARE IS PROVIDED "AS IS" AND ANY AND ALL EXPRESS OR IMPLIED +# WARRANTIES ARE DISCLAIMED, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED +# WARRANTIES OF TITLE, MERCHANTABILITY, AGAINST INFRINGEMENT, AND FITNESS +# FOR A PARTICULAR PURPOSE. +# +############################################################################## +"""Request method based access control + +The requestsecurity module contains helper functions to restrict the access +to resources based on the request method. For example you can deny a GET +request to a method that should only be available for forms over POST +requests. + +The restrictions are available in two flavors: whitelist and blacklist: + + o A blacklist contains a single or more methods that are FORBIDDEN. Every + method that is NOT listed is ALLOWED. + + o A whitelist contains a single or more methods that are ALLOWED. Every + method that is NOT listed is DENIED. + +In general a whitelist is more secure than a blacklist but harder to define. + +The blacklistRequestMethod and whitelistRequestMethod are available in +restricted python code like scripts, too. + +>>> from AccessControl.ZopeGuards import guarded_import +>>> mod = guarded_import('AccessControl.requestsecurity', +... fromlist=('blacklistRequestMethod', )) +>>> mod = guarded_import('AccessControl.requestsecurity', +... fromlist=('whitelistRequestMethod', )) + +Examples: + +boiler plate +>>> context = object() +>>> name = 'testobject' +>>> request = {'REQUEST_METHOD' : 'GET', +... 'URL' : '/testobject' } + +Black list tests +>>> blacklistRequestMethod(context, 'GET', request=request, name=name) +Traceback (most recent call last): +... +Unauthorized: GET request to 'testobject' is not allowed. + +>>> blacklistRequestMethod(context, 'POST', request=request, name=name) + +>>> blacklistRequestMethod(context, ('POST', 'GET'), request=request) +Traceback (most recent call last): +... +Unauthorized: GET request to '/testobject' is not allowed. + +White lists are working the opposite way +>>> whitelistRequestMethod(context, 'GET', request=request, name=name) + +>>> whitelistRequestMethod(context, 'POST', request=request, name=name) +Traceback (most recent call last): +... +Unauthorized: GET request to 'testobject' is not allowed. + +>>> whitelistRequestMethod(context, ('POST', 'GET'), request=request, name=name) + +$Id: $ +""" +from AccessControl.unauthorized import Unauthorized + + +def blacklistRequestMethod(context, blacklist='GET', request=None, name=None): + """Deny access with forbidden request methods + """ + return _restrictRequestMethod(context, blacklist=blacklist, + request=request, name=name) + +def whitelistRequestMethod(context, whitelist='POST', request=None, name=None): + """Deny access except for allowed request methods + """ + return _restrictRequestMethod(context, whitelist=whitelist, + request=request, name=name) + +__all__ = ('blacklistRequestMethod', 'whitelistRequestMethod',) + +# internal stuff +def _restrictRequestMethod(context, blacklist=None, whitelist=None, + request=None, name=None): + """Helper method for white or blacklisting request methods + + context should be the requested method or object with an acquistion + context. + + whitelist and blacklist can either be a string or a sequence. You can't + define both. + + request is the request object. If no request is given then the request is + acquired from the context + + name is used for the error message. If no name is given then the URL of + the context or repr(context) is used. + """ + if request is None: + request = getattr(context, 'REQUEST', None) + if request is None: + # XXX no request found, what shall I do? + return + + if not name: + try: + name = request.get('URL', None) + if not name: + name = context.absolute_url() + except AttributeError: + name = repr(context) + + req_method = request.get('REQUEST_METHOD', 'GET').upper() + + if not bool(blacklist) ^ bool(whitelist): + raise ValueError("You have to specify either blacklist or whitelist") + + if blacklist: + if isinstance(blacklist, basestring): + blacklist = (blacklist, ) + if req_method in blacklist: + raise Unauthorized("%s request to '%s' is not allowed." + % (req_method, name)) + return + + if whitelist: + if isinstance(whitelist, basestring): + whitelist = (whitelist, ) + if req_method not in whitelist: + raise Unauthorized("%s request to '%s' is not allowed." + % (req_method, name)) + return + Index: lib/python/AccessControl/SecurityInfo.py =================================================================== --- lib/python/AccessControl/SecurityInfo.py (revision 70214) +++ lib/python/AccessControl/SecurityInfo.py (working copy) @@ -42,8 +42,8 @@ from logging import getLogger import Acquisition - from AccessControl.ImplPython import _what_not_even_god_should_do +from AccessControl.checker import checkClassHasMethod LOG = getLogger('SecurityInfo') @@ -160,6 +160,7 @@ # Collect protected attribute names in ac_permissions. ac_permissions = {} for name, access in self.names.items(): + checkClassHasMethod(classobj, name, log=True) if access in (ACCESS_PRIVATE, ACCESS_PUBLIC, ACCESS_NONE): setattr(classobj, '%s__roles__' % name, access) else: Index: lib/python/AccessControl/decorator.py =================================================================== --- lib/python/AccessControl/decorator.py (revision 0) +++ lib/python/AccessControl/decorator.py (revision 0) @@ -0,0 +1,144 @@ +############################################################################## +# +# Copyright (c) 2006 Zope Corporation and Contributors. +# All Rights Reserved. +# +# This software is subject to the provisions of the Zope Public License, +# Version 2.1 (ZPL). A copy of the ZPL should accompany this distribution. +# THIS SOFTWARE IS PROVIDED "AS IS" AND ANY AND ALL EXPRESS OR IMPLIED +# WARRANTIES ARE DISCLAIMED, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED +# WARRANTIES OF TITLE, MERCHANTABILITY, AGAINST INFRINGEMENT, AND FITNESS +# FOR A PARTICULAR PURPOSE. +# +############################################################################## +"""Security decorators + +Security decorators are an alternative syntax to define security declarations +on classes. + +>>> from ExtensionClass import Base +>>> from AccessControl import ClassSecurityInfo +>>> from AccessControl.decorator import declarePublic +>>> from AccessControl.decorator import declarePrivate +>>> from AccessControl.decorator import declareProtected +>>> from AccessControl.Permissions import view as View +>>> from Globals import InitializeClass + +>>> class DecoratorExample(Base): +... '''decorator example''' +... +... security = ClassSecurityInfo() +... +... @declarePublic +... def publicMethod(self): +... "public method" +... +... @declarePrivate +... def privateMethod(self): +... "private method" +... +... @declareProtected(View) +... def protectedByView(self): +... "method protected by View" +... +>>> InitializeClass(DecoratorExample) + +$Id: $ +""" +import sys + +def declarePublic(method): + """Declare names to be publicly accessible. + + Returns the same method object + """ + __security_decorator__ = True + security = _getSecurityInfoFromStack() + name = _getFunctionName(method) + security.declarePublic(name) + return method + +def declarePrivate(method): + """Declare names to be inaccessible to restricted code. + + Returns the same method object + """ + __security_decorator__ = True + security = _getSecurityInfoFromStack() + name = _getFunctionName(method) + security.declarePrivate(name) + return method + +def declareProtected(permission_name): + """Declare names to be associated with a permission. + + Returns the same method object + """ + __security_decorator__ = True + security = _getSecurityInfoFromStack() + def wrapper(method): + name = _getFunctionName(method) + security.declareProtected(permission_name, name) + return method + return wrapper + +__all__ = ('declarePublic', 'declarePrivate', 'declareProtected') + +# internal helper functions + +def _getSecurityInfoFromStack(): + """Get the security object from the caller stack of a decorator + + There is no direct way to access class variables from a decorator method. + This method tries to get the security var from the caller stack. Yeah it + is a bit hacky but even zope.interface's implements() is using the same + trick. + + If the local namespace contains a var __security_decorator__ then the + functions is trying to get the security object from the next stack level. + This allows the stacking of two or more decorators. The + __security__decorator__ var minimizes the risk to acquire a different + security object from the stack. + """ + # frame 1 is the decorator function, 2 is the class unless you use more + # than one decorator. + index = 2 + while True: + frame = sys._getframe(index) + loc = frame.f_locals + security = loc.get('security', None) + sd = loc.has_key('__security_decorator__') + del loc # delete frame and locals to avoid memory leaks + del frame + if security and hasattr(security, '__security_info__'): + return security + if sd: + index+=1 + continue + raise TypeError('A security decorator must be defined on a method ' + 'of a class with a security information object!') + +def _getFunctionName(func): + """Get function name from a method, function or classfunction + + >>> def function(): pass + >>> class Foo(object): + ... def method(self): pass + ... @classmethod + ... def clsmethod(cls): pass + >>> foo = Foo() + + >>> _getFunctionName(function) + 'function' + >>> _getFunctionName(Foo.method) + 'method' + >>> _getFunctionName(Foo.clsmethod) + 'clsmethod' + >>> _getFunctionName(foo.method) + 'method' + >>> _getFunctionName(foo.clsmethod) + 'clsmethod' + """ + func = getattr(func, 'im_func', func) + return func.func_name + Index: lib/python/ZPublisher/BaseRequest.py =================================================================== --- lib/python/ZPublisher/BaseRequest.py (revision 70214) +++ lib/python/ZPublisher/BaseRequest.py (working copy) @@ -18,6 +18,7 @@ import xmlrpc from zExceptions import Forbidden, Unauthorized, NotFound from Acquisition import aq_base +from AccessControl.checker import checkObjectHasSecurityInfo from zope.interface import implements, providedBy, Interface from zope.component import queryMultiAdapter @@ -126,6 +127,10 @@ "published." % URL ) + #from Globals import DevelopmentMode + #if DevelopmentMode: + # checkObjectHasSecurityInfo(subobject, log=True) + # Hack for security: in Python 2.2.2, most built-in types # gained docstrings that they didn't have before. That caused # certain mutable types (dicts, lists) to become publishable Index: lib/python/Shared/DC/ZRDB/DA.py =================================================================== --- lib/python/Shared/DC/ZRDB/DA.py (revision 70214) +++ lib/python/Shared/DC/ZRDB/DA.py (working copy) @@ -113,7 +113,7 @@ security.declareProtected(view_management_screens, 'manage_advancedForm') manage_advancedForm=DTMLFile('dtml/advanced', globals()) - security.declarePublic('test_url') + security.declarePublic('test_url_') def test_url_(self): 'Method for testing server connection information' return 'PING' Index: lib/python/webdav/Lockable.py =================================================================== --- lib/python/webdav/Lockable.py (revision 70214) +++ lib/python/webdav/Lockable.py (working copy) @@ -43,7 +43,7 @@ # Protect methods using declarative security security = ClassSecurityInfo() security.declarePrivate('wl_lockmapping') - security.declarePublic('wl_isLocked', 'wl_getLock', 'wl_isLockedByUser', + security.declarePublic('wl_isLocked', 'wl_getLock', 'wl_lockItems', 'wl_lockValues', 'wl_lockTokens',) security.declareProtected('WebDAV Lock items', 'wl_setLock') security.declareProtected('WebDAV Unlock items', 'wl_delLock')
You have many good points in your list of troubles. Many of them are resolved by using security declarations through ZCML instead. It would be interesting to here your views on this. //Lennart
Lennart Regebro schrieb:
You have many good points in your list of troubles. Many of them are resolved by using security declarations through ZCML instead. It would be interesting to here your views on this.
In general I preferre old and well tested security code over new security related code. Martjin, Phillip and all the other people are doing a great job with Five but well ... it's new code. New code tends to break because it is not as well tested as old code. Here is a list of my views and concerns about ZCML and Five security. Some or all of my points might be wrong. I had only one hour time to read code and I did no debugging or unit testing of my concerns. (NYV = not yet verified) * ZCML security declarations are great for Zope3 and Five classes because their default security policy is DENY unless explictly allowed. [NYV] The default object security of ClassSecurityInfo is declareObjectPrivate. [NYV] * But if you mix in subclasses of SimpleItem and others (Image, File and many more) you ar gaining their default security setting declareObjectPublic or declareObjectProtected(View)! It means that every method is availableunless explictly restricted. This could lead to security breaches. IMO it is easier to find an unprotected method by reading code when the security declaration sits next to the method. A checker method for unit tests could be useful. * Comments like <!--deny attributes="baz" /--> <!-- XXX not yet supported --> are adding a bad gut feeling ... * As far as I understand the security system Zope2's can't protect attributes and descriptors (properties) with security.declarePrivate('attributename'). The default object permission always wins. A ZCML directive that protects an attribute or descriptor can lead to false security assumptions. [NYV] It is possible to protect attributes with different permissons by hooking into __allow_access_to_unprotected_subobjects__. The class var can be set to a bool (1 = public, 0 = private), a string (permission name), a dict (attribute name -> 0/1) or a callable. I have an idea how to protect descriptors and attributes based on some work from Sidney for Archetypes. * A long time ago somebody has told me that Zope2's security works only for objects that subclass from ExtensionClass.Base or an Acquisition class. Is this still true or is it a false information? Christian
Hey Christian, welcome back! :) Christian Heimes wrote:
Lennart Regebro schrieb:
You have many good points in your list of troubles. Many of them are resolved by using security declarations through ZCML instead. It would be interesting to here your views on this.
In general I preferre old and well tested security code over new security related code. Martjin, Phillip and all the other people are doing a great job with Five but well ... it's new code. New code tends to break because it is not as well tested as old code.
There isn't much new in terms of security regarding what ZCML does in Five.
Here is a list of my views and concerns about ZCML and Five security. Some or all of my points might be wrong.
This is hazardous. We're about to spend serious time on discussions that may be pointless due to the lack of proper research...
* ZCML security declarations are great for Zope3 and Five classes because their default security policy is DENY unless explictly allowed.
ZCML does NOT change the security policy of Zope 2. ZCML is just an *spelling* of security declarations. So, it's not much new code at all.
[NYV] The default object security of ClassSecurityInfo is declareObjectPrivate. [NYV]
Where? In Five or in Zope 2? And what's the point?
* But if you mix in subclasses of SimpleItem and others (Image, File and many more) you ar gaining their default security setting declareObjectPublic or declareObjectProtected(View)! It means that every method is availableunless explictly restricted.
That's Zope 2 for you.
This could lead to security breaches. IMO it is easier to find an unprotected method by reading code when the security declaration sits next to the method. A checker method for unit tests could be useful.
Yup.
* Comments like <!--deny attributes="baz" /--> <!-- XXX not yet supported --> are adding a bad gut feeling ...
<deny /> is soemthing that's not in Zope 3 and I don't know what Sidnei (who did the ZCML-Zope2-security integration) intended there. It's certainly nothing that poses a security threat. We don't operate on bad gut feelings. If you see definite problems with Five code, I'll be happy to discuss them.
* As far as I understand the security system Zope2's can't protect attributes and descriptors (properties) with security.declarePrivate('attributename'). The default object permission always wins. A ZCML directive that protects an attribute or descriptor can lead to false security assumptions. [NYV]
Again, ZCML is just a different spelling of security.declare*, but one that can help you avoid typo problems (e.g. when you use interfaces). It cannot solve deeply rooted problems of Zope 2's security machienry and the confusion it may create. Developers using security.declarePrivate may just as well be misled as someone using ZCML.
* A long time ago somebody has told me that Zope2's security works only for objects that subclass from ExtensionClass.Base or an Acquisition class. Is this still true or is it a false information?
At least currently the Zope 2 security policy needs an acquisition context because it aq_acquires __roles__. I have some work in a local sandbox that will make the dependency on Acquisition-based classes go away. Philipp
On Tue, Sep 19, 2006 at 04:34:55PM +0200, Philipp von Weitershausen wrote: | >In general I preferre old and well tested security code over new | >security related code. Martjin, Phillip and all the other people are | >doing a great job with Five but well ... it's new code. New code tends | >to break because it is not as well tested as old code. | | There isn't much new in terms of security regarding what ZCML does in Five. In fact all it does is to map Zope 3 security directives to Zope 2 ClassSecurityInfo-style. | >* ZCML security declarations are great for Zope3 and Five classes | >because their default security policy is DENY unless explictly allowed. | | ZCML does NOT change the security policy of Zope 2. ZCML is just an | *spelling* of security declarations. So, it's not much new code at all. And in fact it has tests. | >* Comments like <!--deny attributes="baz" /--> <!-- XXX not yet | >supported --> are adding a bad gut feeling ... | | <deny /> is soemthing that's not in Zope 3 and I don't know what Sidnei | (who did the ZCML-Zope2-security integration) intended there. It's | certainly nothing that poses a security threat. We don't operate on bad | gut feelings. If you see definite problems with Five code, I'll be happy | to discuss them. I believe Zope 3 had <deny /> at some point. It might not have it anymore those days. If I recall, the motivation was to be able to add the notion of 'deny by default' which exists in Zope 3. -- Sidnei da Silva Enfold Systems http://enfoldsystems.com Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214
Christian Heimes wrote at 2006-9-19 16:02 +0200:
... * As far as I understand the security system Zope2's can't protect attributes and descriptors (properties) with security.declarePrivate('attributename').
Since Zope 2.8, this is no longer true. You can protect simple type attributes and descriptors in the same way as methods. -- Dieter
I think it's great that you did this... nice job! I have some specific disagreements (while I think it's a reasonable constraint, and I think something should enforce it, I don't believe it's the job of something that we call a *security policy* to enforce whether a method is called, e.g. via POST rather than GET), I think you did a wonderful job analyzing this.. +1 on the developer-helper tools portion of this that finds declarations that have no corresponding method. Thanks! - C On Sep 18, 2006, at 11:20 AM, Christian Heimes wrote:
Hey guys!
In the past few months I fiddled around with Zope2's security and access control code. I analysied my own code and code from other developers to search for common errors. Also I tried to think of ways to make the security system easier and more verbose on coding errors
I have not yet implemented all my ideas but Zope 2.10 is on the door steps. Here is my first set of improvements.
Issue 1 =======
Zope's security declarations have to be called with a method *name* AS STRING. Developers are human beeings and human beeings tend to make small errors like typos. Or they forget to change the security declaration when they rename a method. Zope doesn't raise an error when a developer adds a security declaration for a non existing method.
Have a look at the following example. It contains a tiny but devastating typo::
security.declarePrivate('chooseProtocol') def chooseProtocols(self, request): ...
These kinds or errors are extremly hard to find and may lead to big security holes. By the way this example was taken from a well known and well tested Zope addon!
Solution --------
The solution was very easy to implement. I created a small helper function checkClassHasMethod() and called it in the apply() method of AccessControl.SecurityInfo.ClassSecurityInfo. The apply() method is called at startup. The code doesn't slow down requests.
Issue 2 =======
Another way to introduce security breaches is to forget the InitializeClass() call. The call is responsable for applying the informations from a ClassSecurityInfo. Without the call all security settings made through security.declare* are useless.
The good news is: Even if a developer forgets to call the method explictly in his code the function is called implictly.
The implicit call was hard to find for me at first. I struggled with the code in OFS.Image because there is an import of InitializeClass but it is never called. Never the less the security informations are somehow merged automagically. After some digging I found the code that is responsible for the magic. It's ExtensionClass' __class_init__ magic and OFS.ObjectManager.ObjectManager.__class_init__
Now the bad news: The magic doesn't work under some conditions. For example if you class doesn't subclass ObjectManager or if you monkey patch a class with security info object you have to call InitializeClass explictly.
Solution --------
Not yet finished. I've created a function checkObjectHasSecurityInfo() and added a call to ZPublisher.BaseRequest.DefaultPublishTraverse but it is untested.
Issue 3 =======
Developers are lazy and they like to make typos. No one likes to type security.declarePrivate('chooseProtocol') so we are using copy & paste which may cause even more typos. Wouldn't it be cool to get rid of security. and typing the name of the method twice? Let's use decorators! Here is the doc test example from my patch:
Solution --------
Security decorators are an alternative syntax to define security declarations on classes.
from ExtensionClass import Base from AccessControl import ClassSecurityInfo from AccessControl.decorator import declarePublic from AccessControl.decorator import declarePrivate from AccessControl.decorator import declareProtected from AccessControl.Permissions import view as View from Globals import InitializeClass
class DecoratorExample(Base): ... '''decorator example''' ... ... security = ClassSecurityInfo() ... ... @declarePublic ... def publicMethod(self): ... "public method" ... ... @declarePrivate ... def privateMethod(self): ... "private method" ... ... @declareProtected(View) ... def protectedByView(self): ... "method protected by View" ... InitializeClass(DecoratorExample)
With the new syntax you have to type only 15 letters instead of 41!
Issue 4 =======
Some methods shouldn't be callable from certain types of request methods. For example there is no need that the webdav DELETE and PROP* methods should be callable from an ordinary HTTP GET request. I don't want to go into details. Some people know why :)
Solution --------
Only a small subset is implemented. There are two methods in my patch to either whitelist or blacklist a request method. An older version of my patch contained even code to distinguish between request types (ftp, http, ftp, xml-rpc) but Jim told me in a private mail it's kind of YAGNI.
At the moment blacklistRequestMethod() and whitelistRequestMethod() have to be called explictly inside a method. There is no way to protect methods via security.blacklistRequestMethod() or @blacklistRequestMethod.
I have two ideas to implement such security declarations but both ways are complicated and hard to implement. An ordinary decorator doesn't work because it messes up with ZPublisher.mapply.
The following code does NOT work with POST requests because mapply is using introspection to get the names and default values of a method. The decorator has a different signatur.
def blacklistRequestMethod(*dargs): """Blacklists the available request methods """ __security_decorator__ = True def wrapper(method): name = _getFunctionName(method) def decorator(self, *oargs, **okwargs): _blacklistMethod(self, blacklist=dargs) return method(self, *oargs, **okwargs) decorator.__doc__ = method.__doc__ decorator.func_name = name return decorator return wrapper
The problem could be solved by either creating a decorator with a correct signatur using eval/compile/exec (ugly!) or altering the mapply magic.
My first approach stored some informations in the class similar to methodname__roles__ and some explicit checks in the request broker and DefaultPublishTraverse.
Comments? :)
The attached patch is a svn diff against the 2.10 tree.
Christian Index: lib/python/AccessControl/__init__.py =================================================================== --- lib/python/AccessControl/__init__.py (revision 70214) +++ lib/python/AccessControl/__init__.py (working copy) @@ -26,6 +26,8 @@ from ZopeGuards import full_write_guard, safe_builtins
ModuleSecurityInfo('AccessControl').declarePublic ('getSecurityManager') +ModuleSecurityInfo('AccessControl.requestsecurity').declarePublic ('blacklistRequestMethod') +ModuleSecurityInfo('AccessControl.requestsecurity').declarePublic ('whitelistRequestMethod')
import DTML del DTML Index: lib/python/AccessControl/checker.py =================================================================== --- lib/python/AccessControl/checker.py (revision 0) +++ lib/python/AccessControl/checker.py (revision 0) @@ -0,0 +1,164 @@ +##################################################################### ######### +# +# Copyright (c) 2006 Zope Corporation and Contributors. +# All Rights Reserved. +# +# This software is subject to the provisions of the Zope Public License, +# Version 2.1 (ZPL). A copy of the ZPL should accompany this distribution. +# THIS SOFTWARE IS PROVIDED "AS IS" AND ANY AND ALL EXPRESS OR IMPLIED +# WARRANTIES ARE DISCLAIMED, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED +# WARRANTIES OF TITLE, MERCHANTABILITY, AGAINST INFRINGEMENT, AND FITNESS +# FOR A PARTICULAR PURPOSE. +# +##################################################################### ######### +"""Misc security related check functions + +$Id: $ +""" +from logging import getLogger + +from Acquisition import aq_base +from ExtensionClass import Base as ExtensionClassBase +from AccessControl.decorator import _getFunctionName + +LOG = getLogger('SecurityCheck') + +def checkClassHasMethod(classobj, name='', log=True): + """Check if a class has a given method + + This checker is used to find security declarations for methods that don't + exist. Per definition an empty name always exists since an empty name + has a special meaning. + + >>> from ExtensionClass import Base + >>> class Foo(Base): + ... def method(self): + ... pass + + >>> checkClassHasMethod(Foo, '', log=False) + True + >>> checkClassHasMethod(Foo, 'method', log=False) + True + >>> checkClassHasMethod(Foo, 'nomethod', log=False) + False + """ + if not name or hasattr(classobj, name): + return True + + if log: + LOG.warn("Class '%s' has a security setting for a non " + "existing method '%s'" % (_dottedName(classobj), name)) + return False + + +def checkObjectHasSecurityInfo(obj, log=True, paranoid=False): + """Check if the class of an object has a security info object + + obj can be an instance, class or a bound method. + + Under some rare circumstances a class can still have a security info + object. In most cases InitializeClass() is called either explictly by the + programmer or implictly by the ExtensionClass's __class_init__ () magic if + the class is a subclass of OFS.ObjectManager. + Edge cases are for example monkey patching with a security info object or + classes that don't subclass from ObjectManager. + + >>> from ExtensionClass import Base + >>> class FakeSecurityInfo(Base): + ... __security_info__ = 1 + ... + + Test class with no security info + >>> class NoSecurity(Base): + ... def method(self): + ... pass + >>> nosecurity = NoSecurity() + >>> checkObjectHasSecurityInfo(NoSecurity, log=False) + False + >>> checkObjectHasSecurityInfo(NoSecurity.method, log=False) + False + >>> checkObjectHasSecurityInfo(nosecurity, log=False) + False + >>> checkObjectHasSecurityInfo(nosecurity.method, log=False) + False + + Test class with a security info object + >>> class WithSecurity(NoSecurity): + ... security = FakeSecurityInfo() + >>> withsecurity = WithSecurity() + >>> checkObjectHasSecurityInfo(WithSecurity, log=False) + True + >>> checkObjectHasSecurityInfo(WithSecurity.method, log=False) + True + >>> checkObjectHasSecurityInfo(withsecurity, log=False) + True + >>> checkObjectHasSecurityInfo(withsecurity.method, log=False) + True + + Test class with a security info object not named security + >>> class HiddenSecurity(NoSecurity): + ... hidden = FakeSecurityInfo() + >>> hiddensecurity = HiddenSecurity() + >>> checkObjectHasSecurityInfo(hiddensecurity, log=False) + False + >>> checkObjectHasSecurityInfo(hiddensecurity.method, log=False) + False + >>> checkObjectHasSecurityInfo(hiddensecurity, log=False, paranoid=True) + True + >>> checkObjectHasSecurityInfo(hiddensecurity.method, log=False, paranoid=True) + True + + checkObjectHasSecurityInfo() shouldn't bark and fail with other object + >>> checkObjectHasSecurityInfo(None, log=False, paranoid=True) + False + >>> checkObjectHasSecurityInfo(1, log=False, paranoid=True) + False + >>> checkObjectHasSecurityInfo('1', log=False, paranoid=True) + False + >>> checkObjectHasSecurityInfo(object(), log=False, paranoid=True) + False + """ + target = aq_base(obj) + if hasattr(target, 'im_class'): + target = target.im_class + name = _dottedName(target) + '.' + _getFunctionName(obj) + elif isinstance(target, ExtensionClassBase): + target = target.__class__ + name = _dottedName(target) + else: + name = _dottedName(target) + + found = [] + if paranoid: + if not hasattr(target, '__dict__'): + return False + for key, value in target.__dict__.items(): + if hasattr(value, '__security_info__'): + found.append(key) + found = ','.join(found) + else: + security = getattr(target, 'security', None) + if security is not None and hasattr(security, '__security_info__'): + found = 'security' + + if not found: + return False + + if log: + LOG.warn("Object %s has a security info object %s. This problem " + "is probably caused by a missing InitializeClass() call and may " + "introduce severe security breaches!" % (name, found)) + return True + +# internal helper functions +def _dottedName(obj, method=None): + """Get the dotted name of an object + """ + modname = getattr(obj, '__module__', 'UNKNOWN') + try: + clsname = getattr(obj, '__name__', None) + if clsname is None: + clsname = obj.__class__.__name__ + except AttributeError: + clsname = 'UNKNOWN' + return modname + '.' + clsname Index: lib/python/AccessControl/tests/test_checker.py =================================================================== --- lib/python/AccessControl/tests/test_checker.py (revision 0) +++ lib/python/AccessControl/tests/test_checker.py (revision 0) @@ -0,0 +1,25 @@ +##################################################################### ######### +# +# Copyright (c) 2002 Zope Corporation and Contributors. All Rights Reserved. +# +# This software is subject to the provisions of the Zope Public License, +# Version 2.1 (ZPL). A copy of the ZPL should accompany this distribution. +# THIS SOFTWARE IS PROVIDED "AS IS" AND ANY AND ALL EXPRESS OR IMPLIED +# WARRANTIES ARE DISCLAIMED, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED +# WARRANTIES OF TITLE, MERCHANTABILITY, AGAINST INFRINGEMENT, AND FITNESS +# FOR A PARTICULAR PURPOSE +# +##################################################################### ######### +"""Unit tests for AccessControl.checker +""" + +import unittest +from zope.testing.doctest import DocTestSuite + +def test_suite(): + suite = unittest.TestSuite() + suite.addTest(DocTestSuite('AccessControl.checker')) + return suite + +if __name__ == '__main__': + unittest.main(defaultTest='test_suite') Index: lib/python/AccessControl/tests/testClassSecurityInfo.py =================================================================== --- lib/python/AccessControl/tests/testClassSecurityInfo.py (revision 70214) +++ lib/python/AccessControl/tests/testClassSecurityInfo.py (working copy) @@ -10,66 +10,79 @@ # FOR A PARTICULAR PURPOSE #
###################################################################### ######## -""" Unit tests for ClassSecurityInfo. +"""Unit tests for ClassSecurityInfo and some decorator stuff """
import unittest
+import Globals +from App.class_init import default__class_init__ +from ExtensionClass import Base +from AccessControl.SecurityInfo import ClassSecurityInfo +from AccessControl.SecurityInfo import ACCESS_PRIVATE +from AccessControl.SecurityInfo import ACCESS_PUBLIC +from AccessControl.decorator import declareProtected +from AccessControl.decorator import declarePrivate +from AccessControl.decorator import declarePublic
-class ClassSecurityInfoTests(unittest.TestCase): +# Setup a test class with default role -> permission decls. +class Test(Base): + """Test class + """ + __ac_roles__ = ('Role A', 'Role B', 'Role C')
+ meta_type = "Test"
- def _getTargetClass(self): + security = ClassSecurityInfo()
- from AccessControl.SecurityInfo import ClassSecurityInfo - return ClassSecurityInfo + security.setPermissionDefault('Make food', ('Chef',))
- def test_SetPermissionDefault(self): + security.setPermissionDefault( + 'Test permission', + ('Manager', 'Role A', 'Role B', 'Role C') + )
- # Test setting default roles for permissions. + security.declareProtected('Test permission', 'foo') + def foo(self, REQUEST=None): + """ """ + pass + + @declareProtected('Test permission') + def protected(self, REQUEST=None): + """ """
- import Globals # XXX: avoiding import cycle - from App.class_init import default__class_init__ - from ExtensionClass import Base + @declarePrivate + def private(self, REQUEST=None): + """ """
- ClassSecurityInfo = self._getTargetClass() + @declarePublic + def public(self, REQUEST=None): + """ """
- # Setup a test class with default role -> permission decls. - class Test(Base): - """Test class - """ - __ac_roles__ = ('Role A', 'Role B', 'Role C') +# Do class initialization. +default__class_init__(Test)
- meta_type = "Test" +class ClassSecurityInfoTests(unittest.TestCase):
- security = ClassSecurityInfo() + + def _checkPerm(self, func__roles__): + imPermissionRole = [r for r in func__roles__ + if not r.endswith('_Permission')] + self.failUnless(len(imPermissionRole) == 4)
- security.setPermissionDefault('Make food', ('Chef',)) + for item in ('Manager', 'Role A', 'Role B', 'Role C'): + self.failUnless(item in imPermissionRole)
- security.setPermissionDefault( - 'Test permission', - ('Manager', 'Role A', 'Role B', 'Role C') - ) - - security.declareProtected('Test permission', 'foo') - def foo(self, REQUEST=None): - """ """ - pass - - # Do class initialization. - default__class_init__(Test) - + def test_SetPermissionDefault(self): # Now check the resulting class to see if the mapping was made # correctly. Note that this uses carnal knowledge of the internal # structures used to store this information! object = Test() - imPermissionRole = [r for r in object.foo__roles__ - if not r.endswith('_Permission')] - self.failUnless(len(imPermissionRole) == 4) - - for item in ('Manager', 'Role A', 'Role B', 'Role C'): - self.failUnless(item in imPermissionRole) - + self._checkPerm(object.foo__roles__) + self._checkPerm(object.protected__roles__) + self.failUnless(object.private__roles__ == ACCESS_PRIVATE) + self.failUnless(object.public__roles__ == ACCESS_PUBLIC) + # Make sure that a permission defined without accompanying method # is still reflected in __ac_permissions__ self.assertEquals([t for t in Test.__ac_permissions__ if not t[1]], Index: lib/python/AccessControl/tests/test_requestsecurity.py =================================================================== --- lib/python/AccessControl/tests/test_requestsecurity.py (revision 0) +++ lib/python/AccessControl/tests/test_requestsecurity.py (revision 0) @@ -0,0 +1,25 @@ +##################################################################### ######### +# +# Copyright (c) 2002 Zope Corporation and Contributors. All Rights Reserved. +# +# This software is subject to the provisions of the Zope Public License, +# Version 2.1 (ZPL). A copy of the ZPL should accompany this distribution. +# THIS SOFTWARE IS PROVIDED "AS IS" AND ANY AND ALL EXPRESS OR IMPLIED +# WARRANTIES ARE DISCLAIMED, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED +# WARRANTIES OF TITLE, MERCHANTABILITY, AGAINST INFRINGEMENT, AND FITNESS +# FOR A PARTICULAR PURPOSE +# +##################################################################### ######### +"""Unit tests for AccessControl.requestsecurity +""" + +import unittest +from zope.testing.doctest import DocTestSuite + +def test_suite(): + suite = unittest.TestSuite() + suite.addTest(DocTestSuite('AccessControl.requestsecurity')) + return suite + +if __name__ == '__main__': + unittest.main(defaultTest='test_suite') Index: lib/python/AccessControl/requestsecurity.py =================================================================== --- lib/python/AccessControl/requestsecurity.py (revision 0) +++ lib/python/AccessControl/requestsecurity.py (revision 0) @@ -0,0 +1,141 @@ +##################################################################### ######### +# +# Copyright (c) 2006 Zope Corporation and Contributors. +# All Rights Reserved. +# +# This software is subject to the provisions of the Zope Public License, +# Version 2.1 (ZPL). A copy of the ZPL should accompany this distribution. +# THIS SOFTWARE IS PROVIDED "AS IS" AND ANY AND ALL EXPRESS OR IMPLIED +# WARRANTIES ARE DISCLAIMED, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED +# WARRANTIES OF TITLE, MERCHANTABILITY, AGAINST INFRINGEMENT, AND FITNESS +# FOR A PARTICULAR PURPOSE. +# +##################################################################### ######### +"""Request method based access control + +The requestsecurity module contains helper functions to restrict the access +to resources based on the request method. For example you can deny a GET +request to a method that should only be available for forms over POST +requests. + +The restrictions are available in two flavors: whitelist and blacklist: + + o A blacklist contains a single or more methods that are FORBIDDEN. Every + method that is NOT listed is ALLOWED. + + o A whitelist contains a single or more methods that are ALLOWED. Every + method that is NOT listed is DENIED. + +In general a whitelist is more secure than a blacklist but harder to define. + +The blacklistRequestMethod and whitelistRequestMethod are available in +restricted python code like scripts, too. + +>>> from AccessControl.ZopeGuards import guarded_import +>>> mod = guarded_import('AccessControl.requestsecurity', +... fromlist=('blacklistRequestMethod', )) +>>> mod = guarded_import('AccessControl.requestsecurity', +... fromlist=('whitelistRequestMethod', )) + +Examples: + +boiler plate +>>> context = object() +>>> name = 'testobject' +>>> request = {'REQUEST_METHOD' : 'GET', +... 'URL' : '/testobject' } + +Black list tests +>>> blacklistRequestMethod(context, 'GET', request=request, name=name) +Traceback (most recent call last): +... +Unauthorized: GET request to 'testobject' is not allowed. + +>>> blacklistRequestMethod(context, 'POST', request=request, name=name) + +>>> blacklistRequestMethod(context, ('POST', 'GET'), request=request) +Traceback (most recent call last): +... +Unauthorized: GET request to '/testobject' is not allowed. + +White lists are working the opposite way +>>> whitelistRequestMethod(context, 'GET', request=request, name=name) + +>>> whitelistRequestMethod(context, 'POST', request=request, name=name) +Traceback (most recent call last): +... +Unauthorized: GET request to 'testobject' is not allowed. + +>>> whitelistRequestMethod(context, ('POST', 'GET'), request=request, name=name) + +$Id: $ +""" +from AccessControl.unauthorized import Unauthorized + + +def blacklistRequestMethod(context, blacklist='GET', request=None, name=None): + """Deny access with forbidden request methods + """ + return _restrictRequestMethod(context, blacklist=blacklist, + request=request, name=name) + +def whitelistRequestMethod(context, whitelist='POST', request=None, name=None): + """Deny access except for allowed request methods + """ + return _restrictRequestMethod(context, whitelist=whitelist, + request=request, name=name) + +__all__ = ('blacklistRequestMethod', 'whitelistRequestMethod',) + +# internal stuff +def _restrictRequestMethod(context, blacklist=None, whitelist=None, + request=None, name=None): + """Helper method for white or blacklisting request methods + + context should be the requested method or object with an acquistion + context. + + whitelist and blacklist can either be a string or a sequence. You can't + define both. + + request is the request object. If no request is given then the request is + acquired from the context + + name is used for the error message. If no name is given then the URL of + the context or repr(context) is used. + """ + if request is None: + request = getattr(context, 'REQUEST', None) + if request is None: + # XXX no request found, what shall I do? + return + + if not name: + try: + name = request.get('URL', None) + if not name: + name = context.absolute_url() + except AttributeError: + name = repr(context) + + req_method = request.get('REQUEST_METHOD', 'GET').upper() + + if not bool(blacklist) ^ bool(whitelist): + raise ValueError("You have to specify either blacklist or whitelist") + + if blacklist: + if isinstance(blacklist, basestring): + blacklist = (blacklist, ) + if req_method in blacklist: + raise Unauthorized("%s request to '%s' is not allowed." + % (req_method, name)) + return + + if whitelist: + if isinstance(whitelist, basestring): + whitelist = (whitelist, ) + if req_method not in whitelist: + raise Unauthorized("%s request to '%s' is not allowed." + % (req_method, name)) + return + Index: lib/python/AccessControl/SecurityInfo.py =================================================================== --- lib/python/AccessControl/SecurityInfo.py (revision 70214) +++ lib/python/AccessControl/SecurityInfo.py (working copy) @@ -42,8 +42,8 @@ from logging import getLogger
import Acquisition - from AccessControl.ImplPython import _what_not_even_god_should_do +from AccessControl.checker import checkClassHasMethod
LOG = getLogger('SecurityInfo')
@@ -160,6 +160,7 @@ # Collect protected attribute names in ac_permissions. ac_permissions = {} for name, access in self.names.items(): + checkClassHasMethod(classobj, name, log=True) if access in (ACCESS_PRIVATE, ACCESS_PUBLIC, ACCESS_NONE): setattr(classobj, '%s__roles__' % name, access) else: Index: lib/python/AccessControl/decorator.py =================================================================== --- lib/python/AccessControl/decorator.py (revision 0) +++ lib/python/AccessControl/decorator.py (revision 0) @@ -0,0 +1,144 @@ +##################################################################### ######### +# +# Copyright (c) 2006 Zope Corporation and Contributors. +# All Rights Reserved. +# +# This software is subject to the provisions of the Zope Public License, +# Version 2.1 (ZPL). A copy of the ZPL should accompany this distribution. +# THIS SOFTWARE IS PROVIDED "AS IS" AND ANY AND ALL EXPRESS OR IMPLIED +# WARRANTIES ARE DISCLAIMED, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED +# WARRANTIES OF TITLE, MERCHANTABILITY, AGAINST INFRINGEMENT, AND FITNESS +# FOR A PARTICULAR PURPOSE. +# +##################################################################### ######### +"""Security decorators + +Security decorators are an alternative syntax to define security declarations +on classes. + +>>> from ExtensionClass import Base +>>> from AccessControl import ClassSecurityInfo +>>> from AccessControl.decorator import declarePublic +>>> from AccessControl.decorator import declarePrivate +>>> from AccessControl.decorator import declareProtected +>>> from AccessControl.Permissions import view as View +>>> from Globals import InitializeClass + +>>> class DecoratorExample(Base): +... '''decorator example''' +... +... security = ClassSecurityInfo() +... +... @declarePublic +... def publicMethod(self): +... "public method" +... +... @declarePrivate +... def privateMethod(self): +... "private method" +... +... @declareProtected(View) +... def protectedByView(self): +... "method protected by View" +... +>>> InitializeClass(DecoratorExample) + +$Id: $ +""" +import sys + +def declarePublic(method): + """Declare names to be publicly accessible. + + Returns the same method object + """ + __security_decorator__ = True + security = _getSecurityInfoFromStack() + name = _getFunctionName(method) + security.declarePublic(name) + return method + +def declarePrivate(method): + """Declare names to be inaccessible to restricted code. + + Returns the same method object + """ + __security_decorator__ = True + security = _getSecurityInfoFromStack() + name = _getFunctionName(method) + security.declarePrivate(name) + return method + +def declareProtected(permission_name): + """Declare names to be associated with a permission. + + Returns the same method object + """ + __security_decorator__ = True + security = _getSecurityInfoFromStack() + def wrapper(method): + name = _getFunctionName(method) + security.declareProtected(permission_name, name) + return method + return wrapper + +__all__ = ('declarePublic', 'declarePrivate', 'declareProtected') + +# internal helper functions + +def _getSecurityInfoFromStack(): + """Get the security object from the caller stack of a decorator + + There is no direct way to access class variables from a decorator method. + This method tries to get the security var from the caller stack. Yeah it + is a bit hacky but even zope.interface's implements() is using the same + trick. + + If the local namespace contains a var __security_decorator__ then the + functions is trying to get the security object from the next stack level. + This allows the stacking of two or more decorators. The + __security__decorator__ var minimizes the risk to acquire a different + security object from the stack. + """ + # frame 1 is the decorator function, 2 is the class unless you use more + # than one decorator. + index = 2 + while True: + frame = sys._getframe(index) + loc = frame.f_locals + security = loc.get('security', None) + sd = loc.has_key('__security_decorator__') + del loc # delete frame and locals to avoid memory leaks + del frame + if security and hasattr(security, '__security_info__'): + return security + if sd: + index+=1 + continue + raise TypeError('A security decorator must be defined on a method ' + 'of a class with a security information object!') + +def _getFunctionName(func): + """Get function name from a method, function or classfunction + + >>> def function(): pass + >>> class Foo(object): + ... def method(self): pass + ... @classmethod + ... def clsmethod(cls): pass + >>> foo = Foo() + + >>> _getFunctionName(function) + 'function' + >>> _getFunctionName(Foo.method) + 'method' + >>> _getFunctionName(Foo.clsmethod) + 'clsmethod' + >>> _getFunctionName(foo.method) + 'method' + >>> _getFunctionName(foo.clsmethod) + 'clsmethod' + """ + func = getattr(func, 'im_func', func) + return func.func_name + Index: lib/python/ZPublisher/BaseRequest.py =================================================================== --- lib/python/ZPublisher/BaseRequest.py (revision 70214) +++ lib/python/ZPublisher/BaseRequest.py (working copy) @@ -18,6 +18,7 @@ import xmlrpc from zExceptions import Forbidden, Unauthorized, NotFound from Acquisition import aq_base +from AccessControl.checker import checkObjectHasSecurityInfo
from zope.interface import implements, providedBy, Interface from zope.component import queryMultiAdapter @@ -126,6 +127,10 @@ "published." % URL )
+ #from Globals import DevelopmentMode + #if DevelopmentMode: + # checkObjectHasSecurityInfo(subobject, log=True) + # Hack for security: in Python 2.2.2, most built-in types # gained docstrings that they didn't have before. That caused # certain mutable types (dicts, lists) to become publishable Index: lib/python/Shared/DC/ZRDB/DA.py =================================================================== --- lib/python/Shared/DC/ZRDB/DA.py (revision 70214) +++ lib/python/Shared/DC/ZRDB/DA.py (working copy) @@ -113,7 +113,7 @@ security.declareProtected(view_management_screens, 'manage_advancedForm') manage_advancedForm=DTMLFile('dtml/advanced', globals())
- security.declarePublic('test_url') + security.declarePublic('test_url_') def test_url_(self): 'Method for testing server connection information' return 'PING' Index: lib/python/webdav/Lockable.py =================================================================== --- lib/python/webdav/Lockable.py (revision 70214) +++ lib/python/webdav/Lockable.py (working copy) @@ -43,7 +43,7 @@ # Protect methods using declarative security security = ClassSecurityInfo() security.declarePrivate('wl_lockmapping') - security.declarePublic('wl_isLocked', 'wl_getLock', 'wl_isLockedByUser', + security.declarePublic('wl_isLocked', 'wl_getLock', 'wl_lockItems', 'wl_lockValues', 'wl_lockTokens',) security.declareProtected('WebDAV Lock items', 'wl_setLock') security.declareProtected('WebDAV Unlock items', 'wl_delLock') _______________________________________________ Zope-Dev maillist - Zope-Dev@zope.org http://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - http://mail.zope.org/mailman/listinfo/zope-announce http://mail.zope.org/mailman/listinfo/zope )
Christian Heimes wrote:
Zope's security declarations have to be called with a method *name* AS STRING. Developers are human beeings and human beeings tend to make small errors like typos. Or they forget to change the security declaration when they rename a method. Zope doesn't raise an error when a developer adds a security declaration for a non existing method.
Have a look at the following example. It contains a tiny but devastating typo::
security.declarePrivate('chooseProtocol') def chooseProtocols(self, request): ...
These kinds or errors are extremly hard to find and may lead to big security holes. By the way this example was taken from a well known and well tested Zope addon!
THis problem is easily adressed when using ZCML security declarations + interfaces. Your decorator idea would also work.
Issue 2 =======
Another way to introduce security breaches is to forget the InitializeClass() call. The call is responsable for applying the informations from a ClassSecurityInfo. Without the call all security settings made through security.declare* are useless.
The good news is: Even if a developer forgets to call the method explictly in his code the function is called implictly.
The implicit call was hard to find for me at first. I struggled with the code in OFS.Image because there is an import of InitializeClass but it is never called. Never the less the security informations are somehow merged automagically. After some digging I found the code that is responsible for the magic. It's ExtensionClass' __class_init__ magic and OFS.ObjectManager.ObjectManager.__class_init__
Now the bad news: The magic doesn't work under some conditions. For example if you class doesn't subclass ObjectManager or if you monkey patch a class with security info object you have to call InitializeClass explictly.
The InitializeClass call is a big problem. I once discovered a big CMF security breach because of a missing InitializeClass call. Magic isn't a solution here, though. ZCML, for example, calls InitializeClass every time it changes the security declarations for a class. If you use ZCML, you're on the safe side.
Issue 4 =======
Some methods shouldn't be callable from certain types of request methods. For example there is no need that the webdav DELETE and PROP* methods should be callable from an ordinary HTTP GET request. I don't want to go into details. Some people know why :)
I have no idea how you want to distinguish HTTP and WebDAV requests. They're pretty much the same. But you're right, Zope allows too much. I'm not convinced this is a security responsibility though. I think in the long term we'll adopt a solution like in Zope 3: look up views based on the request type.
Only a small subset is implemented. There are two methods in my patch to either whitelist or blacklist a request method. An older version of my patch contained even code to distinguish between request types (ftp, http, ftp, xml-rpc) but Jim told me in a private mail it's kind of YAGNI.
I wonder what he meant by that. In Zope 3 we certainly do it. In general, I think a sane way to improve Zope's security system is to adopt certian Zope 3 idioms until we essentially are using Zope 3's security machinery, but probably with a Zope 2 specific security policy. I find that Zope 3's approach with security proxies is a *much* easier approach than manual guarded_getattr etc., and Zope 3's checkers are much more flexible than Zope 2's ClassSecurityInfo stuff. I personally think that efforts in improving Zope 2 security should primarily look at backporting useful features from Zope 3. That doesn't mean Zope 3 has to always have the right way of doing it, but at least changes in Zope 2 shouldn't go without an investigation of how Zope 3 does it. Philipp
participants (6)
-
Chris McDonough -
Christian Heimes -
Dieter Maurer -
Lennart Regebro -
Philipp von Weitershausen -
Sidnei da Silva