Add `DEFAULT_PATH` to match de-facto Linux standards #119

TheDcoder · 2023-06-15T14:51:25Z

DEFAULT_PATH replaces safepath for setting the PATH variable in the executed process's environment.

DEFAULT_PATH follows the de-facto standard of Linux distributions which place /usr/local directories before their non-local counterparts in $PATH.

Unlike BSD, Linux distributions don't put packaged executables under /usr/local, instead it is used by the local user to place their own executables, potentially to replace system executables.

This fixes #117

Duncaen · 2023-06-15T15:55:16Z

I feel like the argument that BSD's install ports to /usr/local is more an argument for removing /usr/local from the default path for linux, not that it should have higher priority by default.

If this is changed then it should probably be a build time configuration option and not just a new order that is the de-facto standard for linux distributions.

TheDcoder · 2023-06-15T16:15:51Z

I agree that it's more of an argument for removing /usr/local, however since this has always been present in doas and most of OpenDoas's users are on Linux anyway, we should use this for simplicity and backward compatibility's sake.

I propose that we make safepath configurable at build time but let's set the default value to the one I proposed in this patch.

Duncaen · 2023-06-15T16:39:55Z

Looking over this again, I don't think safepath should be changed. The safepath variable is for running specifically permitted commands and I don't really want to change that behavior.

I think there should be a new variable like DEFAULTPATH that is used for the resulting environment.

IMHO doas.c:400 should use that default path, this is where opendoas differes from the BSDs where setusercontext sets the default path from /etc/login.conf, opendoas used safepath instead:

OpenDoas/doas.c

Lines 387 to 402 in b96106b

    
           #ifdef HAVE_LOGIN_CAP_H 
        
           	if (setusercontext(NULL, targpw, target, LOGIN_SETGROUP | 
        
           	    LOGIN_SETPATH | 
        
           	    LOGIN_SETPRIORITY | LOGIN_SETRESOURCES | LOGIN_SETUMASK | 
        
           	    LOGIN_SETUSER) != 0) 
        
           		errx(1, "failed to set user context for target"); 
        
           #else 
        
           	if (setresgid(targpw->pw_gid, targpw->pw_gid, targpw->pw_gid) != 0) 
        
           		err(1, "setresgid"); 
        
           	if (initgroups(targpw->pw_name, targpw->pw_gid) != 0) 
        
           		err(1, "initgroups"); 
        
           	if (setresuid(target, target, target) != 0) 
        
           		err(1, "setresuid"); 
        
           	if (setenv("PATH", safepath, 1) == -1) 
        
           		err(1, "failed to set PATH '%s'", safepath); 
        
           #endif

This way the behavior of looking up specific binaries doesn't change, but the resulting environment of executed commands reflects the more lax de-facto standard path and is more like the workaround with setenv { PATH=/usr/local/bin... }

TheDcoder · 2023-06-15T17:17:01Z

Ah, that makes more sense. In that case I think we're better off removing /usr/local from safepath entirely and implementing DEFAULTPATH. A way to make it user-configurable would be nice too but I don't think it can be easily added to doas.conf, or am I wrong?

Duncaen · 2023-06-15T17:52:10Z

I think changing safepath now would be too much of a regression for very little to no real benefit, the important part IMHO is the order so that we don't change what commands existing rules for specific commands may execute.

I don't think making safepath configurable is an option, I don't want to divert from the configuration format or the behavior of the original doas to avoid fragmentation.

The default path for rules that basically permit everything this is different since this is also configurable through login.conf in the original doas and opendoas diverted here and used the restrictive safepath instead. Users who want a different binary than the ones that comes from the safepath lookup can use permit /some/random/path/bin if required.

TheDcoder · 2023-06-15T18:06:16Z

Understandable, I wasn't really suggesting making safepath configurable, but I do want the default path to be user configurable so that they won't have to use the env option for each rule.

I also agree that deviating from the configuration format is a bad idea... however maybe we can add something which still conforms to the existing format spec? Or maybe we can add an additional config file which is optional but it can be used to change the default path.

The latter seems overkill to me since we don't really need it for anything else.

For now though let's just make default path configurable at build time, that should satisfy everyone.

Duncaen · 2023-06-15T18:09:50Z

The default path is already configurable through setenv or am I misunderstanding this?

TheDcoder · 2023-06-15T18:15:51Z

Maybe I misunderstood what you meant by this:

I think there should be a new variable like DEFAULTPATH that is used for the resulting environment.

I thought we were going to implement a new DEFAULTPATH which will be used for the resulting environment for the program which will be executed once authentication is successful. safepath would still be present but it will only be used for searching for the executable if the full path is not specified in config.

Duncaen · 2023-06-15T18:32:49Z

I thought we were going to implement a new DEFAULTPATH which will be used for the resulting environment for the program which will be executed once authentication is successful.

Correct, and with will still be overwritable with setenv { PATH="something else" }.

safepath would still be present but it will only be used for searching for the executable if the full path is not specified in config.

Correct safepath is used when there is a rule for a specific command like permit as root cmd poweroff.
If a user wants to allow poweroff from rules/users can define specific binaries if safepath is too restrictive for them.
The PATH variable for those commands will be the safepath by default, but setenv can still overwrite it.
The search path/safepath is not and IMHO should not be configurable to have at least a somewhat predictable behavior.

