tcb_(un)convert: Check for UID and EUID to be 0 before proceeding.

[besser82]

Ensuring the program is run with root privileges on startup is the safer approach instead on relying that mode 0700 may prevent the tool being run by a regular system user.

[solardiz]

Why do this instead of keeping mode 700?

Also, I'd write "proceeding" in place of "performing".

[besser82]

Why do this instead of keeping mode 700?

Technically I think this the safer approach, than relying on filesystem modes.

Also, I'd write "proceeding" in place of "performing".

Fixed.

[solardiz]

I think the code should be checking euid, not uid.

Technically I think this the safer approach, than relying on filesystem modes.

This depends on context. Each distro relies on filesystem modes for a lot of things anyway. So within a distro we control, relying on the modes may actually be safer.

Also with the 700 file modes these programs are normally excluded from the shell's tab completion when working as a user. I think this is good.

Overall, I'm not convinced this PR is an improvement, but I suppose it could fit some distros' packaging guidelines better, and is also defense against sloppy packaging. So I don't mind.

As an option, we could add the code checks to support different distros' packaging conventions, but keep our mode 700 defaults (which packages may override).

[besser82]

I think the code should be checking euid, not uid.

Well, euid == 0 would also be true, if the file has SETUID capabilities and is executed, which could be dangerous; uid == 0 is only true, if the file is executed from a root login or run through sudo or su.

Also with the 700 file modes these programs are normally excluded from the shell's tab completion when working as a user. I think this is good.

Reverted.

[solardiz]

Well, euid == 0 would also be true, if the file has SETUID capabilities and is executed, which could be dangerous;

Do we seriously want to defend against such misconfiguration? Most programs in the system would be dangerous if installed SUID or the like.

uid == 0 is only true, if the file is executed from a root login or run through sudo or su.

Yet it is technically possible to have uid == 0 && euid != 0, in which case with your current patch the programs will proceed to run, but will fail. So to me it makes most sense to check for the privileges they actually need to do their job, for which euid == 0 is a closer match. OTOH, indeed it is also technically possible to have euid == 0 yet no capabilities to chown files, which these programs need, so would we also be checking for this capability? This could get ridiculous.

So I think it is reasonable that we either don't make these changes at all (don't merge this PR) or require euid == 0 or require uid == 0 && euid == 0.

We could also check what precedent there is - how do other sysadmin tools that require to be run by root handle this?

[besser82]

So I think it is reasonable that we either don't make these changes at all (don't merge this PR) or require euid == 0 or require uid == 0 && euid == 0.

Fixed.

We could also check what precedent there is - how do other sysadmin tools that require to be run by root handle this?

Some tools do it the same way, others check setuid(0) == 0 before performing; there are even tools that perform SELinux checks on permissions and allowance for certain type transitions.

[ldv-alt]

I don't think this extra check would break anything.