From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752297AbdAYVcg (ORCPT ); Wed, 25 Jan 2017 16:32:36 -0500 Received: from shadbolt.e.decadent.org.uk ([88.96.1.126]:40969 "EHLO shadbolt.e.decadent.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751986AbdAYVce (ORCPT ); Wed, 25 Jan 2017 16:32:34 -0500 Message-ID: <1485379919.2998.159.camel@decadent.org.uk> Subject: Re: [PATCH 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory From: Ben Hutchings To: Andy Lutomirski , security@kernel.org Cc: Konstantin Khlebnikov , Alexander Viro , Kees Cook , Willy Tarreau , "linux-mm@kvack.org" , Andrew Morton , yalin wang , Linux Kernel Mailing List , Jan Kara , Linux FS Devel Date: Wed, 25 Jan 2017 21:31:59 +0000 In-Reply-To: <826ec4aab64ec304944098d15209f8c1ae65bb29.1485377903.git.luto@kernel.org> References: <826ec4aab64ec304944098d15209f8c1ae65bb29.1485377903.git.luto@kernel.org> Content-Type: multipart/signed; micalg="pgp-sha512"; protocol="application/pgp-signature"; boundary="=-JwyhHvofVkiTz0Fz3eNL" X-Mailer: Evolution 3.22.3-1 Mime-Version: 1.0 X-SA-Exim-Connect-IP: 2a02:8011:400e:2:6f00:88c8:c921:d332 X-SA-Exim-Mail-From: ben@decadent.org.uk X-SA-Exim-Scanned: No (on shadbolt.decadent.org.uk); SAEximRunCond expanded to false Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-JwyhHvofVkiTz0Fz3eNL Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 2017-01-25 at 13:06 -0800, Andy Lutomirski wrote: > Currently, if you open("foo", O_WRONLY | O_CREAT | ..., 02777) in a > directory that is setgid and owned by a different gid than current's > fsgid, you end up with an SGID executable that is owned by the > directory's GID.=C2=A0=C2=A0This is a Bad Thing (tm).=C2=A0=C2=A0Exploiti= ng this is > nontrivial because most ways of creating a new file create an empty > file and empty executables aren't particularly interesting, but this > is nevertheless quite dangerous. >=20 > Harden against this type of attack by detecting this particular > corner case (unprivileged program creates SGID executable inode in > SGID directory owned by a different GID) and clearing the new > inode's SGID bit. >=20 > > Signed-off-by: Andy Lutomirski > --- > =C2=A0fs/inode.c | 21 +++++++++++++++++++-- > =C2=A01 file changed, 19 insertions(+), 2 deletions(-) >=20 > diff --git a/fs/inode.c b/fs/inode.c > index f7029c40cfbd..d7e4b80470dd 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -2007,11 +2007,28 @@ void inode_init_owner(struct inode *inode, const = struct inode *dir, > =C2=A0{ > =C2=A0 inode->i_uid =3D current_fsuid(); > =C2=A0 if (dir && dir->i_mode & S_ISGID) { > + bool changing_gid =3D !gid_eq(inode->i_gid, dir->i_gid); [...] inode->i_gid hasn't been initialised yet. This should compare with current_fsgid(), shouldn't it? Ben. --=20 Ben Hutchings It is easier to write an incorrect program than to understand a correct one. --=-JwyhHvofVkiTz0Fz3eNL Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEErCspvTSmr92z9o8157/I7JWGEQkFAliJGU8ACgkQ57/I7JWG EQmC4RAApNScdegP2BE86vVROI7ZNOJKaxf0SFqYSiXmQFuJZR7X5bxCqswyQwUh soNh7TNp0z2bXhxQJaqJqBsBbnVXF3nEgLSoKiw8nEK0TGVjclB3uU9oS9Os5ehb i4WmvnEi4Y4BWFi7vbhAikEOf9dexr7ypN9cmaX2mGfr1aIbdAza3kq3sFF5vXqK 50o3wnNPH9mTw/rRte3sxoCr60TtbQ4KFutxvcA7v7G40lnoVyt3u0NIh9jQ1FHx C8LB8e9zIsXu5D8IV+FIxPlckavqxxfCgYZqW+cJnDChRXvawM0fUGh2jEIIBVnn YCdR3Pg+8bxH+6mTgcPy/jGf7MNNXuTFwEMLgIctxEnZIeeOHneRwBUb5UiByjn7 b6NchWMh8ZAiCz/FB9xV1kEU1UwyodOqPQ8c+JSrQEusHv0VrUNjTlVZWG92KO6d ZFlrzp0D8u/Sp+qk4RTtD3M0HHNWmCzi3HZ+4ugaTKkN8FDw6hkYfFLQQdFvFI19 V1BJfoRUrCoq1AuInT4Gl2SiL7wJfU9sagvdXf6UAPQZ0FlyL0lm9cZ0TQoiCo/D biOdR88rxksZXZprFY/Zs5D2+Ma7A5fc+vafzt9U6zOodg6polUaWRJYHTFykn0i ElgdgSCHMhAUOZXC7lhnzcfmYnvy8oGxelNSWGCYuW93Jgmz/DI= =4zSj -----END PGP SIGNATURE----- --=-JwyhHvofVkiTz0Fz3eNL--