exec'ing a setuid binary from a threaded program sometimes fails to setuid

Bug #1672819 reported by John Lenton on 2017-03-14
14
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Linux
Unknown
Unknown
linux (Ubuntu)
High
Colin Ian King
Xenial
High
Colin Ian King
Yakkety
High
Colin Ian King
Zesty
High
Colin Ian King

Bug Description

== SRU REQUEST XENIAL, YAKKETY, ZESTY ==

Due to two race conditions in check_unsafe_exec(), exec'ing a setuid binary from a threaded program sometimes fails to setuid.

== Fix ==

Sauce patch for Xenial, Yakkety + Zesty:

https://lists.ubuntu.com/archives/kernel-team/2017-May/084102.html

This fix re-executes the unsafe check if there is a discrepancy between the expected fs count and the found count during the racy window during thread exec or exit. This re-check occurs very infrequently and saves a lot of addition locking on per thread structures that would make performance of fork/exec/exit prohibitively expensive.

== Test case ==

See the example C code in the patch, https://lists.ubuntu.com/archives/kernel-team/2017-May/084102.html

Run the test code as follows: for i in $(seq 1000); do ./a; done

With the patch, no messages are emitted, without the patch, one sees a message:

"Failed, got euid 1000 (expecting 0)"

..which shows the setuid program failed the check_unsafe_exec() because of the race.

== Regression potential ==

breaking existing safe exec semantics.

====================

This can be reproduced with
https://gist.github.com/chipaca/806c90d96c437444f27f45a83d00a813

With that, and go 1.8, if you run “make” and then

for i in `seq 99`; do ./a_go; done

you'll see a variable number of ”GOT 1000” (or whatever your user id is). If you don't, add one or two more 9s on there.

That's a simple go reproducer. You can also use “a_p” instead of “a_go” to see one that only uses pthreads. “a_c” is a C version that does *not* reproduce the issue.

But it's not pthreads: if in a_go.go you comment out the “import "C"”, you'll still see the “GOT 1000” messages, in a static binary that uses no pthreads, just clone(2). You'll also see a bunch of warnings because it's not properly handling an EAGAIN from clone, but that's unrelated.

If you pin the process to a single thread using taskset, you don't get the issue from a_go; a_p continues to reproduce the issue. In some virtualized environments we haven't been able to reproduce the issue either (e.g. some aws instances), but kvm works (you need -smp to see the issue from a_go).

ProblemType: Bug
DistroRelease: Ubuntu 16.04
Package: linux-image-4.4.0-64-generic 4.4.0-64.85
ProcVersionSignature: Ubuntu 4.4.0-64.85-generic 4.4.44
Uname: Linux 4.4.0-64-generic x86_64
NonfreeKernelModules: zfs zunicode zcommon znvpair zavl
ApportVersion: 2.20.1-0ubuntu2.5
Architecture: amd64
AudioDevicesInUse:
 USER PID ACCESS COMMAND
 /dev/snd/pcmC0D0p: john 2354 F...m pulseaudio
 /dev/snd/controlC0: john 2354 F.... pulseaudio
CurrentDesktop: Unity
Date: Tue Mar 14 17:17:23 2017
HibernationDevice: RESUME=UUID=b9fd155b-dcbe-4337-ae77-6daa6569beaf
InstallationDate: Installed on 2014-04-27 (1051 days ago)
InstallationMedia: Ubuntu 14.04 LTS "Trusty Tahr" - Release amd64 (20140417)
MachineType: Dell Inc. Latitude E6510
ProcFB: 0 inteldrmfb
ProcKernelCmdLine: BOOT_IMAGE=/vmlinuz-4.4.0-64-generic root=/dev/mapper/ubuntu--vg-root ro enable_mtrr_cleanup mtrr_spare_reg_nr=8 mtrr_gran_size=32M mtrr_chunk_size=32M quiet splash
RelatedPackageVersions:
 linux-restricted-modules-4.4.0-64-generic N/A
 linux-backports-modules-4.4.0-64-generic N/A
 linux-firmware 1.157.8
SourcePackage: linux
SystemImageInfo: Error: command ['system-image-cli', '-i'] failed with exit code 2:
UpgradeStatus: Upgraded to xenial on 2015-06-18 (634 days ago)
dmi.bios.date: 12/05/2013
dmi.bios.vendor: Dell Inc.
dmi.bios.version: A16
dmi.board.vendor: Dell Inc.
dmi.chassis.type: 9
dmi.chassis.vendor: Dell Inc.
dmi.modalias: dmi:bvnDellInc.:bvrA16:bd12/05/2013:svnDellInc.:pnLatitudeE6510:pvr0001:rvnDellInc.:rn:rvr:cvnDellInc.:ct9:cvr:
dmi.product.name: Latitude E6510
dmi.product.version: 0001
dmi.sys.vendor: Dell Inc.

