[Zope-dev] Re: Traversal issue which affects Five

2006-04-17 Thread Florent Guillaume

Hi Alec,

Five traversal is definitely very touchy, and the interactions with Zope 
numerous. So I'm sure the problem you observed is real and that a 
solution must be found. The unit tests you propose would go a long way 
to having a fix committed asap, so yes please, provide one.


Florent

Alec Mitchell wrote:

It seems that the way OFS.Traversable.restrictedTraverse() handles
security checking on objects with __bobo_traverse__ methods is
considerably different from the way it normally checks security.  The
result is that traversal cannot obtain attributes using acquisition
from objects that are marked five:traversable.  In the normal case,
security is checked using guarded_getattr, which gets an attribute and
checks the permissions on the retrieved object in its original
context.  For __bobo_traverse__methods which return simple properties
(say strings), it is impossible to determine the container from which
the returned attribute originates, so unless the attribute was not
acquired, an Unauthorized error will always be raised.

Objects that are Five Traversable always have __bobo_traverse__
methods which attempt to mimic normal traversal, which effectively
means that the security checks end up preventing acquisition of simple
properties using traversal from ever working on these objects (say
using a TAL path expression 'context/attribute' which you expect to be
acquired).  Unfortunately, because Five has no control over the
security checks done during traversal, this cannot be fixed directly
in Five.  However, IMHO fixing this makes sense for Zope itself,
provided there aren't any undesirable consequences.  I propose that if
the validation of a __bobo_traverse result raises Unauthorized, that
we make one last check to see if the result o 'guarded_getattr(obj,
name)' is identical to the result of the __bobo_traverse__ call and
allow it if that's the case.  Here is my proposed patch against Zope
2.9 trunk:

--- Traversable.py  (revision 66323)
+++ Traversable.py  (working copy)
@@ -201,9 +201,18 @@
 else:
 # Can't determine container
 container = _none
-if not securityManager.validate(
-obj, container, name, next):
-raise Unauthorized, name
+try:
+if not securityManager.validate(
+obj, container, name, next):
+raise Unauthorized, name
+except Unauthorized:
+# If next is a simple unwrapped property, it's
+# parentage is indeterminate, but it may have been
+# acquired safely.  In this case validate
will raise
+# an error, and we can check that our value was
+# acquired safely.
+if guarded_getattr(obj, name, marker) is not next:
+raise Unauthorized, name
 else:
 if restricted:
 next = guarded_getattr(obj, name, marker)

At the moment Plone 2.5 is really struggling with this issue, and it
would be wonderful if a fix for this could go into Zope 2.8 and 2.9
soon.  I'm happy to write tests for this, I just want to make sure
that I'm not proposing something really wrong/inappropriate here. 
Generally, the validate() call should return a True or False value, so

this change should have little performance impact except in the case
where 'container == _none' and validate would otherwise raise a very
unhelpful unauthorized error.  Your feedback is much appreciated.

Alec Mitchell
___
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 )




--
Florent Guillaume, Nuxeo (Paris, France)   Director of RD
+33 1 40 33 71 59   http://nuxeo.com   [EMAIL PROTECTED]
___
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 )


[Zope-dev] Re: Traversal issue which affects Five

2006-04-17 Thread Alec Mitchell
On 4/17/06, Florent Guillaume [EMAIL PROTECTED] wrote:
 Hi Alec,

 Five traversal is definitely very touchy, and the interactions with Zope
 numerous. So I'm sure the problem you observed is real and that a
 solution must be found. The unit tests you propose would go a long way
 to having a fix committed asap, so yes please, provide one.
...
 Alec Mitchell wrote:
  It seems that the way OFS.Traversable.restrictedTraverse() handles
  security checking on objects with __bobo_traverse__ methods is
  considerably different from the way it normally checks security.  The
  result is that traversal cannot obtain attributes using acquisition
  from objects that are marked five:traversable.  In the normal case,
  security is checked using guarded_getattr, which gets an attribute and
  checks the permissions on the retrieved object in its original
  context.  For __bobo_traverse__methods which return simple properties
  (say strings), it is impossible to determine the container from which
  the returned attribute originates, so unless the attribute was not
  acquired, an Unauthorized error will always be raised.
 
  Objects that are Five Traversable always have __bobo_traverse__
  methods which attempt to mimic normal traversal, which effectively
  means that the security checks end up preventing acquisition of simple
  properties using traversal from ever working on these objects (say
  using a TAL path expression 'context/attribute' which you expect to be
  acquired).  Unfortunately, because Five has no control over the
  security checks done during traversal, this cannot be fixed directly
  in Five.  However, IMHO fixing this makes sense for Zope itself,
  provided there aren't any undesirable consequences.  I propose that if
  the validation of a __bobo_traverse result raises Unauthorized, that
  we make one last check to see if the result o 'guarded_getattr(obj,
  name)' is identical to the result of the __bobo_traverse__ call and
  allow it if that's the case.  Here is my proposed patch against Zope
  2.9 trunk:

snip old patch

  At the moment Plone 2.5 is really struggling with this issue, and it
  would be wonderful if a fix for this could go into Zope 2.8 and 2.9
  soon.  I'm happy to write tests for this, I just want to make sure
  that I'm not proposing something really wrong/inappropriate here.
  Generally, the validate() call should return a True or False value, so
  this change should have little performance impact except in the case
  where 'container == _none' and validate would otherwise raise a very
  unhelpful unauthorized error.  Your feedback is much appreciated.


Ok, I've attached a refined patch with tests.  Only one of these will
fail with the original version of Traversable.py
(testBoboTraverseToAcquiredAttribute), the other three just make sure
that expected security restrictions and behavior are preserved.  I'll
reiterate that fixing this issue is quite essential for Plone, and
likely for any other reasonably complex Zope 2 project which wishes to
use Five/Zope3 extensively.  I'll file a collector issue later today.

Alec


bobo_traverse_security.diff
Description: Binary data
___
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 )