sudo-project / sudo

Utility to execute a command as another user
https://www.sudo.ws
Other
1.2k stars 224 forks source link

tty path checking unhelpfully hides problems #329

Closed gdt closed 1 year ago

gdt commented 1 year ago

I am using 1.9.14p3 on NetBSD. I changed from /dev/ttyp to /dev/pts, but did not reboot. I found that sudo when executed as part of a package rebuild ("make replace") did not respect cached authtentication. The root causes are 1) I had not run dev_mkdb, and devname(3) returned stale paths 2) sudo took the device major/minor, converted it to a path, and then checked if the path was valid. If so, the dev_t was stored in the timestamp file. If not, sudo silently* flipped to ppid mode.

To figure this out, I had to read code and write my own code to parse the timestamp file.

I would suggest 1) If the tty path does not exist, log a very loud error and error out. This is a failure of something that ought to be true always, and it is both unhelpful and unsafe to do something else instead 2) If what matters is the dev_t, do not convert to names. Just use dev_t.

millert commented 1 year ago

Resolved by 3dfbf93 and https://github.com/sudo-project/sudo/commit/a85494b5c4f8afd24c9fcadd0e4102e6a8c4fbc7.

  1. If sudo can determine the user's tty device but cannot resolve it to a file name, it will now warn the user (previously it was a debug message) and behave as if no terminal is present. This is safer than erroring out on systems where there is no root password and sudo is the only way to access the root account.
  2. The dev_t is now passed from the front-end to the policy module where it can be used as-is in the time stamp file.
  3. The tsdump program in the sudo source distribution can be used to list the contents of a sudo time stamp file. It is not currently installed by default, perhaps it could go in sudo's libexec directory.
gdt commented 1 year ago

Thanks very much for addressing.

About tsdump, I concluded that the sources had no such program after seeing comments that were apparently sort of from the project indicating that such a program was harmful and people who needed it should write their own. Perhaps I misread. A possible reasonable approach would be to mention it in the README, build it as part of the build, but not install it.

millert commented 1 year ago

@gdt I just committed changes to build tsdump by default and mention it in the sudoers and sudoers_timestamp man pages.

gdt commented 1 year ago

Awesome, thank you for listening and improving.