CSRF Token Problem

[devxtech]

I have a function for my script which will generate the CSRF token for the form and then display the template. The problem I have is that 1 out of say 10 tries will throw “Token Invalid”. Could this be caused by the fact that I set my token for the session inside a function?

Display Function

function displayLogin($sError = null, $sUsername = null, $sPassword = null){
	global $template;
	
	$sToken = generateToken();
	$_SESSION['token'] = $sToken;
	$template->token = $sToken;
	if(!empty($sError)){
		$template->sError = $sError;
	}
	if(!empty($sUsername)){
		$template->sUsername = $sUsername;
	}
	if(!empty($sPassword)){
		$template->sPassword = $sPassword;
	}
	$template->display('templates/login.tpl.php');
}

Token Generating Function

// Generate a unique and random token
function generateToken(){
	$sToken = md5(uniqid(rand(), true));
	return $sToken;
}

HTML Form in login.tpl.php

                                <?php if(!empty($this->sError)){ echo "<div class=\\"errorMessage\\">$this->sError</div>"; }?>
                                <form id="login" action="index.php" method="post">
                                    <fieldset>
                                            <ol>
                                                <li>
                                                        <label for="username">Username:</label>

                                                        <input type="text" name="username" id="username" value="<?php if(!empty($this->sUsername)){ echo $this->sUsername; }?>"/>
                                                </li>
                                                <li>
                                                        <label for="password">Password:</label>
                                                        <input type="password" name="password" id="password" value="<?php if(!empty($this->sPassword)){ echo $this->sPassword; }?>"/>
                                                </li>
                                            </ol>
                                    </fieldset>

                                    <fieldset class="submit">
                                            <input type="hidden" name="token" value="<?php echo $this->sToken; ?>" />
                                            <input type="hidden" name="submitted" value="true" />
                                            <div class="buttons">
                                                <button class="positive" type="submit"><img src="./images/lock.png" alt=""/>Login</button>
                                            </div>
                                    </fieldset>
                                </form>

index.php condition statement for submitted form

	if( isset($_POST['submitted']) ){
		if($_POST['token'] == $_SESSION['token'])
		{
			if( empty($_POST['username']) || empty($_POST['password']) ){
				$sError = 'Check that you provided both Username & Password';
			}
			if( !empty($sError) ){
				displayLogin($sError, $_POST['username'], $_POST['password']);
			} else {
				// Begin validation of submitted data.
				$sUsername = filter_var($_POST['username'],FILTER_SANITIZE_STRING);
				$sPassword = md5(PW_SALT.$_POST['password']);
				if( _isValidLogin($sUsername, $sPassword) ){
					createSession();
					_redirect('main.php');					
				} else {
					displayLogin("Invalid username and or password.");
				}
			}
			
		} else {
			// @todo(Log error and handle exit properly)
			die('Token is invalid');
		}
	}

Any help is appreciated.

[crmalibu]

Are you using your browsers back button? This could cause it.

I don’t see any issue in the code you posted. But, other possibilities exist. I would do some logging. Sprinkle you code with some debugging statments that log some info to a file. For example, log the location in the code where you’re calling displayLogin(). log the time with it. log the values of the token in the session, and the value that was posted. Do you get more log entries than expected?

Off Topic:

You also need to verify that $_SESSION[‘token’] actually has a value before comparing to the posted value. Otherwise your security is easily defeated.

[system]

As far as I understand CSRF mechanism, protection needed only for users who already logged in.
And, also, it does not need regeneration during session. One token per session would be enough.

[lampcms_com]

What do you mean by “session inside a function”?
A session is a session, it does not make any difference if you call it inside a function.

The reason for this maybe that you regenerating the new value of token on every page load, so if user opens a second browser window, (maybe you have a popup windows), then the token in his first opened window automatically becomes invalid

