diff roundup/cgi/client.py @ 5210:7da56980754d

Remove csrf keys used with get The original code didn't delete keys used with a get. Detect this and invalidate the keys. Get keys are more likely to leak (in urls etc.) so they have to be removed once used. Also a little code cleanup and added testing.
author John Rouillard <rouilj@ieee.org>
date Sun, 19 Mar 2017 15:32:14 -0400
parents 47bd81998ddc
children f4b6a2a3e605
line wrap: on
line diff
--- a/roundup/cgi/client.py	Sun Mar 19 11:21:21 2017 -0400
+++ b/roundup/cgi/client.py	Sun Mar 19 15:32:14 2017 -0400
@@ -929,9 +929,38 @@
             debugging seems to be an advantage.
 
         '''
+        # Create the otks handle here as we need it almost immediately.
+        # If this is perf issue, set to None here and check below
+        # once all header checks have passed if it needs to be opened.
+        otks=self.db.getOTKManager()
+
         # Assume: never allow changes via GET
         if self.env['REQUEST_METHOD'] not in ['POST', 'PUT', 'DELETE']:
+            if "@csrf" in self.form:
+                # We have a nonce being used with a method it should
+                # not be. If the nonce exists, report to admin so they
+                # can fix the nonce leakage and destroy it. (nonces
+                # used in a get are more exposed than those used in a
+                # post.) Note, I don't attempt to validate here since
+                # existance here is the sign of a failure.  If nonce
+                # exists try to report the referer header to try to
+                # find where this comes from so it can be fixed.  If
+                # nonce doesn't exist just ignore it. Maybe we should
+                # report, but somebody could spam us with a ton of
+                # invalid keys and fill up the logs.
+                if 'HTTP_REFERER' in self.env:
+                    referer = self.env['HTTP_REFERER']
+                else:
+                    referer = self._("Referer header not available.")
+                key=self.form['@csrf'].value
+                if otks.exists(key):
+                    logger.error(
+                        self._("csrf key used with wrong method from: %s"),
+                        referer)
+                    otks.destroy(key)
+                    self.db.commit()
             return True
+
         config=self.instance.config
         user=self.db.getuid()
 
@@ -1045,7 +1074,6 @@
                 logger.error(self._("csrf X-REQUESED-WITH xmlrpc required header check failed for user%s."), user)
                 return False
 
-        otks=self.db.getOTKManager()
         enforce=config['WEB_CSRF_ENFORCE_TOKEN']
         if "@csrf" not in self.form:
             if enforce == 'required':
@@ -1061,8 +1089,14 @@
         key=self.form['@csrf'].value
         uid = otks.get(key, 'uid', default=None)
         sid = otks.get(key, 'sid', default=None)
+        if __debug__:
+            ts = otks.get(key, '__timestamp', default=None)
+            print("Found key %s for user%s sess: %s, ts %s, time %s"%(key, uid, sid, ts, time.time()))
         current_session = self.session_api._sid
-        # validate against user and session
+
+        # The key has been used or compromised. Delete it to prevent replay.
+        otks.destroy(key)
+        self.db.commit()
 
         '''
         # I think now that LogoutAction redirects to
@@ -1100,13 +1134,11 @@
                 self.add_error_message("Reload window before logging in.")
         '''
 
-        # The key has been used or compromised. Delete it to prevent replay.
-        otks.destroy(key)
-        self.db.commit()
-
+        # validate against user and session
         if uid != user:
             if enforce in ('required', "yes"):
                 self.add_error_message(self._("Invalid csrf token found: %s")%key)
+                logger.error(self._("csrf uid mismatch for user%s."), user)
                 return False
             elif enforce == 'logfailure':
                     logger.warning(self._("csrf uid mismatch for user%s."), user)
@@ -1114,6 +1146,7 @@
             if enforce in ('required', "yes"):
                 self.add_error_message(self._(
                     "Invalid csrf session found: %s")%key)
+                logger.error(self._("csrf session mismatch for user%s."), user)
                 return False
             elif enforce == 'logfailure':
                     logger.warning(self._("csrf session mismatch for user%s."), user)

Roundup Issue Tracker: http://roundup-tracker.org/