userdel: fix user busy detection for threads#1623
userdel: fix user busy detection for threads#1623haxtibal wants to merge 2 commits intoshadow-maint:masterfrom
Conversation
|
Reproducer: setuid.py #!/usr/bin/env python3
import ctypes, os, pwd, sys, threading, time
SYS_setuid = 105 # setuid, x86_64
libc = ctypes.CDLL(None)
libc.syscall.restype = ctypes.c_long
libc.syscall.argtypes = [ctypes.c_long, ctypes.c_long]
uid = pwd.getpwnam(sys.argv[1]).pw_uid
def thread_func():
libc.syscall(SYS_setuid, uid)
while True:
print(f"thread running as uid {uid} (pid={os.getpid()})", flush=True)
time.sleep(5)
threading.Thread(target=thread_func, daemon=True).start()
time.sleep(60)Start snippet as root. It's main thread has uid 0, spawned thread has uid of testuser. Then delete user while program is running. |
| char buf[512], buf2[512]; | ||
|
|
||
| stprintf_a(path, "/proc/%s/ns/user", sname); | ||
| stprintf_a(path, "/proc/%d/task/%d/ns/user", pid, tid); |
There was a problem hiding this comment.
As far as I know, pid_t is not required by POSIX to be int. If we want to use %d, we should at least have -Wformat=2 -Werror=format to make sure this doesn't produce UB. Alternatively, you may want to use intmax_t for printing, although if we're pretty confident that pid_t is an int, %d should be good if combined with the compiler error.
There was a problem hiding this comment.
As far as I know, pid_t is not required by POSIX to be int
This is true for POSIX, but we're now inside #ifdef __linux__, and for Linux we have
typedef int __kernel_pid_t;
for all platforms (see uapi/asm-generic/posix_types.h). Further, the existing code also uses %d for pid_t in some places.
However the existing code does it inconsistently. If you tell what's your preference I could add a commit that aligns pid_t formatting for the whole user_busy.c file.
Could you please use only C and sh(1) (or bash(1)) for the reproducer? Python is hard to understand to me. |
On Linux, userdel/usermod check all /proc/<pid> status files to ensure a to-be-modified user has no more running tasks, or abort modification otherwise. However, the check failed to detect threads running as the user if the corresponding main thread ran as a different user. The user is deleted despite still being busy. This is due to passing a wrong value to check_status. The caller passed "<pid>/task", rather than "<pid>/task/<tid>". In consequence check_status tried to open "/proc/<pid>/task/status" - a wrong path that never exists - open fails, and check_status always returns 0. The correct status file name would have been "/proc/<pid>/task/<tid>/status" instead. The bug can only be reproduced by rather exotic code using raw syscalls. POSIX does not allow threads to have different UIDs. To fix it, construct the correct path to the tid status file in the caller, before passing it to check_status. Behavior without fix: userdel testuser # testuser has threads with tid uid != pid uid => no output, testuser deleted, thread still running With the fix: userdel testuser # testuser has threads with tid uid != pid uid userdel: user testuser is currently used by process 178863 => testuser not deleted Signed-off-by: Tobias Deiminger <tobias.deiminger@linutronix.de>
Change the interface of check_status and different_namespace to take pid and tid instead of a partially constructed path string. This is simpler and counters bugs like 821db7c ("userdel: fix user busy detection for threads") by design. Signed-off-by: Tobias Deiminger <tobias.deiminger@linutronix.de>
6d51bbe to
a4681d8
Compare
Sure, try this: setuid_thread.c #include <pthread.h>
#include <pwd.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/syscall.h>
#include <unistd.h>
static uid_t target_uid;
static void *user_thread(void *arg)
{
syscall(SYS_setuid, (long)target_uid);
for (;;) {
printf("thread running as uid %d (pid=%d)\n", (int)target_uid,
(int)getpid());
sleep(5);
}
return NULL;
}
int main(int argc, char *argv[])
{
if (argc < 2) {
fprintf(stderr, "Usage: %s <username>\n", argv[0]);
return 1;
}
struct passwd *pw = getpwnam(argv[1]);
if (!pw) {
fprintf(stderr, "user not found: %s\n", argv[1]);
return 1;
}
target_uid = pw->pw_uid;
pthread_t tid;
pthread_create(&tid, NULL, user_thread, NULL);
sleep(60);
return 0;
}Then gcc setuid_thread.c -o setuid_thread
sudo useradd --no-create-home testuser
sudo ./setuid_thread testuser &
sudo userdel testuser # should detect thread running as testuser and abort, but doesn't detect and deletes testuser |
On Linux, userdel/usermod check all /proc/ status files to ensure a to-be-modified user has no more running tasks, or abort modification otherwise.
However, the check failed to detect threads running as the user if the corresponding main thread ran as a different user. The user is deleted despite still being busy. This is due to passing a wrong value to check_status. The caller passed "/task", rather than "/task/". In consequence check_status tried to open "/proc//task/status" - a wrong path that never exists - open fails, and check_status always returns 0. The correct status file name would have been "/proc//task//status" instead.
The bug can only be reproduced by rather exotic code using raw syscalls. POSIX does not allow threads to have different UIDs.
To fix it, construct the correct path to the tid status file. Also change the interface of check_status and different_namespace to take pid and tid instead of a partially constructed path string. This is simpler and makes similar bugs less likely.
Behavior without fix:
userdel testuser # testuser has threads with tid uid != pid uid=> no output, testuser deleted despite being busy
With the fix:
=> testuser detected as busy, not deleted