John Lenton (chipaca) wrote :
John Lenton (chipaca) wrote :

I also tried this in 4.10.0-11-generic, same results.

Changed in linux (Ubuntu):
status: New → Triaged
importance: Undecided → High
Changed in linux (Ubuntu Xenial):
status: New → Triaged
importance: Undecided → High
tags: added: kernel-key
Kamal Mostafa (kamalmostafa) wrote :

I can reproduce this with the simple pthreads-only reproducer (loop of ./a_p running setuid binary ./b) running 4.4.0-57-generic on bare metal.

$ for i in `seq 10`; do ./a_p; done
GOT 1000
GOT 1000

$ for i in `seq 1000`; do ./a_p; done | wc -l
117

Kamal Mostafa (kamalmostafa) wrote :

An AWS instance (t2.xlarge with 4 vCPU's) running 4.4.0-1001-aws reproduces the problem:

$ for i in `seq 10000`; do ./a_p; done | wc -l
124

Michael Hudson-Doyle (mwhudson) wrote :

I had a bit of a stare at the kernel source and suspected that the downgrade of uid is happening here: https://github.com/torvalds/linux/blob/v4.4/security/commoncap.c#L547-L548

I added a "WARN(1, "downgrading in subprocess %d %d\n", bprm->unsafe, (int)capable(CAP_SETUID))" which revealed that bprm->unsafe is 1 aka LSM_UNSAFE_SHARE.

The only place (I can find) that bprm->unsafe is set to LSM_UNSAFE_SHARE is this check in check_unsafe_exec here (from https://github.com/torvalds/linux/blob/v4.4/fs/exec.c#L1281):

 t = p;
 n_fs = 1;
 spin_lock(&p->fs->lock);
 rcu_read_lock();
 while_each_thread(p, t) {
  if (t->fs == p->fs)
   n_fs++;
 }
 rcu_read_unlock();

 if (p->fs->users > n_fs)
  bprm->unsafe |= LSM_UNSAFE_SHARE;
 else
  p->fs->in_exec = 1;
 spin_unlock(&p->fs->lock);

So I think (and here it gets a bit sketchy) we're racing with copy_process in kernel/fork.c: that calls copy_fs (which is what increments p->fs->users) some way before it does the stuff necessary to make the new thread be included in the while_each_thread(p, t) loop. So n_fs is too low, the check triggers and the setuid bits get ignored.

No idea at all how to fix this of course.

tags: added: kernel-da-key
removed: kernel-key
tags: added: kernel-key
removed: kernel-da-key
Changed in linux (Ubuntu Xenial):
assignee: nobody → Colin Ian King (colin-king)
status: Triaged → In Progress
Colin Ian King (colin-king) wrote :

The following seems to fix it, but I need to exercise this a bit more to be 100% certain it is rock solid:

diff --git a/fs/fs_struct.c b/fs/fs_struct.c
index 7dca743..cd7175e2 100644
--- a/fs/fs_struct.c
+++ b/fs/fs_struct.c
@@ -98,8 +98,10 @@ void exit_fs(struct task_struct *tsk)
                int kill;
                task_lock(tsk);
                spin_lock(&fs->lock);
+ rcu_read_lock();
                tsk->fs = NULL;
                kill = !--fs->users;
+ rcu_read_unlock();
                spin_unlock(&fs->lock);
                task_unlock(tsk);
                if (kill)

Colin Ian King (colin-king) wrote :

Nope, that fails too.

Colin Ian King (colin-king) wrote :

So the thread fs has been torn down and so t->fs is null which then triggers the miscounting of n_fs; so I'm sspeculating we may need to try:

 while_each_thread(p, t) {
  if (t->fs == p->fs || !t->fs)
   n_fs++;
 }

Colin Ian King (colin-king) wrote :

With the change mentioned in comment #8 I now cannot reproduce the issue.

Zygmunt Krynicki (zyga) wrote :

This also happens on Fedora 25 running 4.10.8-200.fc25.x64_64

Colin Ian King (colin-king) wrote :

This bug has been around since at least 2009.

Kernel Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=195453

tags: added: kernel-da-key
removed: kernel-key
Colin Ian King (colin-king) wrote :

exec'ing from a thread is an interesting problem; the semantics of exec should be to terminal all the threads before the exec occurs according to http://maxim.int.ru/bookshelf/PthreadsProgram/htm/r_44.html

The normal idiom would be to do:
  fork()
      child exec's
      parent waits for child

I'm not sure in your case if you desire all the threads to terminate after the exec, so the wait() may be in fact be replaced by pthread termination calls on all the threads for your implementation.

Unfortunately there is an issue with fork'ing in a thread; any mutex held by another thread at the moment of fork becomes locked forever since we have once mutex locked by the parent and one by the child. Normally one would therefore use pthread_atfork() to help workaround this issue, see https://stackoverflow.com/questions/14407544/mixing-threads-fork-and-mutexes-what-should-i-watch-out-for

Colin Ian King (colin-king) wrote :

"to terminal all the threads" should read "to terminate all the threads"

On 8 May 2017 at 10:32, Colin Ian King <email address hidden> wrote:

> exec'ing from a thread is an interesting problem; the semantics of exec
> should be to terminal all the threads before the exec occurs according
> to http://maxim.int.ru/bookshelf/PthreadsProgram/htm/r_44.html
>
> The normal idiom would be to do:
> fork()
> child exec's
> parent waits for child
>
> I'm not sure in your case if you desire all the threads to terminate
> after the exec, so the wait() may be in fact be replaced by pthread
> termination calls on all the threads for your implementation.
>

The original bug report was about a process calling execve directly, not a
fork/exec situation. So yes, the expectation is that all threads are gone.

> Unfortunately there is an issue with fork'ing in a thread; any mutex
> held by another thread at the moment of fork becomes locked forever
> since we have once mutex locked by the parent and one by the child.
> Normally one would therefore use pthread_atfork() to help workaround
> this issue, see https://stackoverflow.com/questions/14407544/mixing-
> threads-fork-and-mutexes-what-should-i-watch-out-for

Go doesn't support forking (except for some very careful code that calls
exec in the child), for exactly this sort of reason.

Colin Ian King (colin-king) wrote :

I think I've found the simplest solution that avoids costly locking overhead and seems to work in my tests. I've uploaded the debs for Xenial in:

http://kernel.ubuntu.com/~cking/lp-1672819/

Would you mind testing these and seeing if it helps.

Changed in linux (Ubuntu Xenial):
status: In Progress → Incomplete
John Lenton (chipaca) wrote :

With the kernel from #16 I am no longer able to reproduce the issue, not with the simplified reproducers described in this bug, nor with the original (slower and more convoluted) snapd reproducer.

John Lenton (chipaca) on 2017-05-11
Changed in linux (Ubuntu Xenial):
status: Incomplete → In Progress
description: updated
Seth Forshee (sforshee) on 2017-05-17
Changed in linux (Ubuntu):
status: Triaged → Fix Committed
Changed in linux (Ubuntu Zesty):
status: New → Fix Committed
Changed in linux (Ubuntu Yakkety):
status: New → Fix Committed
Changed in linux (Ubuntu Xenial):
status: In Progress → Fix Committed
Changed in linux (Ubuntu Yakkety):
assignee: nobody → Colin Ian King (colin-king)
Changed in linux (Ubuntu Zesty):
assignee: nobody → Colin Ian King (colin-king)
importance: Undecided → High
Changed in linux (Ubuntu Yakkety):
importance: Undecided → High

This bug is awaiting verification that the kernel in -proposed solves the problem. Please test the kernel and update this bug with the results. If the problem is solved, change the tag 'verification-needed-xenial' to 'verification-done-xenial'. If the problem still exists, change the tag 'verification-needed-xenial' to 'verification-failed-xenial'.

If verification is not done by 5 working days from today, this fix will be dropped from the source code, and this bug will be closed.

See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you!

tags: added: verification-needed-xenial
tags: added: verification-needed-yakkety

This bug is awaiting verification that the kernel in -proposed solves the problem. Please test the kernel and update this bug with the results. If the problem is solved, change the tag 'verification-needed-yakkety' to 'verification-done-yakkety'. If the problem still exists, change the tag 'verification-needed-yakkety' to 'verification-failed-yakkety'.

If verification is not done by 5 working days from today, this fix will be dropped from the source code, and this bug will be closed.

See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you!

tags: added: verification-needed-zesty

This bug is awaiting verification that the kernel in -proposed solves the problem. Please test the kernel and update this bug with the results. If the problem is solved, change the tag 'verification-needed-zesty' to 'verification-done-zesty'. If the problem still exists, change the tag 'verification-needed-zesty' to 'verification-failed-zesty'.

If verification is not done by 5 working days from today, this fix will be dropped from the source code, and this bug will be closed.

See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you!

Changed in linux (Ubuntu):
assignee: nobody → Colin Ian King (colin-king)
Colin Ian King (colin-king) wrote :

tested on xenial, 4.4.0-80-generic #101-Ubuntu, passed the test.

tags: added: verification-done-xenial
removed: verification-needed-xenial
Colin Ian King (colin-king) wrote :

tested on yakkety, 4.8.0-55-generic #58-Ubuntu, passed the test.

tags: added: verification-done-yakkety
removed: verification-needed-yakkety
Colin Ian King (colin-king) wrote :

tested on zesty, 4.10.0-23-generic #25-Ubuntu, passed the test.

tags: added: verification-done-zesty
removed: verification-needed-zesty
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.