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

Cookie preg_match should check for json, not serialized strings #4356

Closed
wants to merge 4 commits into
base: release-2.1
from

Conversation

Projects
None yet
6 participants
@sbulen
Contributor

sbulen commented Oct 14, 2017

In the cookie/session handling logic, cookie values were being checked vs serialized strings; they need to be checked for json encoded strings. Found & addressed 2 instances of preg_matches that needed to be updated

Two other changes:

  • No need to de-serialize the cookie string. It can't get that far...
  • During login, session checks should not default to normal post/get session checks. You're not posting a message, you're trying to login & create a new session.

This is the last PR to address #4297 (login failures after cookie setting changes). There has been existing logic in setLoginCookie to detect cookie state changes, but it was not being invoked due to the preg_match fail. This is why the cookies/sessions were not re-established properly after cookie settings were changed by the admin.

sbulen added some commits Oct 14, 2017

Dont confuse login check with regular post check
Signed by Shawn Bulen, bulens@pacbell.net
Validate cookie vs json
Signed by Shawn Bulen, bulens@pacbell.net
@jdarwood007

This comment has been minimized.

Show comment
Hide comment
@jdarwood007

jdarwood007 Oct 15, 2017

Member

Upgrades from 2.0 to 2.1 will cause all users to log out with this change as the stored data will be different. I think it would be better if we tried the json first and then fall back to trying to get it via serialized data. Sometime in the future we can completely remove it.

Member

jdarwood007 commented Oct 15, 2017

Upgrades from 2.0 to 2.1 will cause all users to log out with this change as the stored data will be different. I think it would be better if we tried the json first and then fall back to trying to get it via serialized data. Sometime in the future we can completely remove it.

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Oct 15, 2017

Contributor

The preg_match had been written to look at the serialized string... I just rewrote it to look at the json string... The problem is that the preg_match is for one or the other.

I.e., it will never get to the serialized string check if the preg_match filter just before it checks for json.

Or, we could get rid of the preg_match altogether? It's meant to ensure the cookie looks OK.

Contributor

sbulen commented Oct 15, 2017

The preg_match had been written to look at the serialized string... I just rewrote it to look at the json string... The problem is that the preg_match is for one or the other.

I.e., it will never get to the serialized string check if the preg_match filter just before it checks for json.

Or, we could get rid of the preg_match altogether? It's meant to ensure the cookie looks OK.

Show outdated Hide outdated Sources/LogInOut.php Outdated
Show outdated Hide outdated Sources/LogInOut.php Outdated
@jdarwood007

This comment has been minimized.

Show comment
Hide comment
@jdarwood007

jdarwood007 Oct 15, 2017

Member

@sbulen ,

Since in SMF 2.0 it is serialized, upon upgrade to 2.0, all users will be in effect logged out due to the cookie changing formats. I was suggesting we support the old method as a fallback for now. The old method doesn't need to support TFA, just cookies for login session. Cookies will get updated over time to use the json format.

Member

jdarwood007 commented Oct 15, 2017

@sbulen ,

Since in SMF 2.0 it is serialized, upon upgrade to 2.0, all users will be in effect logged out due to the cookie changing formats. I was suggesting we support the old method as a fallback for now. The old method doesn't need to support TFA, just cookies for login session. Cookies will get updated over time to use the json format.

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Oct 15, 2017

Contributor

@jdarwood007 - Understood. I was jut pointing out that due to the preg_match, the "fallback" would never be reached. We would have to restructure the logic a little bit, maybe even remove the preg_match altogether.

I was asking if you thought it was OK to remove the preg_match.

Note that @albertlast has removed the preg_match in his PR #4358, which is intended to address issues from PR #4353. My issue with albertlast's approach is exactly what you point out - he has redefined the data portion of the SMF login cookie.

With the data portion of the cookie redefined, users cannot logon in 2.1 post migration without deleting their cookies.

