Session poisoning when using datastore writer

[skonieczny]

Session poisoning is quite easy when using gaeutilities with standard configuration.

Following steps should reproduce the problem:

1. Create new appengine project, install gaeutilities v. 1.4, use standard configuration (datastore writer).

2. Create the following file "bug.py" inside your project (it simply shows the session content):

   from google.appengine.ext import webapp
   from google.appengine.ext.webapp.util import run_wsgi_app
   import logging
   
   from appengine_utilities.sessions import Session  
   from cgi import escape
   
   class Page(webapp.RequestHandler):
   
      def get(self):
          session = Session()
          value = session.get('key', '')
          self.response.out.write('Session is: %s <br />' % escape(str(session)))
          self.response.out.write('Key is: %s <br />' % escape(repr(value)))
   
   application = webapp.WSGIApplication([('/.*', Page)], debug=True)
   
   def main():
      run_wsgi_app(application)
   
   if __name__ == "__main__":
      main()

3. Also add handler for this file in app.yaml:
   • url: /bug script: bug.py
4. Visit "/bug". Your session is empty, but cookie named "gaeutilities_session_data" has been created.
5. Edit this cookie on the client side, changing it's value to: "{\"key\": 7}" (with all of double quotes).
6. Visit "/bug" again. Your session is now {'key': 7}.

Expected output of this script should be: Session is: {} Key is: ''

Instead, it is: Session is: {"key" = "7"} Key is: 7

I am using version 1.4, but the same issue is with 1.3. Session poisoning work both on the development server and appengine server.

After some investigation of the code, I have found the following in sessions.py, lines 548-559:

    try:
        self.cookie_vals = \
            simplejson.loads(self.cookie["%s_data" % (self.cookie_name)].value)
            # sync self.cache and self.cookie_vals which will make those
            # values available for all gets immediately.
        for k in self.cookie_vals:
            self.cache[k] = self.cookie_vals[k]
            # sync the input cookie with the output cookie
            self.output_cookie["%s_data" % (self.cookie_name)] = \
                self.cookie["%s_data" % (self.cookie_name)]
    except:
        self.cookie_vals = {}

Commenting it out and adding one line instead: self.cookie_vals = {} solves the problem, but I do not know, if it breaks some other functionality (cookie writer?).

[joerussbowman]

This is in place to support the transition from cookie based sessions to datastore based sessions. When you flip from the Cookie Session to the Datastore Session, you may not want to lose any values you had in the cookie session.

My intention when I wrote this is that any data in the Datastore would take priority over data in the Cookie. So, if you set the cookie to {'key': '7'}, but you had a value {'key': '1'} in the datastore, session['key'] would be '1'.

If this isn't the case, please let me know and I'll try to look at this. If you want to discuss ways to make this process more intuitive and whether or not I'm handling this the best way, I'll leave this ticket open for a while. One of the problems with having 1 person write everything is sometimes it only makes sense to him. As I'm not actively using this project myself anymore, I'd be more than happy to make changes the developer community that is using it would have, albeit with my extremely limited time to do so.

[skonieczny]

I have made some tests and it turns out, that data in datastore takes priority over the data from the cookie, if value in datastore is not considered by python as false value. Ie., if there is value 0 or '' in datastore, it is updated with value from cookie.

I am afraid, it may by a serious security issue for people that use datastore writer and believe, that data kept in datastore session is secure. Quite common case for using sessions is to keep information about currently logged user ie. by storing current user ID in some session variable. People may not be aware, that they must initialize this session variable upon session creation to some value not considered by python as false, when the user in not logged in. In result, there may be an easy way to log in as any chosen user into their site.

I have seen, that there been many downloads of this library and I am afraid, that there are some sites, that are easy to break into. At least, that would be my case, if I had not read the code myself.

If this is intended behavior of gaeutilities, I suppose, there should be some kind of warning somewhere in the docs or on the page. Perhaps something like: "even when using only data store writer, do not trust any session values, that might have not been initialized, or might had been set to value considered by python as false".

If it is not intended behavior, I would be happy to repair that issue, but I am quite busy with some other things and I guess, I will not have enough free time for that.

[joerussbowman]

Yea, unfortunately I'm pretty tied up right now as well. I've actually been looking for help supporting the project for a while now, as I'm not even working on appengine anymore.

Anyway, for this issue, the primary problem is as implied above, I'm checking the value and if it's not true, then I'm assigning the cookie key/value to the session. At first, the fix would appear to be validating if the key already exists in the datastore session or not.

However, perhaps a better solution is to move cookie values to something like session['cookie_values'], so that cookie values and datastore values never share the same key space? Anyone have any thoughts?

[skonieczny]

I think that separating cookie and datastore values is a very good idea.

[joerussbowman]

ok, well at this point I'm going to have to put this in the queue of things to get to. If I do get around to it, it will more than likely be a part of the rewrite to convert session to being a decorator, as there will be plenty of functionality breaking changes.