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

Should cookie should be refreshed for every request #4

Closed
mebjas opened this issue Jun 4, 2014 · 14 comments
Closed

Should cookie should be refreshed for every request #4

mebjas opened this issue Jun 4, 2014 · 14 comments

Comments

@mebjas
Copy link
Owner

mebjas commented Jun 4, 2014

Currently the cookies are refreshed for each request that is sent.
Otherwise if we keep token constant until it expires, there is risk of token exposure, in a long run and it can later be used for CSRF attacks.

There are problems involved with refreshing cookies for every request as well, on the client side we need to update tokens added to entities like forms, xhr objects etc as soon as cookie is refreshed.

@abiusx
Copy link
Collaborator

abiusx commented Jun 5, 2014

If by cookie, you mean CSRF token (cookies are used for a multitude of objectives), a token should be generated and expired only when consumed (or timed out). It other words, it should behave like a CAPTCHA. You don't need to have one active token in the server side at a time, you probably need many more. Please point to the code for this so that I can understnad.

@mebjas
Copy link
Owner Author

mebjas commented Jun 5, 2014

Currently its like, token is refreshed for every request. code: https://github.com/mebjas/CSRF-Protector-PHP/blob/master/libs/csrf/csrfprotector.php#L142

By consumed you mean used?

@abiusx
Copy link
Collaborator

abiusx commented Jun 6, 2014

First of all, $_GET and $_POST can be set for each request. Second, do not unset them, instead set them to an empty array. Unsetting them will break most apps.

Third, you have a lot of repeated code. Change your logic to minimize them.

Fourth, to the actual point. The way you’re doing it is wrong, it does not allow an application to have more than ONE simultaneus operation running. If you open two tabs, you will always fail on tokens. Only consume a token when its valid, or when it times out.

You are also storing the valid token in the cookies! That is totally wrong. You should store all valid CSRF tokens with their respective URLs in session (server-side) instead of in client cookies.

Please create an issue for this on GitHub so that we can track.
-A


Notice: This message is digitally signed, its source and integrity are verifiable.
If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Jun 5, 2014, at 2:14 PM, minhaz [email protected] wrote:

Currently its like, token is refreshed for every request. code: https://github.com/mebjas/CSRF-Protector-PHP/blob/master/libs/csrf/csrfprotector.php#L142

By consumed you mean used?


Reply to this email directly or view it on GitHub.

@mebjas
Copy link
Owner Author

mebjas commented Jun 6, 2014

for $_GET and $_POST +1, and I have implemented it.
In my last commit I have tried to remove code repetition, but if you have some certain line of code in mind which I might have not noticed, please point it to me

@mebjas
Copy link
Owner Author

mebjas commented Jun 6, 2014

for the fourth point, even by using different tokens for each request, we can operate on two or more tabs. The js code, attaches the token at the point where request is made, which means even if I did some other operation in other tab, and token has changed in cookie. the code will pick the latest token and bind it with the current request. And later it will be validated correctly at the server.

But still I feel uncomfortable with the concept, of using different tokens for each request. But I do not understand what you mean by consume, are you suggesting cookie shall change for every successful validation of csrftoken?

@mebjas
Copy link
Owner Author

mebjas commented Jun 6, 2014

For the last point, The whole concept of this method is based on client side cookie :octocat:
Then only the js code can pick the token from cookie and attach it to every valid request. And then on server side we validate the token in (POST/GET) query against one in cookie.
How do we implement this logic using server-side cookies (or sessions)?

@mebjas
Copy link
Owner Author

mebjas commented Jun 6, 2014

Also this is already an issue on github. here's the link
#4

Minhaz,
minhaz.cistoner.org || cistoner.org

On Fri, Jun 6, 2014 at 6:14 AM, AbiusX [email protected] wrote:

First of all, $_GET and $_POST can be set for each request. Second, do not
unset them, instead set them to an empty array. Unsetting them will break
most apps.

Third, you have a lot of repeated code. Change your logic to minimize
them.

Fourth, to the actual point. The way you’re doing it is wrong, it does not
allow an application to have more than ONE simultaneus operation running.
If you open two tabs, you will always fail on tokens. Only consume a token
when its valid, or when it times out.