rules like permit :wheel don't use safepath at all, those basically grant everything, they use the executing user PATH, but will set the PATH to DEFAULTPATH (currently safepath) or whatever is defined in the configuration as permit :wheel setenv { PATH=... }.

TheDcoder · 2023-06-15T18:42:29Z

I'm sorry for the confusion, I'll outline what I'm proposing:

safepath should not be configurable at all
DEFAULTPATH should get assigned a new value at build time, this value will put /usr/local paths before their counterparts.
DEFAULTPATH should be configurable at build time so that it can be adjusted by distributors for their specific needs
DEFAULTPATH is the default value for setenv { PATH = ...}
(optional) In the future, ideally we will figure out a way to set DEFAULTPATH with user configuration so that they don't have to use setenv for each rule.

I hope I'm clear now 😄

Duncaen · 2023-06-15T19:05:01Z

Agreed.

doas.c

`DEFAULT_PATH` replaces `safepath` for setting the `PATH` variable in the executed process's environment. `DEFAULT_PATH` follows the de-facto standard of Linux distributions which place `/usr/local` directories before their non-local counterparts in $PATH. Unlike BSD, Linux distributions don't put packaged executables under `/usr/local`, instead it is used by the local user to place their own executables, potentially to replace system executables.

Wabuo · 2024-04-28T12:07:05Z

Just checking in.

Is this project still alive?
Just stumbled on this issue following a link from the ArchWiki ...

TheDcoder · 2024-04-28T19:54:39Z

@Wabuo I don't think so. I've started writing an alternative doas implementation in Rust, the basics are working but I've to implement PAM support so it might be a good replacement once it is released.

Wabuo · 2024-05-02T07:12:03Z

Cheers, so I might as well uninstall it again ...

@TheDcoder
Are you aware of https://github.com/LeChatP/RootAsRole, and https://github.com/memorysafety/sudo-rs the sudo rewrite in rust?
And this Reddit https://www.reddit.com/r/rust/comments/165ack9/rootasrole_a_secure_alternative_to_sudosu_on/

TheDcoder · 2024-05-04T15:17:12Z

@Wabuo I was aware of the sudo rewrite project but it does not interest me as I don't like sudo. I wasn't aware of RootAsRole, it is an interesting project taking a novel approach, thanks for sharing.

I like doas because it's pretty simple and fits my needs, no need for complex configuration... and you probably should reconsider if you really want to be able to do something as root while logged in as a user if it requires complex configuration which can't be provided by simple instructions in doas.con.

TheDcoder force-pushed the patch-1 branch from 69b5347 to f57ec7e Compare June 15, 2023 21:18

TheDcoder changed the title ~~Fix safepath to match de-facto Linux standards~~ Add DEFAULT_PATH to match de-facto Linux standards Jun 15, 2023

Duncaen reviewed Jun 15, 2023

View reviewed changes

doas.c Outdated Show resolved Hide resolved

TheDcoder force-pushed the patch-1 branch from f57ec7e to 51ad1b7 Compare June 15, 2023 21:28

TheDcoder requested a review from Duncaen June 15, 2023 21:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `DEFAULT_PATH` to match de-facto Linux standards #119

Add `DEFAULT_PATH` to match de-facto Linux standards #119

TheDcoder commented Jun 15, 2023 •

edited

Loading

Duncaen commented Jun 15, 2023

TheDcoder commented Jun 15, 2023

Duncaen commented Jun 15, 2023

TheDcoder commented Jun 15, 2023

Duncaen commented Jun 15, 2023 •

edited

Loading

TheDcoder commented Jun 15, 2023 •

edited

Loading

Duncaen commented Jun 15, 2023

TheDcoder commented Jun 15, 2023

Duncaen commented Jun 15, 2023

TheDcoder commented Jun 15, 2023 •

edited

Loading

Duncaen commented Jun 15, 2023

Wabuo commented Apr 28, 2024

TheDcoder commented Apr 28, 2024

Wabuo commented May 2, 2024 •

edited

Loading

TheDcoder commented May 4, 2024

Add DEFAULT_PATH to match de-facto Linux standards #119

Are you sure you want to change the base?

Add DEFAULT_PATH to match de-facto Linux standards #119

Conversation

TheDcoder commented Jun 15, 2023 • edited Loading

Duncaen commented Jun 15, 2023

TheDcoder commented Jun 15, 2023

Duncaen commented Jun 15, 2023

TheDcoder commented Jun 15, 2023

Duncaen commented Jun 15, 2023 • edited Loading

TheDcoder commented Jun 15, 2023 • edited Loading

Duncaen commented Jun 15, 2023

TheDcoder commented Jun 15, 2023

Duncaen commented Jun 15, 2023

TheDcoder commented Jun 15, 2023 • edited Loading

Duncaen commented Jun 15, 2023

Wabuo commented Apr 28, 2024

TheDcoder commented Apr 28, 2024

Wabuo commented May 2, 2024 • edited Loading

TheDcoder commented May 4, 2024

Add `DEFAULT_PATH` to match de-facto Linux standards #119

Add `DEFAULT_PATH` to match de-facto Linux standards #119

TheDcoder commented Jun 15, 2023 •

edited

Loading

Duncaen commented Jun 15, 2023 •

edited

Loading

TheDcoder commented Jun 15, 2023 •

edited

Loading

TheDcoder commented Jun 15, 2023 •

edited

Loading

Wabuo commented May 2, 2024 •

edited

Loading