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

Replacing hashing password scheme with something more robust #3576

Open
jsjoeio opened this issue Jun 8, 2021 · 7 comments
Open

Replacing hashing password scheme with something more robust #3576

jsjoeio opened this issue Jun 8, 2021 · 7 comments
Labels
enhancement Some improvement that isn't a feature security Security related
Milestone

Comments

@jsjoeio
Copy link
Contributor

jsjoeio commented Jun 8, 2021

While hashing is a major step forward, the problem with this approach is that it still allows attackers who have access to the hash to just submit it as-is and gain access to code-server - effectively not very different from storing it in plaintext.

We should definitely look into replacing this with something more robust so that password hashing isn't just a placebo.

As security expert @oxy points out, our current approach for hashing the password and storing it in a cookie is more like a placebo-effect than a real security approach.

#3422 (comment)

Related:

@jsjoeio jsjoeio added enhancement Some improvement that isn't a feature security Security related labels Jun 8, 2021
@jsjoeio jsjoeio added this to the On Deck milestone Jun 8, 2021
@jsjoeio jsjoeio added this to ⏳ To do in 🗺️ Roadmap via automation Jun 8, 2021
@oxy
Copy link

oxy commented Jun 9, 2021

One possible route forward is to generate a token when code-server is started, and then return that token if authentication was successful - this means that you'd need to re-auth if you restart code-server, but I think its an acceptable middle ground between security and ease-of-use.

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jun 9, 2021

this means that you'd need to re-auth if you restart code-server, but I think its an acceptable middle ground between security and ease-of-use

Agreed! I like the sound of that approach.

@Livven
Copy link

Livven commented Jun 11, 2021

If I understand correctly the password hash is directly set as a cookie, instead of using some kind of session token, which is why a leak of the password hash on the server would allow attackers to authenticate with the password hash without knowing the original password, right?

In that case what you could do is to store sha256(argon2(password)) on the server and set argon2(password) as a cookie. This way, if an attacker steals sha256(argon2(password)) from the server, they would still not be able to authenticate.

Note that a simple sha256 is adequate here since argon2 already takes care of the key stretching.

As a bonus, you could even offload argon2 computation to the client as server relief, thanks to the additional sha256 on the server.

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jun 11, 2021

If I understand correctly the password hash is directly set as a cookie, instead of using some kind of session token, which is why a leak of the password hash on the server would allow attackers to authenticate with the password hash without knowing the original password, right?

Exactly!

In that case what you could do is to store sha256(argon2(password)) on the server and set argon2(password) as a cookie. This way, if an attacker steals sha256(argon2(password)) from the server, they would still not be able to authenticate.

Wow, this seems very straightforward to implement (and test too). Thanks so much for the suggestion and the tips! 🙌 Really appreciate it.

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 3, 2022

Note to self: we may actually move away from argon2. It requires native binaries and has caused a lot of problems with Termux, Raspberry Pi and other devices/environments.

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 3, 2022

We've also talked about moving to a session token which I believe VS Code has built-in support for.

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 3, 2022

node-rs now publishes an argon2 package. Might help here: https://github.com/napi-rs/node-rs/tree/main/packages/argon2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Some improvement that isn't a feature security Security related
Projects
Development

No branches or pull requests

3 participants