[ZCM] [ZC] 1774/ 9 Resolve "fix for checkPermission"
Collector: Zope Bugs, Features,
and Patches ...
zope-coders-admin at zope.org
Thu Dec 1 17:49:31 EST 2005
Issue #1774 Update (Resolve) "fix for checkPermission"
Status Resolved, Zope/bug+solution medium
To followup, visit:
http://www.zope.org/Collectors/Zope/1774
==============================================================
= Resolve - Entry #9 by tseaver on Dec 1, 2005 5:49 pm
Status: Pending => Resolved
After profiling, I've ripped out the C version of
'checkPermission' altogether, mixing in the Python class.
Now checked in to the 2.8 branch:
- http://svn.zope.org/Zope/?rev=40459&view=rev
the 2.9 branch:
- http://svn.zope.org/Zope/?rev=40460&view=rev
and the head:
- http://svn.zope.org/Zope/?rev=40461&view=rev
________________________________________
= Comment - Entry #8 by tseaver on Nov 30, 2005 7:34 pm
OK, I have checked in the current stuff (my patch, Yuppies tests,
Jim's feedback) at:
svn+ssh://svn.zope.org/repos/main/Zope/branches/tseaver-collector_1774
I *think* this is ready to merge to the 2.8 and 2.9 branches and the trunk (Yuppie's new tests pass, for instance).
________________________________________
= Comment - Entry #7 by yuppie on Nov 30, 2005 1:56 pm
Uploaded: "tests_for_updated-collector_1774.patch"
- http://www.zope.org/Collectors/Zope/1774/tests_for_updated-collector_1774.patch/view
Uploading a patch for the patch that adds two new tests. They exercise the two issues mentioned in entry #6. One test still fails because updated-collector_1774.patch uses getOwner() where I proposed to use getWrappedOwner().
________________________________________
= Comment - Entry #6 by tseaver on Nov 30, 2005 8:15 am
Uploaded: "updated-collector_1774.patch"
- http://www.zope.org/Collectors/Zope/1774/updated-collector_1774.patch/view
Yuppie replies:
> I found 2 issues in the
> Python version. My proposed fixes are not tested - I might be missing
> something.
>
>
> 1.) CMF's _checkPermission is based on an older version of
> ZopeSecurityPolicy.validate. While the comment in the C code seems to
> have the right Python code, the actual Python code uses the old lines
> from _checkPermission. I think this should be changed like this:
>
> if proxy_roles:
> - if object is not aq_base(object):
> - if not owner._check_context(object):
> - return 0
> + owner = eo.getWrappedOwner()
> + if owner is not None:
> + if object is not aq_base(object):
> + if not owner._check_context(object):
> + return 0
> for r in proxy_roles:
>
>
> 2.) Proxy roles can limit access. Your code has lost a "return 0" that
> is necessary to implement the same policy as in
> ZopeSecurityPolicy.validate (validate raises an error in this place):
>
> for r in proxy_roles:
> if r in roles:
> return 1
> + return 0
> return result
I'm uploading an adjusted version of the patch incorporating
those changes (although I'm not sure how to test them).
________________________________________
= Comment - Entry #5 by tseaver on Nov 29, 2005 10:17 pm
Uploaded: "collector_1774.patch"
- http://www.zope.org/Collectors/Zope/1774/collector_1774.patch/view
OK, I have worked up a version of the patch against the 2.8
branch (it applies cleanly to 2.9 and the trunk, as well).
I need somebody to review the C part for refcount goofs; otherwise,
it should be ready to check in.
________________________________________
= Comment - Entry #4 by tseaver on May 12, 2005 8:29 am
Uploaded: "AccessControl.patch"
- http://www.zope.org/Collectors/Zope/1774/AccessControl.patch/view
Uploading an updated patch from the submitter, received via
e-mail:
Here's the updated patch. It's been manually tested on our end,
but we haven't written tests for it, obviously. I'll see if I
can someone working on that. It is pretty darn close to the
code in validate (95% I would say) though...
The patch applies cleanly to the head of the 2.7 branch, and does
not cause any tests to fail. It does *not* apply cleanly to the
SVN trunk, but that isn't too surprising at this point:
[/home/tseaver/projects/Zope-CVS/Zope-SVN-trunk/lib/python/AccessControl]
$ patch --dry-run -p1 < /tmp/AccessControl.patch
patching file ImplPython.py
Hunk #1 succeeded at 342 (offset 4 lines).
patching file cAccessControl.c
Hunk #1 succeeded at 1298 (offset 22 lines).
Hunk #2 FAILED at 1318.
1 out of 2 hunks FAILED -- saving rejects to file cAccessControl.c.rej
________________________________________
= Comment - Entry #3 by tseaver on May 6, 2005 10:48 am
Another "substantive" note:
- Patching the tests with a testcase which fails before the
main patch, but succeeds afterwards, ensures that the fix
doesn't get regressed later. Especially for the securit
machinery, such testing is probably critical, and it will
take longer for somebody who doesn't understand the intent
to write than for the original author of the patch.
________________________________________
= Comment - Entry #2 by tseaver on May 6, 2005 10:46 am
Thanks for the patch. A couple of "mechanical" things I've noted which will help with future sumisisons:
- Please don't use tab characters in Zope's Python source.
- If you can, creating a unified diff or a context diff makes
the patch more comprehensible.
And a substantive one:
- The 'ownerous' check should either be dropped, or it should
be enforced across the board (e.g., not short-circuited
by 'context.user_allowed'). The point of that machinery is
to prevent "Trojan horse" attacks, where a malicious-but-
unprivileged user tricks a more privileged user into viewing
a page.
In this case, I'm inclined to drop the fix; the
'checkPermission' API is used only by code which is explicitly
security conscious, and should be expected to handle such
things itself.
________________________________________
= Request - Entry #1 by tmclaugh on May 6, 2005 8:31 am
Uploaded: "checkPermission.zip"
- http://www.zope.org/Collectors/Zope/1774/checkPermission.zip/view
Patches for ImplPython.py and cAccessControl.c to make checkPermission honor Role proxies. Patches against 2.7.5.
==============================================================
More information about the Zope-Collector-Monitor
mailing list