The root cause of the original problem ( #4297 ) is in fact this issue with the preg_match. This logic worked great in 2.0, and the only thing that broke it in 2.1 was the fact that the preg_match was still looking at the data portion of the cookie as if it were a serialized string, when it is now json. Changing or removing the preg_match is all that is needed to fix #4297 .

This is a challenge with Github development... Two people can work on different incompatible fixes to the same problem at the same time.

I'll leave it to the dev team to determine how to proceed. I'll withdraw this PR if requested.

Contributor

sbulen commented Oct 15, 2017

@jdarwood007 - Understood. I was jut pointing out that due to the preg_match, the "fallback" would never be reached. We would have to restructure the logic a little bit, maybe even remove the preg_match altogether.

I was asking if you thought it was OK to remove the preg_match.

Note that @albertlast has removed the preg_match in his PR #4358, which is intended to address issues from PR #4353. My issue with albertlast's approach is exactly what you point out - he has redefined the data portion of the SMF login cookie.

With the data portion of the cookie redefined, users cannot logon in 2.1 post migration without deleting their cookies.

The root cause of the original problem ( #4297 ) is in fact this issue with the preg_match. This logic worked great in 2.0, and the only thing that broke it in 2.1 was the fact that the preg_match was still looking at the data portion of the cookie as if it were a serialized string, when it is now json. Changing or removing the preg_match is all that is needed to fix #4297 .

This is a challenge with Github development... Two people can work on different incompatible fixes to the same problem at the same time.

I'll leave it to the dev team to determine how to proceed. I'll withdraw this PR if requested.

@jdarwood007

This comment has been minimized.

Show comment
Hide comment
@jdarwood007

jdarwood007 Oct 15, 2017

Member

Honestly, I would leave the preg_match as its a good sanity check prior to attempting to load the cookie. It should properly read the cookie for all cases, which can be difficult but this makes it safer since it is handling user input

Member

jdarwood007 commented Oct 15, 2017

Honestly, I would leave the preg_match as its a good sanity check prior to attempting to load the cookie. It should properly read the cookie for all cases, which can be difficult but this makes it safer since it is handling user input

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Oct 16, 2017

Contributor

Thanks @jdarwood007 . If the dev team wants to keep the old cookie format, I'll mod this PR.

If the dev team wants to rewrite the cookie format, all such changes should be in PR #4353 & PR #4358 , and this PR should be closed.

Contributor

sbulen commented Oct 16, 2017

Thanks @jdarwood007 . If the dev team wants to keep the old cookie format, I'll mod this PR.

If the dev team wants to rewrite the cookie format, all such changes should be in PR #4353 & PR #4358 , and this PR should be closed.

Fix cookie preg_matches, allow for 2.1 & 2.0
Signed by Shawn Bulen, bulens@pacbell.net
@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Oct 16, 2017

Contributor

I followed the suggestions here & made the changes. All tests well, but on the old cookie format.

Contributor

sbulen commented Oct 16, 2017

I followed the suggestions here & made the changes. All tests well, but on the old cookie format.

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Oct 16, 2017

Collaborator

With the data portion of the cookie redefined, users cannot logon in 2.1 post migration without deleting their cookies.

Sure they can, it will be migrated like the old version

Honestly, I would leave the preg_match as its a good sanity check prior to attempting to load the cookie. It should properly read the cookie for all cases, which can be difficult but this makes it safer since it is handling user input

This make no sense: When the user got a valid cookie we check,
when not valid cookie we do nothing???

Collaborator

albertlast commented Oct 16, 2017

With the data portion of the cookie redefined, users cannot logon in 2.1 post migration without deleting their cookies.

Sure they can, it will be migrated like the old version

Honestly, I would leave the preg_match as its a good sanity check prior to attempting to load the cookie. It should properly read the cookie for all cases, which can be difficult but this makes it safer since it is handling user input

This make no sense: When the user got a valid cookie we check,
when not valid cookie we do nothing???

@jdarwood007

This comment has been minimized.

Show comment
Hide comment
@jdarwood007

jdarwood007 Oct 16, 2017

Member

@albertlast
If their cookie doesn't match what we know to be a valid json string for a login session and doesn't match a valid serialized string for a login session, we can assume that the cookie is dirty and contains input we don't want to try to parse. We know exactly what that cookie should contain and thats why I think its safe and easy here to do the check.

Member

jdarwood007 commented Oct 16, 2017

@albertlast
If their cookie doesn't match what we know to be a valid json string for a login session and doesn't match a valid serialized string for a login session, we can assume that the cookie is dirty and contains input we don't want to try to parse. We know exactly what that cookie should contain and thats why I think its safe and easy here to do the check.

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Oct 16, 2017

Collaborator

The cookie is at the point already parsed and
ther is no check "when pattern is wrong fix the cookie".
This is here the right place to do any check and fix it when possible.

Collaborator

albertlast commented Oct 16, 2017

The cookie is at the point already parsed and
ther is no check "when pattern is wrong fix the cookie".
This is here the right place to do any check and fix it when possible.

@@ -97,21 +97,25 @@ function Login2()
if (isset($_GET['sa']) && $_GET['sa'] == 'salt' && !$user_info['is_guest'])
{
if (isset($_COOKIE[$cookiename]) && preg_match('~^a:[34]:\{i:0;i:\d{1,7};i:1;s:(0|128):"([a-fA-F0-9]{128})?";i:2;[id]:\d{1,14};(i:3;i:\d;)?\}$~', $_COOKIE[$cookiename]) === 1)
// First check for 2.1 json-format cookie in $_COOKIE
if (isset($_COOKIE[$cookiename]) && preg_match('~^\[\d{1,7},"([a-fA-F0-9]{128})?",\d{1,14}(,[0-3])?\]$~', $_COOKIE[$cookiename]) === 1)

This comment has been minimized.

@albertlast

albertlast Oct 16, 2017

Collaborator

As fare my test goes the regex pattern didn't work for json...

@albertlast

albertlast Oct 16, 2017

Collaborator

As fare my test goes the regex pattern didn't work for json...

This comment has been minimized.

@sbulen

sbulen Oct 16, 2017

Contributor

I've tested it repeatedly. Works fine. The difference of is probably that you're using your redefined cookies.

@sbulen

sbulen Oct 16, 2017

Contributor

I've tested it repeatedly. Works fine. The difference of is probably that you're using your redefined cookies.

This comment has been minimized.

@albertlast
@albertlast

albertlast Oct 16, 2017

Collaborator

This comment has been minimized.

@sbulen

sbulen Oct 16, 2017

Contributor

Here is a screenshot of the point in question. Note the format of the data portion of the cookie. Note also that, in the debugger, that line of code would not have been executed without a successful preg_match.

Note also that this is the logic for detecting cookie state changes - the heart of the original issue.
This logic worked fine in 2.0. And it works fine in 2.1 after updating the preg_match. No need to redefine the cookie.

image

@sbulen

sbulen Oct 16, 2017

Contributor

Here is a screenshot of the point in question. Note the format of the data portion of the cookie. Note also that, in the debugger, that line of code would not have been executed without a successful preg_match.

Note also that this is the logic for detecting cookie state changes - the heart of the original issue.
This logic worked fine in 2.0. And it works fine in 2.1 after updating the preg_match. No need to redefine the cookie.

image

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Oct 16, 2017

Collaborator
Collaborator

albertlast commented Oct 16, 2017

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Oct 16, 2017

Collaborator
Collaborator

albertlast commented Oct 16, 2017

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Oct 16, 2017

Contributor

This emulates the code exactly, as is demonstrated in my screenshot.

Contributor

sbulen commented Oct 16, 2017

This emulates the code exactly, as is demonstrated in my screenshot.

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Oct 16, 2017

Collaborator

And why your are sure that this format is used and not this:
{"0":7,"1":"abcd0123","2":44444,"3":2}
Which create the same output in your test case,
what only show that it wrong from the start to make pattern check.

Collaborator

albertlast commented Oct 16, 2017

And why your are sure that this format is used and not this:
{"0":7,"1":"abcd0123","2":44444,"3":2}
Which create the same output in your test case,
what only show that it wrong from the start to make pattern check.

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Oct 16, 2017

Contributor

(1) The debugger screenshot above shows what is actually being returned.
(2) The utility which I provided the link to is in alignment with the screenshot.
(3) Testing reveals the proper 2.0 behavior has been restored. The change made the issues go away.

Contributor

sbulen commented Oct 16, 2017

(1) The debugger screenshot above shows what is actually being returned.
(2) The utility which I provided the link to is in alignment with the screenshot.
(3) Testing reveals the proper 2.0 behavior has been restored. The change made the issues go away.

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Oct 16, 2017

Collaborator

and the question keeps up, how you safe this json array is used instead of a json object.

Collaborator

albertlast commented Oct 16, 2017

and the question keeps up, how you safe this json array is used instead of a json object.

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Oct 16, 2017

Contributor

I did not alter the cookie in any way. What is passed to the cookie is a simple array, as it has always been, in 2.0 & 2.1. As noted before, I do not think it was necessary or desirable to redefine the cookie contents.

Contributor

sbulen commented Oct 16, 2017

I did not alter the cookie in any way. What is passed to the cookie is a simple array, as it has always been, in 2.0 & 2.1. As noted before, I do not think it was necessary or desirable to redefine the cookie contents.

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Oct 16, 2017

Contributor

Sorry the links above are wonky. I guess the site is having ssl issues. Screenshots here are in alignment with SMF behavior:
image
image

Contributor

sbulen commented Oct 16, 2017

Sorry the links above are wonky. I guess the site is having ssl issues. Screenshots here are in alignment with SMF behavior:
image
image

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Oct 16, 2017

Collaborator

The links work fine when you replace en with www, so i was able to test this.
And based on this i came up with the question how you safe that the json keeps an array json and
get not a object json.

Collaborator

albertlast commented Oct 16, 2017

The links work fine when you replace en with www, so i was able to test this.
And based on this i came up with the question how you safe that the json keeps an array json and
get not a object json.

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Oct 16, 2017

Contributor

By reading the code, and seeing it is being passed a simple array, not an object.

Further, by looking at it in the debugger and observing the behavior, confirming the above.

Finally, by testing, and confirming that the logic for cookie state changes is once again operational.

Contributor

sbulen commented Oct 16, 2017

By reading the code, and seeing it is being passed a simple array, not an object.

Further, by looking at it in the debugger and observing the behavior, confirming the above.

Finally, by testing, and confirming that the logic for cookie state changes is once again operational.

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Oct 16, 2017

Collaborator

this is not a good check, when a mod write a key as a string like me with "path" than the thing get a object,
so this is very danger zone where you move.

Collaborator

albertlast commented Oct 16, 2017

this is not a good check, when a mod write a key as a string like me with "path" than the thing get a object,
so this is very danger zone where you move.

@jdarwood007

This comment has been minimized.

Show comment
Hide comment
@jdarwood007

jdarwood007 Oct 16, 2017

Member

When in doubt, check the RFC: https://tools.ietf.org/html/rfc7159
Check Page 11 for examples. Both are valid, but we shouldn't be using a object here, should be an array

Member

jdarwood007 commented Oct 16, 2017

When in doubt, check the RFC: https://tools.ietf.org/html/rfc7159
Check Page 11 for examples. Both are valid, but we shouldn't be using a object here, should be an array

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Oct 24, 2017

Collaborator

#4366 is not talking about this,
the case is here that a board is moved from /smf/ to /smf/sub,
and this case is not coverd by your pr (i already tested this with your pr):
grafik

Collaborator

albertlast commented Oct 24, 2017

#4366 is not talking about this,
the case is here that a board is moved from /smf/ to /smf/sub,
and this case is not coverd by your pr (i already tested this with your pr):
grafik

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Oct 24, 2017

Contributor

Ok. No, this PR does not cover actual board relocations.

Contributor

sbulen commented Oct 24, 2017

Ok. No, this PR does not cover actual board relocations.

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Oct 24, 2017

Collaborator

I know,
but than please edit you comment in the issue or
cover relocation with this pr.

Collaborator

albertlast commented Oct 24, 2017

I know,
but than please edit you comment in the issue or
cover relocation with this pr.

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Oct 24, 2017

Collaborator

Good than only the missing check of session_var and sc is in my eyes.
The test case here for is,
open the login form, edit in browser ide (f12) the numbers in the hidden input and
notice that the login is successfull -> no check are done.
unchaged:
grafik

changed:
grafik
--> successfull login -> checks are broken (this behavior is not wanted)

in the "live dev" version you got an error (this is right behavior)

Collaborator

albertlast commented Oct 24, 2017

Good than only the missing check of session_var and sc is in my eyes.
The test case here for is,
open the login form, edit in browser ide (f12) the numbers in the hidden input and
notice that the login is successfull -> no check are done.
unchaged:
grafik

changed:
grafik
--> successfull login -> checks are broken (this behavior is not wanted)

in the "live dev" version you got an error (this is right behavior)

@emanuele45

This comment has been minimized.

Show comment
Hide comment
@emanuele45

emanuele45 Oct 24, 2017

Contributor
Contributor

emanuele45 commented Oct 24, 2017

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Oct 24, 2017

Contributor

Look at the original issue, #4297. You would get locked in an unending 'session timed out' error loop while trying to login. That is what was illogical - you didn't have a session to timeout.

After changing to 'login', you would at least get a more appropriate message informing you your cookie had issues.

(And converting the preg_match to json fixed the actual cookie issue.)

Contributor

sbulen commented Oct 24, 2017

Look at the original issue, #4297. You would get locked in an unending 'session timed out' error loop while trying to login. That is what was illogical - you didn't have a session to timeout.

After changing to 'login', you would at least get a more appropriate message informing you your cookie had issues.

(And converting the preg_match to json fixed the actual cookie issue.)

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Oct 24, 2017

Contributor

I had been tempted to use 'get' instead of 'login'. One could argue that existing edit made sense. Again, I'm deferring to dev input.

Contributor

sbulen commented Oct 24, 2017

I had been tempted to use 'get' instead of 'login'. One could argue that existing edit made sense. Again, I'm deferring to dev input.

Revert login param on checkSession
Signed by Shawn Bulen, bulens@pacbell.net
@Sesquipedalian

This comment has been minimized.

Show comment
Hide comment
@Sesquipedalian

Sesquipedalian Jan 8, 2018

Member

With checkSession() reverted back to the original, I like the way the logic is laid out in this solution to #4297. But before I merge it, I also want to read @albertlast's answers to my questions on #4373. I also like some of the things he is doing there to address the other issues he has identified. I may ask you both to make a few more tweaks to your PRs so that we can merge in the best from both of them.

Member

Sesquipedalian commented Jan 8, 2018

With checkSession() reverted back to the original, I like the way the logic is laid out in this solution to #4297. But before I merge it, I also want to read @albertlast's answers to my questions on #4373. I also like some of the things he is doing there to address the other issues he has identified. I may ask you both to make a few more tweaks to your PRs so that we can merge in the best from both of them.

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Jan 8, 2018

Collaborator

With the reverting of checkSession() you got the problem,
that the message error session timeout... apears again.

Collaborator

albertlast commented Jan 8, 2018

With the reverting of checkSession() you got the problem,
that the message error session timeout... apears again.

@Sesquipedalian

This comment has been minimized.

Show comment
Hide comment
@Sesquipedalian

Sesquipedalian Jan 8, 2018

Member

With the reverting of checkSession() you got the problem,
that the message error session timeout... apears again.

Indeed it does. Perhaps I wasn't clear, but that's actually what I was asking about, @sbulen: Would these incorrect error messages still show up if we don't passing a bogus parameter to checkSession()?

Since it does, we need to go back to the drawing board. Bypassing the security checks here using checkSession('login') isn't a good option, but neither is removing setLoginCookie(-3600, 0) from validatePasswordFlood(), as in #4373.

The root issue must be something deeper. In 2.0.x, checkSession() is called at this point in Login2() and setLoginCookie(-3600, 0) is called near the beginning of validatePasswordFlood(), and no problems arise. Something else in the logic flow must have changed in order to cause this mess.

Member

Sesquipedalian commented Jan 8, 2018

With the reverting of checkSession() you got the problem,
that the message error session timeout... apears again.

Indeed it does. Perhaps I wasn't clear, but that's actually what I was asking about, @sbulen: Would these incorrect error messages still show up if we don't passing a bogus parameter to checkSession()?

Since it does, we need to go back to the drawing board. Bypassing the security checks here using checkSession('login') isn't a good option, but neither is removing setLoginCookie(-3600, 0) from validatePasswordFlood(), as in #4373.

The root issue must be something deeper. In 2.0.x, checkSession() is called at this point in Login2() and setLoginCookie(-3600, 0) is called near the beginning of validatePasswordFlood(), and no problems arise. Something else in the logic flow must have changed in order to cause this mess.

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Jan 8, 2018

Contributor

I'll take a peek over the next few days - very busy at work. My initial thought is that other changes in the interim have changed behavior.

Contributor

sbulen commented Jan 8, 2018

I'll take a peek over the next few days - very busy at work. My initial thought is that other changes in the interim have changed behavior.

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Jan 8, 2018

Contributor

I have not performed a merge on this branch recently, which is why it still works for me. I.e., this branch works:
https://github.com/sbulen/SMF2.1/tree/login_tweaks

So... There have been changes in this area over the last few months that make it so this PR won't work now. I'll keep digging when I can.

Contributor

sbulen commented Jan 8, 2018

I have not performed a merge on this branch recently, which is why it still works for me. I.e., this branch works:
https://github.com/sbulen/SMF2.1/tree/login_tweaks

So... There have been changes in this area over the last few months that make it so this PR won't work now. I'll keep digging when I can.

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Jan 9, 2018

Contributor

If I take the very latest from Github, and merge in the login_tweaks branch, everything works. I cannot reproduce the problem reported in #4297 ... (Specifically, change local or global cookie settings, log out & try to log back on...)

This PR tests OK. What steps were you following to make it fail?

So now I've tested this PR both ways & it works both ways - with the code as it was in October & the code as it is today.

Contributor

sbulen commented Jan 9, 2018

If I take the very latest from Github, and merge in the login_tweaks branch, everything works. I cannot reproduce the problem reported in #4297 ... (Specifically, change local or global cookie settings, log out & try to log back on...)

This PR tests OK. What steps were you following to make it fail?

So now I've tested this PR both ways & it works both ways - with the code as it was in October & the code as it is today.

@Sesquipedalian

This comment has been minimized.

Show comment
Hide comment
@Sesquipedalian

Sesquipedalian Jan 9, 2018

Member

This PR tests OK. What steps were you following to make it fail?

  1. Log in as admin user.
  2. Go to admin area, toggle "Enable local storage of cookies" setting, and save.
  3. Log out.
  4. Log in as admin user.
Member

Sesquipedalian commented Jan 9, 2018

This PR tests OK. What steps were you following to make it fail?

  1. Log in as admin user.
  2. Go to admin area, toggle "Enable local storage of cookies" setting, and save.
  3. Log out.
  4. Log in as admin user.
@Sesquipedalian

This comment has been minimized.

Show comment
Hide comment
@Sesquipedalian

Sesquipedalian Jan 9, 2018

Member

Before you do any more work on this, @sbulen, give me a chance to test some things.

Member

Sesquipedalian commented Jan 9, 2018

Before you do any more work on this, @sbulen, give me a chance to test some things.

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Jan 9, 2018

Contributor

Those have been my exact test steps throughout. Make sure you're testing with the unmodified cookie format.

Contributor

sbulen commented Jan 9, 2018

Those have been my exact test steps throughout. Make sure you're testing with the unmodified cookie format.

@Sesquipedalian

This comment has been minimized.

Show comment
Hide comment
@Sesquipedalian

Sesquipedalian Jan 9, 2018

Member

I am. ;)

Member

Sesquipedalian commented Jan 9, 2018

I am. ;)

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Jan 9, 2018

Contributor

The only thing this PR does at the moment is update the preg_match to json. If we're keeping the preg_match at all, we need this change, since the cookie is now encoded in json. The only wrinkle is trying to keep support for 2.0/serialized in addition to 2.1/json; it's slightly complicated because of that.

The root cause of the problem was that due to the serial/json mismatch, the cookie & session weren't destroyed upon logout. But the member's hash salt value gets updated upon logout. So, upon logon, the system gets confused as it finds a cookie that contains a hashed password, that is out of date - as it is based on the old salt. If it is allowed to clean up after itself properly on logout, the issues go away.

The specific logic that it can't get to is the "out with the old, in with the new" logic in setLoginCookie, in subs-auth around line 50. This is where the cookie should be destroyed and rebuilt. (setLoginCookie is also called during logout.) This logic exists specifically for "cookie_state" changes, and by "cookie_state", it means a change in the local or global cookie settings, i.e., the circumstances that trigger this bug.

Contributor

sbulen commented Jan 9, 2018

The only thing this PR does at the moment is update the preg_match to json. If we're keeping the preg_match at all, we need this change, since the cookie is now encoded in json. The only wrinkle is trying to keep support for 2.0/serialized in addition to 2.1/json; it's slightly complicated because of that.

The root cause of the problem was that due to the serial/json mismatch, the cookie & session weren't destroyed upon logout. But the member's hash salt value gets updated upon logout. So, upon logon, the system gets confused as it finds a cookie that contains a hashed password, that is out of date - as it is based on the old salt. If it is allowed to clean up after itself properly on logout, the issues go away.

The specific logic that it can't get to is the "out with the old, in with the new" logic in setLoginCookie, in subs-auth around line 50. This is where the cookie should be destroyed and rebuilt. (setLoginCookie is also called during logout.) This logic exists specifically for "cookie_state" changes, and by "cookie_state", it means a change in the local or global cookie settings, i.e., the circumstances that trigger this bug.

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Jan 9, 2018

Collaborator

A weaknes of this pr is,
that you don't define which type of json your want.
But you define with only one as right with your preg match.

Collaborator

albertlast commented Jan 9, 2018

A weaknes of this pr is,
that you don't define which type of json your want.
But you define with only one as right with your preg match.

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Jan 9, 2018

Contributor

It supports the json pattern for the current cookie definition. You are correct in that if we redefine the cookie somehow (e.g., as an object) then this would need to change.

Contributor

sbulen commented Jan 9, 2018

It supports the json pattern for the current cookie definition. You are correct in that if we redefine the cookie somehow (e.g., as an object) then this would need to change.

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Jan 9, 2018

Collaborator

and where you define that the json is saved as json array?

Collaborator

albertlast commented Jan 9, 2018

and where you define that the json is saved as json array?

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Jan 9, 2018

Contributor

The primary location is here, in setLoginCookie, around line 58:
$data = $smcFunc['json_encode'](empty($id) ? array(0, '', 0) : array($id, $password, time() + $cookie_length, $cookie_state));
No other parameters are passed to json_encode, so the default logic is used (objects are not forced). Note the data is an array as well.

Contributor

sbulen commented Jan 9, 2018

The primary location is here, in setLoginCookie, around line 58:
$data = $smcFunc['json_encode'](empty($id) ? array(0, '', 0) : array($id, $password, time() + $cookie_length, $cookie_state));
No other parameters are passed to json_encode, so the default logic is used (objects are not forced). Note the data is an array as well.

@sbulen

This comment has been minimized.

Show comment
Hide comment
@sbulen

sbulen Jan 11, 2018

Contributor

This has been superseded by PR #4481, so I'll close.

Contributor

sbulen commented Jan 11, 2018

This has been superseded by PR #4481, so I'll close.

@sbulen sbulen closed this Jan 11, 2018

Sesquipedalian added a commit to Sesquipedalian/SMF2.1 that referenced this pull request Jan 12, 2018

@sbulen sbulen deleted the sbulen:login_tweaks branch Jan 15, 2018

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