trifectatechfoundation / sudo-rs

A memory safe implementation of sudo and su.
Other
2.9k stars 79 forks source link

visudo does not resolve editor path #724

Closed oneElectron closed 1 year ago

oneElectron commented 1 year ago

Currently visudo does not resolve a path through the PATH variable.

Example

visudo will not find nvim because it does not look for nvim in the PATH. The correct usage would be this:

pvdrz commented 1 year ago

mmm yeah that's an issue.

One option would be to call which. I don't love this option because it means spawning a process just to resolve a path.

The other option would be to manually parse $PATH and check for the editor path relative to each one of the paths in $PATH. Not fun either.

oneElectron commented 1 year ago

Ogsudo reads the path variable. https://github.com/sudo-project/sudo/blob/bdde6dfa112f17d47bf5f4f8991023ece884780c/plugins/sudoers/editor.c#L127

If this is ok then I would like to work on this issue

pvdrz commented 1 year ago

sure why not?

squell commented 1 year ago

There is code for resolving the path in resolve.rs which probably can be re-used by visudo I would say; take note that for editors probably the same logic that parses PATH a little bit differently (i.e. ignoring . if it is in PATH) probably also applies here, but check that.

squell commented 1 year ago

https://github.com/memorysafety/sudo-rs/blob/be4876fcfcc089931c1746089810b046c71e3ed2/src/common/resolve.rs#L132

oneElectron commented 1 year ago

Sorry i may have found a slight security bug in the resolve_path function. I don't know where and in which contexts you use the function so let me know if I need to report this by email.

squell commented 1 year ago

Great (well, great that you vetted the code, not that you think something might be wrong with it)! Even though we have not made a stable release yet and therefore we could just discus it here, this may be a great opportunity for us to do a trial-run of our security policy and vulnerability reporting system. So if you want to assist in that, please follow the steps in our security policy.

oneElectron commented 1 year ago

It's not too serious (at least not enough that ogsudo did anything about it). When the function runs it does a check to see if "." is part of the PATH. If the PATH contains "./" resolve_path does not recognize it as "." and treats it as any other path.

EDIT: link to ogsudo with comment about the issue: https://github.com/sudo-project/sudo/blob/d404f544fc673959adf2049e105fa790f011445c/plugins/sudoers/find_path.c#L122

squell commented 1 year ago

Funny you should point this one out, because (when working on that code) we also discussed this internally, that is whether to add support for every relative path (such as someone that would have bin or .. in there), and we consulted with Todd about that (since it would be a minor break with ogsudo behaviour).

The reason for the special treatment of . seems more to prevent accidents when users have . in their PATH (which used to be more common). I.e. accident prevention, not as a "security feature". In particular, since the PATH is under control of the secure_path variable, it's kind of like the administrator is asking for this behaviour if they explicitly put ./ there.

oneElectron commented 1 year ago

Ok. Then I see no issue with using this to resolve the editor path. If you want I can make a pr, but it seems like the fix will be 3 lines of code and not worth a pr.

squell commented 1 year ago

Note: we've had PR's consisting of less than three lines, so everything is worth a PR :-)

squell commented 1 year ago

I've assigned you to this issue.