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

Sudo loop temporarily locking user accounts (and is probably broken in general) #2170

Open
drws opened this issue May 18, 2023 · 9 comments
Open

Comments

@drws
Copy link

drws commented May 18, 2023

Affected Version

yay v12.0.4 - libalpm v13.0.2

Describe the bug

--sudoloop runs sudo in a loop to prevent password input dialogue timeout, which sounds very simple at first glance and should work. Unfortunately, it's not that simple. In modern distros such approach usually causes trouble due to the fact that each sudo execution resulting in timeout counts as an unsuccessful login attempt, in combination with:

As of pambase 20200721.1-2, pam_faillock.so is enabled by default to lock out users for 10 minutes after 3 failed login attempts in a 15 minute period (see FS#67644).

(source: ArchWiki)

It's possible to disable this feature, but that's not a solution since it might be desired otherwise. The net result of everything is that --sudoloop only seemingly prevents sudo timeout, but in reality it only prevents input prompt timeout while it does more harm than good in the background. Account lockups have been reported before in #1379. --sudoloop is probably broken. Also, even if the correct password is put in after the account has been locked, both sudo and yay fail awkwardly and uninformatively:

[sudo] password for user: 
error installing: [.../yay/build/...pkg.tar.zst] - exit status 1
sudo: unable to read password: Input/output error
sudo: unable to restore terminal settings: Input/output error
Sorry, try again.
[sudo] password for user: 

Again, user sees this output after putting in the correct password after some time. Seeing such errors I didn't even dare to try with an incorrect one! :)

Reproduction Steps

  1. Try to build and install a package with yay -Sy(u) --sudoloop ...
  2. When sudo asks for password (before installlation), wait for it to timeout multiple times (given the pam_faillock settings)
  3. Try to enter the password and install package(s)

Expected behavior

Both presented solutions solve the issue, but the second one is only appropriate as an optional one due to security compromises. The best solution would be to incorporate both since they are compatible.

Solution 1: default

A KISS --sudoloop replacement as proposed before: yay tries to authenticate the user only once and waits for them to be ready before retrying. This not only replaces --sudoloop with a less bruteforce approach, but is also probably safe enough to be default.

Solution 2: optional

It has been requested many times for yay to be able to acquire privileges in the beginning (ask for sudo password only once) and then execute privileged and non-privileged tasks as necessary:

--sudoloop is often mentioned as a solution, but a proper solution of this feature request would effectively make --sudoloop obsolete and resolve the present issue as well. There's also an argument against:

This is a really bad idea, as sudo credentials are cached by the TTY and if you walk away from the computer while yay is running, another person can come by, CTRL+C the running program, and gain access to a sudo session.

The argument stands, but it ultimately falls under physical security, which software can try to help with, but can never guarantee. Users running --sudoloop already make compromises so there's no reason an optional, more comfy and less secure way is provided if --sudoloop is/was as well. With all the proper warnings of course.

Furthermore, yay could make use of a child process with elevated privileges and not acquire or drop them elsewhere, hardening the security a bit to prevent simplest physical attacks such as the one above. But this is an extension probably belonging in a separate feature request.

@AladW
Copy link
Contributor

AladW commented Jun 13, 2023

This is a really bad idea, as sudo credentials are cached by the TTY and if you walk away from the computer while yay is running, another person can come by, CTRL+C the running program, and gain access to a sudo session.

That's pretty far fetched... a more realistic scenario (and a problem with time outs in general) is that building process have persistent root access. For example, one package (overlayfs-tools) I maintain calls sudo in its test suite, and would be able to do so successfully. Other (non-malicious) cases are when someone has no idea on how to write PKGBUILDs and writes sudo install in the package function().

I vaguely recall yay already implemented a privilege dropping mechanism. A nice addition would be no_new_privs

@Jguer
Copy link
Owner

Jguer commented Jun 24, 2023

I'm more tempted to remove sudoloop altogether and just advising users to add <USER> ALL=(ALL) NOPASSWD: /usr/bin/yay to their config if password entry is not a criteria in their threat model.

Just have no way of measuring how much it's used and if it fills a niche that sudoers config doesn't fulfill

@AladW
Copy link
Contributor

AladW commented Jun 24, 2023

That rule would allow to run sudo yay without password. Not sure there's much benefit there, since with the proper privilege dropping and sudo yay you only need one password prompt for the entire build queue.

Which from a user perspective (assuming any build completes within the configured timeout) is the same as sudoloop but with less of the drawbacks.

@Jguer
Copy link
Owner

Jguer commented Jun 26, 2023

my bad, <USER> ALL=(ALL) NOPASSWD: /usr/bin/pacman 🙂.

Privilege dropping as it's implemented now drops you into a temp user if you're using actually root or if the SUDO_USER env is set changes the uid and gid of the command.

sudo, doas:

	ogCaller := ""
	if caller := os.Getenv("SUDO_USER"); caller != "" {
		ogCaller = caller
	} else if caller := os.Getenv("DOAS_USER"); caller != "" {
		ogCaller = caller
	}

	if userFound, err := user.Lookup(ogCaller); err == nil {
		cmd.SysProcAttr = &syscall.SysProcAttr{}
		uid, _ := strconv.Atoi(userFound.Uid)
		gid, _ := strconv.Atoi(userFound.Gid)
		cmd.SysProcAttr.Credential = &syscall.Credential{Uid: uint32(uid), Gid: uint32(gid)}

		return cmd
	}

su:

cmdArgs := []string{
        "systemd-run",
	"--service-type=oneshot",
	"--pipe", "--wait", "--pty", "--quiet",
	"-p", "DynamicUser=yes",
	"-p", "CacheDirectory=yay",
	"-E", "HOME=/tmp",
}

Not a big fan of running as root but it does work for this usecase. Could argue that setting sudo to nopassword could be more dangerous as it's systemwide

@AladW
Copy link
Contributor

AladW commented Jun 26, 2023

Could argue that setting sudo to nopassword could be more dangerous as it's systemwide

It's probably fine if you use a separate user for that which does no other tasks and has a password itself

@drws
Copy link
Author

drws commented Jul 1, 2023

a problem with time outs in general is that building process have persistent root access

Building processes shouldn't need or get any root access. If there is no other way (experimental dev builds) sudo can still be used in the PKGBUILD and the user is asked to authenticate when needed during a normal build under an unprivileged user. And Yay can't really help with that since sudo calls are fired from makepkg.

In the quote above it is also emphasized that the problem is with persistent root access. While it shouldn't be persistent, Yay could still be able to help by caching it for selected operations during its run. --sudoloop actually tries to help by delaying it, but unfortunately produces even more issues. I also believe that system-wide solutions are not the final answer here.

Other (non-malicious) cases are when someone has no idea on how to write PKGBUILDs and writes sudo install in the package function().

Malicious or not these calls shouldn't get the privileges even if they were cached. Yay would use them only for selected operations and if it ordinarily didn't ask for sudo password more than once, it would also alarm the user if asked during the build.

A nice addition would be no_new_privs

This seems like something needed for a secure privilege-caching implementation. Yay could fork a privileged process with no_new_privs for running elevated operations, run as the current user and then call pacman as root through the forked process and makepkg as nobody. Only selected calls would go through the forked process that would only be able to communicate through IPC, preventing simple physical attacks (the CTRL+C scare).

This would render --sudoloop obsolete, solve this issue and of course the long-standing feature request to input credentials only in the beginning no matter the build time. But in any case, --sudoloop seems to be broken and in need of a fix.

@dagelf
Copy link

dagelf commented Apr 25, 2024

Sorry haven't read this whole thread... but sitting in waiting for sudo isn't hammering it right, so not sure how this could make it hit pam 3-strikes limit...

I'm here because of a related reason, not sure it it warrants a ticket of its own yet... just found the issue tracker and code... will jump in and see... (https://forum.endeavouros.com/t/yay-deadlock-shouldnt-give-up-should-keep-trying/54313/22)

Just thinking out loud here - but what if, instead of going into sudo right away, it just asks "Press any key to proceed"? And then carries on?

@dagelf
Copy link

dagelf commented Apr 25, 2024

I did a quick scan through the source, and couldn't find the point at which that could potentially be inserted... but I did notice an obvious alternative approach: yay ALL=(ALL) NOPASSWD: ALL.

Problem in general, that I also don't yet understand, is how is root abuse by package maintainers, prevented? Any pointers would be appreciated.

@drws
Copy link
Author

drws commented May 2, 2024

Sorry haven't read this whole thread...

No, you haven't read any of this thread and I think it's high time you do it. More than half of your questions/proposals, such as an explanation on hitting the 3-strikes limit, are explained already in the OP, which you apparently didn't read either. And the discussion already includes your other proposals, and further, so next time please add something new.

And remember that sometimes it's easier to, you know, just read the thread. And when you don't still read the OP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants