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: rouilj

roundup/cgi/client.py

@@ -929,9 +929,38 @@ def handle_csrf(self, xmlrpc=False):
             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 @@ def handle_csrf(self, xmlrpc=False):
                 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 @@ def handle_csrf(self, xmlrpc=False):
         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,20 +1134,19 @@ def handle_csrf(self, xmlrpc=False):
                 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)
         if current_session != sid:
             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)

test/test_cgi.py

@@ -972,31 +972,75 @@ def hasPermission(s, p, classname=None, d=None, e=None, **kw):
         del(cl.env['HTTP_X-FORWARDED-HOST'])
         del(out[0])
 
-        import copy
+        # header checks succeed
+        # check nonce handling.
+        cl.env['HTTP_REFERER'] = 'http://whoami.com/path/'
 
+        import copy
         form2 = copy.copy(form)
         form2.update({'@csrf': 'booogus'})
         # add a bogus csrf field to the form and rerun the inner_main
         cl.form = makeForm(form2)
 
-        cl.env['HTTP_REFERER'] = 'http://whoami.com/path/'
         cl.inner_main()
         match_at=out[0].find('Invalid csrf token found: booogus')
         self.assertEqual(match_at, 36)
         del(out[0])
 
         form2 = copy.copy(form)
         nonce = anti_csrf_nonce(cl, cl)
+        # verify that we can see the nonce
+        otks = cl.db.getOTKManager()
+        isitthere = otks.exists(nonce)
+        self.assertEqual(isitthere, True)
+
         form2.update({'@csrf': nonce})
         # add a real csrf field to the form and rerun the inner_main
         cl.form = makeForm(form2)
         cl.inner_main()
         # csrf passes and redirects to the new issue.
         match_at=out[0].find('Redirecting to <a href="http://whoami.com/path/issue1?@ok_message')
         self.assertEqual(match_at, 0)
-        del(cl.env['HTTP_REFERER'])
         del(out[0])
 
+        # try a replay attack
+        cl.inner_main()
+        # This should fail as token was wiped by last run.
+        match_at=out[0].find('Invalid csrf token found: %s'%nonce)
+        print "replay of csrf after post use", out[0]
+        self.assertEqual(match_at, 36)
+        del(out[0])
+
+        # make sure that a get deletes the csrf.
+        cl.env['REQUEST_METHOD'] = 'GET' 
+        cl.env['HTTP_REFERER'] = 'http://whoami.com/path/'
+        form2 = copy.copy(form)
+        nonce = anti_csrf_nonce(cl, cl)
+        form2.update({'@csrf': nonce})
+        # add a real csrf field to the form and rerun the inner_main
+        cl.form = makeForm(form2)
+        cl.inner_main()
+        # csrf passes but fail creating new issue because not a post
+        match_at=out[0].find('<p>Invalid request</p>')
+        self.assertEqual(match_at, 33)
+        del(out[0])
+        
+        # the token should be gone
+        isitthere = otks.exists(nonce)
+        self.assertEqual(isitthere, False)
+
+        # change to post and should fail w/ invalid csrf
+        # since get deleted the token.
+        cl.env.update({'REQUEST_METHOD': 'POST'})
+        print cl.env
+        cl.inner_main()
+        match_at=out[0].find('Invalid csrf token found: %s'%nonce)
+        print "post failure after get", out[0]
+        self.assertEqual(match_at, 36)
+        del(out[0])
+
+        del(cl.env['HTTP_REFERER'])
+        
         # clean up from email log
         if os.path.exists(SENDMAILDEBUG):
             os.remove(SENDMAILDEBUG)