Code

Fix first part of Password handling security issue2550688 (thanks
authorschlatterbeck <schlatterbeck@57a73879-2fb5-44c3-a270-3262357dd7e2>
Thu, 14 Apr 2011 12:24:59 +0000 (12:24 +0000)
committerschlatterbeck <schlatterbeck@57a73879-2fb5-44c3-a270-3262357dd7e2>
Thu, 14 Apr 2011 12:24:59 +0000 (12:24 +0000)
Joseph Myers for reporting and Eli Collins for fixing)

Small change against original patch: We still accept plaintext passwords
(in known_schemes) when parsing encrypted password (e.g. from database).
This way existing databases with plaintext passwords continue to work (I
don't know of any, this would need patching on the users side) and all
regression tests pass.

git-svn-id: http://svn.roundup-tracker.org/svnroot/roundup/roundup/trunk@4589 57a73879-2fb5-44c3-a270-3262357dd7e2

CHANGES.txt
doc/acknowledgements.txt
roundup/backends/back_anydbm.py
roundup/backends/rdbms_common.py
roundup/hyperdb.py
roundup/password.py
roundup/roundupdb.py
test/db_test_base.py
test/test_security.py

index 5b945994acf96c3a51dee96226a405d1a5df85d5..142d8ae36dc26f3e123ea16f9487945468d2df8d 100644 (file)
@@ -75,6 +75,8 @@ Fixed:
   (Ralf Schlatterbeck)
 - Fixed bug in mailgw refactoring, patch issue2550697 (thanks Hubert
   Touvet)
+- Fix first part of Password handling security issue2550688 (thanks
+  Joseph Myers for reporting and Eli Collins for fixing)
 
 2010-10-08 1.4.16 (r4541)
 
index 2380b8ab8db3a73767fd03c61d9dbd70ac9bfb38..5454f1d2cca458ecc628bcb2912321852d50bb29 100644 (file)
@@ -22,6 +22,7 @@ Titus Brown,
 Steve Byan,
 Brett Cannon,
 Godefroid Chapelle,
+Eli Collins,
 Roch'e Compaan,
 Wil Cooley,
 Joe Cooper,
@@ -92,6 +93,7 @@ Ulrik Mikaelsson,
 John Mitchell,
 Ramiro Morales,
 Toni Mueller,
+Joseph Myers,
 Stefan Niederhauser,
 Truls E. Næss,
 Bryce L Nordgren,
index 4ea2a2dcdccdc5ad160733de44e8b47b9289327b..f8ba81399c6a232a9c9642f22260facdff304d5c 100644 (file)
@@ -504,9 +504,7 @@ class Database(FileStorage, hyperdb.Database, roundupdb.Database):
             elif isinstance(prop, hyperdb.Interval) and v is not None:
                 d[k] = date.Interval(v)
             elif isinstance(prop, hyperdb.Password) and v is not None:
-                p = password.Password()
-                p.unpack(v)
-                d[k] = p
+                d[k] = password.Password(encrypted=v)
             else:
                 d[k] = v
         return d
@@ -1744,7 +1742,7 @@ class Class(hyperdb.Class):
                 l.append((OTHER, k, [float(val) for val in v]))
 
         filterspec = l
-        
+
         # now, find all the nodes that are active and pass filtering
         matches = []
         cldb = self.db.getclassdb(cn)
@@ -2028,9 +2026,7 @@ class Class(hyperdb.Class):
             elif isinstance(prop, hyperdb.Interval):
                 value = date.Interval(value)
             elif isinstance(prop, hyperdb.Password):
-                pwd = password.Password()
-                pwd.unpack(value)
-                value = pwd
+                value = password.Password(encrypted=value)
             d[propname] = value
 
         # get a new id if necessary
index 3ba789897e29cc8a92ce354fec182c09e25e0a52..09425b2ac35ce557ac24c11d5f93829cb8fbd827 100644 (file)
@@ -2832,9 +2832,7 @@ class Class(hyperdb.Class):
             elif isinstance(prop, hyperdb.Interval):
                 value = date.Interval(value)
             elif isinstance(prop, hyperdb.Password):
-                pwd = password.Password()
-                pwd.unpack(value)
-                value = pwd
+                value = password.Password(encrypted=value)
             elif isinstance(prop, String):
                 if isinstance(value, unicode):
                     value = value.encode('utf8')
index 0c86ede0636fb88e9e8a91ba88f8eea784bb9af4..365921577741ca7efe2442a285e47d4744cddbb6 100644 (file)
@@ -72,24 +72,12 @@ class Password(_Type):
     def from_raw(self, value, **kw):
         if not value:
             return None
-        m = password.Password.pwre.match(value)
-        if m:
-            # password is being given to us encrypted
-            p = password.Password()
-            p.scheme = m.group(1)
-            if p.scheme not in 'SHA crypt plaintext'.split():
-                raise HyperdbValueError, \
-                        ('property %s: unknown encryption scheme %r') %\
-                        (kw['propname'], p.scheme)
-            p.password = m.group(2)
-            value = p
-        else:
-            try:
-                value = password.Password(value)
-            except password.PasswordValueError, message:
-                raise HyperdbValueError, \
-                        _('property %s: %s')%(kw['propname'], message)
-        return value
+        try:
+            return password.Password(encrypted=value, strict=True)
+        except password.PasswordValueError, message:
+            raise HyperdbValueError, \
+                    _('property %s: %s')%(kw['propname'], message)
+
     def sort_repr (self, cls, val, name):
         if not val:
             return val
@@ -1307,9 +1295,7 @@ class Class:
                     elif isinstance(prop, Interval):
                         value = date.Interval(value)
                     elif isinstance(prop, Password):
-                        pwd = password.Password()
-                        pwd.unpack(value)
-                        value = pwd
+                        value = password.Password(encrypted=value)
                     params[propname] = value
             elif action == 'create' and params:
                 # old tracker with data stored in the create!
@@ -1337,7 +1323,7 @@ class Class:
 
     def has_role(self, nodeid, *roles):
         '''See if this node has any roles that appear in roles.
-           
+
            For convenience reasons we take a list.
            In standard schemas only a user has a roles property but
            this may be different in customized schemas.
index 201a6a979edd837918fdb5c6dd8e16c8039994c4..fcf8386bdc131afab8d096bd3d5fe6aff5d583a6 100644 (file)
 __docformat__ = 'restructuredtext'
 
 import re, string, random
+from base64 import b64encode, b64decode
 from roundup.anypy.hashlib_ import md5, sha1
 try:
     import crypt
 except ImportError:
     crypt = None
 
+_bempty = ""
+_bjoin = _bempty.join
+
+def getrandbytes(count):
+    return _bjoin(chr(random.randint(0,255)) for i in xrange(count))
+
+#NOTE: PBKDF2 hash is using this variant of base64 to minimize encoding size,
+#      and have charset that's compatible w/ unix crypt variants
+def h64encode(data):
+    """encode using variant of base64"""
+    return b64encode(data, "./").strip("=\n")
+
+def h64decode(data):
+    """decode using variant of base64"""
+    off = len(data) % 4
+    if off == 0:
+        return b64decode(data, "./")
+    elif off == 1:
+        raise ValueError("invalid bas64 input")
+    elif off == 2:
+        return b64decode(data + "==", "./")
+    else:
+        return b64decode(data + "=", "./")
+
+try:
+    from M2Crypto.EVP import pbkdf2 as _pbkdf2
+except ImportError:
+    #no m2crypto - make our own pbkdf2 function
+    from struct import pack
+    from hmac import HMAC
+    try:
+        from hashlib import sha1
+    except ImportError:
+        from sha import new as sha1
+
+    def xor_bytes(left, right):
+        "perform bitwise-xor of two byte-strings"
+        return _bjoin(chr(ord(l) ^ ord(r)) for l, r in zip(left, right))
+
+    def _pbkdf2(password, salt, rounds, keylen):
+        digest_size = 20 # sha1 generates 20-byte blocks
+        total_blocks = int((keylen+digest_size-1)/digest_size)
+        hmac_template = HMAC(password, None, sha1)
+        out = _bempty
+        for i in xrange(1, total_blocks+1):
+            hmac = hmac_template.copy()
+            hmac.update(salt + pack(">L",i))
+            block = tmp = hmac.digest()
+            for j in xrange(rounds-1):
+                hmac = hmac_template.copy()
+                hmac.update(tmp)
+                tmp = hmac.digest()
+                #TODO: need to speed up this call
+                block = xor_bytes(block, tmp)
+            out += block
+        return out[:keylen]
+
+def pbkdf2(password, salt, rounds, keylen):
+    """pkcs#5 password-based key derivation v2.0
+
+    :arg password: passphrase to use to generate key (if unicode, converted to utf-8)
+    :arg salt: salt string to use when generating key (if unicode, converted to utf-8)
+    :param rounds: number of rounds to use to generate key
+    :arg keylen: number of bytes to generate
+
+    If M2Crypto is present, uses it's implementation as backend.
+
+    :returns:
+        raw bytes of generated key
+    """
+    if isinstance(password, unicode):
+        password = password.encode("utf-8")
+    if isinstance(salt, unicode):
+        salt = salt.encode("utf-8")
+    if keylen > 40:
+        #NOTE: pbkdf2 allows up to (2**31-1)*20 bytes,
+        # but m2crypto has issues on some platforms above 40,
+        # and such sizes aren't needed for a password hash anyways...
+        raise ValueError, "key length too large"
+    if rounds < 1:
+        raise ValueError, "rounds must be positive number"
+    return _pbkdf2(password, salt, rounds, keylen)
+
 class PasswordValueError(ValueError):
     """ The password value is not valid """
     pass
@@ -37,7 +121,33 @@ def encodePassword(plaintext, scheme, other=None):
     """
     if plaintext is None:
         plaintext = ""
-    if scheme == 'SHA':
+    if scheme == "PBKDF2":
+        if other:
+            #assume it has format "{rounds}${salt}${digest}"
+            if isinstance(other, unicode):
+                other = other.encode("ascii")
+            try:
+                rounds, salt, digest = other.split("$")
+            except ValueError:
+                raise PasswordValueError, "invalid PBKDF2 hash (wrong number of separators)"
+            if rounds.startswith("0"):
+                raise PasswordValueError, "invalid PBKDF2 hash (zero-padded rounds)"
+            try:
+                rounds = int(rounds)
+            except ValueError:
+                raise PasswordValueError, "invalid PBKDF2 hash (invalid rounds)"
+            raw_salt = h64decode(salt)
+        else:
+            raw_salt = getrandbytes(20)
+            salt = h64encode(raw_salt)
+            #FIXME: find way to access config, so default rounds
+            # can be altered for faster/slower hosts via config.ini
+            rounds = 10000
+        if rounds < 1000:
+            raise PasswordValueError, "invalid PBKDF2 hash (rounds too low)"
+        raw_digest = pbkdf2(plaintext, raw_salt, rounds, 20)
+        return "%d$%s$%s" % (rounds, salt, h64encode(raw_digest))
+    elif scheme == 'SHA':
         s = sha1(plaintext).hexdigest()
     elif scheme == 'MD5':
         s = md5(plaintext).hexdigest()
@@ -80,24 +190,26 @@ class Password:
     >>> 'not sekrit' != p
     1
     """
+    #TODO: code to migrate from old password schemes.
 
-    default_scheme = 'SHA'        # new encryptions use this scheme
+    default_scheme = 'PBKDF2'        # new encryptions use this scheme
+    known_schemes = [ "PBKDF2", "SHA", "MD5", "crypt", "plaintext" ]
     pwre = re.compile(r'{(\w+)}(.+)')
 
-    def __init__(self, plaintext=None, scheme=None, encrypted=None):
+    def __init__(self, plaintext=None, scheme=None, encrypted=None, strict=False):
         """Call setPassword if plaintext is not None."""
         if scheme is None:
             scheme = self.default_scheme
         if plaintext is not None:
             self.setPassword (plaintext, scheme)
         elif encrypted is not None:
-            self.unpack(encrypted, scheme)
+            self.unpack(encrypted, scheme, strict=strict)
         else:
             self.scheme = self.default_scheme
             self.password = None
             self.plaintext = None
 
-    def unpack(self, encrypted, scheme=None):
+    def unpack(self, encrypted, scheme=None, strict=False):
         """Set the password info from the scheme:<encryted info> string
            (the inverse of __str__)
         """
@@ -109,6 +221,8 @@ class Password:
         else:
             # currently plaintext - encrypt
             self.setPassword(encrypted, scheme)
+        if strict and self.scheme not in self.known_schemes:
+            raise PasswordValueError, "unknown encryption scheme: %r" % (self.scheme,)
 
     def setPassword(self, plaintext, scheme=None):
         """Sets encrypts plaintext."""
@@ -160,6 +274,22 @@ def test():
     assert 'sekrit' == p
     assert 'not sekrit' != p
 
+    # PBKDF2 - low level function
+    from binascii import unhexlify
+    k = pbkdf2("password", "ATHENA.MIT.EDUraeburn", 1200, 32)
+    assert k == unhexlify("5c08eb61fdf71e4e4ec3cf6ba1f5512ba7e52ddbc5e5142f708a31e2e62b1e13")
+
+    # PBKDF2 - hash function
+    h = "5000$7BvbBq.EZzz/O0HuwX3iP.nAG3s$g3oPnFFaga2BJaX5PoPRljl4XIE"
+    assert encodePassword("sekrit", "PBKDF2", h) == h
+
+    # PBKDF2 - high level integration
+    p = Password('sekrit', 'PBKDF2')
+    assert p == 'sekrit'
+    assert p != 'not sekrit'
+    assert 'sekrit' == p
+    assert 'not sekrit' != p
+
 if __name__ == '__main__':
     test()
 
index fd4e0ba6c33b0e6615d73e5b86f1664af2218cc4..06d3a6018a1c2fcedc8d8d5f656cf8eefa104457 100644 (file)
@@ -103,8 +103,7 @@ class Database:
             elif isinstance(proptype, hyperdb.Interval):
                 props[propname] = date.Interval(value)
             elif isinstance(proptype, hyperdb.Password):
-                props[propname] = password.Password()
-                props[propname].unpack(value)
+                props[propname] = password.Password(encrypted=value)
 
         # tag new user creation with 'admin'
         self.journaltag = 'admin'
@@ -241,7 +240,7 @@ class IssueClass:
                 user or a user who has already seen the message.
                 Also check permissions on the message if not a system
                 message: A user must have view permission on content and
-                files to be on the receiver list. We do *not* check the 
+                files to be on the receiver list. We do *not* check the
                 author etc. for now.
             """
             allowed = True
index e508edf226a70ba0af33681258ea4b060d00d416..fe35b5d86b30577cc97f2390c094346e5da10edb 100644 (file)
@@ -35,7 +35,7 @@ config.RDBMS_NAME = "rounduptest"
 config.RDBMS_HOST = "localhost"
 config.RDBMS_USER = "rounduptest"
 config.RDBMS_PASSWORD = "rounduptest"
-#config.RDBMS_TEMPLATE = "template0"
+config.RDBMS_TEMPLATE = "template0"
 #config.logging = MockNull()
 # these TRACKER_WEB and MAIL_DOMAIN values are used in mailgw tests
 config.MAIL_DOMAIN = "your.tracker.email.domain.example"
index c7d51286b4c35dcd5970f6a9821cf72936c4c2b4..0d25c6b5188235c4842dab8f0a144cfc44785f88 100644 (file)
@@ -23,7 +23,7 @@
 import os, unittest, shutil
 
 from roundup import backends
-from roundup.password import Password
+import roundup.password
 from db_test_base import setupSchema, MyTestCase, config
 
 class PermissionTest(MyTestCase):
@@ -233,6 +233,10 @@ class PermissionTest(MyTestCase):
         self.assertEquals(has(uimu, 'issue', 'messages.recipients'), 1)
         self.assertEquals(has(uimu, 'issue', 'messages.recipients.username'), 1)
 
+    # roundup.password has its own built-in test, call it.
+    def test_password(self):
+        roundup.password.test()
+
 def test_suite():
     suite = unittest.TestSuite()
     suite.addTest(unittest.makeSuite(PermissionTest))