-
-
Notifications
You must be signed in to change notification settings - Fork 646
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
Security: Store api token hashed #325
Comments
Yeah, this is kinda intentional. Otherwise, tokens can't be viewed at any time inside the webui. The bcrypt default can be changed. |
Nope, nothing happened yet. |
Why should token be hashed? I am a little confused. The reason passwords are derived is that they have patterns that reduce entropy and grant additional scope if the user do not use unique passwords:
Tokens are randomized to begin with and grants no additional scope than the session originally has, if you can get your hand on the DB entry of that token chances are the whole service is already compromised (everything we do is in the database), session integrity is already not a concern now. The only additional thing an attacker can do is maybe delete all messages if somehow their original database access is read only? The only thing that is useful I think is to avoid showing tokens back to the user once it is viewed to prevent session hijacking or impersonation that is caused by vulnerability on the client side. Hashing on the server side has nothing to do with this. @jmattheis Do you think this is worth patching? If not I think we can close this. |
Passwords are hashed so users can't be impersonated if the db leaks (with a backup publicly accessible for instance). They are hashed with a salt to not make obvious when a password is reused (and avoid rainbow table) and hashed with a resource intensive algorithm because passwords are often weak (pattern etc.), to avoid brute force/dictionary. With API token, you don't want users to be impersonate when the db leaks too. But the API tokens are unique and need to be unique: so you must not use a salt. And token are randomly generated, so you'll prefer a fast algorithm (like SHA256). Most of the time, token are displayed only once to users, and that's (hopefully) the reason, or one of them. But tokens won't be able to be seen again |
Thanks for the reply. If the DB is leaked, what is the additional thing an attacker can gain by obtaining an access token? The only scenario this would be worth it is if you somehow get a read only copy of the DB, and are not satisfied with seeing essentially all data on Gotify but need to actively modify it. I think this precise level of initial foothold and desired outcome is not common in the context of Gotify. Password is different because they may be reused hence grant additional scope in an attack. |
If the leak isn't notice, they can continue to get data about users, or modify some subscriptions.
|
Thanks for the follow up, understand your position better now.
This is true regardless whether the token is leaked or not.
These dependent on Gotify being used for integrity critical scenarios which we did not make such an claim, nor did we discourage such use. So this is not invalid scenario but is an attack precondition out of the attacker's control. The other exploit cases can be done with DB access alone. My assessment is CVSS:3.1/AV:N/AC:L/PR:H/UI:N/S:U/C:N/I:L/A:L - 3.8 Low. The score is low because by CVSS standards the effect of integrity compromise is limited (you are only allowed to do what the user can do legitimately, not override the application, and the effect of the integrity compromise does not by itself lead to changed scope such as the IDS scenario you mentioned). You are welcome to open a security advisory on GitHub if you want to be credited. Let me know if you are working on it. I think this can be fixed if @jmattheis agrees. |
If you want to report, I also encourage you to think creatively about ways to chain weaknesses like these together for a bigger impact :) (My previous job is threat hunting before I started doing academics instead). But please don't publish them beforehand of course. Thanks! |
Sure, but the thing with readable tokens is you can read live content from a snapshot of the DB, without access to the actual DB. So it would be C:L/I:N/A:L or even C:H/I:N/A:H depending on the content, may contain personal data etc. I:N because we can't modify content, only delete it. Nevertheless, I agree having a snapshot of the DB is a huge condition, and the risk is limited |
CVSS is neutral in what kind of content is actually leaked/modified. Low simply means you are still bound by something (what the application usually allows the user to do, what the application would typically allow the user to see, etc), no matter if it is my secret recipe or my banking information. I think this can be classified as C:L in certain scenarios (like a snapshot as opposed to direct access). It is definitely not high as you no longer have reliable access to all information, it is dependent on the token you use still being valid. Regardless this is an edge case scenario as you have mentioned. As an open source project it is usually assumed that no more security warranties once you tell me to connect to a compromised DB. |
Currently the android app depends on the availability of the plaintext application tokens for sending messages with certain applications https://github.com/gotify/android/blob/master/app/src/main/kotlin/com/github/gotify/sharing/ShareActivity.kt#L135 We need a different way to achieve this functionality, if the tokens are hashed. Maybe the /message endpoint can be accessed with a client token and the application-id is passed via query param/header. |
@jmattheis (I deleted my previous comment, I misunderstood) Yes this looks reasonable or A more "complete" fix is to use HMAC signed requests as opposed to just a token. However it will be not backwards compliant and pushing a message would become much harder to do by hand. |
@eternal-flame-AD Could explain what the correct interpretation of jmattheis comment is? I think i have the same misunderstanding. I thought this issue is about storing the secrets hashed on the server. I'm confused how this influences applications. Is jmattheis proposing JWTs or something like that? I think a hmac signature would be reasonable. Couldn't it be an optional field and disabled or enabled via settings? Of course that would mean that there is an additional feature to maintain. |
The android app need to impersonate an app to "share" the message so it needs to query for the token after it is created. Since clients can already create more applications we should just allow it to impersonate one without access to the actual token of that app. |
There can be one app token per client token with an API endpoint to re-generate the application token for the device, for instance: GET /application/token return and the server stores the hash of the token. If the client loses the token, it can generate a new token that will override the previous |
Is your feature request related to a problem? Please describe.
Tokens shouldn't be stored in plain text. (There isn't security issue to fill so I'm publishing here)
Describe the solution you'd like
The easiest to do is to store them hashed, like passwords. But unlike passwords, they need to be unique. Also, they aren't user input, that means that we can have a really long token and we can use fast hashing algorithm (long to crack but fast with the token)
So the main solution is to use a non-salted hash (e.g sha256) with a longer API token (e.g. 128 chars instead of 14).
Describe alternatives you've considered
An other solution would be to have an unique token_id with a salted hash.
Additional context
nothing
(One thing apart, the default work factor for bcrypt should be 12, as OWASP recommend)
The text was updated successfully, but these errors were encountered: