Skip to content

Conversation

@Xync
Copy link

@Xync Xync commented Mar 6, 2014

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?

Added a check to ensure the session key contains only alphanumeric characters, '_', or '-'.  This is to prevent a minor directory traversal issue where cache files could be forced to be created in the data_dir or the parent of file_dir.
Included a simple test to demonstrate the issue/assert it is fixed.
@amol-
Copy link
Collaborator

amol- commented Jan 28, 2015

The idea is good, but it should probably provide two different behaviours when the session is loaded from a cookie or the id is explicitly passed by the developer.

In case it is explicitly passed it should raise an exception otherwise it would be considered invalid when the session is loaded back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants