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

Security tokens error #12392

Closed
emiliodeg opened this Issue Nov 3, 2016 · 12 comments

Comments

Projects
8 participants
@emiliodeg
Copy link
Member

emiliodeg commented Nov 3, 2016

Well I think this is an error

echo $this->security->getSessionToken();
echo $this->security->getToken();
echo $this->security->getSessionToken();
echo $this->security->getToken();

//expeted output
RjlLK2k0bEZSd2hadThscnp3bWMwUT09 
bXdDMmx6cWZyMTRXdWF3SVdXc3VVUT09 
RjlLK2k0bEZSd2hadThscnp3bWMwUT09 
bXdDMmx6cWZyMTRXdWF3SVdXc3VVUT09 

//real output
RjlLK2k0bEZSd2hadThscnp3bWMwUT09 
bXdDMmx6cWZyMTRXdWF3SVdXc3VVUT09 
bXdDMmx6cWZyMTRXdWF3SVdXc3VVUT09 <<--- here getToken() rewrite getSessionToken()
bXdDMmx6cWZyMTRXdWF3SVdXc3VVUT09 

getToken function https://github.com/phalcon/cphalcon/blob/master/phalcon/security.zep#L354
getToken() should not change the value of getSessionToken() until the next request

I think this error generates the misunderstanding that makes programmers confuse us also removes the possibility of using the Validator Identical in our forms assigning the accepted value getSessionToken()

What do you think?

@mixinix

This comment has been minimized.

Copy link

mixinix commented Nov 3, 2016

I have same problem with this too. v3.0.1

I'm beginner, CSRF looks like easy in Phalcon, but not easy.

@temuri416

This comment has been minimized.

Copy link
Contributor

temuri416 commented Nov 4, 2016

Looks like you've come across the same issue:

https://forum.phalconphp.com/discussion/8093/csrf-problems-on-206#C21945

@emiliodeg

This comment has been minimized.

Copy link
Member Author

emiliodeg commented Nov 5, 2016

@temuri416 yes you are right, here some other examples

//some action view in volt 
{{ dump(security.checkToken(null, null, false)) }}
{{ security.getSessionToken() }}
{{ security.getToken() }}//here sessionToken was rewrited
{{ security.getSessionToken() }}
{{ security.getToken() }}
{{ dump(security.checkToken(null, null, false)) }}

<form action="" method="post">
    <input type="text" name="{{ security.getTokenKey() }}" value="{{ security.getToken() }}">
    <input type="submit">
</form>

//ouput
boolean true
aVAza2FRMy9ydzVqdGgvdU9GQTMrQT09
YTBuUlRpMGtBTlYwc0ZVeHdJakc5UT09
YTBuUlRpMGtBTlYwc0ZVeHdJakc5UT09
YTBuUlRpMGtBTlYwc0ZVeHdJakc5UT09
boolean false 

//other example
{{ dump(security.checkToken(null, null, false)) }}
{{ dump(security.checkToken()) }}
{{ dump(security.checkToken()) }}
{{ security.getSessionToken() }}//empty!!!
{{ security.getToken() }}//new token was generated

boolean true
boolean true
boolean false
----EMPTY SESSION TOKEN----
YTBuUlRpMGtBTlYwc0ZVeHdJakc5UT09

Thats works fine at first time but an getter modifying another getter it's an strange behavior
I think we must change this getToken() behavior

@sergeyklay

This comment has been minimized.

Copy link
Member

sergeyklay commented Dec 2, 2016

@emiliodeg Could you please provide the Phalcon version, OS, and way you initialized the Security service. I'll try to sort out

@mbrostami

This comment has been minimized.

Copy link
Contributor

mbrostami commented Dec 13, 2016

Consider that we have already a token in session e.g. aaa

1. echo $this->security->getSessionToken(); // returns STORED value "aaa"
2. echo $this->security->getToken(); // new value stores in session "bbb"
3. echo $this->security->getSessionToken(); // returns STORED value "bbb"
4. echo $this->security->getToken(); // returns generated value in step 2 "bbb"

In this scenario by default session is storing in a file that we read that, write that, and again read that. So it is expectable that after modifying file(session) content in step 2, we read the new content of file in step 3. For better understanding look at the below code:

1. echo $_SESSION['token']; // returns "aaa"
2. $generatedToken = ($generatedToken ?: $_SESSION['token'] = "bbb"); // $generatedToken is now "bbb"
     "bbb" is a random
3. echo $_SESSION['token']; // returns "bbb"
4. $generatedToken = ($generatedToken ?: $_SESSION['token'] = "ccc"); // $generatedToken is "bbb" because already generated

Same issue in the second example:

1. {{ dump(security.checkToken(null, null, false)) }} // returns true
2. {{ security.getSessionToken() }} // returns "aaa"
3. {{ security.getToken() }} // Modifies session content : "bbb"
4. {{ security.getSessionToken() }} // returns "bbb"
5. {{ security.getToken() }} // returns "bbb" already generated in step 3
6. {{ dump(security.checkToken(null, null, false)) }} // returns false because posted token is "aaa" that compares with "bbb"

