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

FIX(client): Permission for settings.json and backup was too open #6667

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

theboywholived
Copy link

@theboywholived theboywholived commented Dec 14, 2024

Changes permission for settings.json and corresponding backup file to 600.

Fixes #6652

Checks

Changes permission for settings.json and corresponding backup file to 600.

Fixes mumble-voip#6652
Copy link
Member

@Krzmbrzl Krzmbrzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there no way to set the permissions directly when writing the file?
If we're young this in a 2-step process, then there always is a short period of time in which the file still has the wrong permissions. Admittedly, that period is very short but ideally we wouldn't have that at all 👀

@davidebeatrici
Copy link
Member

I believe we should set the default permissions on the folder itself, perhaps with a warning when we detect sensitive files to be too accessible.

@theboywholived
Copy link
Author

theboywholived commented Dec 19, 2024

Is there no way to set the permissions directly when writing the file? If we're young this in a 2-step process, then there always is a short period of time in which the file still has the wrong permissions. Admittedly, that period is very short but ideally we wouldn't have that at all 👀

Right now I am not able to see any way to do this without using very low-level commands of the system. Qt does not offer any constructor while creating the files to set permissions.

I believe we should set the default permissions on the folder itself, perhaps with a warning when we detect sensitive files to be too accessible.

Does that seem possible?

@Krzmbrzl
Copy link
Member

I believe we should set the default permissions on the folder itself, perhaps with a warning when we detect sensitive files to be too accessible.

Not sure if this is really needed 👀
Doing permissions on an individual file level as needed seems fine to me.

Qt does not offer any constructor while creating the files to set permissions.

Alright, then it is as it is 🤷

@Hartmnt Hartmnt added client linux macOS feature-request This issue or PR deals with a new feature labels Dec 22, 2024
@Hartmnt
Copy link
Member

Hartmnt commented Dec 22, 2024

@theboywholived FYI, it looks like your commit does not contain valid author information. This is technically not a problem, but your commit will not be referenced to you or your GitHub account. You might want to update the commit with a valid user.name and/or user.email

Copy link
Member

@Hartmnt Hartmnt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not quite work, yet. It correctly sets the permissions on the mumble_settings.json at first.
But when I click OK in the settings dialog, it reliably changes the permissions back to 644 while Mumble is running. Only when I close the application properly it becomes 600 again.

@theboywholived
Copy link
Author

I will need to test my changes a bit more - I will get back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client feature-request This issue or PR deals with a new feature linux macOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better certificate protection - 600
4 participants