fix: keep the system report readable only by the user who asked for it - #30
Open
ehlinazgumus wants to merge 1 commit into
Open
fix: keep the system report readable only by the user who asked for it#30ehlinazgumus wants to merge 1 commit into
ehlinazgumus wants to merge 1 commit into
Conversation
generate_report() runs as root through pkexec and collects data the requesting
user cannot otherwise read: the full system journal (journalctl runs unfiltered
as root), dmidecode, blkid, netstat -neopa, the root environment and the system
logs under /var/log. It then finished with:
subprocess.run(["chown", pkexec_user, "-R", ARCHIVE_DIR])
subprocess.run(["chmod", "755", "-R", ARCHIVE_DIR])
`chmod 755 -R` applies to the files as well as the directories, so everything
ended up world readable under /tmp. On a test system the collected
journal_system was 2.5 MB and contained 3083 lines from root-owned units, while
the same user could not read any of it directly:
$ id # not in adm, not in systemd-journal
$ journalctl -q _UID=0 -n 2 # prints nothing
$ ls -l /tmp/pardus_system_report/<user>/journal_system
-rwxr-xr-x 1 <user> root 2525860 journal_system
The tree was also never removed. The only rmtree call is at the start of the
next generate_report(), so the data stayed in /tmp until another report was
generated or the machine rebooted.
Replace the blanket chmod with restrict_to_owner(), which walks the tree and
sets directories to 0700 and files to 0600, owned by the requesting user, and
add cleanup() to drop the tree once the archive has been written. Call
restrict_to_owner() from generate_user_report() too, since that runs as the
user and adds files after the root pass.
The fixed path under /tmp is kept: generate_report() runs in a separate
privileged process from the one that archives the result, so both sides need
to agree on the location. Tightening the directory modes closes the exposure
without changing that contract.
Member
|
I thinkg this is a false positive report from security perspective. There is no need to remove the /tmp report file and no need to change chmod of it because it is already copied on user's desktop with user owned permissions. The user can do whatever he wants with the file. |
This file contains hidden or 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
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.
generate_report() runs as root through pkexec and collects data the requesting
user cannot otherwise read: the full system journal (journalctl runs unfiltered
as root), dmidecode, blkid, netstat -neopa, the root environment and the system
logs under /var/log. It then finished with:
chmod 755 -Rapplies to the files as well as the directories, so everythingended up world readable under /tmp. On a test system the collected
journal_system was 2.5 MB and contained 3083 lines from root-owned units, while
the same user could not read any of it directly:
The tree was also never removed. The only rmtree call is at the start of the
next generate_report(), so the data stayed in /tmp until another report was
generated or the machine rebooted.
Replace the blanket chmod with restrict_to_owner(), which walks the tree and
sets directories to 0700 and files to 0600, owned by the requesting user, and
add cleanup() to drop the tree once the archive has been written. Call
restrict_to_owner() from generate_user_report() too, since that runs as the
user and adds files after the root pass.
The fixed path under /tmp is kept: generate_report() runs in a separate
privileged process from the one that archives the result, so both sides need
to agree on the location. Tightening the directory modes closes the exposure
without changing that contract.