Attachment 598761 Details for Bug 832124 – Patch to embed id (associated with login) in all forms to combat CSRF

[patch] Patch to embed id (associated with login) in all forms to combat CSRF

CSRF.patch (text/plain), 7.55 KB, created by Trevor McKay on 2012-07-17 22:48:07 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Trevor McKay

Created: 2012-07-17 22:48:07 UTC

Size: 7.55 KB

patch

obsolete

>Index: wooly/python/wooly/forms.py
>===================================================================
>--- wooly/python/wooly/forms.py	(revision 5434)
>+++ wooly/python/wooly/forms.py	(working copy)
>@@ -7,6 +7,9 @@
> log = logging.getLogger("wooly.forms")
> strings = StringCatalog(__file__)
> 
>+class CSRFException(Exception):
>+    pass
>+
> class Form(Widget):
>     def __init__(self, app, name):
>         super(Form, self).__init__(app, name)
>@@ -16,9 +19,33 @@
> 
>         self.form_params = set()
> 
>+        # Make this a parameter so that it will be parsed
>+        # by the session on a POST request...
>+        self.csrf_value = Form.CSRFString(app, "csrf")
>+        self.add_parameter(self.csrf_value)
>+
>+        # ... but add it to form_params so that we
>+        # control the rendering explicitly in render_hidden_inputs.
>+        # We never want to set this value in the session except
>+        # during parsing of a POST request.
>+        self.form_params.add(self.csrf_value)
>+
>+    def validate_post(self, session):
>+        if session.client_session.get_csrf() != self.csrf_value.get(session):
>+            log.info("Possible CSRF attempt using %s" % self)
>+            raise CSRFException("Possible CSRF attempt")
>+        super(Form, self).validate_post(session)
>+
>+    class CSRFString(StringParameter):
>+        # Make sure that this value is never written
>+        # in a URL. This could happen if we generate
>+        # a link in the render pass on a POST.
>+        def appear_in_url(self):
>+            return False
>+
>     def validate(self, session):
>         log.debug("Validating %s", self)
>-
>+        
>     def render_hidden_inputs(self, session, *args):
>         writer = Writer()
> 
>@@ -45,6 +72,12 @@
>                     svalue = param.marshal(value)
>                     self.write_hidden_input(key, svalue, writer)
> 
>+        # Generate this explicitly.  This value is never set in the
>+        # session except during processing of a POST
>+        self.write_hidden_input(self.csrf_value.path, 
>+                                session.client_session.get_csrf(), 
>+                                writer)
>+
>         return writer.to_string()
> 
>     def write_hidden_input(self, name, value, writer):
>Index: wooly/python/wooly/__init__.py
>===================================================================
>--- wooly/python/wooly/__init__.py	(revision 5434)
>+++ wooly/python/wooly/__init__.py	(working copy)
>@@ -80,6 +80,9 @@
> 
>         self.widget.page.init_parameter(self)
> 
>+    def appear_in_url(self):
>+        return True
>+
>     def marshal(self, object):
>         if object == None:
>             string = ""
>@@ -301,6 +304,9 @@
>                 return True
>         return False
> 
>+    def validate_post(self, session):
>+        pass
>+
>     def process(self, session, *args):
>         if self.check_login and not self.authorized(session):
>             # redirect to the login page
>@@ -310,6 +316,9 @@
>             self.redirect.set(session, sess.marshal())
>             return
> 
>+        if session.post:
>+            self.validate_post(session)
>+
>         if not self.app.authorize_cb(session, self):
>             self.redirect_on_not_authorized(session)
>         else:
>@@ -712,6 +721,7 @@
>         self.request_environment = None
> 
>         self.messages = list()
>+        self.post = False
> 
>     def branch(self):
>         session = Session(self.page)
>@@ -782,7 +792,7 @@
>             key = param.path
>             value = values_by_path.get(key)
> 
>-            if value is not None:
>+            if value is not None and param.appear_in_url():
>                 if param.is_collection:
>                     for item in value:
>                         sitem = quote(param.marshal(item))
>Index: wooly/python/wooly/server.py
>===================================================================
>--- wooly/python/wooly/server.py	(revision 5434)
>+++ wooly/python/wooly/server.py	(working copy)
>@@ -170,6 +170,7 @@
>         session.unmarshal_url_vars(env["QUERY_STRING"])
> 
>         if env["REQUEST_METHOD"] == "POST":
>+            session.post = True
>             content_type = env["CONTENT_TYPE"]
> 
>             if content_type == "application/x-www-form-urlencoded":
>@@ -262,6 +263,8 @@
> class ClientSession(object):
>     def __init__(self):
>         self.id = unique_id()
>+        self.reset_csrf()
>+
>         self.created = datetime.now()
>         self.visited = None
> 
>@@ -270,6 +273,12 @@
>     def check_owner(self, owner):
>         user = self.attributes["login_session"].user.name
>         return owner == user
>+
>+    def get_csrf(self):
>+        return self.csrf
>+
>+    def reset_csrf(self):
>+        self.csrf = unique_id()
>         
>     def __repr__(self):
>         args = (self.__class__.__name__, self.id, self.created)
>Index: cumin/python/cumin/account/widgets.py
>===================================================================
>--- cumin/python/cumin/account/widgets.py	(revision 5434)
>+++ cumin/python/cumin/account/widgets.py	(working copy)
>@@ -84,6 +84,7 @@
>         if self.logout.get(session):
>             try:
>                 del session.client_session.attributes["login_session"]
>+                session.client_session.reset_csrf()
>             except KeyError:
>                 pass
> 
>@@ -148,6 +149,7 @@
>                             user = name
>                         login = LoginSession(self.app, user, roles)
>                         session.client_session.attributes["login_session"] = login
>+                        session.client_session.reset_csrf()
>                         url = self.page.origin.get(session)
>                         self.page.redirect.set(session, url)
>                 else:
>Index: cumin/python/cumin/widgets.py
>===================================================================
>--- cumin/python/cumin/widgets.py	(revision 5434)
>+++ cumin/python/cumin/widgets.py	(working copy)
>@@ -1364,14 +1364,14 @@
>                     user = username
>                 login = LoginSession(self.app, user, roles)
>                 session.client_session.attributes["login_session"] = login
>+                session.client_session.reset_csrf()
>                 return True
>         return False
> 
>     def redirect_on_exception(self, session):
>-        # If we have an exception from a missing object, redirect to the
>-        # main page with a notice instead of using the not_found_tmpl below.
>-        # Test for presence on the main page already to avoid any possibility
>-        # of an infinite redirect loop.
>+        # If we have certain exceptions, redirect to the main page instead 
>+        # of using the standard templates. Test for presence on the main 
>+        # page already to avoid any possibility of an infinite redirect loop.
>         cls, value, traceback = sys.exc_info()
>         if self.app.authorizator.is_enforcing():
>             mainpage = self.app.authorizator.find_mainpage(session)
>@@ -1380,12 +1380,17 @@
>         else:
>             mainpage = "/index.html"
> 
>-        if cls is RosemaryNotFound and \
>-           session.request_environment["REQUEST_URI"] != mainpage:
>-            session.add_notice(Notice(
>-                    "An object being displayed became unavailable"))
>-            return mainpage
>+        if session.request_environment["REQUEST_URI"] != mainpage:
>+            if cls is RosemaryNotFound:
>+                session.add_notice(Notice(
>+                        "An object being displayed became unavailable"))
>+            elif cls is CSRFException:
>+                session.add_notice(Notice("An invalid form was submitted"))
>+            else:
>+                mainpage = None
> 
>+        return mainpage
>+
>     def render_error(self, session):
>         cls, value, traceback = sys.exc_info()
>

Actions: View | Diff

Attachments on bug 832124: 598739 | 598761 | 599010