Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Fix security bug caused by __iter__ checking on strings under Python 3

  • Loading branch information...
commit 52ca12afbf96621ffa225133529bae0a6a70a446 1 parent 1ca9703
@mcdonc mcdonc authored
View
27 CHANGES.txt
@@ -9,6 +9,33 @@ Bug Fixes
the documentation as an API method was a mistake, and it has been renamed
to something private.
+- Bug in ACL authentication checking on Python 3: the ``permits`` and
+ ``principals_allowed_by_permission`` method of
+ ``pyramid.authorization.ACLAuthenticationPolicy`` could return an
+ inappropriate ``True`` value when a permission on an ACL was a string
+ rather than a sequence, and then only if the ACL permission string was a
+ substring of the ``permission`` value passed to the function.
+
+ This bug effects no Pyramid deployment under Python 2; it is a bug that
+ exists only in deployments running on Python 3. It has existed since
+ Pyramid 1.3a1.
+
+ This bug was due to the presence of an ``__iter__`` attribute on strings
+ under Python 3 which is not present under strings in Python 2. I've been
+ assured by multiple Python cognoscenti that this difference in behavior
+ between Python 2 and Python 3 makes complete sense. Iterating over a
+ string character by character is of course something everyone wants to do
+ as often as possible and it would just be too darn slow to need to call a
+ method in order to turn a string into a list. Announcing that a string is
+ iterable by adding an ``__iter__`` to it simply canonizes its amazing,
+ speedy usefulness! So lest you think that Python 3's addition of an
+ ``__iter__`` to strings was a useless, pointless, harmful,
+ developer-hostile change, you're clearly mistaken, and quite possibly
+ brain-damaged. I feel for you. It's clearly much better to have a bug
+ that goes uncaught for nine alphas and one beta and almost leads to a
+ latent security hole that might have led to indiscriminate data
+ disclosure.
+
1.3b1 (2012-02-26)
==================
View
6 pyramid/authorization.py
@@ -4,6 +4,8 @@
from pyramid.location import lineage
+from pyramid.compat import is_nonstr_iter
+
from pyramid.security import (
ACLAllowed,
ACLDenied,
@@ -81,7 +83,7 @@ def permits(self, context, principals, permission):
for ace in acl:
ace_action, ace_principal, ace_permissions = ace
if ace_principal in principals:
- if not hasattr(ace_permissions, '__iter__'):
+ if not is_nonstr_iter(ace_permissions):
ace_permissions = [ace_permissions]
if permission in ace_permissions:
if ace_action == Allow:
@@ -118,7 +120,7 @@ def principals_allowed_by_permission(self, context, permission):
denied_here = set()
for ace_action, ace_principal, ace_permissions in acl:
- if not hasattr(ace_permissions, '__iter__'):
+ if not is_nonstr_iter(ace_permissions):
ace_permissions = [ace_permissions]
if (ace_action == Allow) and (permission in ace_permissions):
if not ace_principal in denied_here:
View
25 pyramid/tests/test_authorization.py
@@ -119,6 +119,20 @@ def test_permits(self):
result.acl,
'<No ACL found on any object in resource lineage>')
+ def test_permits_string_permissions_in_acl(self):
+ from pyramid.security import Allow
+ root = DummyContext()
+ root.__acl__ = [
+ (Allow, 'wilma', 'view_stuff'),
+ ]
+
+ policy = self._makeOne()
+
+ result = policy.permits(root, ['wilma'], 'view')
+ # would be True if matching against 'view_stuff' instead of against
+ # ['view_stuff']
+ self.assertEqual(result, False)
+
def test_principals_allowed_by_permission_direct(self):
from pyramid.security import Allow
from pyramid.security import DENY_ALL
@@ -132,6 +146,17 @@ def test_principals_allowed_by_permission_direct(self):
policy.principals_allowed_by_permission(context, 'read'))
self.assertEqual(result, ['chrism'])
+ def test_principals_allowed_by_permission_string_permission(self):
+ from pyramid.security import Allow
+ context = DummyContext()
+ acl = [ (Allow, 'chrism', 'read_it')]
+ context.__acl__ = acl
+ policy = self._makeOne()
+ result = policy.principals_allowed_by_permission(context, 'read')
+ # would be ['chrism'] if 'read' were compared against 'read_it' instead
+ # of against ['read_it']
+ self.assertEqual(list(result), [])
+
def test_principals_allowed_by_permission(self):
from pyramid.security import Allow
from pyramid.security import Deny
Please sign in to comment.
Something went wrong with that request. Please try again.