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 #127] Data dir permission is too wide #128

Merged
merged 1 commit into from
May 16, 2021

Conversation

ssiuhk
Copy link

@ssiuhk ssiuhk commented Jul 25, 2020

to:
cc: @subspacecommunity/subspace-maintainers
related to:
resolves: #127

Background

Data directory default permission is too wide that allows anyone from the system to read the configuration files

Changes

  • Summary of changes
  • Changed umask of the user during the section creating configuration files
  • Added special handling for config.json to update the permission upon next start-up

Testing

Using the updated entrypoint.sh

[root@localhost subspace]# rm -rf data
[root@localhost subspace]# docker-compose up -d
Creating subspace ... done
[root@localhost subspace]# ls -alh data
total 8.0K
drwxr-xr-x. 3 root root 42 Jul 26 02:29 .
drwxr-xr-x. 8 root root 156 Jul 26 02:28 ..
-rw-r--r--. 1 root root 4.3K Jul 26 02:29 config.json <- It was generated at later stage by the subspace process
drwx------. 4 root root 96 Jul 26 02:28 wireguard <- Permission tightened
[root@localhost subspace]# docker-compose restart
Restarting subspace ... done
[root@localhost subspace]# ls -alh data
total 8.0K
drwxr-xr-x. 3 root root 42 Jul 26 02:29 .
drwxr-xr-x. 8 root root 156 Jul 26 02:28 ..
-rw-------. 1 root root 4.3K Jul 26 02:29 config.json <- Permission tightened upon 2nd run
drwx------. 4 root root 96 Jul 26 02:28 wireguard

- Changed umask of the user during the section creating configuration files
- Added special handling for config.json to update the permission upon next start-up
@sonarcloud
Copy link

sonarcloud bot commented Jul 25, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Collaborator

@agonbar agonbar left a comment

Choose a reason for hiding this comment

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

Testing this, it seems to work, the config is no longer public, the folders and files all get the correct permissions and the server keeps access to them on startup.

Only thing missing, the new files in clients and peers get 644 permission from subspace, but at least this is an improvement.

@ssiuhk
Copy link
Author

ssiuhk commented Jul 28, 2020

Yeah, for that probably need to change default umask when starting the subspace application, but not sure about impact of that.
Anyway, upper level folder locked up should be good enough

@jacksgt
Copy link

jacksgt commented Oct 12, 2020

Yeah, for that probably need to change default umask when starting the subspace application, but not sure about impact of that.
Anyway, upper level folder locked up should be good enough

Thank you for this patch! I really appreciate someone talking a look at the small security details.

I don't think changing the umask will have any negative impact, since it only affects newly created files (i.e. there should be no regressions for people upgrading).

@agonbar agonbar merged commit f1e1cd8 into subspacecommunity:master May 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data dir permission is too wide
3 participants