-
Notifications
You must be signed in to change notification settings - Fork 158
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
Minimise user write access to container service files #241
Merged
+4
−8
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Confirmed that Palworld Dedicated Server is working correctly:
Confirmed that all files have correct permissions within the container:
|
Callum027
commented
Mar 2, 2024
This PR reduces the number of files that the container user is given write access to before the user jail is started. This reduces the risk of files being modified by potential attackers if they managed to break into the container environment (through, for example, a vulnerability in Palworld.) The following files/directories have had their ownership changed to `root:root`: * `/entrypoint.sh` * `/PalWorldSettings.ini.template` * `/scripts` * `/includes` The container user still has full read access to these files. `PalWorldSettings.ini.template` is still copied by the user to the Palworld config dir (with correct ownership), and `server.sh` can set configuration values in it without issues. The only thing that has changed is that the container user can no longer *modify* these files. `PalWorldSettings.ini.template` and `rcon.yaml` have also had execute permissions removed, as they do not need to be executable.
Callum027
force-pushed
the
file-permission-changes
branch
from
March 3, 2024 00:29
69de5af
to
e757dae
Compare
Callum027
commented
Mar 3, 2024
jammsen
approved these changes
Mar 6, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, this will be merged when the other PRs will get more to the finishline too, that way i have less testing overhead.
jammsen
added
enhancement
New feature or request
security
This is a security topic
labels
Mar 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR reduces the number of files that the container user is given write access to before the user jail is started.
This reduces the risk of files being modified by potential attackers if they managed to break into the container environment (through, for example, a vulnerability in Palworld.)
The following files/directories have had their ownership changed to
root:root
:/entrypoint.sh
/PalWorldSettings.ini.template
/scripts
/includes
The container user still has full read access to these files.
PalWorldSettings.ini.template
is still copied by the user to the Palworld config dir (with correct ownership), andserver.sh
can set configuration values in it without issues. The only thing that has changed is that the container user can no longer modify these files.PalWorldSettings.ini.template
andrcon.yaml
have also had execute permissions removed, as they do not need to be executable.