-
Notifications
You must be signed in to change notification settings - Fork 342
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
[GHSA-x72p-g37q-4xr9] SFTPGo's JWT implmentation lacks certain security measures #4645
[GHSA-x72p-g37q-4xr9] SFTPGo's JWT implmentation lacks certain security measures #4645
Conversation
Hey @drakkan, apologies for the noise this advisory has caused you. After reading the source blog post I think I'm aligned with you that this is not a valid advisory. The cookie stealing indeed does seem like it would grant total access and is only mentioned as a one liner in the post
The term As best I can tell the most relevant claim is that a JWT is made for user_1 which is then used to access user_2's data (along with the cookie). I am not overly familiar with JWTs so, for the benefit of myself and for the public record would you mind digging into JWTs with me? Reading from the wikipedia article on the subject it does seem that identity (user_1 vs user_2) can be one of the claims embedded in the token
The blog post mentions that SFTPGo uses JWTs for session management (though it does not substantiate that claim)
The most generous interpretation I have is that user_1 gets a JWT for them and that JWT does not encode user information or is somehow usable in the context of user_2. Is it the case that if I have user_2's session cookie and user_1's JWT that I would gain access to something more than have either alone? |
@darakian thanks for taking the time to read the blog post. It contains technically incorrect information and can easily cause confusion. SFTPGo WebClient uses a cookie. The value of this cookie is a JWT token. So we don't have a JWT and a cookie but a cookie containing a JWT. The cookie is set this way:
and then the browser uses this cookie for successive requests. The jwt included in the cookie uses the In this case the author of the blog post has replaced a valid JWT contained in user1's cookie with a valid JWT contained in user2's cookie; it is quite obvious that this way it is possible to impersonate another user. It is irrelevant for the purposes of this report but I will add for completeness that the cookie is associated with the IP address for which it was generated, so a stolen cookie can only be used from the same IP address for which it was generated. Regarding the IDOR, if you try to reference resources belonging to user2 using user1's cookie, you will get a "not found" error. I hope I have answered your questions and clarified any doubts, if not, just ask. I also contacted MITRE about this issue. I hope this nonsense will be removed from all vulnerability databases soon and I won't have to contact them one by one. Thank you |
Happy to do it. That's what I'm here for 👍
Oh I see. Thank you for the clarification there. I was reading the post as if these were two independent objects which encoded separate permissions/capabilities/context. If it's all one object then indeed a loss of the cookie is total account access.
Yep, agreed. An attacker signing their own JWTs is out of scope of what this advisory is discussing and would likely be an operational error rather than an error in your code (assuming you don't expose the signing key in plaintext by default or whatever but that's also a different issue 😄)
Interesting. I think that is actually a relevant point at least to say out loud. Assuming the author ran their POC locally they would have likely missed that.
You absolutely have. Give me a few to get this marked as withdrawn and add a comment about the lack of validity. |
cf1c63b
into
drakkan/advisory-improvement-4645
Hi @drakkan! Thank you so much for contributing to the GitHub Advisory Database. This database is free, open, and accessible to all, and it's people like you who make it great. Thanks for choosing to help others. We hope you send in more contributions in the future! |
@darakian Thank you!
Many applications do not implement this additional check, but that does not mean they are vulnerable (for example I tested the same against a Django application and, at least by default, the cookie can also be used from different IP addresses). Unfortunately I wasted a lot of time trying to explain to the author why this report is invalid, I also mentioned this additional check, but it seemed like we were speaking different languages. I'm glad it's over now, thanks again. |
Happy to help :)
Ya, I would expect most applications would allow for ips to shift during the session for mobile/wifi/whatever reasons. Hence my surprise 😉
That sucks 😞 |
Updates
Comments
This CVE is invalid because it relies on cookie theft and the cookie was not stolen by exploiting an application layer vulnerability (e.g. XSS).
The reported issue affects every application that uses cookies.
If you steal users' cookies, you can obviously impersonate them and access their resources; this is a security issue if you can steal the cookie by exploiting a vulnerability in the application (SFTPGo in this case), but that's not what happens here.
This is not new: for example, Google is working on a new standard, Device Bound Session Credentials (DBSC), to mitigate issues like this.
The blog post also reports an Insecure Direct Object Reference (IDOR) vulnerability. Without stealing a cookie, no IDOR is possible, proper access controls are in place.