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

3 files changed, 56 insertions, 2 deletions

diff --git a/CHANGES.txt b/CHANGES.txt
index 2c3d2c3a8..84de3c642 100644
--- a/CHANGES.txt
+++ b/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)
 ==================
 
diff --git a/pyramid/authorization.py b/pyramid/authorization.py
index fc711e88b..943f8bd00 100644
--- a/pyramid/authorization.py
+++ b/pyramid/authorization.py
@@ -4,6 +4,8 @@ from pyramid.interfaces import IAuthorizationPolicy
 
 from pyramid.location import lineage
 
+from pyramid.compat import is_nonstr_iter
+
 from pyramid.security import (
     ACLAllowed,
     ACLDenied,
@@ -81,7 +83,7 @@ class ACLAuthorizationPolicy(object):
             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 @@ class ACLAuthorizationPolicy(object):
             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:
diff --git a/pyramid/tests/test_authorization.py b/pyramid/tests/test_authorization.py
index ed461e2ba..27f2a18b4 100644
--- a/pyramid/tests/test_authorization.py
+++ b/pyramid/tests/test_authorization.py
@@ -119,6 +119,20 @@ class TestACLAuthorizationPolicy(unittest.TestCase):
             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 @@ class TestACLAuthorizationPolicy(unittest.TestCase):
             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