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

Introduce a way to set file properties recursively (e.g. SetFileProperty --recursive) #26

Open
sakhnik opened this issue Jun 16, 2018 · 14 comments

Comments

@sakhnik
Copy link

sakhnik commented Jun 16, 2018

The directory /boot is often formatted vfat. Thus, lots of files are detected having wrong permissions with a recommendation: SetFileProperty /boot/cmdline.txt mode 755
It would be nice to be able to mark the whole mount point /boot to disregard file permissions at all. Consider IgnorePermissionsPath '/boot/*'

@CyberShadow
Copy link
Owner

What's /boot/cmdline.txt, is that a custom file or owned by some package? If the former, you can just add the permissions to the end of the CreateFile helper invocation.

I'm also using EFI boot with the ESP mounted on /boot, and it's not causing issues for me.

@sakhnik
Copy link
Author

sakhnik commented Jun 16, 2018

It's rather owned by linux-raspberrypi in my case. Another one is /boot/config.txt, for example.

@alecmev
Copy link

alecmev commented Sep 29, 2018

I think an IgnorePermissionsPath (or at least SetFileProperty respecting IgnorePath) is still a useful helper to have. For example, I have this blob of negligence I'd like to get rid of:

But, unfortunately, can't.

Edit

It does respect IgnorePath, and, in my case, that's actually enough to get rid of the /usr/lib/node_modules nonsense, but I've got some more files/directories which I'd like to keep, but not change any properties of:

SetFileProperty /var/lib/bluetooth mode 700
SetFileProperty /var/lib/colord group colord
SetFileProperty /var/lib/colord owner colord
SetFileProperty /var/lib/libvirt/qemu/channel group kvm
SetFileProperty /var/lib/libvirt/qemu/channel owner nobody
SetFileProperty /var/lib/libvirt/qemu/ram group kvm
SetFileProperty /var/lib/libvirt/qemu/ram owner nobody
SetFileProperty /var/lib/libvirt/qemu group kvm
SetFileProperty /var/lib/libvirt/qemu owner nobody
SetFileProperty /var/lib/unifi/data group unifi
SetFileProperty /var/lib/unifi/data owner unifi
SetFileProperty /var/lib/unifi/run group unifi
SetFileProperty /var/lib/unifi/run owner unifi
SetFileProperty /var/lib/unifi/work group unifi
SetFileProperty /var/lib/unifi/work owner unifi
SetFileProperty /var/log/journal group systemd-journal
SetFileProperty /var/log/unifi group unifi
SetFileProperty /var/log/unifi owner unifi

All are created when installing or using corresponding software, and I trust them to be able to set their own permissions correctly, but would still like to keep some configs located deeper in some of these.

@CyberShadow
Copy link
Owner

I think an IgnorePermissionsPath (or at least SetFileProperty respecting IgnorePath) is still a useful helper to have.

I don't disagree, but

For example, I have this blob of negligence I'd like to get rid of:

Unless that's on a filesystem that doesn't support UNIX permissions (like FAT32), that looks like a problem better solved elsewhere. The permission flags 777 are potentially dangerous, as they allow any process running under any user to modify those files, and change the behavior of software invoking those node modules, potentially causing a privilege elevation.

It does respect IgnorePath, and, in my case, that's actually enough to get rid of the /usr/lib/node_modules nonsense, but I've got some more files/directories which I'd like to keep, but not change any properties of:

Those look right. Any idea why they deviate from the packaged directory properties on your system, and any reason not to delete those lines from your configuration and allow aconfmgr apply to restore the default properties?

One reason I've encountered (rarely) is that two packages sometimes include the same directory path, but with different permissions. In this case, ignoring it (or keeping the latter's value in your configuration) is necessary. One might be inclined to say that it is a bug in one of the packages, as the resulting filesystem will then depend on the order in which they are installed.

@alecmev
Copy link

alecmev commented Oct 2, 2018

Thanks for the reply!

that looks like a problem better solved elsewhere

This is out of my hands, unfortunately: npm/npm#9359

However, I went ahead and reinstalled node-nativefier, and looks like this time around all permissions are inline with what Pacman is expecting, so that's something (it's still 777 all over the place though). I think they somehow went out of sync when the AUR package got updated.

Any idea why they deviate from the packaged directory properties on your system

Looked into /var/log/journal. Don't know what's wrong, at the time of the installation it was root:systemd-journal, and it still is. There's even a comment, pointing out that this line is there to make sure Pacman gets the permissions right.

any reason not to delete those lines from your configuration and allow aconfmgr apply to restore the default properties

I'm going to do that. I'm using aconfmgr to prepare for a reinstall (encrypting my drive), so, hopefully, all these will iron themselves out in the end. But still, nice to understand what's going on.

Another SetFileProperty I got on a package I have installed today, for the first time ever:

SetFileProperty /var/games/vitetris group games
SetFileProperty /var/games/vitetris mode 775

