From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from thejh.net ([37.221.195.125]:35132 "EHLO thejh.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752877AbcKBVjh (ORCPT ); Wed, 2 Nov 2016 17:39:37 -0400 Date: Wed, 2 Nov 2016 22:39:32 +0100 From: Jann Horn To: Oleg Nesterov Cc: Alexander Viro , Roland McGrath , John Johansen , James Morris , "Serge E. Hallyn" , Paul Moore , Stephen Smalley , Eric Paris , Casey Schaufler , Kees Cook , Andrew Morton , Janis Danisevskis , Seth Forshee , "Eric . Biederman" , Thomas Gleixner , Benjamin LaHaise , Ben Hutchings , Andy Lutomirski , Linus Torvalds , linux-fsdevel@vger.kernel.org, linux-security-module@vger.kernel.org, security@kernel.org Subject: Re: [PATCH v2 4/8] futex: don't leak robust_list pointer Message-ID: <20161102213932.GA13748@pc.thejh.net> References: <1474663238-22134-1-git-send-email-jann@thejh.net> <1474663238-22134-5-git-send-email-jann@thejh.net> <20160930145256.GB12862@redhat.com> <20161030171650.GB2558@pc.thejh.net> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="HcAYCG3uE/tztfnV" Content-Disposition: inline In-Reply-To: <20161030171650.GB2558@pc.thejh.net> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: --HcAYCG3uE/tztfnV Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sun, Oct 30, 2016 at 06:16:50PM +0100, Jann Horn wrote: > On Fri, Sep 30, 2016 at 04:52:57PM +0200, Oleg Nesterov wrote: > > On 09/23, Jann Horn wrote: > > > > > > This prevents an attacker from determining the robust_list or > > > compat_robust_list userspace pointer of a process created by executing > > > a setuid binary. Such an attack could be performed by racing > > > get_robust_list() with a setuid execution. The impact of this issue i= s that > > > an attacker could theoretically bypass ASLR when attacking setuid bin= aries. > >=20 > > Well. I am not sure this actually needs a fix, but I won't argue. > >=20 > > I can't really understand what this patch actually fixes, > >=20 > > > @@ -3007,31 +3007,43 @@ SYSCALL_DEFINE3(get_robust_list, int, pid, > > > if (!futex_cmpxchg_enabled) > > > return -ENOSYS; > > > > > > - rcu_read_lock(); > > > - > > > - ret =3D -ESRCH; > > > - if (!pid) > > > + if (!pid) { > > > p =3D current; > > > - else { > > > + get_task_struct(p); > > > + } else { > > > + rcu_read_lock(); > > > p =3D find_task_by_vpid(pid); > > > + /* pin the task to permit dropping the RCU read lock before > > > + * acquiring the mutex > > > + */ > > > + if (p) > > > + get_task_struct(p); > > > + rcu_read_unlock(); > > > if (!p) > > > - goto err_unlock; > > > + return -ESRCH; > > > } > > > > > > + ret =3D mutex_lock_killable(&p->signal->cred_guard_light); > > > + if (ret) > > > + goto err_put; > > > + > > > ret =3D -EPERM; > > > if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS)) > > > goto err_unlock; > > > > > > head =3D p->robust_list; > > > - rcu_read_unlock(); > >=20 > > OK, suppose it races with setuid exec, and mutex_lock_killable() + > > ptrace_may_access() comes after flush_old_exec() but before > > install_exec_creds(), in this case ptrace_may_access() can wrongly > > succeed. >=20 > I take cred_guard_light in flush_old_exec() and release it in > install_exec_creds(), so that shouldn't work, I think. >=20 >=20 > > In theory, it is possible that the execing thread can complete exec, > > return to user-mode and call sys_set_robust_list() before we read > > head =3D p->robust_list. Yes, this is unlikely, but unless I am totally > > confused the race you are trying to fix is equally unlikely? > >=20 > > perhaps we can make a much simpler change to prevent this, see below. > > We can rely on fact that both ptrace_may_access() and exec_mmap() > > takes the same task_lock(). Sure, this can "leak" robust_list too, > > a set-uid binary can exec and/or lower its credentials after we > > read p->robust_list, but personally I think we do not care. > >=20 > > Or I missed something else? >=20 > No - I think your patch would work, too, apart from the potential > leak you mentioned. Changing my opinion: This does not just affect setuid binaries. It also affects daemons like cron and atd that execute processes with dropped privileges. This is how atd runs jobs (strace output, with irrelevant stuff removed): [...] clone(child_stack=3D0, flags=3DCLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGC= HLD, child_tidptr=3D0x7fa81b1099d0) =3D 14915 Process 14915 attached [...] [pid 14915] set_robust_list(0x7fa81b1099e0, 24) =3D 0 [...] [pid 14915] setregid(0, 1) =3D 0 [pid 14915] setreuid(0, 1) =3D 0 [pid 14915] close(0) =3D 0 [pid 14915] close(1) =3D 0 [pid 14915] close(2) =3D 0 [pid 14915] clone(Process 14916 attached child_stack=3D0, flags=3DCLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, c= hild_tidptr=3D0x7fa81b1099d0) =3D 14916 [pid 14916] set_robust_list(0x7fa81b1099e0, 24) =3D 0 [pid 14915] wait4(14916, [pid 14916] lseek(6, 0, SEEK_SET) =3D 0 [pid 14916] dup2(6, 0) =3D 0 [pid 14916] dup2(5, 1) =3D 1 [pid 14916] dup2(5, 2) =3D 2 [pid 14916] close(6) =3D 0 [pid 14916] close(5) =3D 0 [pid 14916] setreuid(1, 0) =3D 0 [pid 14916] setregid(1, 0) =3D 0 [...] [pid 14916] setgroups(13, [1000, [...]]) =3D 0 [pid 14916] setgid(1000) =3D 0 [pid 14916] setuid(1000) =3D 0 [pid 14916] chdir("/") =3D 0 [pid 14916] execve("/bin/sh", ["sh"], [/* 0 vars */]) =3D 0 [...] Basically, you can see that the pointer 0x7fa81b1099e0, which reveals information about the address space layout, is the robust list of pid 14916 when it calls execve(), and after that execve() call, pid 14916 will be ptraceable for the user (modulo LSMs). So I think that my patch is a bit safer. Yes, there aren't many local daemons whose address space layout you can discover this way, but it's still not great. --HcAYCG3uE/tztfnV Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJYGl0UAAoJED4KNFJOeCOo85IQAMcQvwEu2u6do7GWKinLMnjP c5MSkCRhR1VHdLqvM6kQtfGjCbm79gaZ12JeX6fksrUKjoLqFVjnPLQmFrh9zRPz hW+RM5iXR0n2rjuZcz74CWMF1FQ5Uk5ZnsX6w8TD2LMIlAW4rWSAZ+gmeiPx94va MPaVWoFHQr8P95tpebI51KEY6bLflK8a8100Qj+oT9MqZG8mghkVD7U9IOGi7HHi i80eRiB6XgdzaO/KOmctrnNhmPPxgOCxdnK+UJ1UZzIHC5HG38DNro69Xl25RY2X UseYmy8QLFj6mXYFR/aAr3fnczNfKmonkf7ztNYChjADKccHRy2g5PVzqCIaEs4Y hkNEe6ifZ0Q5DNkA5fIwG93Bk4tOuKVBUQS62ngDYe1sjE7WAst11UYgmPPDJc4G ZvWDmqz6gpctwd4qMKYAXo0laJh7/Esl5MjjZfHYLGmUFxZd/LsL8Dd00bCmfIyu hbNRsKeK8/LGF3aFZzci8EAnvhK4Ctu/opHTdUd1hZwD4ckLB0dFLtVup4lTOR2T wDaDWDIiOtEdvgm8TKwAfQCAy1gcbrv1Ef97NhwbVQyLMk7BDF8N8NWr+OYNUdZV IdmU4epKOGqREigAd+blChHDaeGFJYuAjh+I5ykIIqogm+y/5U6ClRRmvgXsIr9G 9F0N3ZXZ5PR3TLLdIN4f =b5XA -----END PGP SIGNATURE----- --HcAYCG3uE/tztfnV--