-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Newly created configuration files are world readable #4571
Comments
There is no sane default you can configure here. Some users are relying on the world readable to be able to backup for example. Also, there unfortunately is no quick and easy solution to solve this exxcept by always setting the modes during all write actions. We might be able to centralize all file writing actions maybe and allow something like a umask behavior. Only file uploads need to be handled differently since those are done by Tempfile. Though, the default then still would first be 644/755 i think to not break existing installations. |
It is often best to be secure-by-default, and allow users to be less secure if they have a need for that like with your backup example (even though they should configure that through the group and add their backup user to the vw group). Especially for a product like a password manager, the sane default should be as strict as what is at minimum needed for vw to run.
Aren't there something like a pre-start or post-release-upgrade hooks?
It would probably be best to come up with a migration path that would allow us to fix this behaviour without breaking user space indeed. Cause the F.e. a migration path could look something like this:
For docker, it would probably be best to just always run the vw server as a user instead of root, but let the user specify UID & GID through environmental variables. In docker it should then also be safe to change the default UMASK for that user to 027 |
A quick reply regarding the docker user, that isn't something easily changed, as that also breaks existing setups. If something like that is to be done it needs to be announced upfront for a long time. I do agree that the current way isn't ideal, but changing it also is not that easy with all the different setups already running. |
I "fixed" the issue on my side by setting the permissions of the I'm using vaultwarden w/o docker and with MySQL, but your data dir is a bind mount, so you can always change the dir permissions easily. |
I discovered this configuration error accidentally after installing my vaultwarden instance. I'm sorry if this is unhappy for you to hear, but there is no way that the rsa_key should ever be world-readable by default. I don't know enough about Bitwarden to say whether the sqlite databases should be private, too, but I'm hoping these are encrypted and shouldn't require special care. Having said that, it will be nice to have them not world-readable, too. With the current settings, Vaultwarden is a security liability and I would not recommend it to anyone. |
Hmm, I don't think you know what you are talking about. It is certainly not ideal, but it is not a security liability. I suggest you look at the architecture and the security implementation. |
Was taking a look at this today, and it's actually very easy to solve this. Ill see if i can add this in a simple way by default though via an ENV. Simple example: scripts/umask.sh: #!/usr/bin/env sh
umask 077 docker run --rm -it \
-e DISABLE_ADMIN_TOKEN=true \
-e ROCKET_PORT=8080 \
-v "${PWD}/data/:/data" \
-v "${PWD}/scripts/:/etc/vaultwarden.d/:ro" \
--network=host \
--name vwtest \
vaultwarden/server:testing-alpine Or via docker compose: ---
services:
vaultwarden:
image: vaultwarden/server:testing-alpine
network_mode: host
container_name: vwtest
restart: unless-stopped
volumes:
- ./data/:/data/
- ./scripts/:/etc/vaultwarden.d/:ro
environment:
- TZ=Europe/Amsterdam
- ROCKET_PORT=8080
- LOG_FILE=/data/vaultwarden.log
- DISABLE_ADMIN_TOKEN=true |
To provide a way to add more security regarding file/folder permissions this PR adds a way to allow setting a custom `UMASK` variable. This allows people to set a more secure default like only allowing the owner the the process/container to read/write files and folders. Examples: - `UMASK=022` File: 644 | Folder: 755 (Default of the containers) This means Owner read/write and group/world read-only - `UMASK=027` File: 640 | Folder: 750 This means Owner read/write, group read-only, world no access - `UMASK=077` File: 600 | Folder: 700 This measn Owner read/write and group/world no access resolves dani-garcia#4571 Signed-off-by: BlackDex <black.dex@gmail.com>
To provide a way to add more security regarding file/folder permissions this PR adds a way to allow setting a custom `UMASK` variable. This allows people to set a more secure default like only allowing the owner the the process/container to read/write files and folders. Examples: - `UMASK=022` File: 644 | Folder: 755 (Default of the containers) This means Owner read/write and group/world read-only - `UMASK=027` File: 640 | Folder: 750 This means Owner read/write, group read-only, world no access - `UMASK=077` File: 600 | Folder: 700 This measn Owner read/write and group/world no access resolves dani-garcia#4571 Signed-off-by: BlackDex <black.dex@gmail.com>
I have created a PR which allows setting a custom |
Ill create/update a wiki once tagged. |
Subject of the issue
After setting up a new (docker) instance of vaultwarden, all configuration files/folders have 644/755 permissions
Deployment environment
Your environment (Generated via diagnostics page)
Steps to reproduce
Start a new vaultwarden docker instance and mount the
/data
volume to a new folderExpected behaviour
Vaultwarden configuration files should not be world readable, maybe even don't let groups have (write) access by default. So all files under /data should have at least 660/770 permissions and maybe 640/750 or 600/700
Actual behaviour
Vaultwarden configuration files are world readable
Troubleshooting data
The text was updated successfully, but these errors were encountered: