From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id A0F47C433FE for ; Thu, 6 Oct 2022 15:36:16 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2F6E66B0073; Thu, 6 Oct 2022 11:36:16 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 2CCCC8E0002; Thu, 6 Oct 2022 11:36:16 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 16CD58E0001; Thu, 6 Oct 2022 11:36:16 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 04FFC6B0073 for ; Thu, 6 Oct 2022 11:36:16 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id B1A0E1A10F7 for ; Thu, 6 Oct 2022 15:36:15 +0000 (UTC) X-FDA: 79990925910.26.1F3DD9F Received: from mail-il1-f178.google.com (mail-il1-f178.google.com [209.85.166.178]) by imf04.hostedemail.com (Postfix) with ESMTP id 4B84B4001F for ; Thu, 6 Oct 2022 15:36:14 +0000 (UTC) Received: by mail-il1-f178.google.com with SMTP id m14so1161340ilf.12 for ; Thu, 06 Oct 2022 08:36:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=kBsN+WV474YxARCF7+nqmiK1jJMhN3dpQirfp2PoxlI=; b=JjMbJ/6iXZaadLFSJQk/HRtX7HrZ/RZDwb2mntZuqfcRGukztaLnaRtozSNqvZ38A4 YIoQ/S3pghhDMLpw8KTrVksZJi+qZfmtmFFtL6r+hlek1RDiFjHhO28+25YkHLvlCc/G 2ton1aYig3VnJwiK390amDUqhjkwHT2v8cCIJriuz9pU8Vq1hyKD89eS6KbebFUKLB7f StxxrRJteAZtp0mitV1ByWgBDUKx7ejR8uKWfR+ZopLjyxWlWBgzgq30TO7uWMlo0TOX Ot9J/qTOjnGajAnOuylFkpSg9k4OtDzH80hAqWJVInjfgYhV53xWDOLUG2oWhzMmJmJ7 oo0g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=kBsN+WV474YxARCF7+nqmiK1jJMhN3dpQirfp2PoxlI=; b=D3SM7xPLipE7yOGIQjb0AKXwbrRDLax0l78Wed2gZ+GOo8vQx5wmF/R6mxxzaC1CPy P7P8H3dvYqPO8gBS6AyWaOqQLEDQfuEkPEZhBa/1A7CClv8jziHBu5otus10kh2kcxhf x+nCsNo3Iqx1e9fKHfPAwzGckUMHcEYVquXNn13ua6DUzaamd3XSS3sw6f9jDcO7Z+fK RFaIw8QgW32DCJ2aHaw52AtexU1VP+DmRIZxxc108uPzctoz+t5Cmp9vjERUhwZaxQOc PRqzOAk0gfwJPWzPkPXvzLsl34vhn26/ylBc6fn3bELW8qPYYl///A26BHhadsK05pxU bjyg== X-Gm-Message-State: ACrzQf1LvVk6HhoQbY1IppoYVzNSHPRaX56355ECIYNJPuB6900t10z3 SAPm/DrEbEkjN03n4s/qhHeRwh16bEO7MNqEGhtDNw== X-Google-Smtp-Source: AMsMyM4b3/P730KDfkxwcyTWlRlnjgpSRQAwOlc/NfsO6cxtR79rz7+RL4b861zTtxfuwZkTO8fgQgVJ7C0o9SGCGVA= X-Received: by 2002:a05:6e02:1989:b0:2f6:45ca:410d with SMTP id g9-20020a056e02198900b002f645ca410dmr99039ilf.187.1665070573386; Thu, 06 Oct 2022 08:36:13 -0700 (PDT) MIME-Version: 1.0 References: <20221006082735.1321612-1-keescook@chromium.org> <20221006082735.1321612-2-keescook@chromium.org> <20221006090506.paqjf537cox7lqrq@wittgenstein> <86CE201B-5632-4BB7-BCF6-7CB2C2895409@chromium.org> In-Reply-To: <86CE201B-5632-4BB7-BCF6-7CB2C2895409@chromium.org> From: Jann Horn Date: Thu, 6 Oct 2022 17:35:37 +0200 Message-ID: Subject: Re: [PATCH 1/2] fs/exec: Explicitly unshare fs_struct on exec To: Kees Cook Cc: Christian Brauner , Eric Biederman , Jorge Merlino , Alexander Viro , Thomas Gleixner , Andy Lutomirski , Sebastian Andrzej Siewior , Andrew Morton , linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, John Johansen , Paul Moore , James Morris , "Serge E. Hallyn" , Stephen Smalley , Eric Paris , Richard Haines , Casey Schaufler , Xin Long , "David S. Miller" , Todd Kjos , Ondrej Mosnacek , Prashanth Prahlad , Micah Morton , Fenghua Yu , Andrei Vagin , linux-kernel@vger.kernel.org, apparmor@lists.ubuntu.com, linux-security-module@vger.kernel.org, selinux@vger.kernel.org, linux-hardening@vger.kernel.org Content-Type: text/plain; charset="UTF-8" ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1665070574; a=rsa-sha256; cv=none; b=VcoDpWu8m72gbappYijbyIJQbizHrr+CJicTHoFJLkrgXGvV7QHkDPipbQo6zC+CiqazwM e5fxiZuR3fkeXdP+2wsOSyi9P46gwEEWeTgHgYzxZJIo5ze7IpRmspYZa6UkhD2Nv0qk04 FJpQwK2oKZtntzgU7AifsLO2OMJt9Eo= ARC-Authentication-Results: i=1; imf04.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b="JjMbJ/6i"; spf=pass (imf04.hostedemail.com: domain of jannh@google.com designates 209.85.166.178 as permitted sender) smtp.mailfrom=jannh@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1665070574; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=kBsN+WV474YxARCF7+nqmiK1jJMhN3dpQirfp2PoxlI=; b=zM6u3PHcrZUMxleXpoGebM6EV6X/sZpfWj7oHYs+VxPPU4snPxZ9zffSx6DJZXHxcl21GF +eDgwtU24JvPzehxvgS7mKa0AI/s6yjzyd9OJ/0FqdlDeEj0Av7nvBT5zhBtSTebjlsAfE xjHg9ITezhwwIyEy6xuHWB3dfeQtJlk= X-Rspam-User: Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b="JjMbJ/6i"; spf=pass (imf04.hostedemail.com: domain of jannh@google.com designates 209.85.166.178 as permitted sender) smtp.mailfrom=jannh@google.com; dmarc=pass (policy=reject) header.from=google.com X-Stat-Signature: y6ahiimjsurbw674a3q5qmyijz63pyfg X-Rspamd-Queue-Id: 4B84B4001F X-Rspamd-Server: rspam01 X-HE-Tag: 1665070574-774596 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, Oct 6, 2022 at 5:25 PM Kees Cook wrote: > On October 6, 2022 7:13:37 AM PDT, Jann Horn wrote: > >On Thu, Oct 6, 2022 at 11:05 AM Christian Brauner wrote: > >> On Thu, Oct 06, 2022 at 01:27:34AM -0700, Kees Cook wrote: > >> > The check_unsafe_exec() counting of n_fs would not add up under a heavily > >> > threaded process trying to perform a suid exec, causing the suid portion > >> > to fail. This counting error appears to be unneeded, but to catch any > >> > possible conditions, explicitly unshare fs_struct on exec, if it ends up > >> > >> Isn't this a potential uapi break? Afaict, before this change a call to > >> clone{3}(CLONE_FS) followed by an exec in the child would have the > >> parent and child share fs information. So if the child e.g., changes the > >> working directory post exec it would also affect the parent. But after > >> this change here this would no longer be true. So a child changing a > >> workding directoro would not affect the parent anymore. IOW, an exec is > >> accompanied by an unshare(CLONE_FS). Might still be worth trying ofc but > >> it seems like a non-trivial uapi change but there might be few users > >> that do clone{3}(CLONE_FS) followed by an exec. > > > >I believe the following code in Chromium explicitly relies on this > >behavior, but I'm not sure whether this code is in active use anymore: > > > >https://source.chromium.org/chromium/chromium/src/+/main:sandbox/linux/suid/sandbox.c;l=101?q=CLONE_FS&sq=&ss=chromium > > Oh yes. I think I had tried to forget this existed. Ugh. Okay, so back to the drawing board, I guess. The counting will need to be fixed... > > It's possible we can move the counting after dethread -- it seems the early count was just to avoid setting flags after the point of no return, but it's not an error condition... Random idea that I haven't thought about a lot: One approach might be to not do it by counting, but instead have a flag on the fs_struct that we set when someone does a clone() with CLONE_FS but without CLONE_THREAD? Then we'd end up with the following possible states for fs_struct: - single-process, normal - single-process, pending execve past check_unsafe_exec() (prevent concurrent CLONE_FS) - shared between processes The slight difference from the old semantics would be that once you've used CLONE_FS without CLONE_THREAD, you can never do setuid execve() from your current process again (without calling unshare()), even if the child disappears in the meantime. I think that might be an acceptably small UAPI break.