- issue2550864: Potential information leakage via journal/history

Original code didn't fully implement the security checks.

Users with only Edit access on a property were not able to view the
journal entry for the property. This patch fixes that.

Also had additional info leakage: the target object of a link or
multilink must be viewable or editable in order for the journal entry
to be shown. Otherwise the existance of the target is exposed via the
journal while it is blocked from searches, direct access etc.

Author: rouilj

CHANGES.txt

@@ -203,10 +203,10 @@ Features:
   config.ini with new options and help text. (John Rouillard)
 - issue2550864: Potential information leakage via journal/history
   Hyperdb history function now only returns properties that the user
-  has View permissions on. Can be overridden by setting a parameter
-  when calling the method. Also restructured code that implemented
-  issue1714899 moving it from the templating class to the hyperdb.
-  (John Rouillard)
+  can View or Edit and links to objects the user can see. Can be
+  overridden by setting a parameter when calling the method.
+  Also restructured code that implemented issue1714899 moving it
+  from the templating class to the hyperdb. (John Rouillard)
 - Improves diagnostics for mail processing: When using logging level = DEBUG,
   bounces and bounce problems are logged. (Bernhard Reiter)
 

roundup/hyperdb.py

@@ -1024,48 +1024,82 @@ def history(self, nodeid, enforceperm=True, skipquiet=True):
 
         debug_logging = logger.isEnabledFor(logging.DEBUG)
 
+        uid=self.db.getuid() # id of the person requesting the history
+
         for j in self.db.getjournal(self.classname, nodeid):
+            # hide/remove journal entry if:
+            #   property is quiet
+            #   property is not (viewable or editable)
             id, evt_date, user, action, args = j
             if debug_logging:
                 j_repr = "%s"%(j,)
             else:
                 j_repr=''
             if args and type(args) == type({}):
-                for k in args.keys():
-                    if skipquiet and self.properties[k].quiet:
-                        logger.debug("skipping quiet property %s in %s",
-                                     k, j_repr)
-                        del j[4][k]
+                for key in args.keys():
+                    if skipquiet and self.properties[key].quiet:
+                        logger.debug("skipping quiet property"
+                                     " %s::%s in %s",
+                                     self.classname, key, j_repr)
+                        del j[4][key]
                         continue
-                    if enforceperm and not perm("View",
-                                self.db.getuid(),
+                    if enforceperm and not ( perm("View",
+                                uid,
+                                self.classname,
+                                property=key ) or perm("Edit",
+                                uid,
                                 self.classname,
-                                property=k ):
-                        logger.debug("skipping unViewable property %s in %s",
-                                     k, j_repr)
-                        del j[4][k]
+                                property=key )):
+                        logger.debug("skipping unaccessible property %s::%s seen by user%s in %s",
+                                self.classname, key, uid, j_repr)
+                        del j[4][key]
                         continue
                 if not args:
                     logger.debug("Omitting journal entry for  %s%s"
-                                 " all props quiet in: %s",
+                                 " all props removed in: %s",
                                  self.classname, nodeid, j_repr)
                     continue
                 journal.append(j)
             elif action in ['link', 'unlink' ] and type(args) == type(()):
+                # hide/remove journal entry if:
+                #   link property (key) is quiet
+                #   link property is not (viewable or editable)
+                #   id/object (linkcl, linkid) that is linked/unlinked is not
+                #       (viewable or editable)
                 if len(args) == 3:
+                    # e.g. for issue3 blockedby adds link to issue5 with:
+                    # j = id, evt_date, user, action, args
+                    # 3|20170528045201.484|5|link|('issue', '5', 'blockedby')
                     linkcl, linkid, key = args
                     cls = self.db.getclass(linkcl)
-                    if skipquiet and cls.properties[key].quiet:
-                        logger.debug("skipping quiet property %s in %s",
-                                     key, j_repr)
+                    # is the updated property quiet?
+                    if skipquiet and self.properties[key].quiet:
+                        logger.debug("skipping quiet property"
+                                     " %s::%s in %s",
+                                     self.classname, key, j_repr)
                         continue
-                    if enforceperm and not perm("View",
-                            self.db.getuid(),
+                    # property check on item we want history for
+                    if enforceperm and not (perm("View",
+                            uid,
+                            self.classname,
+                            property=key) or perm("Edit",
+                            uid,
                             self.classname,
-                            property=key):
-                         logger.debug("skipping unViewable property %s in",
-                                      key, j_repr)
-                         continue
+                            property=key)):
+                        logger.debug("skipping unaccessible property %s::%s seen by user%s in %s",
+                                self.classname, key, uid, j_repr)
+                        continue
+                    # check on object linked
+                    if enforceperm and not (perm("View",
+                            uid,
+                            cls.classname,
+                            itemid=linkid) or perm("Edit",
+                            uid,
+                            cls.classname,
+                            itemid=linkid)):
+                        logger.debug("skipping unaccessible target %s%s for user%s in %s",
+                                      cls.classname, linkid, uid, j_repr)
+                        continue
                     journal.append(j)
                 else:
                     logger.error("Invalid %s journal entry for %s%s: %s",