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

Installing piranha to standard paths breaks the system #16

Open
NexAdn opened this issue Aug 5, 2021 · 1 comment
Open

Installing piranha to standard paths breaks the system #16

NexAdn opened this issue Aug 5, 2021 · 1 comment
Assignees
Labels

Comments

@NexAdn
Copy link

NexAdn commented Aug 5, 2021

Well... I was just trying to check out this software and I tried to do it right and package it for my operating system.

I've changed the prefix to /usr as usual (and also patched piranha to use more standard paths like /var/lib/piranha, /usr/share/man, /var/log, etc., and respect DESTDIR in the makefile to support installing into a temporary install directory for packaging, but that isn't relevant in this case) and merged the package into my system. Piranha failed to start with some error message (something with socket, I don't have the respective terminal open anymore, but it isn't relevant to this case either).

The command to start piranha was sudo /usr/bin/piranhactl start (as I said, I installed to standard system paths).

After that I had to notice that sudo didn't work anymore and I had to manually fix it by running:

$ su -
# chown root:root /usr/bin/sudo
# chmod 4755 /usr/bin/sudo

Then I had a look at the source code and noticed something very dangerous:

mychown(PATH, config.uid, config.gid, 0);

This causes piranha to chown() the whole /usr directory as during configuration --prefix=/usr was set, causing -DPATH=/usr to be set as compiler parameter and thus causing this line to chown /usr.

This is very dangerous.

--prefix is a standard configure option which is used for build systems to differentiate between a local installation (where not package manager is used) to /usr/local and a package manager installation to /usr. To abuse this option in a different way is not only confusing (at least my system broke because of that), but dangerous. You chowned the whole /usr tree to nobody. I was lucky to have a root password and that /bin/su wasn't affected by this. Otherwise my system would have been become more or less unusable without breaking into it using a live ISO.

--prefix is meant to point to an installation prefix for files which do not change and the only software that applies any changes in the prefix directory should be the package manager, not a service executable. You should use a different location inside /var (like /var/lib/piranha – the home directory of the system user piranha) for variable files.

If you don't intend to make piranha installable system-wide, please deprecate and remove the --prefix option so other people don't get confused again.

My personal suggestion is retrieving the various paths via command line parameters ant configure time with sensible defaults, like /usr/local als prefix, /var/lib/piranha as variable files directory, /var/log/ as logging directory, etc. and moving any necessary chown calls from runtime to the install phase where in most packaging systems all calls happen in a sandboxed or otherwise secured environment without causing harm to the system. That way, package managers can also detect collisions with the installed directories' owner/group/umask settings and report a possible misconfiguration.

Please forgive me that I could write only bad things in this issue, but this is a very dangerous matter and because of this I am currently not very motivated to do any further testing of your software as I don't want to risk further problems with my system. The current state of this software makes it too dangerous for me to use it.

@spale75 spale75 self-assigned this Aug 5, 2021
@spale75 spale75 added the bug label Aug 5, 2021
@spale75
Copy link
Owner

spale75 commented Aug 5, 2021

Hello @NexAdn,

Thanks for the extensive report and all the efforts you put in it. You are absolutely right and I'll fix it for the next release 1.1.5.

EDIT: Sorry for the annoyance it caused :(

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

No branches or pull requests

2 participants