Security-modifiers should be marked with the postonly decorator

[mj]

POST-only protection

With the advent of Zope versions 2.8.9, 2.9.7 and 2.10.3, and the recent Hotfix_20070320 release, Zope now supports a 'postonly' decorator. This decorator limits URL-callable methods to POST requests only, removing a possible XSS-attack vector.

All Plone security-modifiers should be protected by this decorator. This includes PlonePAS and GroupUserFolder code, and python scripts in the skin that use security modifiers directly, both in Plone 2.5 and in 3.0.

I have added the postonly decorator to CMFPlone.utils (in the trunk and in the 2.5 branch), so Plone code can import it from there without having to worry about Zope versions.

Where to apply

Someone will have to audit all Plone code for security modifiers, such as adding, removing and editing users, groups, roles and local roles.

Note that the decorator only protects against calling a method through a direct URL. Calling another method that then calls the protected method is not protected, unless the HTTP Request object is passed along in the REQUEST parameter explicitly.

Applying the decorator

In regular python code, the postonly decorator is used as follows:

1. Find web-callable security methods. The decorator only makes sense when a method can be called directly through a URL. Methods that do not have a docstring, start with an underscore, or are marked with setPrivate are not web-callable.

2. Import the decorator:

• Plone 3.0 can rely on Zope 2.10.3, so import the decorator directly:

from AccessControl.requestmethod import postonly

• Plone 2.5 defines postonly in utils, to avoid verbose import tests. Use this for Plone 2.5 and Plone2.5 versions of 3rd-party Products:

from Products.CMFPlone.utils import postonly

• Products like GroupUserFolder, where the same version is used in multiple Plone versions, it will need to do it's own availability checking:

try:
    # Zope 2.8.9, 2.9.7 and 2.10.3 (and up)
    from AccessControl.requestmethod import postonly
except ImportError:
    try:
        # Try the hotfix too
        from Products.Hotfix_20070320 import postonly
    except ImportError:
        def postonly(callable): return callable

3. Ensure that the method has a REQUEST parameter. If it doesn't, add it as a keyword parameter defaulting to None, the decorator relies on it:

    security.declareProtected(modify_security, 'modifySecuritySettings')
    def modifySecuritySettings(self, key, value, REQUEST=None):
        """Set key to value"""
        self._settings[key] = value

4. Apply the decorator:

• For Plone 3.0 and related products that only run on python2.4 and up, use the new @postonly syntax, directly above the method:

  security.declareProtected(modify_security, 'modifySecuritySettings')
  @postonly
  def modifySecuritySettings(self, key, value, REQUEST=None):
      """Set key to value"""
      self._settings[key] = value

• For Plone 2.5, and any product that still needs to work with python2.3 (like GroupUserFolder (!)), call the decorator directly and replace the method with the result directly after the method body:

  security.declareProtected(modify_security, 'modifySecuritySettings')
  def modifySecuritySettings(self, key, value, REQUEST=None):
      """Set key to value"""
      self._settings[key] = value
  modifySecuritySettings = postonly(modifySecuritySettings)

5) Run the tests; postonly will complain if it cannot find a REQUEST parameter, for example.

Examples

I have checked in some fixes for PlonePAS and GroupUserFolder already, to demonstrate what needs to be done:

• PlonePAS trunk, tools/groups.py (python2.4 syntax)
• PlonePAS 2.x, tools/groups.py and tools/plonetool.py (python2.3 syntax)
• GroupUserFolder trunk, GroupsTool.py (python2.3 syntax)

Python Scripts

Python scripts calling into a POST-only security modifier should pass along the REQUEST parameter:

request = self.REQUEST

portal_security_manager.changeSecuritySettings(key, value, REQUEST=request)

Note that this causes POST protection to kick in whenever the Python script is called, even indirectly! (python script -> python script -> security method) so all use of the script will need to be tested.

Zope3/Five views

It may be that Zope3-style views need to be protected as well, if they can be used to alter security settings. No strategy has been devised yet, please let me know if any such views are found.

[limi]

It probably makes sense to create special branches for these changes, as they span a lot of templates, and it's important that we get them right. With a branch, it's easier to see what changes have been done, and audit them once they are merged back.

It would also be awesome if we could add some testbrowser tests that makes sure that HTTP GETs fail, and HTTP POSTs work. I'm just the UI guy, so I'm not sure how feasible this is, though. ;)

[mj]

Python scripts

Python scripts calling into a POST-only security modifier should pass along the REQUEST parameter:

request = self.REQUEST

portal_security_manager.changeSecuritySettings(key, value, REQUEST=request)

Note that this causes POST protection to kick in whenever the Python script is called, even indirectly! (python script -> python script -> security method) so all use of the script will need to be tested.

[mj]

I moved the python script section into the description, and updated the decorator import step to reflect that Zope 2.10.3 is required for Plone 3.0.

[mj]

Updated import stanzas again, special casing GRUF.

[mj]

Replying to limi:

