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

Patch session fixation vulnerability #1101

Merged
merged 1 commit into from Feb 27, 2017

Conversation

Arinerron
Copy link
Contributor

This is the pull request for the first vuln I reported. I'm not entirely sure about the second one yet because I don't know where the csrf token for the user is stored/how you'd want it.

Here's the documentation for that function:
http://php.net/manual/en/function.session-regenerate-id.php

And here's about session fixation:
https://www.owasp.org/index.php/Session_fixation

@dpeca
Copy link
Collaborator

dpeca commented Feb 27, 2017

Do you tested patched code?
Does it generate new seesion id ?

@Arinerron
Copy link
Contributor Author

Arinerron commented Feb 27, 2017

@dpeca I don't have an installation to test it on, but I made a page to test the function session_regenerate_id that was added:

<?php
session_start(); // start session
$_SESSION['test'] = "hello";

echo "before " . $_SESSION['test'] . " && sessid=" . session_id() . "\n";

session_regenerate_id(); // regenerate sessionid

echo "after " . $_SESSION['test'] . " && sessid=" . session_id();
?>

Here's the output:

before hello && sessid=9rvpritmpvka272thk36kcnip3
after hello && sessid=tdvnjcoalbaftunu5scqhe9ek0

As you can see, the session ID changed, but the data that the session stored remained intact.

@anton-reutov
Copy link
Collaborator

The commit should be checked by Serghey Rodin or Dmitry Naumov-Socolov

@naumov-socolov naumov-socolov merged commit c6393c8 into outroll:master Feb 27, 2017
@naumov-socolov
Copy link
Contributor

thanks @Arinerron

@Arinerron
Copy link
Contributor Author

Arinerron commented Feb 27, 2017 via email

@naumov-socolov
Copy link
Contributor

naumov-socolov commented Feb 27, 2017 via email

@Arinerron
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants