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

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

Open
VasilisThePikachu opened this issue Feb 28, 2024 · 12 comments
Open
Labels
feature New feature or request niche Niche request/bug fix

Comments

@VasilisThePikachu
Copy link

VasilisThePikachu commented Feb 28, 2024

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.

@VasilisThePikachu VasilisThePikachu added the feature New feature or request label Feb 28, 2024
@PJB3005
Copy link

PJB3005 commented Feb 28, 2024

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

@DubyaDude
Copy link
Contributor

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
Copy link
Contributor

Nekromateion commented Feb 28, 2024

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
Copy link
Contributor

DubyaDude commented Feb 28, 2024

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
Copy link
Contributor

Just to re-iterate issues brought up:

  • DPAPI is not as secure as it may seem, various tools exist for dumping it.
  • Utilizing DPAPI or an operating systems credential manager opens you to a much wider attack vector. Rather than just worrying about people targeting VRCX, you'd now need to deal with attacks on the much more targeted OS credential manager.
  • I would generally like an implementation that would work smoothly under Linux/Wine.

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

Myrkie commented Feb 29, 2024

@VasilisThePikachu
Copy link
Author

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

@VasilisThePikachu
Copy link
Author

VasilisThePikachu commented Apr 4, 2024

@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
Copy link
Contributor

@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
Copy link
Author

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.

@Natsumi-sama Natsumi-sama reopened this Apr 4, 2024
@Natsumi-sama Natsumi-sama added the niche Niche request/bug fix label Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request niche Niche request/bug fix
Projects
None yet
Development

No branches or pull requests

6 participants