AUR link.

Though I feel like this is deviating a bit from the main topic. Where can we discuss this, if you don't mind? Doesn't feel like worth filing a new issue (yet).

@CyberShadow
Copy link
Owner

This is out of my hands, unfortunately: npm/npm#9359

Something's not right.

  • If the files were installed by pacman, then npm shouldn't have a reason to touch them.
  • If the files were installed by npm, then they shouldn't have been in pacman's database.

It sounds like in this case, the files were initially installed by pacman, then upgraded by npm?

In any case, running programming languages' package managers as root is often not recommended for this very reason: they conflict with the distribution's package manager. If the program is needed by the root user or multiple system users, it should be packaged appropriately as a distro package; for single users, they're better off installed under the user's home directory (see e.g. here for npm).

it's still 777 all over the place though

I think it's worth reporting this to packages which wrongly create globally writable objects on the filesystem.

There's even a comment, pointing out that this line is there to make sure Pacman gets the permissions right.

I agree, sounds strange. Might be worth looking at the paccheck output for that, and perhaps check pacman -Qo /var/log/journal.

Another SetFileProperty I got on a package I have installed today, for the first time ever:

That looks like it's created by tmpfiles. Maybe at some point aconfmgr will learn to read systemd's tmpfiles (as a plugin or such), but for now they have to be repeated in the configuration.

Though I feel like this is deviating a bit from the main topic. Where can we discuss this, if you don't mind?

I haven't thought about setting up / adopting a medium for open-ended discussion yet. I suppose we could use Reddit or the Arch Linux forums, or create an IRC channel.

@alecmev
Copy link

alecmev commented Oct 4, 2018

@alecmev
Copy link

alecmev commented Oct 4, 2018

As for an IgnorePermissionsPath command, I think we need to determine some well-defined, practical use cases where we know that such a helper is definitely the best solution. The issue's original comment described one such situation (FAT32 filesystems), however "ignoring" permissions doesn't entirely fit the problem: it would be slightly more correct to say that we expect that the permissions on the FAT32 drive to be 777. In that case, we can achieve the goal with a helper which modifies the expected system state programmatically, without modifying aconfmgr's comparison logic.

Can we extract similar well-defined use cases from the problems you've encountered?

Your suggestion sounds good to me! Assertion is definitely better that just outright ignoring (AKA sweeping under the rug). Say, ExpectFileProperty, with the same signature as SetFileProperty? If in the future it stops being true, during an apply, then tell the user to comment out the affected expectations, and see what gets put into 99-unsorted when they run save.

@CyberShadow
Copy link
Owner

That doesn't sound necessary to me; the entire configuration is already, in a way, asserting that the system has a certain state. You know that the assertion no longer holds when aconfmgr save emits directives that override previously defined configuration, or when aconfmgr apply suggests modifications to the system. So, SetFileProperty sounds good already; the only problem remaining is to make it apply recursively to a directory on the machine's filesystem.

@alecmev
Copy link

alecmev commented Oct 4, 2018

Ah, right, the "There are changes, proceed?" applies to properties too. Sorry, haven't run an apply yet, still slowly refining the output of save. My use cases are fully satisfied then, it appears. Thanks.

@CyberShadow CyberShadow changed the title Introduce a command to ignore permissions on FAT Introduce a way to set file properties recursively (e.g. SetFileProperty --recursive) Nov 22, 2021
@PkmX
Copy link

PkmX commented Aug 31, 2023

So, what's the current take on this?

systemd-boot now stores a random seed on /boot/loader/random-seed, which should be non-world-readable. Since /boot is usually on vfat, the only way to achieve this is by mounting the filesystem with fmask=0077, which changes the permission of every file under /boot. This causes aconfmgr to think that every file under /boot has different permission than the one from the official packages.

@CyberShadow
Copy link
Owner

Fun. Is it the systemd maintainers' consensus that ESP /boot should now be mounted with fmask=0077?

@PkmX
Copy link

PkmX commented Aug 31, 2023

bootctl install or bootctl random-seed prints the following warnings:

⚠️ Mount point '/boot' which backs the random seed file is world accessible, which is a security hole! ⚠️
⚠️ Random seed file '/boot/loader/random-seed' is world accessible, which is a security hole! ⚠️

With ESP typically being formatted as vfat, it basically requires a fmask that masks out the other-readable permission for every file on the filesystem.

I also tried to hack some solutions like mount -o bind,X-mount.mode=0770 /boot/loader/random-seed /boot/load/random-seed just to modify the permission of this partiular file, but it doesn't seem to work if the base filesystem is vfat.

@CyberShadow
Copy link
Owner

That is a pickle.

It might be worth asking the Arch folks if they were thinking about making files in packages (that have files under /boot) to change their mode to non-world-readable to conform to systemd's expectations. After all, this also affects e.g. pacman -Qkk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants