Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@ CHANGES
4.0.1 (unreleased)
------------------

- TBD
- Added ability to perform checks based on values assigned to
attributes, or operands of in-place mutation operations.

- Added ability to check for attribute deletion separately from
attribute assignment.

4.0.0 (2013-07-09)
------------------
Expand Down
4 changes: 4 additions & 0 deletions docs/api/interfaces.rst
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ Utilities
:members:
:member-order: bysource

.. autointerface:: IValueBasedChecker
:members:
:member-order: bysource

.. autointerface:: ISecurityPolicy
:members:
:member-order: bysource
Expand Down
98 changes: 68 additions & 30 deletions src/zope/security/_proxy.c
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,11 @@ static PyObject *__class__str = 0, *__name__str = 0, *__module__str = 0;
DECLARE_STRING(__3pow__);
DECLARE_STRING(__call__);
DECLARE_STRING(check);
DECLARE_STRING(check_with_value);
DECLARE_STRING(check_getattr);
DECLARE_STRING(check_setattr);
DECLARE_STRING(check_setattr_with_value);
DECLARE_STRING(check_delattr);
DECLARE_STRING(__cmp__);
DECLARE_STRING(__coerce__);
DECLARE_STRING(__contains__);
Expand Down Expand Up @@ -161,25 +164,50 @@ static PyTypeObject SecurityProxyType;
*/

