-
Notifications
You must be signed in to change notification settings - Fork 38
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
Storageless don't prevent replay attacks! #66
Comments
Any other servers can do this also with traditional sessions by just impersonating the user. If an action is not supposed to be repeated, a separate flag should be set in place in the storage specific to this feature. Could you please check the limitations docs and see if the scenario matches? |
This is a hint for experienced users. But your CSRF tokens don't be save.
If I could capture your "100 € submit" to an API with storageless session ... ;) Here another example:
I think that this is a pitfall. What do you mean with "public" and "private"?
In this case your publicOnly servers can never validate the JWT. I'm wrong? |
This kind of stuff is extremely risky and is done with idempotent flags that prevent double submission.
I'm all in for improving it! In general, this package covers well over the 90% use-case scenario, which is authentication and CSRF token generation
Correct, but the tokens are not encrypted in this library. All what consumers would do is validate the signature, which is done with a published public key. Token encryption is out of scope for now, and already goes over the 90% scenario (seriously, don't store confidential information in the token, use a vault!) Does that clarify your doubt on this bit? How can we improve the docs about it? |
@jensklose isn't that covered on https://github.com/psr7-sessions/storageless/blob/master/docs/limitations.md#sessions-cannot-be-invalidated? I mean, there're ways to revoke tokens and don't accept a token which is no longer valid (as in doesn't contain the last state of the session) but that requires a storage system - at least a cache - to either create a whitelist or a blacklist and validate the token accordingly. This library means to be 100% storageless and the users should use it accepting all the limitations it brings. Do you have any suggestion on how to make that clearer? |
@lcobucci I know that I was not involved in the original discussion, but let me make a suggestion:
I know the example is limited and not even 3/4 accurate, but it works for me and people I talk to and makes it a lot either to explain some implications |
That's the point. People like me, looking for a session handler and then come to this useful piece of code, but there is something different in it as promised. Again
CSRF tokens don't work. Without a storage your token has an unlimited validity. Maximum limited by the expiration time when set. Don't call it session. Don't compare it to session. It is JWT! |
Uhm, yeh, they work? I wrote ocramius/psr7-csrf and it does prevent CSRF
attacks.
…On 1 Jun 2017 20:30, "Jens Klose" ***@***.***> wrote:
That's the point. People like me, looking for a session handler and then
come to this useful piece of code, but there is something different in it
as promised.
Again
Storing information such as ... CSRF tokens ...
CSRF tokens don't work. Without a storage your token has an unlimited
validity. Maximum limited by the expiration time when set.
Don't call it session. Don't compare it to session. It is JWT
<https://jwt.io>!
Suggestion: Rework the whole README to better exposing the JWT character
of the module. Just give an indication of the storageless variant in some
applications.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#66 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJakO5qfBXHBAfqh2NaT0celWxdDRgzks5r_wNFgaJpZM4Nqonh>
.
|
Disclaimer: I know @jensklose personally. I think the point Jens brings up is that this particular session library states that it is usable for replacing traditional PHP sessions, but eventually fails to deliver. There is a dedicated file describing all the shortcomings of this library, but this is somehow hidden and has to be discovered. I suppose you are aware of the problems with using JWT for session data. I'd like to add two more links that state this one more time: http://cryto.net/~joepie91/blog/2016/06/13/stop-using-jwt-for-sessions/ and http://cryto.net/~joepie91/blog/2016/06/19/stop-using-jwt-for-sessions-part-2-why-your-solution-doesnt-work/ I won't tell you to delete this library because it does things some people don't like or consider dangerous. However my point is that the untrained developer will probably miss the file describing the problems. When you come to the front page, you are greeted with a very nice README that first tells you how to install and use it, then explains that ext/session has "huge" problems (IMO this exaggerates the problems way too much, this part scares devs into not using PHP sessions). After that it explains what this library does, and lists a number of advantages. The known limitations (or disadvantages) are not listed in this document, but hidden behind a link. There is no "security considerations" section in the README or in the limitations document, and the way JWT is used to store data is never even linked to having security issues. The fact that all data stored in the JWT is publicly accessible is a security issue. The technical limitations like finite amount of data being stored, a certain computational overhead validating the token itself, or the problems with concurrent requests changing the cookie token are no security issues, but technical limitations that I would consider putting into that existing document. To be fair, the impact of the security issue part depends on what kind of application makes use of this library. But the security problems should be mentioned more prominently on the front page, to make developers really consider if the supposed benefit of being independent from a persistent storage outweighs the problems of having to deal with JWT "sessions". |
See also: #22 #57 (comment) #38 |
Which is what the limitations state clearly. What needs to happen here is:
I already use this library in production systems that use CSRF and authentication. We already figured out the mitigation scenarios for breaches (where a breach would be a leaked token), and besides that, the "log me out from all computers" is not something that most apps that I build have (scope of leaked session or authentication data is too small, security-wise). Therefore I'm going to suggest to stop the discussion on this thread, and instead suggest patches to the documentation. And also, repeating myself: this is a general purpose session, where general purpose follows the Pareto principle. |
Closing here meanwhile - this is solved and already documented in known limitations. Note that since 4.0.0, TL;DR: if you need to selectively clear user sessions, this library is indeed not for you, and you should indeed use a server-side storage. |
Attention! IMO, your approache don't work like promised.
What is the problem?
The obvious solution against replay attacks: When a request comes in, you check a token value, check that it is the current value, and if it is, process it, and then change the current value.
If you share the current value with the client without some 3th party storage, then the whole request is ready for playing again and again. All of your servers will validate the JWT and accept the included token as current.
Your solution is a JWT middleware.
This should work. The documentation promises that this is a substitute for server side storage. This is dangerous.
The text was updated successfully, but these errors were encountered: