RFC: backward compatibility of PythonScript bindings for 2.6.4 / 2.7.0
Hi all - We recently made Zope 2.6.4-rc1 and 2.7.0-rc1 releases which fix a number of issues that were introduced in the Q4 security audit work and the resulting 2.6.3 and 2.7.0-b4 releases. The initial feedback is very good, and at this point I am aware of only one outstanding issue - but it is a sticky one and I want to get some community feedback before deciding exactly how to address it. Background: one of the holes fixed in the security audit was that the current user's access to 'context' and 'container' in a Python Script was not checked when those names were bound to the script for execution. This meant that theoretically a script could have a handle to an object (like the script container) that the user really didn't have permission for. Subsequent attribute accesses or attempted method calls would likely still fail since a security check *would* be performed on those actions, but in principle the script shouldn't have been allowed to get that initial handle to the object. The fix for this was straightforward - do a security check on 'context' and 'container' before binding those objects when the script is getting ready to execute. While this is definitely the Right Thing To Do (tm) from a security standpoint, it poses a bit of a backwards compatibility problem because: - By default, all Python Scripts bind both 'context' and 'container'. This means that all existing Python Scripts in applications, portal tools, etc. have those bindings unless they have been manually changed. - A standard CMF / Plone idiom is to put scripts inside of portal tools (workflow is a primary example). Very often the users who execute the scripts (as a part of workflow, for example) **do not have access to the tool containing the scripts**. These two facts did not cause a problem before, because the security check for the 'container' binding was not being performed at bind-time. Now that it is, many scripts will not work because the user will not have access to the tool containing the script. Worse, the binding check will cause an unauthorized error **even if the script doesn't actually use the 'container' variable, because all existing Python Scripts already have that binding defined by default ;( So as things stand, the way the binding check currently works will cause problems for most everyone using a workflow tool or other tools containing scripts where the user doesn't have access to the script container (the tool itself). This is likely to be a fairly large number of people / sites. I'd like to propose a solution that will hopefully affect far fewer people, while still doing the right thing with regard to security. Currently, if the executing user does not have access to either 'context' or 'container', an Unauthorized exception will be raised at bind-time. *This will happen even if the script does not use either of those names*, which is the root of the problem. I propose to change that behavior so that if the security check on either 'context' or 'container' fails, that name will be bound to None rather than raise an Unauthorized. The effect of that change on existing sites would be: - Existing scripts living in tools which *don't* reference 'container' in their code will continue to work as before without any changes on the part of the site admin - Existing scripts that *do* refer to 'container' in their code (and if the user doesn't have access to it) will break, since 'container' will now be 'None' rather than what the script thought it would be. The fix for such sites is to make sure the user has the appropriate access on the script container. What I like about this is that it potentially requires direct action from fewer site admins. IOW, you only have to fix your permissions if you were already doing something that the security system should have been preventing before. What I don't like is that it is somewhat magical, and now the error you would get (probably 'None has no attribute xxx') if the user doesn't have access to the container doesn't tell you the real problem. That is likely to be compounded by the fact that some site admins will be saved from having to care immediately by this magic, then run into it later, when they might be thoroughly confused if they didn't happen to read this thread as it went by ;) The alternative is to take the 'hard-line' and say that site admins need to visit their script containers and ensure that either a) the users of the scripts have the appropriate permissions on the container or b) they remove the 'container' binding from their scripts if it is not appropriate to give the users access to the script container. Sorry this was so long, but I think these kinds of backward compatibility vs. consistency conundrums require some community buy-in on the path chosen, since *someone* will be unhappy and have to do some manual fixing no matter what. Thoughts? Brian Lloyd brian@zope.com V.P. Engineering 540.361.1716 Zope Corporation http://www.zope.com
Hi! Brian Lloyd wrote:
What I don't like is that it is somewhat magical, and now the error you would get (probably 'None has no attribute xxx') if the user doesn't have access to the container doesn't tell you the real problem.
Could we add a ContainerDummy object instead of None that raises a less magical error? And maybe log a deprecation warning?
The alternative is to take the 'hard-line' and say that site admins need to visit their script containers and ensure that either a) the users of the scripts have the appropriate permissions on the container or b) they remove the 'container' binding from their scripts if it is not appropriate to give the users access to the script container.
I think this should not be done before Zope 2.8. Cheers, Yuppie
On Wed, 2004-01-21 at 10:42, Brian Lloyd wrote:
What I don't like is that it is somewhat magical, and now the error you would get (probably 'None has no attribute xxx') if the user doesn't have access to the container doesn't tell you the real problem.
What if you used a special object that would produce a useful error message if the user tries to access the container. Assuming an access involves an attribute access: class UnauthorizedContext: def __getattr__(self, attr): raise Unauthorized("user does not have access to context") I'm sure the details aren't right, but I think the idea is clear enough. Jeremy
Jeremy Hylton wrote:
What if you used a special object that would produce a useful error message if the user tries to access the container.
I like this. Make it a singleton, and put it in the global namespace for Scripts, so that we can write: if context is Inaccessible: # Do without access to context Cheers, Evan @ 4-am
Jeremy Hylton wrote:
What if you used a special object that would produce a useful error message if the user tries to access the container.
I like this. Make it a singleton, and put it in the global namespace for Scripts, so that we can write:
if context is Inaccessible: # Do without access to context
I've checked in the changes to the 2.6 branch, 2.7 branch and the head to change the binding behavior for 'container' and 'context': - If the user does not have access to the item, the script will bind an UnauthorizedBinding object instead of the real object, rather than throw an exception at binding time. - Any attribute or item access on the UnauthorizedBinding will throw an Unauthorized, including the name of the binding that the user didn't have access to. The result is that if you have scripts where the script container is inaccessible to the users of the script: - If the script does not reference 'container' in its code, things will work without any action on the part of the site admin - If the script *does* reference 'container' then a meaningful Unauthorized error will be raised. Site admins can either give users the appropriate roles on the script container or give appropriate proxy roles to the scripts to fix any problems. Note that I *didn't* put the UnauthorizedBinding in the script globals to implement the Inaccessible idea above, because: - it is kind of 'featurish', at least in that it really should have some associated documentation etc. - I want to make only absolutely necessary changes at this point and get 2.6.4 and 2.7.0 finalized. If any of the Plone folk who have been running into this issue can try the changes from cvs, I'd appreciate it. thx, Brian Lloyd brian@zope.com V.P. Engineering 540.361.1716 Zope Corporation http://www.zope.com
On Wed, Jan 21, 2004 at 02:06:24PM -0500, Brian Lloyd wrote:
I've checked in the changes to the 2.6 branch, 2.7 branch and the head to change the binding behavior for 'container' and 'context':
- If the user does not have access to the item, the script will bind an UnauthorizedBinding object instead of the real object, rather than throw an exception at binding time.
- Any attribute or item access on the UnauthorizedBinding will throw an Unauthorized, including the name of the binding that the user didn't have access to.
This sounds reasonable. Are you going to do another set of RC releases? I'd like to run our dev environment on 2.6.4-RCx for a day or two but I don't know when I'll be able to set it up. -- Paul Winkler http://www.slinkp.com Look! Up in the sky! It's IN TEMPESTUOUS LEGIONAIRE! (random hero from isometric.spaceninja.com)
Brian Lloyd wrote:
Jeremy Hylton wrote:
What if you used a special object that would produce a useful error message if the user tries to access the container.
I like this. Make it a singleton, and put it in the global namespace for Scripts, so that we can write:
if context is Inaccessible: # Do without access to context
I've checked in the changes to the 2.6 branch, 2.7 branch and the head to change the binding behavior for 'container' and 'context':
- If the user does not have access to the item, the script will bind an UnauthorizedBinding object instead of the real object, rather than throw an exception at binding time.
- Any attribute or item access on the UnauthorizedBinding will throw an Unauthorized, including the name of the binding that the user didn't have access to.
The result is that if you have scripts where the script container is inaccessible to the users of the script:
- If the script does not reference 'container' in its code, things will work without any action on the part of the site admin
- If the script *does* reference 'container' then a meaningful Unauthorized error will be raised. Site admins can either give users the appropriate roles on the script container or give appropriate proxy roles to the scripts to fix any problems.
Note that I *didn't* put the UnauthorizedBinding in the script globals to implement the Inaccessible idea above, because:
- it is kind of 'featurish', at least in that it really should have some associated documentation etc.
- I want to make only absolutely necessary changes at this point and get 2.6.4 and 2.7.0 finalized.
If any of the Plone folk who have been running into this issue can try the changes from cvs, I'd appreciate it.
thx,
Brian Lloyd brian@zope.com V.P. Engineering 540.361.1716 Zope Corporation http://www.zope.com
_______________________________________________ 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 )
I did check with a fresh 2.6 xx A DCWorkflow script that was not not called with the version from a few hours ago is now called but produces the following traceback This happens when the container binding is set to "container" and also when it is cleared. Traceback (innermost last): Module ZPublisher.Publish, line 98, in publish Module ZPublisher.mapply, line 88, in mapply Module ZPublisher.Publish, line 39, in call_object Module Products.CMFCore.FSPythonScript, line 92, in __call__ Module Shared.DC.Scripts.Bindings, line 298, in __call__ Module Shared.DC.Scripts.Bindings, line 329, in _bindAndExec Module Products.CMFCore.FSPythonScript, line 126, in _exec - __traceback_info__: ({'traverse_subpath': [], 'container': <PloneSite instance at 95efa58>, 'context': <PloneFolder instance at 9615280>, 'script': <FSPythonScript at /zehnder/zehnder/createObject used for /zehnder/zehnder/tasklist/Task.2004-01-21.1914/Attachments>}, (None, 'File', None), {}, (None, None, None)) Module None, line 12, in createObject Module Products.CMFCore.PortalFolder, line 362, in invokeFactory Module Products.CMFCore.TypesTool, line 824, in constructContent Module Products.CMFCore.TypesTool, line 516, in constructInstance Module Products.CMFCore.TypesTool, line 420, in _finishConstruction Module Products.CMFCore.CMFCatalogAware, line 101, in notifyWorkflowCreated Module Products.CMFPlone.WorkflowTool, line 26, in notifyCreated Module Products.CMFCore.WorkflowTool, line 362, in notifyCreated Module Products.DCWorkflow.DCWorkflow, line 367, in notifyCreated Module Products.DCWorkflow.DCWorkflow, line 440, in _changeStateOf Module Products.DCWorkflow.DCWorkflow, line 543, in _executeTransition Module Shared.DC.Scripts.Bindings, line 298, in __call__ Module Shared.DC.Scripts.Bindings, line 329, in _bindAndExec Module Products.PythonScripts.PythonScript, line 311, in _exec Module None, line 1, in setTaskOwner - <PythonScript at /zehnder/zehnder/portal_workflow/ZWorkflow/scripts/setTaskOwner> - Line 1 AttributeError: StateChangeInfo instance has no attribute 'getPhysicalRoot' Robert
I did check with a fresh 2.6 xx A DCWorkflow script that was not not called with the version from a few hours ago is now called but produces the following traceback
This happens when the container binding is set to "container" and also when it is cleared.
Traceback (innermost last): Module ZPublisher.Publish, line 98, in publish Module ZPublisher.mapply, line 88, in mapply Module ZPublisher.Publish, line 39, in call_object Module Products.CMFCore.FSPythonScript, line 92, in __call__ Module Shared.DC.Scripts.Bindings, line 298, in __call__ Module Shared.DC.Scripts.Bindings, line 329, in _bindAndExec Module Products.CMFCore.FSPythonScript, line 126, in _exec - __traceback_info__: ({'traverse_subpath': [], 'container': <PloneSite instance at 95efa58>, 'context': <PloneFolder instance at 9615280>, 'script': <FSPythonScript at /zehnder/zehnder/createObject used for /zehnder/zehnder/tasklist/Task.2004-01-21.1914/Attachments>}, (None, 'File', None), {}, (None, None, None)) Module None, line 12, in createObject Module Products.CMFCore.PortalFolder, line 362, in invokeFactory Module Products.CMFCore.TypesTool, line 824, in constructContent Module Products.CMFCore.TypesTool, line 516, in constructInstance Module Products.CMFCore.TypesTool, line 420, in _finishConstruction Module Products.CMFCore.CMFCatalogAware, line 101, in notifyWorkflowCreated Module Products.CMFPlone.WorkflowTool, line 26, in notifyCreated Module Products.CMFCore.WorkflowTool, line 362, in notifyCreated Module Products.DCWorkflow.DCWorkflow, line 367, in notifyCreated Module Products.DCWorkflow.DCWorkflow, line 440, in _changeStateOf Module Products.DCWorkflow.DCWorkflow, line 543, in _executeTransition Module Shared.DC.Scripts.Bindings, line 298, in __call__ Module Shared.DC.Scripts.Bindings, line 329, in _bindAndExec Module Products.PythonScripts.PythonScript, line 311, in _exec Module None, line 1, in setTaskOwner - <PythonScript at /zehnder/zehnder/portal_workflow/ZWorkflow/scripts/setTaskOwner> - Line 1 AttributeError: StateChangeInfo instance has no attribute 'getPhysicalRoot'
Robert
It would be helpful if you could go through that in the debugger and see if you can get any more info - I don't see anything in this that obviously points to any of the recent security changes. That's not to say that one of those changes couldn't still be the cause, but this traceback doesn't point to anything we can look at :( Alternatively, if you can make a copy of the failing script and boil it down to the minimum possible code that demostrates something that should be working but isn't (and that excludes app-specific or Plone objects if possible so that we can turn it into a unit test) I can try to look at it here. thx, Brian Lloyd brian@zope.com V.P. Engineering 540.361.1716 Zope Corporation http://www.zope.com
Brian Lloyd wrote:
I did check with a fresh 2.6 xx A DCWorkflow script that was not not called with the version from a few hours ago is now called but produces the following traceback
This happens when the container binding is set to "container" and also when it is cleared.
Traceback (innermost last): Module ZPublisher.Publish, line 98, in publish Module ZPublisher.mapply, line 88, in mapply Module ZPublisher.Publish, line 39, in call_object Module Products.CMFCore.FSPythonScript, line 92, in __call__ Module Shared.DC.Scripts.Bindings, line 298, in __call__ Module Shared.DC.Scripts.Bindings, line 329, in _bindAndExec Module Products.CMFCore.FSPythonScript, line 126, in _exec - __traceback_info__: ({'traverse_subpath': [], 'container': <PloneSite instance at 95efa58>, 'context': <PloneFolder instance at 9615280>, 'script': <FSPythonScript at /zehnder/zehnder/createObject used for /zehnder/zehnder/tasklist/Task.2004-01-21.1914/Attachments>}, (None, 'File', None), {}, (None, None, None)) Module None, line 12, in createObject Module Products.CMFCore.PortalFolder, line 362, in invokeFactory Module Products.CMFCore.TypesTool, line 824, in constructContent Module Products.CMFCore.TypesTool, line 516, in constructInstance Module Products.CMFCore.TypesTool, line 420, in _finishConstruction Module Products.CMFCore.CMFCatalogAware, line 101, in notifyWorkflowCreated Module Products.CMFPlone.WorkflowTool, line 26, in notifyCreated Module Products.CMFCore.WorkflowTool, line 362, in notifyCreated Module Products.DCWorkflow.DCWorkflow, line 367, in notifyCreated Module Products.DCWorkflow.DCWorkflow, line 440, in _changeStateOf Module Products.DCWorkflow.DCWorkflow, line 543, in _executeTransition Module Shared.DC.Scripts.Bindings, line 298, in __call__ Module Shared.DC.Scripts.Bindings, line 329, in _bindAndExec Module Products.PythonScripts.PythonScript, line 311, in _exec Module None, line 1, in setTaskOwner - <PythonScript at /zehnder/zehnder/portal_workflow/ZWorkflow/scripts/setTaskOwner> - Line 1 AttributeError: StateChangeInfo instance has no attribute 'getPhysicalRoot'
Robert
It would be helpful if you could go through that in the debugger and see if you can get any more info - I don't see anything in this that obviously points to any of the recent security changes.
That's not to say that one of those changes couldn't still be the cause, but this traceback doesn't point to anything we can look at :(
Alternatively, if you can make a copy of the failing script and boil it down to the minimum possible code that demostrates something that should be working but isn't (and that excludes app-specific or Plone objects if possible so that we can turn it into a unit test) I can try to look at it here.
thx,
Brian Lloyd brian@zope.com V.P. Engineering 540.361.1716 Zope Corporation http://www.zope.com
Brian, I tried hard to recreate the problem in isolation but failed. So it must be something whith what we are doing. Strange that our code works fine with 2.62. thanks again Robert
Brian Lloyd wrote at 2004-1-22 10:11 -0500:
I did check with a fresh 2.6 xx A DCWorkflow script that was not not called with the version from a few hours ago is now called but produces the following traceback
This happens when the container binding is set to "container" and also when it is cleared. ... - <PythonScript at /zehnder/zehnder/portal_workflow/ZWorkflow/scripts/setTaskOwner> - Line 1 AttributeError: StateChangeInfo instance has no attribute 'getPhysicalRoot'
This problem has nothing to do with the recent security enhancements: "StateChangeInfo" does not have a "getPhysicalRoot" attribute. It never had and it should not have... Check your "setTaskOwner" script. Almost surely, you do not want to call "getPhysicalRoot" on the "state_change" but on the affected object. -- Dieter
Dieter Maurer wrote:
Brian Lloyd wrote at 2004-1-22 10:11 -0500:
I did check with a fresh 2.6 xx A DCWorkflow script that was not not called with the version from a few hours ago is now called but produces the following traceback
This happens when the container binding is set to "container" and also when it is cleared.
...
- <PythonScript at /zehnder/zehnder/portal_workflow/ZWorkflow/scripts/setTaskOwner> - Line 1 AttributeError: StateChangeInfo instance has no attribute 'getPhysicalRoot'
This problem has nothing to do with the recent security enhancements:
"StateChangeInfo" does not have a "getPhysicalRoot" attribute. It never had and it should not have...
Check your "setTaskOwner" script. Almost surely, you do not want to call "getPhysicalRoot" on the "state_change" but on the affected object.
I'm afraid this code does have to do with the new security: the code which is failing is actually inside ZopeSecurityPolicy.validate:: if proxy_roles: # Verify that the owner actually can state the proxy role # in the context of the accessed item; users in subfolders # should not be able to use proxy roles to access items # above their subfolder! owner = eo.getOwner() # Sigh; the default userfolder doesn't return users wrapped if owner and not hasattr(owner, 'aq_parent'): udb=eo.getOwner(1)[0] root=container.getPhysicalRoot() udb=root.unrestrictedTraverse(udb) owner=owner.__of__(udb) The script being called is invoked during a workflow transition, which calls the script, passing a StateChangeInfo instance; the new code tries to validate access to the SCI and pukes, badly, because the script has proxy roles, but the 'container' isn't wrapped. I *think* that we are seeing this issue because the new security code is tougher about checking access within the script. I tried to work around the issue by adding a 'getPhysicalRoot' method to SCI, merely delegating to the underlying object; however, the script in my app then blows up on access to 'object' (which I may be able to fix as well). Tres. -- =============================================================== Tres Seaver tseaver@zope.com Zope Corporation "Zope Dealers" http://www.zope.com
Jeremy Hylton wrote at 2004-1-21 11:44 -0500:
On Wed, 2004-01-21 at 10:42, Brian Lloyd wrote:
What I don't like is that it is somewhat magical, and now the error you would get (probably 'None has no attribute xxx') if the user doesn't have access to the container doesn't tell you the real problem.
What if you used a special object that would produce a useful error message if the user tries to access the container. Assuming an access involves an attribute access:
class UnauthorizedContext:
def __getattr__(self, attr): raise Unauthorized("user does not have access to context")
I'm sure the details aren't right, but I think the idea is clear enough.
+1 -- Dieter
On Wed, Jan 21, 2004 at 10:42:11AM -0500, Brian Lloyd wrote:
These two facts did not cause a problem before, because the security check for the 'container' binding was not being performed at bind-time.
One question - what *is* bind time? Is the container bound when the script is called? -- Paul Winkler http://www.slinkp.com Look! Up in the sky! It's THE PIDDLE! (random hero from isometric.spaceninja.com)
These two facts did not cause a problem before, because the security check for the 'container' binding was not being performed at bind-time.
One question - what *is* bind time? Is the container bound when the script is called?
Yes - all of the bindings are figured out at the time the script is called. Brian Lloyd brian@zope.com V.P. Engineering 540.361.1716 Zope Corporation http://www.zope.com
participants (8)
-
Brian Lloyd -
Dieter Maurer -
Evan Simpson -
Jeremy Hylton -
Paul Winkler -
robert rottermann -
Tres Seaver -
yuppie