From 56e200d63c6098917037aee2587915a26afd1f8f Mon Sep 17 00:00:00 2001 From: schlatterbeck Date: Thu, 14 Apr 2011 15:42:41 +0000 Subject: [PATCH] Second patch from issue2550688 -- with some changes: - password.py now has a second class JournalPassword used for journal storage. We have some backends that directly store serialized python objects. Also when reading from the journal some backends expected the string read to be usable as a parameter to a Password constructor. This now calls a JournalPassword constructor in all these cases. The new JournalPassword just keeps the scheme and has an empty password. - some factoring, move redundant implementation of "history" from rdbms_common and back_anydbm to hyperdb. git-svn-id: http://svn.roundup-tracker.org/svnroot/roundup/roundup/trunk@4592 57a73879-2fb5-44c3-a270-3262357dd7e2 --- CHANGES.txt | 5 +-- roundup/backends/back_anydbm.py | 19 ++-------- roundup/backends/rdbms_common.py | 21 ++--------- roundup/cgi/templating.py | 12 ++++++- roundup/hyperdb.py | 6 ++-- roundup/password.py | 60 +++++++++++++++++++++++--------- 6 files changed, 66 insertions(+), 57 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 7cc26ae..8e16754 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -76,8 +76,9 @@ 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) +- Fix Password handling security issue2550688 (thanks Joseph Myers for + reporting and Eli Collins for fixing) -- this fixes all observations + by Joseph Myers except for auto-migration of existing passwords. 2010-10-08 1.4.16 (r4541) diff --git a/roundup/backends/back_anydbm.py b/roundup/backends/back_anydbm.py index f8ba813..34ddf93 100644 --- a/roundup/backends/back_anydbm.py +++ b/roundup/backends/back_anydbm.py @@ -1298,6 +1298,8 @@ class Class(hyperdb.Class): raise TypeError('new property "%s" not a ' 'Password'%propname) propvalues[propname] = value + journalvalues[propname] = \ + current and password.JournalPassword(current) elif value is not None and isinstance(prop, hyperdb.Date): if not isinstance(value, date.Date): @@ -1423,23 +1425,6 @@ class Class(hyperdb.Class): raise hyperdb.DatabaseError(_('Database open read-only')) self.db.destroynode(self.classname, nodeid) - def history(self, nodeid): - """Retrieve the journal of edits on a particular node. - - 'nodeid' must be the id of an existing node of this class or an - IndexError is raised. - - The returned list contains tuples of the form - - (nodeid, date, tag, action, params) - - 'date' is a Timestamp object specifying the time of the change and - 'tag' is the journaltag specified when the database was opened. - """ - if not self.do_journal: - raise ValueError('Journalling is disabled for this class') - return self.db.getjournal(self.classname, nodeid) - # Locating nodes: def hasnode(self, nodeid): """Determine if the given nodeid actually exists diff --git a/roundup/backends/rdbms_common.py b/roundup/backends/rdbms_common.py index 09425b2..5798c8d 100644 --- a/roundup/backends/rdbms_common.py +++ b/roundup/backends/rdbms_common.py @@ -1297,7 +1297,7 @@ class Database(FileStorage, hyperdb.Database, roundupdb.Database): continue cvt = self.to_hyperdb_value(property.__class__) if isinstance(property, Password): - params[param] = cvt(value) + params[param] = password.JournalPassword(value) elif isinstance(property, Date): params[param] = cvt(value) elif isinstance(property, Interval): @@ -1864,6 +1864,8 @@ class Class(hyperdb.Class): if not isinstance(value, password.Password): raise TypeError('new property "%s" not a Password'%propname) propvalues[propname] = value + journalvalues[propname] = \ + current and password.JournalPassword(current) elif value is not None and isinstance(prop, Date): if not isinstance(value, date.Date): @@ -1995,23 +1997,6 @@ class Class(hyperdb.Class): raise DatabaseError(_('Database open read-only')) self.db.destroynode(self.classname, nodeid) - def history(self, nodeid): - """Retrieve the journal of edits on a particular node. - - 'nodeid' must be the id of an existing node of this class or an - IndexError is raised. - - The returned list contains tuples of the form - - (nodeid, date, tag, action, params) - - 'date' is a Timestamp object specifying the time of the change and - 'tag' is the journaltag specified when the database was opened. - """ - if not self.do_journal: - raise ValueError('Journalling is disabled for this class') - return self.db.getjournal(self.classname, nodeid) - # Locating nodes: def hasnode(self, nodeid): """Determine if the given nodeid actually exists diff --git a/roundup/cgi/templating.py b/roundup/cgi/templating.py index 11e1449..04cfd76 100644 --- a/roundup/cgi/templating.py +++ b/roundup/cgi/templating.py @@ -1108,6 +1108,13 @@ class _HTMLItem(HTMLInputMixin, HTMLPermissions): cell[-1] += ' -> %s'%current[k] current[k] = val + elif isinstance(prop, hyperdb.Password) and args[k] is not None: + val = args[k].dummystr() + cell.append('%s: %s'%(self._(k), val)) + if current.has_key(k): + cell[-1] += ' -> %s'%current[k] + current[k] = val + elif not args[k]: if current.has_key(k): cell.append('%s: %s'%(self._(k), current[k])) @@ -1561,7 +1568,10 @@ class PasswordHTMLProperty(HTMLProperty): if self._value is None: return '' - return self._('*encrypted*') + value = self._value.dummystr() + if escape: + value = cgi.escape(value) + return value def field(self, size=30, **kwargs): """ Render a form edit field for the property. diff --git a/roundup/hyperdb.py b/roundup/hyperdb.py index a9bb9e1..0ed0360 100644 --- a/roundup/hyperdb.py +++ b/roundup/hyperdb.py @@ -951,7 +951,9 @@ class Class: 'date' is a Timestamp object specifying the time of the change and 'tag' is the journaltag specified when the database was opened. """ - raise NotImplementedError + if not self.do_journal: + raise ValueError('Journalling is disabled for this class') + return self.db.getjournal(self.classname, nodeid) # Locating nodes: def hasnode(self, nodeid): @@ -1309,7 +1311,7 @@ class Class: elif isinstance(prop, Interval): value = date.Interval(value) elif isinstance(prop, Password): - value = password.Password(encrypted=value) + value = password.JournalPassword(encrypted=value) params[propname] = value elif action == 'create' and params: # old tracker with data stored in the create! diff --git a/roundup/password.py b/roundup/password.py index fcf8386..545baeb 100644 --- a/roundup/password.py +++ b/roundup/password.py @@ -168,7 +168,49 @@ def generatePassword(length=8): chars = string.letters+string.digits return ''.join([random.choice(chars) for x in range(length)]) -class Password: +class JournalPassword: + """ Password dummy instance intended for journal operation. + We do not store passwords in the journal any longer. The dummy + version only reads the encryption scheme from the given + encrypted password. + """ + default_scheme = 'PBKDF2' # new encryptions use this scheme + pwre = re.compile(r'{(\w+)}(.+)') + + def __init__ (self, encrypted=''): + if isinstance(encrypted, self.__class__): + self.scheme = encrypted.scheme or self.default_scheme + else: + m = self.pwre.match(encrypted) + if m: + self.scheme = m.group(1) + else: + self.scheme = self.default_scheme + self.password = '' + + def dummystr(self): + """ return dummy string to store in journal + - reports scheme, but nothing else + """ + return "{%s}*encrypted*" % (self.scheme,) + + __str__ = dummystr + + def __cmp__(self, other): + """Compare this password against another password.""" + # check to see if we're comparing instances + if isinstance(other, self.__class__): + if self.scheme != other.scheme: + return cmp(self.scheme, other.scheme) + return cmp(self.password, other.password) + + # assume password is plaintext + if self.password is None: + raise ValueError, 'Password not set' + return cmp(self.password, encodePassword(other, self.scheme, + self.password or None)) + +class Password(JournalPassword): """The class encapsulates a Password property type value in the database. The encoding of the password is one if None, 'SHA', 'MD5' or 'plaintext'. @@ -192,9 +234,7 @@ class Password: """ #TODO: code to migrate from old password schemes. - 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, strict=False): """Call setPassword if plaintext is not None.""" @@ -232,20 +272,6 @@ class Password: self.password = encodePassword(plaintext, scheme) self.plaintext = plaintext - def __cmp__(self, other): - """Compare this password against another password.""" - # check to see if we're comparing instances - if isinstance(other, Password): - if self.scheme != other.scheme: - return cmp(self.scheme, other.scheme) - return cmp(self.password, other.password) - - # assume password is plaintext - if self.password is None: - raise ValueError, 'Password not set' - return cmp(self.password, encodePassword(other, self.scheme, - self.password)) - def __str__(self): """Stringify the encrypted password for database storage.""" if self.password is None: -- 2.30.2