Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regenerate session id only on impossible level #636

Merged
merged 3 commits into from
May 31, 2024

Conversation

basbodart
Copy link
Contributor

To force a new Set-Cookie header in order to update the flags, a call to session_regenerate_id() is made.
This prevents session fixation attacks.

This commit only regenerate the id if the level is impossible.
Otherwise, session_id() is called, with the current id if it exists, which will also force a Set-Cookie header, with the correct flags.

@digininja
Copy link
Owner

I've just checked and it looks good. Could you add a comment around this to explain that it is allowing session fixation by only changing the session name on the impossible level.

@basbodart
Copy link
Contributor Author

Sorry, I'm not sure what you mean. Impossible level will prevent session fixation by regenerating the session id, other levels will not and will keep the existing id. Do you want me to add this kind of comment?

@digininja
Copy link
Owner

Just update your code comment to say why the code is like it is, why the lower levels are reusing the session name so that session fixation becomes an issue but for impossible we create a new name so fixation isn't possible.

So that people reviewing the code can understand what is going on.

@basbodart
Copy link
Contributor Author

There you go. Is that alright? (First time contributing to a public project, so I apologize for any inconvenience)

@digininja
Copy link
Owner

Just to be really picky, can you remove the space before the comment on line 68, after that, it is ready to go.

It is all really good and thanks for the submission. I don't know if anyone uses the session fixation attack, but we know it is there.

@basbodart
Copy link
Contributor Author

Sure. Removed one on line 97 also and an extra new line on 45.

@digininja digininja merged commit 9990b7f into digininja:master May 31, 2024
@digininja
Copy link
Owner

Brilliant, thanks.

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