Added validation of session key format #59
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?