_cookie_signature ignores delimiters #219

Closed
rickyz opened this Issue Feb 14, 2011 · 2 comments

2 participants

@rickyz

I see that there's a comment in the code indicating that this is known, but it seems that the secure cookie function still hmacs the contents of cookies with the delimiter character removed. As a result, a user can cause a 500 error by moving a cookie delimiter to the beginning, causing the int() call to throw a ValueError.

I see that you want to maintain backwards compatibility with the cookie, but in this case, I think it'd really be better to break compatibility and hmac the entire contents of the cookie before processing any of it:

diff --git a/tornado/web.py b/tornado/web.py
index 904336e..0a2f30d 100644
--- a/tornado/web.py
+++ b/tornado/web.py
@@ -321,6 +321,9 @@ class RequestHandler(object):

         To read a cookie set with this method, use get_secure_cookie().
         """
+        if '|' in name:
+            # Disallow delimiter characters in secure cookie names.
+            raise ValueError("Invalid secure cookie %r: %r" % (name, value))
         self.set_cookie(name, self.create_signed_value(name, value),
                         expires_days=expires_days, **kwargs)

@@ -379,10 +382,10 @@ class RequestHandler(object):

     def _cookie_signature(self, *parts):
         self.require_setting("cookie_secret", "secure cookies")
-        hash = hmac.new(self.application.settings["cookie_secret"],
-                        digestmod=hashlib.sha1)
-        for part in parts: hash.update(part)
-        return hash.hexdigest()
+        cookie_hmac = hmac.new(self.application.settings["cookie_secret"],
+                               digestmod=hashlib.sha1)
+        cookie_hmac.update('|'.join(parts))
+        return cookie_hmac.hexdigest()

     def redirect(self, url, permanent=False):
         """Sends a redirect to the given (optionally relative) URL."""
@bdarnell
tornadoweb member

Yes, users can cause their requests to fail by tampering with their cookies (that's kind of the point!). Maybe the exception should be caught and made to return None for consistency with the other ways in which the cookie can be rejected, but this doesn't seem like a big enough issue to break backwards-compatibility with existing cookies.

@bdarnell
tornadoweb member

The old secure_cookie format has finally been replaced in v3.2.1

@bdarnell bdarnell closed this Jun 15, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment