Re: [PATCH] Fix race condition when exec'ing setuid files

Re: [PATCH] Fix race condition when exec'ing setuid files

[Kees Cook]

On Thu, Sep 13, 2022 at 15:03:38 -0700, Kees Cook wrote:
> It seems quite unusual to have a high-load heavily threaded
> process decide to exec.

In looking at this a bunch more, I actually think everything is working
as intended. If a process is actively launching threads while also trying
to exec, they're going to create races for themselves.

So the question, then, is "why are they trying to exec while actively
spawning new threads?" That appears to be the core problem here, and as
far as I can tell, the kernel has behaved this way for a very long time.
I don't think the kernel should fix this, either, because it leads to a
very weird state for userspace, where the thread spawner may suddenly
die due to the exec happening in another thread. This really looks like
something userspace needs to handle correctly (i.e. don't try to exec
while actively spawning threads).

For example, here's a fix to the original PoC:

--- a.c.original        2022-10-06 13:07:13.279845619 -0700
+++ a.c 2022-10-06 13:10:27.702941645 -0700
@@ -8,8 +8,10 @@
        return NULL;
 }
 
+int stop_spawning;
+
 void *target(void *p) {
-       for (;;) {
+       while (!stop_spawning) {
                pthread_t t;
                if (pthread_create(&t, NULL, nothing, NULL) == 0)
                        pthread_join(t, NULL);
@@ -17,18 +19,26 @@
        return NULL;
 }
 
+#define MAX_THREADS    10
+
 int main(void)
 {
+       pthread_t threads[MAX_THREADS];
        struct timespec tv;
        int i;
 
-       for (i = 0; i < 10; i++) {
-               pthread_t t;
-               pthread_create(&t, NULL, target, NULL);
+       for (i = 0; i < MAX_THREADS; i++) {
+               pthread_create(&threads[i], NULL, target, NULL);
        }
        tv.tv_sec = 0;
        tv.tv_nsec = 100000;
        nanosleep(&tv, NULL);
+
+       /* Signal shut down, and collect spawners. */
+       stop_spawning = 1;
+       for (i = 0; i < MAX_THREADS; i++)
+               pthread_join(threads[i], NULL);
+
        if (execl("./b", "./b", NULL) < 0)
                perror("execl");
        return 0;


-- 
Kees Cook

Re: [PATCH] Fix race condition when exec'ing setuid files

[Theodore Ts'o]

On Thu, Oct 06, 2022 at 01:20:35PM -0700, Kees Cook wrote:
> 
> So the question, then, is "why are they trying to exec while actively
> spawning new threads?" That appears to be the core problem here, and as
> far as I can tell, the kernel has behaved this way for a very long time.
> I don't think the kernel should fix this, either, because it leads to a
> very weird state for userspace, where the thread spawner may suddenly
> die due to the exec happening in another thread. This really looks like
> something userspace needs to handle correctly (i.e. don't try to exec
> while actively spawning threads).

One of the classic failure modes is when a threaded program calls a
library, and that library might try to do a fork/exec (or call
system(3) to run some command.  e.g., such as running "lvm create ..."
or to spawn some kind of helper daemon.

There are a number of stack overflow questions about this, and there
are some solutions to _some_ of the problems, such as using
pthread_atfork(), and knowing that you are about to call fork/exec,
and use some out of band mechanism to to make sure no threads get
spawned until the fork/exec is completed --- but if you don't know
that a library is going to do a fork/exec, well, life is tough. 

One technique even advocated by a stack overflow article is "avoid
using threads whenver possible".  :-/

	       	       	  	   - Ted

RE: [PATCH] Fix race condition when exec'ing setuid files

[David Laight]

From: Theodore Ts'o
> Sent: 07 October 2022 02:41
> 
> On Thu, Oct 06, 2022 at 01:20:35PM -0700, Kees Cook wrote:
> >
> > So the question, then, is "why are they trying to exec while actively
> > spawning new threads?" That appears to be the core problem here, and as
> > far as I can tell, the kernel has behaved this way for a very long time.
> > I don't think the kernel should fix this, either, because it leads to a
> > very weird state for userspace, where the thread spawner may suddenly
> > die due to the exec happening in another thread. This really looks like
> > something userspace needs to handle correctly (i.e. don't try to exec
> > while actively spawning threads).
> 
> One of the classic failure modes is when a threaded program calls a
> library, and that library might try to do a fork/exec (or call
> system(3) to run some command.  e.g., such as running "lvm create ..."
> or to spawn some kind of helper daemon.
> 
> There are a number of stack overflow questions about this, and there
> are some solutions to _some_ of the problems, such as using
> pthread_atfork(), and knowing that you are about to call fork/exec,
> and use some out of band mechanism to to make sure no threads get
> spawned until the fork/exec is completed --- but if you don't know
> that a library is going to do a fork/exec, well, life is tough.

Or that a library thread is about to create a new thread.

> One technique even advocated by a stack overflow article is "avoid
> using threads whenver possible".  :-/

Doesn't fork() only create the current thread in the new process?
So by the time exec() runs there is a nice single threaded process
with an fd table that isn't shared.

For helpers there is always (a properly implemented) posix_spawn() :-)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)