Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Be pedantic about disrecommended practices #53

Open
2 tasks
squell opened this issue Mar 1, 2023 · 3 comments
Open
2 tasks

Be pedantic about disrecommended practices #53

squell opened this issue Mar 1, 2023 · 3 comments
Labels
C-checker Permission checking logic enhancement New feature or request
Milestone

Comments

@squell
Copy link
Member

squell commented Mar 1, 2023

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:

  • Inventorise all the disrecommended sudoer-practices (either from the manpage or the wider internet)
  • Implement them during the analysis phase
@squell squell added this to the Milestone 2 milestone Mar 7, 2023
@squell squell modified the milestones: Milestone 2, Milestone 3 Jun 26, 2023
@squell squell modified the milestones: Milestone 3, Milestone 4 Sep 21, 2023
@squell squell added enhancement New feature or request C-checker Permission checking logic labels Aug 26, 2024
@bjorn3
Copy link
Collaborator

bjorn3 commented Oct 14, 2024

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
Copy link
Collaborator

pvdrz commented Oct 17, 2024

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

@squell
Copy link
Member Author

squell commented Oct 22, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-checker Permission checking logic enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants