|Summary:||Dangerous race condition with vfork in posix_spawn|
|Product:||glibc||Reporter:||Rich Felker <bugdal>|
|Component:||libc||Assignee:||Not yet assigned to anyone <unassigned>|
|Severity:||normal||CC:||carlos, drepper.fsp, sionescu+BugTrackers|
Description Rich Felker 2012-10-21 16:58:05 UTC
posix_spawn uses vfork (to avoid momentary doubling commit charge and improve performance) in cases where it seems "safe", or when explictly requested. However, at least one major race condition seems to have been missed: Consider a program running with elevated privileges (perhaps a daemon or suid program which initially has root) which is multi-threaded, and which will drop privileged and then execute untrusted code (perhaps a user-provided script or module). The scenario looks like: Thread A is calling posix_spawn to run a fixed external command (call it child C) that will work fine, and which is safe to invoke, with either the initial privileges or the reduced privileges. Think of something stupid like running "date" to get the current date and time. Thread B is calling setuid() to drop privileges, then executing untrusted code. And let's suppose events happen in the following order: A: vfork C: vfork returns in child B: setuid B: untrusted code runs and pokes at memory A is using C: now running arbitrary code as root C: ... A: vfork returns in parent Fundamentally, the danger of this race is the possibility of it giving rise to two threads/processes sharing an address space, but with different privileges; this kind of situation must never be allowed to arise. The simplest way to avoid the race is by using fork instead of vfork, unless vfork is specifically requested. However, that brings back the double-commit-charge issue. An alternative fix is to hold a lock that prevents changing uids/gids during the vfork window. This is also easy since NPTL is already doing a global lock for set*id() to synchronize the id changes across all the threads (since Linux requires each thread to make its own set*id() syscall).
Comment 1 Carlos O'Donell 2014-09-20 04:13:08 UTC
I agree this should be fixed. I think posix_spawn and setXid functions should serialize against eachother. Note that vfork should not serialize against setXid functions becuase there I think users need to be clever enough to know what they are doing, but we should still provide better documentation and example code.