vrcx-team / VRCX

Friendship management tool for VRChat
MIT License
1.04k stars 188 forks source link

[Feature Request] Use the operating systems credential manager #740

Open VasilisThePikachu opened 8 months ago

VasilisThePikachu commented 8 months ago

Explain in detail what your suggested feature would be used for.

https://support.microsoft.com/en-us/windows/accessing-credential-manager-1b5c916a-6a16-889f-8581-fc16e8165ac0

I don't need a security expert to explain that saving the user password in plain text is a terrible idea. You never know when the user may get their password leaked out cause they had a credential grabber on their operating system. Or if someone sends their database file to someone thinking they are getting troubleshooting.

The easiest fix for this is using the operating systems credential manager.

Describe how it would look if it requires a UI. Remove the "encrypt password" tick in advanced settings. And use this by default ALWAYS

Explain why people would want to use it. Storing passwords in plain text is bad. Also, you don't have a security section so here I am.

PJB3005 commented 8 months ago

The actual windows API in question: https://learn.microsoft.com/en-us/windows/win32/api/wincred/

DubyaDude commented 8 months ago

I do agree with this, this should be protected in some form.

For our use case though, I don't think we'd need to interface with win32 directly, dotnet provides a nice abstraction layer: https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.protecteddata?view=dotnet-plat-ext-8.0

We should also ensure the implementation works on Linux/under Wine.

Nekromateion commented 8 months ago

According to https://security.stackexchange.com/questions/119765 Any application can retrieve the values in plain text so it would be less secure compared to the "encrypt password" option which requires you to input the encryption key to get the password. Since this also centralizes the location of passwords it would allow any attacker to steal several passwords at once without having to specifically get the VRCX database. So using the credential manager would not offer any safety improvements over storing the password in the VRCX DB.

DubyaDude commented 8 months ago

Redacted brought up how easy it is to dump DPAPI in DMs, it alone* is probably not the solution we're looking for here.

DubyaDude commented 8 months ago

Just to re-iterate issues brought up:

The following suggestion was brought up which seems most viable:

If it indeed is not protected and vulnerable, one could still have a mixed scheme where an encryption key is stored in OS credential store, and the actual password is still stored in VRCX database encrypted with that. That would defeat DPAPI enumeration attacks (no password there), and require specific targeting of VRCX plus OS credential store

VasilisThePikachu commented 8 months ago

In a perfect world, you would be storing the login token or something in there instead. I'm assuming you guys have a good reason to save the password and not some kind of login token you can renew

DubyaDude commented 8 months ago

VRChat currently does not supply a renewable login token. Once a token expires that's that, only a user/pass can make a new one afaik.

Myrkie commented 8 months ago

its only been 5 years, it will happen soon don't worry https://feedback.vrchat.com/feature-requests/p/provide-an-authenticationauthorization-model-for-third-party-api-integrations

VasilisThePikachu commented 7 months ago

May I have a commit or PR for when this got resolved?

VasilisThePikachu commented 7 months ago

@Natsumi-sama I request this issue to be reopened. A quick browse into the SQlite database reveals the password is not encrypted by default. Or that the OS creds manager is used in any way.

This issue has not been resolved

Nekromateion commented 7 months ago

@VasilisThePikachu Please read the other comments on this issue. It has already been said in the comments that the OS credential manager is a worse solution than what is in place now.

VasilisThePikachu commented 7 months ago

A solution was brought up in here to generate a random private key to store and use that to encrypt the password.

It's also as bad to store the key in plain text.

It was also not explained to me why the issue was closed. Even after I asked for a commit with a fix or at least an explanation on why it was closed.