You are also storing the valid token in the cookies! That is totally
wrong. You should store all valid CSRF tokens with their respective URLs in
session (server-side) instead of in client cookies.

Please create an issue for this on GitHub so that we can track.
-A


Notice: This message is digitally signed, its source and integrity are
verifiable.
If you mail client does not support S/MIME verification, it will display a
file (smime.p7s), which includes the X.509 certificate and the signature
body. Read more at Certified E-Mail with Comodo and Thunderbird in
AbiusX.com

On Jun 5, 2014, at 2:14 PM, minhaz [email protected] wrote:

Currently its like, token is refreshed for every request. code:
https://github.com/mebjas/CSRF-Protector-PHP/blob/master/libs/csrf/csrfprotector.php#L142

By consumed you mean used?


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub
#4 (comment)
.

@abiusx
Copy link
Collaborator

abiusx commented Jun 6, 2014

The token should be stored in sever session, a copy can be sent to the client for processing, but its the server who defines tokens. We don’t want to let the client define its own tokens (token fixation).


Notice: This message is digitally signed, its source and integrity are verifiable.
If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Jun 6, 2014, at 3:03 AM, minhaz [email protected] wrote:

For the last point, The whole concept of this method is based on client side cookie
Then only the js code can pick the token from cookie and attach it to every valid request. And then on server side we validate the token in (POST/GET) query against one in cookie.
How do we implement this logic using server-side cookies (or sessions)?


Reply to this email directly or view it on GitHub.

@mebjas
Copy link
Owner Author

mebjas commented Jun 6, 2014

Ok we can implement this logic also, that we keep the token as a session variable, send one as cookie header. and validate using session one rather than the cookie one.

But considering the type of attack CSRF is. an attacker cannot modify cookie in victims browser, SOP won't permit attacker to do so. For this to work, attacker has to implement something like (xss + csrf) attack. so if attacker can implement xss -> he can still retrieve the token from cookie!

So isn't using session token an overhead here?

@mebjas
Copy link
Owner Author

mebjas commented Jun 6, 2014

@abiusx I'd like to know when should we refresh the token other than in case of timeout? I couldn't get what you mean by consumed?

@abiusx
Copy link
Collaborator

abiusx commented Jun 6, 2014

Its not that simple. Nowadays XSS/CSRF attacks are very complicated, and its better to be safe than sorry.


Notice: This message is digitally signed, its source and integrity are verifiable.
If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Jun 6, 2014, at 11:58 AM, minhaz [email protected] wrote:

Ok we can implement this logic also, that we keep the token as a session variable, send one as cookie header. and validate using session one rather than the cookie one.

But considering the type of attack CSRF is. an attacker cannot modify cookie in victims browser, SOP won't permit attacker to do so. For this to work, attacker has to implement something like (xss + csrf) attack. so if attacker can implement xss -> he can still retrieve the token from cookie!

So isn't using session token an overhead here?


Reply to this email directly or view it on GitHub.

@abiusx
Copy link
Collaborator

abiusx commented Jun 6, 2014

Consumption happens when a token is validly matched. Then it will be removed from session variables. This is important, because we don’t want a CSRF token to be used twice.
Otherwise the token can sit there for X requests and/or for a timeout ( a few minutes is always a good idea, imagine someone has to fill a huge form, clicks submit, and then haers token is invalid). Typically 30 minutes of timeout is appropriate, or you can let it timeout with session.
-A


Notice: This message is digitally signed, its source and integrity are verifiable.
If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Jun 6, 2014, at 11:59 AM, minhaz [email protected] wrote:

@abiusx I'd like to know when should we refresh the token other than in case of timeout? I couldn't get what you mean by consumed?


Reply to this email directly or view it on GitHub.

@mebjas
Copy link
Owner Author

mebjas commented Jun 6, 2014

Ok, I'll move the validation logic to session.
And token will be changed for valid match, or timeout. timeout duration will be 30minutes
This leads to my another questions, #5

@mebjas
Copy link
Owner Author

mebjas commented Jun 6, 2014

SESSION logic implemented! 03921f3

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

No branches or pull requests

2 participants