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

Security Issue: Setup keys are stored unencrypted in database table "setup_keys" #2763

Closed
florian-obradovic opened this issue Oct 20, 2024 · 5 comments

Comments

@florian-obradovic
Copy link

Describe the problem

The setup keys are stored unencrypted in the database (at least for sqlite3) which is very bad.
If the backend or storage (Cloud & Selfhosted) gets compromised, anyone can use the setup keys to provision new peers.

To Reproduce

  1. sudo sqlite3 /var/lib/docker/volumes/artifacts_netbird-mgmt/_data/store.db Adjust the path to your docker volume
  2. select * from setup_keys;

Expected behavior
Store the setup keys encrypted by using at least salted hashes (More infos: https://www.vaadata.com/blog/how-to-securely-store-passwords-in-database/)

Are you using NetBird Cloud?

Selfhsoted

NetBird version
0.30.2

@sudomoke
Copy link

Setup keys should be set to expire, or limit the number of usage.

If you're using setup keys without any limits you should verify your rules are "very notrusty"

@florian-obradovic
Copy link
Author

florian-obradovic commented Oct 21, 2024

I generally agree, but there are use cases where you need keys with longer expiration dates (docker / kubernetes deployments, etc...).

On the other hand this discussing misses the point.
Unencrypted credentials in the database, especially high privileged credentials without second factor).

There is no good reason, why we should keep storing them unencrypted. No matter what short expiration time you choose.

@mgarces
Copy link
Contributor

mgarces commented Oct 22, 2024

hi @florian-obradovic, thank you for opening this issue.
Like previously discussed on Slack, we are currently working on the migration for these secrets, in the next few weeks, this should remove this security impact you mention here.

@florian-obradovic
Copy link
Author

Thanks a lot!

@heisbrot
Copy link
Contributor

Merged, see PR: #2775

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants