Skip to content

Commit

Permalink
Fix first part of Password handling security issue2550688
Browse files Browse the repository at this point in the history
(thanks 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.
  • Loading branch information
Ralf Schlatterbeck committed Apr 14, 2011
1 parent 6ecd3cc commit c20f920
Show file tree
Hide file tree
Showing 9 changed files with 159 additions and 42 deletions.
2 changes: 2 additions & 0 deletions CHANGES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
2 changes: 2 additions & 0 deletions doc/acknowledgements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ Titus Brown,
Steve Byan,
Brett Cannon,
Godefroid Chapelle,
Eli Collins,
Roch'e Compaan,
Wil Cooley,
Joe Cooper,
Expand Down Expand Up @@ -92,6 +93,7 @@ Ulrik Mikaelsson,
John Mitchell,
Ramiro Morales,
Toni Mueller,
Joseph Myers,
Stefan Niederhauser,
Truls E. Næss,
Bryce L Nordgren,
Expand Down
10 changes: 3 additions & 7 deletions roundup/backends/back_anydbm.py
Original file line number Diff line number Diff line change
Expand Up @@ -504,9 +504,7 @@ def unserialise(self, classname, node):
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
Expand Down Expand Up @@ -1744,7 +1742,7 @@ def _filter(self, search_matches, filterspec, proptree,
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)
Expand Down Expand Up @@ -2028,9 +2026,7 @@ def import_list(self, propnames, proplist):
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
Expand Down
4 changes: 1 addition & 3 deletions roundup/backends/rdbms_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -2832,9 +2832,7 @@ def import_list(self, propnames, proplist):
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')
Expand Down
30 changes: 8 additions & 22 deletions roundup/hyperdb.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -1307,9 +1295,7 @@ def import_journals(self, entries):
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!
Expand Down Expand Up @@ -1337,7 +1323,7 @@ def get_roles(self, nodeid):

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.
Expand Down
140 changes: 135 additions & 5 deletions roundup/password.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,96 @@
__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
Expand All @@ -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()
Expand Down Expand Up @@ -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__)
"""
Expand All @@ -109,6 +221,8 @@ def unpack(self, encrypted, scheme=None):
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."""
Expand Down Expand Up @@ -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()

Expand Down
5 changes: 2 additions & 3 deletions roundup/roundupdb.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,7 @@ def confirm_registration(self, otk):
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'
Expand Down Expand Up @@ -241,7 +240,7 @@ def good_recipient(userid):
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
Expand Down
2 changes: 1 addition & 1 deletion test/db_test_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
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"
Expand Down
6 changes: 5 additions & 1 deletion test/test_security.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -233,6 +233,10 @@ def testTransitiveSearchPermissions(self):
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))
Expand Down

0 comments on commit c20f920

Please sign in to comment.