Too many PATH changes ?

[kreijack]

Hi,

I tried to understand how the PATH is changed by opendoas. What I found is that opendoas update the path several times; the place that I found were:

1. OpenDoas/env.c

   Line 207 in 9474e41

   if (strcmp(val + 1, "PATH") == 0)

2. OpenDoas/env.c

   Line 213 in 9474e41

   if (strcmp(name, "PATH") == 0)

3. OpenDoas/doas.c

   Line 372 in 9474e41

   if (setenv("PATH", safepath, 1) == -1)

and

4. OpenDoas/doas.c

   Line 419 in 9474e41

   if (setenv("PATH", safepath, 1) == -1)

Where only the last one should matter. My understanding is that 3) become useless, because the following lines were removed in the porting from openbsd doas:

426:       if (unveilcommands(getenv("PATH"), cmd) == 0)
427:                   goto fail;

(from https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/doas/doas.c?annotate=1.89)

Instead my understanding about 1) and 2) is that there was a merging of two unrelated changes in the original "openbsd doas".

What about simplifying the code removing the unneeded PATH setting ? Then I would like to discuss how insert the /sbin and /usr/sbin paths in the PATH before invoking the command; currently if I as user don't have /usr/sbin in my path, I have to explicit it in the doas invoking if I want to call a command which is under /sbin:/usr/sbin:

$ doas /usr/sbin/reboot

instead of

$ doas reboot

But this is another story...

[Duncaen]

There is just one place where its redundant in case the rule allows only a specific comment:
https://github.com/Duncaen/OpenDoas/blob/master/doas.c#L372
Which is always set to safepath a few lines later anyways.
https://github.com/Duncaen/OpenDoas/blob/master/doas.c#L400

But for sake of being closer to the upstream sources and simplifying patches/diffs, I don't really think there is a reason to remove the first one.
Edit: I'm going to remove it in the future or bring back the code around it and ifdef it out, not sure yet. The argument that its closer to upstream doesn't really matter because there is already a big change around the lines.

The other two places 1. and 2. are for different features, setenv option i.e. setenv { PATH } and setenv { PATH=$PATH },
both cases are treated specially, they would include the "formerpath" the path of the executing user as opposed to the "safepath" that is used by default.

Then I would like to discuss how insert the /sbin and /usr/sbin paths in the PATH before invoking the command; currently if I as user don't have /usr/sbin in my path, I have to explicit it in the doas invoking if I want to call a command which is under /sbin:/usr/sbin:

But why should doas path lookup be different from path lookup in your shell?

• If your rule allows the user to execute any command then your PATH is used (permit user).
• If your rule allows to execute only a specific command, then the default ("safepath") PATH is used (permit user cmd reboot).

[kreijack]

The other two places 1. and 2. are for different features, setenv option i.e. setenv { PATH } and setenv { PATH=$PATH },
both cases are treated specially, they would include the "formerpath" the path of the executing user as opposed to the
"safepath" that is used by default.

Ok, I confused the two the environments:

• the one set set by prepenv() is the one which is saw by the child process spawned by execvpe()
• the current environment (the one set by setenv()) is the one used by execvpe() to find the correct executable
  So these two environments are separate things.

But why should doas path lookup be different from path lookup in your shell?

• If your rule allows the user to execute any command then your PATH is used (permit user).

I am looking to the "permit user" use case. Sometime I want (e.g.) to run tune2fs which is under /usr/sbin (but it could be any other command under /usr/sbin). So I would like to do

$ doas tune2fs ....

Instead of

$ doas /usr/sbin/tune2fs

Currently doas doesn't allow to extend/change/replace that path.

[Duncaen]

Currently doas doesn't allow to extend/change/replace that path.

I don't know if I understand correctly, do you want that doas prepends some directories to the executing users PATH instead of changing the PATH variable in the users shell?

[kreijack]

I don't know if I understand correctly, do you want that doas prepends some directories to the executing users PATH instead of
changing the PATH variable in the users shell?

doas allows to change the environment of the child. However the child executable is till searched only in the current PATH.

$ ls -l /usr/sbin/tune2fs 
-rwxr-xr-x 1 root root 108960 Jan 29 06:19 /usr/sbin/tune2fs
$ echo $PATH
/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games
ghigo@venice:~$ doas tune2fs
doas: tune2fs: command not found
ghigo@venice:~$ PATH=/usr/sbin:$PATH doas tune2fs
tune2fs 1.45.7 (28-Jan-2021)
Usage: tune2fs [-c max_mounts_count] [-e errors_behavior] [-f] [-g group]
[...]
$ doas env | grep PATH
PATH=/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin:/usr/local/sbin

In the example abowe, the environment variable PATH of the child contains /usr/sbin. However doas doesn't search the excutable in /usr/sbin but only in the current PATH

[Duncaen]

Ok yes that is the same as in the upstream sources, I don't think I really want to divert from that.

My only idea would be to add an option to either enable or disable to use the PATH from the target environment.

Generally I do think it would be better to use the PATH from the targets environment and add a option to enable the current behavior of using formerpath, but this is something that I would like to see happening in upstream to avoid adding new configuration keywords that are implementation specific.

[eugenesvk]

This is a very confusing behavior, I was trying to understand why the setenv {PATH} didn't work in permit nopass setenv {PATH} :staff cmd port despite the same cmd env showing PATH that includes the port executable (on a Mac)

Can this at least be documented that you can't override this "safepath" for specific commands?
Although being able to set custom paths for a specific command would be even better even if that deviates from upstream as maybe they don't have the same use cases