bbangert/beaker — pull request #59 by Xync (2014-03-06 17:38:45+00:00)
Title: Added validation of session key format

After an automated security scan had been done against one of my applications (which uses the 'file' cache type), I observed that there were unusual files in my beaker data_dir beginning with '..' (ie. ..%2f..%2fetc%2fpasswd.cache). I traced this back to a test case where the beaker.session.id cookie was deliberately modified to include the above string. When beaker receives this cookie, it accepts the session key and attempts to find it in the appropriate cache directory. After processing this ends up being 'data_dir/container_file/./../..%2f..%2fetc%2fpasswd.cache'. The .. in that sequence moves the cache file out of the container_file directory and when this file is not found, it ends up being automatically created.

While this would be extremely difficult (impossible?) to exploit from a security perspective since the value is stripped of leading directories and has .cache appended to the name within the util.encoded_path function, it is still messy. This pull request adds three lines to ensure that the session key received from the cookie contains only alphanumeric characters, '_', or '-' which I believe handles all possible generated key variants while preventing unexpected behavior through other characters.

There is a quick test file included as well. Because this is a different sort of test I'm not sure if it should be permanently included or not, but that's your call.

I'm also curious about the logic behind accepting keys that are not found in the cache. From a security perspective this is dangerous because if an attacker has access to a victim's browser either directly or through a shared workstation and could set a non-expiring cookie to a known value, then when a victim logged in to the site, it would use this session identifier. The attacker could then use that known identifier to gain access to the site with the identity of the victim (with the time window limited to the session lifetime). From this perspective it would be better to reject the key if it is not found and silently generate a new key to be returned to the user. Would this break any functionality I'm not thinking of?