trifectatechfoundation / sudo-rs

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

Be pedantic about disrecommended practices #53

Open squell opened 1 year ago

squell commented 1 year ago

There are some well-known anti-patterns in sudo, that the man page warns about; for instance using the negation operator with commands in rules like:

user machine = (ALL:ALL) ALL,!/bin/ls

We can detect those after parsing, during the semantical analysis (where also already complain about alias definitions that appear to be cyclical, etc), and emit a diagnostic about them (while still supporting said behaviour)

This has some subtasks:

bjorn3 commented 1 month ago

When should the warning happen? Only with sudoedit? Before authentication? After authentication for any user? After authentication for just root? Doing it before authentication or after authentication for any user would leak part of /etc/sudoers to the user even if they can't read /etc/sudoers.

pvdrz commented 1 month ago

I'd do it just after the sudoers file is parsed. In that way, both sudo and visudo can throw a warning.

squell commented 1 month ago

Currently sudo-rs (and sudo) already give parse warnings before authenticating (since you don't know if you need to authenticate before having parsed the sudoers file). We've discussed in the past whether we should restrict those warnings to root, but generally it appears the current behaviour of sudo is acceptable.

The thinking here is also as follows: /etc/sudoers is basically a "trusted" file. A sysadmin gets warned about any errors by visudo and it's their responsibility that the file is correct. And users can also glean info by using sudo --list.

I.e., Christian is correct, it is basically an extra analysis phase right after parsing.