static int
check(SecurityProxy *self, PyObject *meth, PyObject *name)
check(SecurityProxy *self, PyObject *meth, PyObject *name, PyObject *value)
{
PyObject *r;

/* If the checker has __setitem__, we call it's slot rather than
/* If the checker has __setitem__, we call its slot rather than
calling check or check_getattr. Why? Because calling operator slots
is much faster than calling methods and security checks are done so
often that speed matters. So we have this hack of using
almost-arbitrary operations to represent methods that we call
alot. */
a lot. */
if (self->proxy_checker->ob_type->tp_as_mapping != NULL
&& self->proxy_checker->ob_type->tp_as_mapping->mp_ass_subscript != NULL
&& meth != str_check_setattr)
return self->proxy_checker->ob_type->tp_as_mapping->
mp_ass_subscript(self->proxy_checker, self->proxy.proxy_object, name);

r = PyObject_CallMethodObjArgs(self->proxy_checker, meth,
self->proxy.proxy_object, name,
NULL);
if (value != NULL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of style preferences here:

  • Please enclose even single-statement suites in braces (FBO later edits by indenttion-centric Python developers).
  • Can you add / fix comments for each suite? I think the one in line #187 is really for line #203.

&& meth == str_check_setattr
&& (PyObject_HasAttr(self->proxy_checker, str_check_setattr_with_value)
== 1))
// value is NULL if this is really a delattr call
r = PyObject_CallMethodObjArgs(self->proxy_checker,
str_check_setattr_with_value,
self->proxy.proxy_object, name,
value, NULL);
else if (value != NULL
&& meth == str_check
&& PyObject_HasAttr(self->proxy_checker, str_check_with_value) == 1)
// value is NULL if this is really a delattr call
r = PyObject_CallMethodObjArgs(self->proxy_checker,
str_check_with_value,
self->proxy.proxy_object, name,
value, NULL);
else if (value == NULL
&& meth == str_check_setattr
&& PyObject_HasAttr(self->proxy_checker, str_check_delattr) == 1)
r = PyObject_CallMethodObjArgs(self->proxy_checker,
str_check_delattr,
self->proxy.proxy_object, name,
value, NULL);
else
r = PyObject_CallMethodObjArgs(self->proxy_checker, meth,
self->proxy.proxy_object, name,
NULL);
if (r == NULL)
return -1;

Expand Down Expand Up @@ -214,7 +242,7 @@ check1(SecurityProxy *self, PyObject *opname, function1 operation)
{
PyObject *result = NULL;

if (check(self, str_check, opname) >= 0) {
if (check(self, str_check, opname, (PyObject *)NULL) >= 0) {
result = operation(self->proxy.proxy_object);
PROXY_RESULT(self, result);
}
Expand All @@ -229,7 +257,8 @@ check2(PyObject *self, PyObject *other,

if (Proxy_Check(self))
{
if (check((SecurityProxy*)self, str_check, opname) >= 0)
if (check((SecurityProxy*)self, str_check, opname, (PyObject *)NULL)
>= 0)
{
result = operation(((SecurityProxy*)self)->proxy.proxy_object,
other);
Expand All @@ -238,7 +267,8 @@ check2(PyObject *self, PyObject *other,
}
else if (Proxy_Check(other))
{
if (check((SecurityProxy*)other, str_check, ropname) >= 0)
if (check((SecurityProxy*)other, str_check, ropname,
(PyObject *)NULL) >= 0)
{
result = operation(self,
((SecurityProxy*)other)->proxy.proxy_object);
Expand All @@ -261,7 +291,7 @@ check2i(SecurityProxy *self, PyObject *other,
{
PyObject *result = NULL;

if (check(self, str_check, opname) >= 0)
if (check(self, str_check, opname, other) >= 0)
{
result = operation(self->proxy.proxy_object, other);
if (result == self->proxy.proxy_object)
Expand Down Expand Up @@ -372,7 +402,7 @@ proxy_iter(SecurityProxy *self)
{
PyObject *result = NULL;

if (check(self, str_check, str___iter__) >= 0)
if (check(self, str_check, str___iter__, (PyObject *)NULL) >= 0)
{
result = PyObject_GetIter(self->proxy.proxy_object);
PROXY_RESULT(self, result);
Expand All @@ -385,7 +415,7 @@ proxy_iternext(SecurityProxy *self)
{
PyObject *result = NULL;

if (check(self, str_check_getattr, str_next) >= 0)
if (check(self, str_check_getattr, str_next, (PyObject *)NULL) >= 0)
{
result = PyIter_Next(self->proxy.proxy_object);
PROXY_RESULT(self, result);
Expand All @@ -398,7 +428,7 @@ proxy_getattro(SecurityProxy *self, PyObject *name)
{
PyObject *result = NULL;

if (check(self, str_check_getattr, name) >= 0)
if (check(self, str_check_getattr, name, (PyObject *)NULL) >= 0)
{
result = PyObject_GetAttr(self->proxy.proxy_object, name);
PROXY_RESULT(self, result);
Expand All @@ -409,7 +439,7 @@ proxy_getattro(SecurityProxy *self, PyObject *name)
static int
proxy_setattro(SecurityProxy *self, PyObject *name, PyObject *value)
{
if (check(self, str_check_setattr, name) >= 0)
if (check(self, str_check_setattr, name, value) >= 0)
return PyObject_SetAttr(self->proxy.proxy_object, name, value);
return -1;
}
Expand Down Expand Up @@ -458,7 +488,7 @@ proxy_str(SecurityProxy *self)
{
PyObject *result = NULL;

if (check(self, str_check, str___str__) >= 0)
if (check(self, str_check, str___str__, (PyObject *)NULL) >= 0)
{
result = PyObject_Str(self->proxy.proxy_object);
}
Expand All @@ -475,7 +505,7 @@ proxy_repr(SecurityProxy *self)
{
PyObject *result = NULL;

if (check(self, str_check, str___repr__) >= 0) {
if (check(self, str_check, str___repr__, (PyObject *)NULL) >= 0) {
result = PyObject_Repr(self->proxy.proxy_object);
}
else {
Expand Down Expand Up @@ -504,7 +534,7 @@ proxy_call(SecurityProxy *self, PyObject *args, PyObject *kwds)
{
PyObject *result = NULL;

if (check(self, str_check, str___call__) >= 0)
if (check(self, str_check, str___call__, (PyObject *)NULL) >= 0)
{
result = PyObject_Call(self->proxy.proxy_object, args, kwds);
PROXY_RESULT(self, result);
Expand Down Expand Up @@ -560,7 +590,8 @@ proxy_pow(PyObject *self, PyObject *other, PyObject *modulus)

if (Proxy_Check(self))
{
if (check((SecurityProxy*)self, str_check, str___pow__) >= 0)
if (check((SecurityProxy*)self, str_check, str___pow__,
(PyObject *)NULL) >= 0)
{
result = PyNumber_Power(((SecurityProxy*)self)->proxy.proxy_object,
other, modulus);
Expand All @@ -569,17 +600,19 @@ proxy_pow(PyObject *self, PyObject *other, PyObject *modulus)
}
else if (Proxy_Check(other))
{
if (check((SecurityProxy*)other, str_check, str___rpow__) >= 0)
if (check((SecurityProxy*)other, str_check, str___rpow__,
(PyObject *)NULL) >= 0)
{
result = PyNumber_Power(self,
((SecurityProxy*)other)->proxy.proxy_object,
((SecurityProxy*)other)->proxy.proxy_object,
modulus);
PROXY_RESULT(((SecurityProxy*)other), result);
}
}
else if (modulus != NULL && Proxy_Check(modulus))
{
if (check((SecurityProxy*)modulus, str_check, str___3pow__) >= 0)
if (check((SecurityProxy*)modulus, str_check, str___3pow__,
(PyObject *)NULL) >= 0)
{
result = PyNumber_Power(self, other,
((SecurityProxy*)modulus)->proxy.proxy_object);
Expand Down Expand Up @@ -608,7 +641,8 @@ proxy_coerce(PyObject **p_self, PyObject **p_other)

assert(Proxy_Check(self));

if (check((SecurityProxy*)self, str_check, str___coerce__) >= 0)
if (check((SecurityProxy*)self, str_check, str___coerce__,
(PyObject *)NULL) >= 0)
{
PyObject *left = ((SecurityProxy*)self)->proxy.proxy_object;
PyObject *right = other;
Expand Down Expand Up @@ -692,7 +726,7 @@ INPLACE(truediv, PyNumber_InPlaceTrueDivide)
static Py_ssize_t
proxy_length(SecurityProxy *self)
{
if (check(self, str_check, str___len__) >= 0)
if (check(self, str_check, str___len__, (PyObject *)NULL) >= 0)
return PyObject_Length(self->proxy.proxy_object);
return -1;
}
Expand Down Expand Up @@ -733,25 +767,26 @@ proxy_slice(SecurityProxy *self, Py_ssize_t start, Py_ssize_t end)
{
PyObject *result = NULL;

if (check(self, str_check, str___getslice__) >= 0) {
if (check(self, str_check, str___getslice__, (PyObject *)NULL) >= 0) {
result = PySequence_GetSlice(self->proxy.proxy_object, start, end);
PROXY_RESULT(self, result);
}
return result;
}

static int
proxy_ass_slice(SecurityProxy *self, Py_ssize_t i, Py_ssize_t j, PyObject *value)
proxy_ass_slice(SecurityProxy *self, Py_ssize_t i, Py_ssize_t j,
PyObject *value)
{
if (check(self, str_check, str___setslice__) >= 0)
if (check(self, str_check, str___setslice__, value) >= 0)
return PySequence_SetSlice(self->proxy.proxy_object, i, j, value);
return -1;
}

static int
proxy_contains(SecurityProxy *self, PyObject *value)
{
if (check(self, str_check, str___contains__) >= 0)
if (check(self, str_check, str___contains__, (PyObject *)NULL) >= 0)
return PySequence_Contains(self->proxy.proxy_object, value);
return -1;
}
Expand All @@ -765,7 +800,7 @@ proxy_getitem(SecurityProxy *self, PyObject *key)
{
PyObject *result = NULL;

if (check(self, str_check, str___getitem__) >= 0)
if (check(self, str_check, str___getitem__, (PyObject *)NULL) >= 0)
{
result = PyObject_GetItem(self->proxy.proxy_object, key);
PROXY_RESULT(self, result);
Expand All @@ -777,11 +812,11 @@ static int
proxy_setitem(SecurityProxy *self, PyObject *key, PyObject *value)
{
if (value == NULL) {
if (check(self, str_check, str___delitem__) >= 0)
if (check(self, str_check, str___delitem__, (PyObject *)NULL) >= 0)
return PyObject_DelItem(self->proxy.proxy_object, key);
}
else {
if (check(self, str_check, str___setitem__) >= 0)
if (check(self, str_check, str___setitem__, value) >= 0)
return PyObject_SetItem(self->proxy.proxy_object, key, value);
}
return -1;
Expand Down Expand Up @@ -999,8 +1034,11 @@ if((str_op_##S = INTERN("__" #S "__")) == NULL) return MOD_ERROR_VAL
INIT_STRING(__3pow__);
INIT_STRING(__call__);
INIT_STRING(check);
INIT_STRING(check_with_value);
INIT_STRING(check_getattr);
INIT_STRING(check_setattr);
INIT_STRING(check_setattr_with_value);
INIT_STRING(check_delattr);
INIT_STRING(__cmp__);
INIT_STRING(__coerce__);
INIT_STRING(__contains__);
Expand Down
9 changes: 7 additions & 2 deletions src/zope/security/checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@

from zope.security.interfaces import IChecker
from zope.security.interfaces import INameBasedChecker
from zope.security.interfaces import IValueBasedChecker
from zope.security.interfaces import ISecurityProxyFactory
from zope.security.interfaces import ForbiddenAttribute
from zope.security.interfaces import Unauthorized
Expand Down Expand Up @@ -101,8 +102,9 @@ def ProxyFactory(object, checker=None):
# This import represents part of the API for the proxy module
from . import proxy
proxy.ProxyFactory = ProxyFactory
_marker = object()

def canWrite(obj, name):
def canWrite(obj, name, value=_marker):
"""Check whether the interaction may write an attribute named name on obj.

Convenience method. Rather than using checkPermission in high level code,
Expand All @@ -111,7 +113,10 @@ def canWrite(obj, name):
obj = ProxyFactory(obj)
checker = getChecker(obj)
try:
checker.check_setattr(obj, name)
if value is not _marker and IValueBasedChecker.providedBy(checker):
checker.check_setattr_with_value(obj, name, value)
else:
checker.check_setattr(obj, name)
except Unauthorized:
return False
except ForbiddenAttribute:
Expand Down
22 changes: 22 additions & 0 deletions src/zope/security/interfaces.py
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,28 @@ def proxy(value):
"""


class IValueBasedChecker(IChecker):
"""Security checker that also checks the operands of attribute
assignment and in-place mutation operations.
"""

def check_delattr(ob, name):
"""As ``check_delattr`` but only applies when the attribute
is being deleted via ``del`` or ``delattr``.
"""

def check_setattr_with_value(ob, name, value):
"""As ``check_setattr`` but also includes the value to be
assigned.
"""

def check_with_value(ob, name, value):
"""As ``check`` but also includes the operand.

Only called for in-place mutation operations.
"""


class INameBasedChecker(IChecker):
"""Security checker that uses permissions to check attribute access."""

Expand Down
10 changes: 8 additions & 2 deletions src/zope/security/proxy.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,15 +101,21 @@ def __setattr__(self, name, value):
return super(PyProxyBase, self).__setattr__(name, value)
wrapped = super(PyProxyBase, self).__getattribute__('_wrapped')
checker = super(PyProxyBase, self).__getattribute__('_checker')
checker.check_setattr(wrapped, name)
if getattr(checker, 'check_setattr_with_value', None) is not None:
checker.check_setattr_with_value(wrapped, name, value)
else:
checker.check_setattr(wrapped, name)
setattr(wrapped, name, value)

def __delattr__(self, name):
if name in ('_wrapped', '_checker'):
raise AttributeError()
wrapped = super(PyProxyBase, self).__getattribute__('_wrapped')
checker = super(PyProxyBase, self).__getattribute__('_checker')
checker.check_setattr(wrapped, name)
if getattr(checker, 'check_delattr', None) is not None:
checker.check_delattr(wrapped, name)
else:
checker.check_setattr(wrapped, name)
delattr(wrapped, name)

@_check_name
Expand Down
Loading