It probably makes sense to create special branches for these changes, as they span a lot of templates, and it's important that we get them right. With a branch, it's easier to see what changes have been done, and audit them once they are merged back.

Absolutely.

It would also be awesome if we could add some testbrowser tests that makes sure that HTTP GETs fail, and HTTP POSTs work. I'm just the UI guy, so I'm not sure how feasible this is, though. ;)

The Hotfix I built uses ZopeTestCase for this as it was available across all the targeted Zope releases:

class NoGETTest(FunctionalTestCase):
    def afterSetUp(self):
        self.folder_path = '/'+self.folder.absolute_url(1)
        self.setRoles(('Manager',))

    def _onlyPOST(self, path, qstring='', success=200, rpath=None):
        basic_auth = '%s:%s' % (user_name, user_password)
        env = dict()
        if rpath:
            env['HTTP_REFERER'] = self.app.absolute_url() + rpath
        response = self.publish('%s?%s' % (path, qstring), basic_auth, env)
        self.assertEqual(response.getStatus(), 403)

        data = StringIO.StringIO(qstring)
        response = self.publish(path, basic_auth, env, request_method='POST',
                                stdin=data)
        self.assertEqual(response.getStatus(), success)

    def test_userFolderAddUser(self):
        path = self.folder_path + '/acl_users/userFolderAddUser'
        qstring = 'name=foo&password=bar&domains=&roles:list=Manager'
        self._onlyPOST(path, qstring)

    # ... etc ...

_onlyPost can test methods that require a referrer (use rpath='..path..') or return a redirect instead of a page (pass success=302).

I can imagine that for Plone 3.0 more current test standards require Five/Zope3 based functional testing, but I have no example code ready for that yet.

[alecm]

Bloodbare (Ramon Navarro) and I have gone through the 2.5 tree to find methods which are likely to have dangerous consequences when used via a GET in an XSS type attack. The methods we found we're:

CMFCore:
        MembershipTool:
                setRoleMapping
                deleteMemberArea
                setLocalRoles
                deleteLocalRoles
                deleteMembers
CMFPlone:
        PloneTool:
                changeOwnershipOf
                acquireLocalRoles
DCWorkflow:
        WorkflowUIMixin:
                addManagedPermission
                delManagedPermissions
                setRoles
                delGroups
                addGroup
        States:
                setPermissions
                setPermission
                setGroups
        Transitions:
                setProperties
CMFPlacefulWorkflow:
        DefaultWorkflowPolicyDefinition:
                manage_addWorkflowPolicy
                manage_changeWorkflows
                setChainForPortalTypes
                setDefaultChain
                setChain
                delChain
CMFCore:
        WorkflowTool:
                manage_changeWorkflows
                setDefaultChain
                setChainForPortalTypes
GRUF:
        GroupUserFolder:
                setRolesOnUsers
                setGroupOwnership
PlonePAS:
        pas:
                userFolderDelUsers
                userFolderEditUser
                userFolderDelGroups
                userFolderEditGroup
        plugins/role:
                assignRolesToPrincipal

We've also updated all scripts that referred to these methods to pass the newly required REQUEST parameter. This is really just a band-aid though, as there remain a number of non-security related methods that can be used to alter a variety of site data using these means.

This work only applies to plone 2.5, applying these changes to 3.0 should be straightforward. The new views in 3.0 are likely to need updates as well, and a backport to 2.1 should be somewhat feasible (especially since GRUF the base CMF tools and GRUF were updated as well). We'll be working on them tomorrow.

[mj]

I recall seeing vulnerable addMember methods, so I grepped for them. There is only one, but it is vulnerable:

CMFCore
    RegistrationTool
        addMember

[alecm]

Missed a couple

CMFCore/RegistrationTool.py: addMember
CMFDefault/RegistrationTool.py: editMember

[hannosch]

A lot of these methods have been protected by now, not a blocker for 3.0 anymore, but still critical.

[justizin]

CMFPlone.utils patch for Plone 2.1, correspond to http://dev.plone.org/plone/changeset/13898

[justizin]

Attached is postonly-plone-21.patch, a patch to CMFPlone.utils for Plone 2.1 to correspond with:

http://dev.plone.org/plone/changeset/13898

[hannosch]

While working on backporting any of these postonly work to Plone 2.1 we need to keep in mind that Plone 2.1 supports Zope 2.7.x for which there exists neither a Hotfix nor a release with the postonly decorator and there most likely never will be any.

[mj]

Replying to hannosch:

While working on backporting any of these postonly work to Plone 2.1 we need to keep in mind that Plone 2.1 supports Zope 2.7.x for which there exists neither a Hotfix nor a release with the postonly decorator and there most likely never will be any.

The Hotfix has now been tested against 2.7 and works just fine. This does mean that a new Plone 2.1 release would need to include the Hotfix.

[hannosch]

I think all the mentioned methods have been protected by now, if this isn't the case please re-open the ticket. This should probably be turned into some piece of documentation on how to write protected methods...