Add default locations to PATH to assure that all basic commands are available

[MichaIng]

By submitting this pull request, I confirm the following:
please fill any appropriate checkboxes, e.g: [X]

• I have read and understood the contributors guide, as well as this entire template.
• I have made only one major change in my proposed changes.
• I have commented my proposed changes within the code.
• I have tested my proposed changes, and have included unit tests where possible.
• I am willing to help maintain this change if there are issues with it later.
• I give this submission freely and claim no ownership.
• It is compatible with the EUPL 1.2 license
• I have squashed any insignificant commits. (git rebase)

---

What does this PR aim to accomplish?:
In rare cases, e.g. when the installer is called via su, an incomplete/unprivileged PATH could be passed which can make basic commands unavailable, e.g. usermod in this example: #3209
To avoid larger coding effort to check (and in case search) for each and every basic external (i.e. not bash internal) command before using it, even that it is available in a usual executable location, it is reasonable to make all default locations available via PATH variable.

How does this PR accomplish the above?:
Add usual default locations to PATH, which matches by entries (not necessarily order) the bash default which is set if no PATH is set, e.g. when using sudo.

What documentation changes (if any) are needed to support this PR?:
None

[DL6ER]

Do we want this for Pi-hole v5.2 ?

[dschaper]

I really, really, really don't like modifying PATH, and I think this is not the right approach. But if you can show me the docs that says the += works with all the versions of bash that we support then I'll remove the blocker.

[MichaIng]

docs that says the += works with all the versions of bash

This is old bash syntax, search the man page for +=: https://linux.die.net/man/1/bash
Here another hint that it works with export as well (and local, declare etc, for completeness): https://stackoverflow.com/a/4182643

I really, really, really don't like modifying PATH

Basically I agree, since users should know about the PATH whenever they run into "command not found" errors. But in fact there are users which open support requests with this (not many though). Before starting to code a check/handler around each and every executed command (as suggested in #3209), which still does not solve the issue for users but might just add more information, this is a much easier and solving approach.
The important question is if appending additional locations to PATH can break anything. It just adds all default paths that you expect to be valid on all UNIX systems according to FHS. Since they are appended as tail, custom paths will be chosen with higher priority, if a command is found there. Since empty, doubled and nonexistent entries (:-separated) do not cause any issues, also from this side I don't see any harm. From the security perspective: Since the script needs to be run as root, (natively or via sudo) or otherwise it upgrades itself via sudo, it has full permissions anyway, and of course adding something to PATH does not circumvent file permissions.

Actually I'd even consider overwriting the PATH to remove any custom binary locations which might contain degraded or unwanted (anyway required) binaries, although this might indeed break some strange custom setups. In fact running the script with sudo starts with the clean (bash-internal) PATH since no profile/bashrc script is sourced on such noninteractive (no -s or -i) calls and sudo by default (no -E) does not pass environment variables (like su does). Not sure what would be required to overwrite the internal default PATH that you get when using sudo, probably a self-compiled bash. I just tested that even /etc/login.defs is not respected with plain sudo bash <script>. But yeah, appending does not break any such custom setup, so is probably the safest to solve the originating issue.

[dschaper]

Okay, did some digging and it looks like it goes back to bash versions 3 (or 3.1, but for all intents and purposes it's 3.)

Safe to use.

[MichaIng]

Nice find, also that appending something to PATH is explicitly mentioned as example.

[dschaper]

Considering the information provided by the package maintainers, I'm second-guessing this merge and considering reverting it.

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=918754#84

[dschaper]

Hello,

As already mentioned in this bug report the relevant change is
documented in NEWS.Debian.

While I no longer maintain util-linux I'm directly responsible for
making the change. This change is intentional and aligns su with its
documented behaviour. I would also like to stress that using plain 'su'
is DANGEROUS because it means you're running a shell as root with the
environment inherited from another user.
I've over the years seen countless bug reports against different
components claiming $SOMETHING completely broke their system beyond all
repair, while in fact the problem was that they where running a root
shell with environment inherited from their main user account.
(Only resetting the PATH does not make it any more safe.)

You should thus ALWAYS use 'su -' (or even better, don't use su at all
in favour of other alternatives like 'sudo -i').

The use of plain 'su' should be deprecated, but this might be hard or
atleast require alot of work since there might be alot of legacy stuff
using it. (Many should likely instead be switched to use either
runuser or possibly setpriv.)

I've already tagged this bug report 'wontfix' and hope the above
explanation is clear enough as to why I'm closing this bug report.

Regards,
Andreas Henriksson

[MichaIng]

Good that someone brings this issues with su to the point. If we are lucky, Debian will switch its default from su to sudo by times, like Ubuntu did, there are plenty of strong arguments for this.

However, while this is an argument against su, it is not one against keeping this commit as a failsafe step if one broke the PATH for any other reason. It is finally easier to debug "missing command" errors if your can rule out a broken PATH in the first place.

But if you want to teach users about su (still the only reason that was found so far for broken PATH) I'll not argue against it 🙂.

[dschaper]

if one broke the PATH for any other reason. It is finally easier to debug "missing command" errors if your can rule out a broken PATH in the first place.

That's a fair point.

[pralor-bot]

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/for-debian-10-4-and-greater-chech-if-useradd-command-exists/40626/14

[pralor-bot]

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-core-web-v5-2-and-ftl-v5-3-released/40909/1