What you should do is to add an extra check
if(empty($_SESSION[‘token’]){
$sToken = generateToken();

$_SESSION['token'] = $sToken;

}

[devxtech]

@crmalibu

Are you using your browsers back button? This could cause it.

I don’t see any issue in the code you posted. But, other possibilities exist. I would do some logging. Sprinkle you code with some debugging statments that log some info to a file. For example, log the location in the code where you’re calling displayLogin(). log the time with it. log the values of the token in the session, and the value that was posted. Do you get more log entries than expected?

Off Topic:

You also need to verify that $_SESSION[‘token’] actually has a value before comparing to the posted value. Otherwise your security is easily defeated.

It seems to happen as a freak occurrence when I constantly batter the form with different attempts. I haven’t implemented a bunch of logging yet to dig deeper as I want to first analyize these other responses as I might not need CSRF on a login form from the sounds of it.

@Shrapnel_N5

As far as I understand CSRF mechanism, protection needed only for users who already logged in.
And, also, it does not need regeneration during session. One token per session would be enough.

Very interesting. Maybe I’m mistaken on the use of CSRF. I totally understand it’s use for action handled via GET such as http://example.com/delete/user/jdoe . So for a login form would with SQL injection protection be sufficient or should I consider other avenues of protection for the login as well?

@lampcms_com

What do you mean by “session inside a function”?
A session is a session, it does not make any difference if you call it inside a function.

The reason for this maybe that you regenerating the new value of token on every page load, so if user opens a second browser window, (maybe you have a popup windows), then the token in his first opened window automatically becomes invalid

What you should do is to add an extra check
if(empty($_SESSION[‘token’]){
$sToken = generateToken();

$_SESSION[‘token’] = $sToken;

}

So this extra check is ensuring that a user gets one token per session until they login and or successfully complete the action that the token is generated? Would you want to regenerate this token say after they successfully delete a user?

[system]

No sensitive action should be taken using GET.

[devxtech]

I believe my question now is if CSRF is a bit overkill for a login form. Thoughts?

@Shrapnel_N5

No sensitive action should be taken using GET.

Understood, I was merely using GET as an example since a lot of CSRF topics I have read talk about that being a primary area for it to be used as those are commonly exploited. Personally I don’t use GET for anything that manipulates or interacts directly with a database.

[Ren]

This is a consequence of storing state on the server.
There are methods of doing this stateless.

http://scholar.google.co.uk/scholar?hl=en&q=Robust+Defenses+for+Cross-Site+Request+Forgery&btnG=Search&as_sdt=2000

Also the Origin request header (http://tools.ietf.org/html/draft-abarth-origin-06) has been implemented in some browsers.

http://blog.chromium.org/2010/01/security-in-depth-new-security-features.html

Which is trivial to implement, and doesn’t hinder non-supporting browsers that I think it probably worth implementing that check together with CSRF tokens.

[system]

I believe my question now is if CSRF is a bit overkill for a login form.

Agree.
CSRF is used to make valid user to do something bad. As we are not logged yet, there are no valid user at all.

[Ren]

@Shrapnel_N5

Agree.
CSRF is used to make valid user to do something bad. As we are not logged yet, there are no valid user at all.

There are attacks based on CSRF exploits logging an unsuspecting user with an attackers account. As outlined the first link I posted above.

[devxtech]

@Ren

This is a consequence of storing state on the server.
There are methods of doing this stateless.

http://scholar.google.co.uk/scholar?hl=en&q=Robust+Defenses+for+Cross-Site+Request+Forgery&btnG=Search&as_sdt=2000

Also the Origin request header (http://tools.ietf.org/html/draft-abarth-origin-06) has been implemented in some browsers.

http://blog.chromium.org/2010/01/security-in-depth-new-security-features.html

Which is trivial to implement, and doesn’t hinder non-supporting browsers that I think it probably worth implementing that check together with CSRF tokens.

I read an article found in your first link and so let me see if I am following correctly. You are suggesting a stateless approach by using a cookie to store the CSRF token on the client side and at the same time putting the same token in the form. Then on the server side checking to see if that CSRF token in the cookie matches the one stored in the form?

So would then pairing a stateless approach with a SSL backed website be a good layer of security? I know this won’t prevent someone which access to that users machine from getting the information but I would think it would cover most scenarios of a remote attacker trying to break in. Thoughts? Any other ways they could get in? I already cover XSS attacks in my code just need to get the CSRF protection implemented.

[Ren]

@devxtech

I read an article found in your first link and so let me see if I am following correctly. You are suggesting a stateless approach by using a cookie to store the CSRF token on the client side and at the same time putting the same token in the form. Then on the server side checking to see if that CSRF token in the cookie matches the one stored in the form?

No, the token is still embedded in the form.

Rather than generating a random token, you can use

function generateToken($secret)
{
    return hash_hmac('sha256', session_id(), $secret);
}

And each form|controller has it’s own secret. So when displaying a form using its secret with generateToken() to generate the CSRF token, and validate when it has been submitted.

This means can omit storing the token in the $_SESSION.

This is similar to what RoR’s protect_from_forgery does.

[devxtech]

@Ren

No, the token is still embedded in the form.

Rather than generating a random token, you can use

function generateToken($secret)
{
    return hash_hmac('sha256', session_id(), $secret);
}

And each form|controller has it’s own secret. So when displaying a form using its secret with generateToken() to generate the CSRF token, and validate when it has been submitted.

This means can omit storing the token in the $_SESSION.

This is similar to what RoR’s protect_from_forgery does.

Sorry but I’m a bit confused now. I’m trying to put this all together and I’ve managed to get myself stuck. So in my below code how would I go about getting the token back into the redisplayed form that shows up when a login fails? Would I just generate a new token and then reset the cookie?

login.tpl.php from above

<form id="login" action="index.php" method="post">
    <fieldset>
            <ol>
                <li>
                        <label for="username">Username:</label>

                        <input type="text" name="username" id="username" value="<?php if(!empty($this->sUsername)){ echo $this->sUsername; }?>"/>
                </li>
                <li>
                        <label for="password">Password:</label>
                        <input type="password" name="password" id="password" value="<?php if(!empty($this->sPassword)){ echo $this->sPassword; }?>"/>
                </li>
            </ol>
    </fieldset>

    <fieldset class="submit">
            <input type="hidden" name="token" value="<?php echo $this->sToken; ?>" />
            <input type="hidden" name="submitted" value="true" />
            <div class="buttons">
                <button class="positive" type="submit"><img src="./images/lock.png" alt=""/>Login</button>
            </div>
    </fieldset>
</form>

revised index.php page with login processor

if( isset($_POST['submitted']) ){
    if(isset($_COOKIE['token']){
        if($_POST['token'] == $_COOKIE['token'])
        {
            if( empty($_POST['username']) || empty($_POST['password']) ){
                $sError = 'Check that you provided both Username & Password';
            }
            if( !empty($sError) ){
                displayLogin($sError, $_POST['username'], $_POST['password']);
            } else {
                $sUsername = filter_var($_POST['username'],FILTER_SANITIZE_STRING);
                $sPassword = md5(PW_SALT.$_POST['password']);
                if( _isValidLogin($sUsername, $sPassword) ){
                    createSession();
                    _redirect('main.php');                    
                } else {
                    displayLogin("Invalid username and or password.");
                }
            }
            
        } else {
            // @todo(Log error and handle exit properly)
            die('Token is invalid');
        }
    } else {
        // Access forbidden:
        header('HTTP/1.1 403 Forbidden');
    }
} else {
    $sToken = generateToken();
    setcookie("token", "$sToken", time()+300);
	displayLogin($sToken);
}

New Display Login

function displayLogin($sToken, $sError = null, $sUsername = null, $sPassword = null){
    global $template;

    $template->token = $sToken;
    if(!empty($sError)){
        $template->sError = $sError;
    }
    if(!empty($sUsername)){
        $template->sUsername = $sUsername;
    }
    if(!empty($sPassword)){
        $template->sPassword = $sPassword;
    }
    $template->display('templates/login.tpl.php');
}

Off Topic:

Should I have a user reenter their password on an incomplete form error? Such as if they forget to put in the username but enter a password, should I then refill in the password for them? I was thinking no.

[Ren]

You don’t need any explicit cookie code. Assuming the session is already started…

define('LOGINSECRET', 'loginsecretkey');

function generateToken($secret)
{
    return hash_hmac('sha256', session_id(), $secret);
} 

if( isset($_POST['submitted']) ){
    if(isset($_POST['token']){
        if($_POST['token'] === generateToken(LOGINSECRET))
        {
            if( empty($_POST['username']) || empty($_POST['password']) ){
                $sError = 'Check that you provided both Username & Password';
            }
            if( !empty($sError) ){
                displayLogin($sError, $_POST['username'], $_POST['password']);
            } else {
                $sUsername = filter_var($_POST['username'],FILTER_SANITIZE_STRING);
                $sPassword = md5(PW_SALT.$_POST['password']);
                if( _isValidLogin($sUsername, $sPassword) ){
                    createSession();
                    _redirect('main.php');                    
                } else {
                    displayLogin("Invalid username and or password.");
                }
            }
            
        } else {
            // @todo(Log error and handle exit properly)
            die('Token is invalid');
        }
    } else {
        // Access forbidden:
        header('HTTP/1.1 403 Forbidden');
    }
} else {
    $sToken = generateToken(LOGINSECRET);
    displayLogin($sToken);
} 

[devxtech]

@Ren

You don’t need any explicit cookie code. Assuming the session is already started…

define('LOGINSECRET', 'loginsecretkey');

function generateToken($secret)
{
    return hash_hmac('sha256', session_id(), $secret);
} 

if( isset($_POST['submitted']) ){
    if(isset($_POST['token']){
        if($_POST['token'] === generateToken(LOGINSECRET))
        {
            if( empty($_POST['username']) || empty($_POST['password']) ){
                $sError = 'Check that you provided both Username & Password';
            }
            if( !empty($sError) ){
                displayLogin($sError, $_POST['username'], $_POST['password']);
            } else {
                $sUsername = filter_var($_POST['username'],FILTER_SANITIZE_STRING);
                $sPassword = md5(PW_SALT.$_POST['password']);
                if( _isValidLogin($sUsername, $sPassword) ){
                    createSession();
                    _redirect('main.php');                    
                } else {
                    displayLogin("Invalid username and or password.");
                }
            }
            
        } else {
            // @todo(Log error and handle exit properly)
            die('Token is invalid');
        }
    } else {
        // Access forbidden:
        header('HTTP/1.1 403 Forbidden');
    }
} else {
    $sToken = generateToken(LOGINSECRET);
    displayLogin($sToken);
} 

Thanks for the response. Is there anyway this could be broken?

[Ren]

@devxtech

Thanks for the response. Is there anyway this could be broken?

To forge a request an attacker would need to know both the session_id of the intended victim, and the application secret key.

The session_id is in a cookie, but as your initial CSRF protection used the cookie indirectly (setting $_SESSION[‘token’]) its no more insecure than that.

If your site has a XSS (Cross site scripting) vulnerability then the CSRF protection can be bypassed, as the attacker can access CSRF token value, and post directly.

PS:
After a successful login, it is recommended that you use session_regenerate_id() to generate a new session_id. Avoiding session fixation attacks. And will invalidate any previously generated CSRF tokens with the old session.

[devxtech]

@Ren

To forge a request an attacker would need to know both the session_id of the intended victim, and the application secret key.

The session_id is in a cookie, but as your initial CSRF protection used the cookie indirectly (setting $_SESSION[‘token’]) its no more insecure than that.

If your site has a XSS (Cross site scripting) vulnerability then the CSRF protection can be bypassed, as the attacker can access CSRF token value, and post directly.

PS:
After a successful login, it is recommended that you use session_regenerate_id() to generate a new session_id. Avoiding session fixation attacks. And will invalidate any previously generated CSRF tokens with the old session.

Thanks for the help.