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.

Author: Ralf Schlatterbeck

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)
 

roundup/backends/back_anydbm.py

@@ -1298,6 +1298,8 @@ def set_inner(self, nodeid, **propvalues):
                     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 @@ def destroy(self, nodeid):
             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

roundup/backends/rdbms_common.py

@@ -1297,7 +1297,7 @@ def getjournal(self, classname, nodeid):
                         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 @@ def set_inner(self, nodeid, **propvalues):
                 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 @@ def destroy(self, nodeid):
             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

roundup/cgi/templating.py

@@ -1108,6 +1108,13 @@ def history(self, direction='descending', dre=re.compile('^\d+$'),
                             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 @@ def plain(self, escape=0):
 
         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.

roundup/hyperdb.py

@@ -951,7 +951,9 @@ def history(self, nodeid):
         '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 @@ def import_journals(self, entries):
                     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!

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 @@ def setPassword(self, plaintext, scheme=None):
         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: