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

Slight quibble about implementation #49

Closed
designermonkey opened this issue Feb 8, 2016 · 7 comments
Closed

Slight quibble about implementation #49

designermonkey opened this issue Feb 8, 2016 · 7 comments

Comments

@designermonkey
Copy link
Member

@designermonkey designermonkey commented Feb 8, 2016

One of my team has been using this and having difficulty in understanding it's implementation due to the fact that a fresh token pair is generated on every single request.

It wasn't until he told me that the ajax side of the project was complicated due to having to get access to the right token for each step of a request system that I looked up CSRF in general.

https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)_Prevention_Cheat_Sheet#General_Recommendation:_Synchronizer_Token_Pattern

This is a resource that I found that suggests it should be one token per session. Is there any reason that this implementation is one per request?

@geggleto

This comment has been minimized.

Copy link
Contributor

@geggleto geggleto commented Feb 9, 2016

I think you are mainly referencing...

Characteristics of a CSRF Token
Unique per user & per user session
Tied to a single user session
Large random value
...
The synchronizer token pattern requires the generating of random "challenge" tokens that are associated with the user's current session.

I hate to be a stickler but the document you linked makes no reference to there being only 1 per session. It suggests that one can create many challenge tokens.

Currently with Slim-Csrf it is quite difficult for users who are client side rendering. This package is mainly for projects who are server side rendering, where you can simply inject the name and value into your template.

For REST APIs i would say this problem is a non-starter since REST apis should not be using sessions at all and other access control packages should be employed.

I hope this makes sense.

@akrabat

This comment has been minimized.

Copy link
Member

@akrabat akrabat commented Feb 9, 2016

I'd take a PR that provides a setting that sets the token once per session.

@akrabat

This comment has been minimized.

Copy link
Member

@akrabat akrabat commented Feb 9, 2016

FWiW, the reasoning for per-request tokens is explained here: http://blog.ircmaxell.com/2013/02/preventing-csrf-attacks.html

@canihavesomecoffee

This comment has been minimized.

Copy link
Contributor

@canihavesomecoffee canihavesomecoffee commented Apr 10, 2016

I've used this package in combination with some AJAX requests, and what I did was storing the CSRF key/value in a variable in javascript, and each time a request was made, append this (and on update, return the updated values so that upon successive requests it still would work).

So basically something like this:

var csrf_name_key, csrf_name_value, csrf_value_key, csrf_value_value;
csrf_name_key = '{{ csrf_name_key }}';
csrf_name_value = '{{ csrf_name_value }}';
csrf_value_key = '{{ csrf_value_key }}';
csrf_value_value = '{{ csrf_value_value }}';

var xhr = new XMLHttpRequest();
xhr.open('POST', url, true);
xhr.onload = function () {
    if (xhr.status === 200) {
        location.reload();
    } else {
        var json = JSON.parse(xhr.responseText);
        csrf_name_key = json.csrf_name_key;
        csrf_name_value = json.csrf_name_value;
        csrf_value_key = json.csrf_value_key;
        csrf_value_value = json.csrf_value_value;
    }
};
var data = new FormData();
data.append(csrf_name_key, csrf_name_value);
data.append(csrf_value_key, csrf_value_value);
xhr.send(data);

This adds a bit of complexity to be completely honest, which could be removed by session tokens, but as mentioned in the article, I'd rather have that little bit of complexity added over the possibility of replay attacks.

@alexweissman

This comment has been minimized.

Copy link
Contributor

@alexweissman alexweissman commented Aug 13, 2016

If you take a look here, it does indeed say that:

In general, developers need only generate this token once for the current session. After initial generation of this token, the value is stored in the session and is utilized for each subsequent request until the session expires. When a request is issued by the end-user, the server-side component must verify the existence and validity of the token in the request as compared to the token found in the session. If the token was not found within the request or the value provided does not match the value within the session, then the request should be aborted, token should be reset and the event logged as a potential CSRF attack in progress.

@akrabat

This comment has been minimized.

Copy link
Member

@akrabat akrabat commented Aug 13, 2016

@alexweissman As per Anthony's article referenced above, if you store in the session, you are still vulnerable to replay attacks. Happy to take a PR that puts Slim-Csrf into a mode where there's one token per session though.

@alexweissman

This comment has been minimized.

Copy link
Contributor

@alexweissman alexweissman commented Aug 13, 2016

Ok. It seems like the only real risk for replay attacks is if the site is not being served over SSL (or the attacker has physical access or browser access, in which case CSRF tokens aren't going to do much anyway).

I guess there are sites that use sessions for user data, and yet aren't using SSL (even though they should be). So, I suppose the "new token per request" does have a use case. However, I suspect that once-per-session will satisfy the security requirements for the vast majority of sites that require protection against CSRF.

And, having a token persist throughout a session will greatly mitigate the problems mentioned in #33.

I'll take a crack at persistence mode. However, I do think that should be the default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.