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

In login form if user has entered wrong username and password then from next time validateRequest() returning false #15

Closed
amolbarewar opened this issue Aug 3, 2016 · 10 comments

Comments

@amolbarewar
Copy link

amolbarewar commented Aug 3, 2016

I am facing an issue while executing login action from the second time.
I can see the issue is with validateRequest() function where once we find valid CSRF token then we unset the current CSRF token (unset($this->session[$this->sessionIndex][$index]);)
I am validating valid CSRF token in the beginning of the action. I know it will work well if I check for CSRF validity at the end of the function but this seems to be not expected.

Can you please help me with this issue? what is the use of unsetting CSRF token inside validateRequest() ?

@paragonie-scott
Copy link
Member

Each token is intended to be used only once, and only on a given endpoint.

@amolbarewar
Copy link
Author

so this mean we need to call validateRequest() at the end of an endpoint when all the validations are complete?

@paragonie-scott
Copy link
Member

Generally,

if (AntiCSRF::validateRequest()) { // Do this only once per HTTP request!
    // Now $_POST can be relied on
}

And don't try to reuse tokens for multiple requests. And don't try to validate more than once.

@kocsismate
Copy link

Hey Scott,

I'd like to ask you what kind of vulnerabilities it opens if the CSRF token is not removed after validation? I know that it is less secure but if this is not a critical security issue then personally, I'd prefer to allow the reuse of tokens for better user experience (when the same form is open on multiple tabs).

I am curious about your answer! And if you agree, I could create a PR to make the reuse of tokens to be configurable.

@kocsismate
Copy link

@paragonie-scott I learned from a post of Anthony Ferrara that session-based CSRF won't protect against replay attacks. In spite of this fact, I am curious if you are wiling to accept a PR that would make the reuse of tokens configurable?

@paragonie-scott
Copy link
Member

I'm not sure you would even need a library in that case. If you need tokens to be reusable,

if (!isset($_SESSION['csrfToken'])) {
    $_SESSION['csrfToken'] = random_bytes(33);
}

if (isset($_POST['csrf'])) {
    if (!hash_equals($_SESSION['csrfToken'], $_POST['csrf'])) {
        throw new SecurityException('Invalid CSRF token');
    }
}

And then

echo '<input type="hidden" name="csrf" value="', base64_encode($_SESSION['csrfToken']), '" />', PHP_EOL;

@paragonie-scott
Copy link
Member

Forgot:

class SecurityException extends Exception { }

@kocsismate
Copy link

kocsismate commented Feb 6, 2017

I was doing something like that before but I was unsure if my token generation is secure (it wasn't of course). That's the reason I wanted to use your lib so that I don't have to worry about it anymore. :) So thank you for the example, I'll keep using what I was using before (with some modifications)!

@explody
Copy link

explody commented May 26, 2017

@paragonie-scott I agree with you, but the enforced single-use really does wreak havoc with AJAX forms. To that end, I hacked up configurable single_use and added configurable age limits.

I think this is a good balance between security and convenience. You still get at minimum, one token per form, but they'll be valid long enough for a user to complete an AJAX form successfully with more than one POST. Plus it will expire aged tokens, which seems like a good idea regardless.

Sent a PR

@paragonie-scott
Copy link
Member

This will be fixed in the next release, with the Reusable class.

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

No branches or pull requests

4 participants