And the last example:

1. {{ dump(security.checkToken(null, null, false)) }} // returns true
2. {{ dump(security.checkToken()) }} // returns true but removes token from session content(file storage)
3. {{ dump(security.checkToken()) }} // returns false because posted token is "aaa" that compares with null
4. {{ security.getSessionToken() }} // there is no token in session storage we removed that in step 2
5. {{ security.getToken() }} // new token stores in session storage, "bbb"

At last it is up to you that when and how to use these functions.

@emiliodeg

This comment has been minimized.

Copy link
Member Author

emiliodeg commented Dec 17, 2016

Well it isn't a bug is a wrong way to do a simple work
session token must not be modified in current request

PHP 7.0.14
Phalcon 3.0.2
Windows 7

Sessin service config

$di['session'] = function () use ($config) {
    $session = new Files(['uniqueId' => $config->session->unique_id]);
    $session->start();
    return $session;
};
@emiliodeg

This comment has been minimized.

Copy link
Member Author

emiliodeg commented Dec 20, 2016

If someone needs a more consistent operation here a small change in the logic of the security component

namespace PhalconFix;

class Security extends \Phalcon\Security
{
    private static $sessionToken = null;
    
    /**
     * {@inheritdoc}
     */
    public function getSessionToken()
    {
        if (self::$sessionToken === null) {
            self::$sessionToken = parent::getSessionToken();
        }
        return self::$sessionToken;
    }

    
    /**
     * {@inheritdoc}
     */
    public function getToken()
    {
        if (self::$sessionToken === null) {
            $this->getSessionToken(); //don't lose real session token, setup!
        }
        return parent::getToken(); //continues normally
    }
}

//services setup same as always
$di['security'] = function () {
    $security = new Security();
    $security->setWorkFactor(12);/*ohm no fluid setters....*/
    return $security;
};

//Now works like a charm
//anywhre in your code, here in volt

{{ security.getToken() }}
{{ security.getSessionToken() }} 
{{ security.getToken() }}
{{ security.getSessionToken() }}
{{ security.getToken() }}

SVpSaGd2S3E4cUV5ZXczMTZra0d5dz09
UFV1S1c0eWFxc3VOQk90RnJyTUcyQT09 
SVpSaGd2S3E4cUV5ZXczMTZra0d5dz09
UFV1S1c0eWFxc3VOQk90RnJyTUcyQT09 //nice work!!! didn't rewrite
SVpSaGd2S3E4cUV5ZXczMTZra0d5dz09

Have a nice new year!

@mbrostami

This comment has been minimized.

Copy link
Contributor

mbrostami commented Dec 21, 2016

Actually If I know your mean, you need a new feature e.g. getRequestToken() to return token for current request, because after the second call security.getToken() token has been changed and getSessionToken returns the current value of token NOT the old one.
The method getSessionToken has two different meaning,
1- get token value from session
2- get token value for current session (or current request!)
I think the first is correct so we need NFR, otherwise you are right and I suggest to name that method getRequestToken().

@emiliodeg

This comment has been minimized.

Copy link
Member Author

emiliodeg commented Dec 21, 2016

Hi @mbrostami I think methods without parameters must have a only one meaning
A new feature request with getRequestToken() could be the solution but the getSessionToken functionality is "strange"

@gguridi

This comment has been minimized.

Copy link
Contributor

gguridi commented Dec 29, 2016

I think the problem is just a misunderstanding. The method getToken not only generates a new token but sets it into the current session. This leaves the developer with the responsibility of keeping the previous token before calling this method and calling the checkToken method before that.

For me, the request token is only important during the validation (because in the post we are receiving the old one), otherwise is fine that when we call getToken again the session token changes. As soon as we regenerate the token we should start using it as the current session one.

I've added a pull request with a change proposal so the checkToken functionality doesn't use the regenerated session but uses the one that was set at the beginning if it existed.

@sergeyklay sergeyklay added this to the 3.0.4 milestone Jan 3, 2017

@sergeyklay sergeyklay modified the milestones: 3.1.x, 3.0.4 Jan 12, 2017

@Jurigag

This comment has been minimized.

Copy link
Member

Jurigag commented Jan 12, 2017

Well since we already got a consensus about what we should do here this issue is NFR and should aim 3.1.0 with PR #12518.

@sergeyklay sergeyklay modified the milestones: 3.2.x, 4.0.0 Jun 18, 2017

@stale stale bot added the stale label Apr 16, 2018

@sergeyklay sergeyklay closed this Apr 16, 2018

@sergeyklay sergeyklay reopened this May 2, 2018

@stale stale bot removed the stale label May 2, 2018

@niden niden added this to In progress in 4.0 Release Nov 29, 2018

@phalcon phalcon deleted a comment from stale bot Dec 9, 2018

@CameronHall

This comment has been minimized.

Copy link
Member

CameronHall commented Dec 9, 2018

This was resolved in this PR #13642.

@niden niden moved this from In progress to Done in 4.0 Release Dec 9, 2018

@sergeyklay sergeyklay closed this Dec 10, 2018

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