Message ID | 20240124192228.work.788-kees@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-37592-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:2553:b0:103:945f:af90 with SMTP id p19csp1205926dyi; Wed, 24 Jan 2024 11:23:00 -0800 (PST) X-Google-Smtp-Source: AGHT+IEEOqQjQ5knqh1DI1vf0tELEtB0K94+FbDhuTLPs8mXxaG1yAFv/fh3eN3ea/yn5ceZCcyS X-Received: by 2002:ad4:5e8f:0:b0:686:3587:4fa6 with SMTP id jl15-20020ad45e8f000000b0068635874fa6mr3368622qvb.29.1706124180729; Wed, 24 Jan 2024 11:23:00 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1706124180; cv=pass; d=google.com; s=arc-20160816; b=Ec1LhCXF9eX2Brdn/yHk0yp0BMvLjcugciTNIlRmSbhVxZT8VuaLgK9c26Ab77daFs 0DFiCdbk91U9/wUWDmKF0BtaMWihaJF3amSpBvDUfgOgC2fKkcmF5AHjc6yzQFpZqp2P N44tTyysVofzdZtTjtmUXBwD0mMo6ZztcRfm8orYayhktG1pqzyTtHZAWWc5uYJaHouT ztGH2yIkD7m1jyW9ar+4WPLNhFPBgltShwsqK6ptVWBxHFNLuzGAIhoXWP1OQyO8GtXR M5XRG5lwizR0XFLhZoNyzbdnMGdsWMGLupUjm5UYcIDeOYBG5yYbSsAY+Wd5z/JbZLP3 x18g== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:message-id:date:subject:cc:to :from:dkim-signature; bh=zmd6L4iLZNNyzU97sSrER5LIO6nwo17DPDhcgnpVxag=; fh=1CGs7ODj2Cqt8wAvE2bg7OQ+l64vkV4mTzLSRjWJcFk=; b=uxpd7dy9Wiz7bFiP/otWPNNXFngQl1z7bYirmG3GXIj9tRNJuSJW8NGRTZsLB9ytN3 sfrgEutk98oWBVIdApkx/n7xuh5tkk9SVczsrQR/0DbxUrRi/LH2S61+5MdiOanMHWWL 7moyLYXBXfwIiLmMhRlQgaQ0QC9AsWayK13QHEPSShChkaH0J27lCPXxJmzfEwGnQVZs xExTr6Snjvz7ufm5zdcmg5tjUSRMmnE3fEkI4uq75tGEyqrybeZIwXyfc4mi3GaPxHR7 +QDgGwaiePqKOXgPxEymw1cJ7Ui0y27/OoPZkb2hprZ/pq6BVRy+jyF5fHg4jUu79Mkz b6OQ== ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=OhyMlP9w; arc=pass (i=1 spf=pass spfdomain=chromium.org dkim=pass dkdomain=chromium.org dmarc=pass fromdomain=chromium.org); spf=pass (google.com: domain of linux-kernel+bounces-37592-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-37592-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: from ny.mirrors.kernel.org (ny.mirrors.kernel.org. [2604:1380:45d1:ec00::1]) by mx.google.com with ESMTPS id s22-20020a0cb316000000b0068530a114f2si11388854qve.86.2024.01.24.11.23.00 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 24 Jan 2024 11:23:00 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-37592-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1; Authentication-Results: mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=OhyMlP9w; arc=pass (i=1 spf=pass spfdomain=chromium.org dkim=pass dkdomain=chromium.org dmarc=pass fromdomain=chromium.org); spf=pass (google.com: domain of linux-kernel+bounces-37592-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-37592-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ny.mirrors.kernel.org (Postfix) with ESMTPS id 645FA1C2088B for <ouuuleilei@gmail.com>; Wed, 24 Jan 2024 19:23:00 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id C7F23132C26; Wed, 24 Jan 2024 19:22:43 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="OhyMlP9w" Received: from mail-pl1-f176.google.com (mail-pl1-f176.google.com [209.85.214.176]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id D0728131E5E for <linux-kernel@vger.kernel.org>; Wed, 24 Jan 2024 19:22:38 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.176 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706124160; cv=none; b=L+ACdYVmR4iskn5wx3SrHaA29XQYQwP/rNpHEyHIrZDoz8kvdf9ot58LbXSAEKEtXoTd7v6vC0Z249kftFqYEInV4z15I03MCmsL6CimxriOfg1g/k43SrGWD+N0q5tCrc7WUJVqc1qqM1ryZn6J6MoI5GGdExYw4YcOEmUKDbQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706124160; c=relaxed/simple; bh=1gaILVY/H6stGS/EvSXd6ZL/n9yg9wEbY6RbWPaWb/g=; h=From:To:Cc:Subject:Date:Message-Id:MIME-Version; b=iJ1wsFaourZUnnFXePule+hEPy3i1L6lz7rfug96ZE4o0bhpEUG9vk/sZMbKBxecIInrRfBfH3TfwEnabPgvaWHVBGf7yPqa3FK/s5AfSfAc3SVVCel/oy2bh5NfmcXSxlzpSPbg/qmps/t3RvHx29pJ4yCnaC4ebNiI9SNag6k= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=chromium.org; spf=pass smtp.mailfrom=chromium.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b=OhyMlP9w; arc=none smtp.client-ip=209.85.214.176 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=chromium.org Received: by mail-pl1-f176.google.com with SMTP id d9443c01a7336-1d751bc0c15so33230155ad.2 for <linux-kernel@vger.kernel.org>; Wed, 24 Jan 2024 11:22:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1706124158; x=1706728958; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=zmd6L4iLZNNyzU97sSrER5LIO6nwo17DPDhcgnpVxag=; b=OhyMlP9wfqzkEkBZYSaSnI/a/DVDGNLpoHCyyRrsjXhwi4U37k1/oTrW3+GX2KLXCi 5W32pHPP2ce6ilKavafL9TLoF//1tL2bxGial5zq9d6+65tbHWCNkpRzpc4pfbGzYyvg oScuvJnw7B2RGvYcZcoLde4foNod+djOxgZqU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1706124158; x=1706728958; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=zmd6L4iLZNNyzU97sSrER5LIO6nwo17DPDhcgnpVxag=; b=fsDBBEdhKL3UH9vlQSeuvBM51Emxl4vd2oR6AXM48IsvSTtQZ7OS9IqogstWThF/WI 0hjJ1N1lhsEW+xK94fjg61cVflWmhfPln7C4NcrVPcrUeq7ZLIR96kCeP/UpBFvpzwfm Emrwny4cEoHI+C26fCaCNlvxz/5qu8Z2GMJGtTELKTJry+lQa7ntQ+Cny46tXHJ9WXSy JpQVPEA4zdtQY3Q5aRHX1YgrHYxp4mVcB0bj+jSrOqIGKXr5NORW211riJZYRZESlQQn y0YGtF6A/Z+MBGE2CQ9SIz67wf75wuMbCHlDKs88lz5KUqCkLAdqSYaUHBtaGdf0cZP2 1XSg== X-Gm-Message-State: AOJu0YyVJvldjqwOCzolKquNaDyb0M8/8KHj7MDmEpzh9XCFa30Sgh3q nkmmNDaT+F5C5t4X72caolK7ybVTqDt9Jxc1Z3rzYehyKq2FQg5lk2O8/eQTAg== X-Received: by 2002:a17:902:6949:b0:1d4:b50d:dba9 with SMTP id k9-20020a170902694900b001d4b50ddba9mr1254455plt.71.1706124158168; Wed, 24 Jan 2024 11:22:38 -0800 (PST) Received: from www.outflux.net ([198.0.35.241]) by smtp.gmail.com with ESMTPSA id s21-20020a17090330d500b001d6fbaaeb56sm8636308plc.145.2024.01.24.11.22.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 24 Jan 2024 11:22:37 -0800 (PST) From: Kees Cook <keescook@chromium.org> To: Josh Triplett <josh@joshtriplett.org>, Kevin Locke <kevin@kevinlocke.name> Cc: Kees Cook <keescook@chromium.org>, Linus Torvalds <torvalds@linux-foundation.org>, John Johansen <john.johansen@canonical.com>, Paul Moore <paul@paul-moore.com>, James Morris <jmorris@namei.org>, "Serge E. Hallyn" <serge@hallyn.com>, Kentaro Takeda <takedakn@nttdata.co.jp>, Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>, Alexander Viro <viro@zeniv.linux.org.uk>, Christian Brauner <brauner@kernel.org>, Jan Kara <jack@suse.cz>, Eric Biederman <ebiederm@xmission.com>, Andrew Morton <akpm@linux-foundation.org>, Sebastian Andrzej Siewior <bigeasy@linutronix.de>, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, apparmor@lists.ubuntu.com, linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org Subject: [PATCH] exec: Check __FMODE_EXEC instead of in_execve for LSMs Date: Wed, 24 Jan 2024 11:22:32 -0800 Message-Id: <20240124192228.work.788-kees@kernel.org> X-Mailer: git-send-email 2.34.1 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=2841; i=keescook@chromium.org; h=from:subject:message-id; bh=1gaILVY/H6stGS/EvSXd6ZL/n9yg9wEbY6RbWPaWb/g=; b=owEBbQKS/ZANAwAKAYly9N/cbcAmAcsmYgBlsWN4cgMm3ifa4AxYv0rR1P9nb2T7XG8BeE5dh dQdKGdd8U+JAjMEAAEKAB0WIQSlw/aPIp3WD3I+bhOJcvTf3G3AJgUCZbFjeAAKCRCJcvTf3G3A Jj8eEACdqWJsXSjCuU2ZGkwBhmHssL73vpbJm9vow7VcgTvUcgVoF7WjPAqU3SkkUee2vuDuEEC uG3G42UgwdGwzUcascCCo7wkaex/Ac7gDV7BRBywIyjF/vLDCeQ5HhIqatIu/mH44Ebni5aTAQU hUYp/I3213FNl/oN3a+PJoqHHB5ORr/0z+NOMW3XT/pJ5DU0fMqAxTHtrF2s/IE9WWreJ4PP9dD X5FSgLuUtR0LuJ8/8gRd5EFCycwwUXuvOBcR9Nt4fBBQcU02uF3kcXzm4eF9JJyib+jYDu2tNP3 eouwdnVwePLu9Xbr2l2lFju9lrgM9QjR7aLXB4J6Bw1nlyZHuj6Hjrc+EYzI1fpVDfrBSTQtFfj Skv/zZriwMgLINYrOds5qiWwNPLT7pTEychRittwryFFuoqSyJwLS/5PkycmxUNlwqaQl4NU5S5 aP++f2Hfr2S7lrzFV1waa0HOn4J2drH3GfOqw+oPKvq/DxMlScVwY76Uvpncoov/alR5EnKKzn4 Pr66BX8S4pcrokYTbSm4BLat0ulOpUJgRCL1ixDnhwT4ftGIkEFBz8KiIYbfwT/bZCe9VHxD8/s bpXJ4Z6N1DuM1EnVnoMQy4GyH63CVXzi2u+ZM1eMNszMiKfYeeWkpZ+ljiyoNyt9iy/mx7u3pPW 6+jc5CV pbke/Mtw== X-Developer-Key: i=keescook@chromium.org; a=openpgp; fpr=A5C3F68F229DD60F723E6E138972F4DFDC6DC026 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1789000868893792877 X-GMAIL-MSGID: 1789000868893792877 |
Series |
exec: Check __FMODE_EXEC instead of in_execve for LSMs
|
|
Commit Message
Kees Cook
Jan. 24, 2024, 7:22 p.m. UTC
After commit 978ffcbf00d8 ("execve: open the executable file before
doing anything else"), current->in_execve was no longer in sync with the
open(). This broke AppArmor and TOMOYO which depend on this flag to
distinguish "open" operations from being "exec" operations.
Instead of moving around in_execve, switch to using __FMODE_EXEC, which
is where the "is this an exec?" intent is stored. Note that TOMOYO still
uses in_execve around cred handling.
Reported-by: Kevin Locke <kevin@kevinlocke.name>
Closes: https://lore.kernel.org/all/ZbE4qn9_h14OqADK@kevinlocke.name
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Fixes: 978ffcbf00d8 ("execve: open the executable file before doing anything else")
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: John Johansen <john.johansen@canonical.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Kentaro Takeda <takedakn@nttdata.co.jp>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Jan Kara <jack@suse.cz>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-mm@kvack.org
Cc: apparmor@lists.ubuntu.com
Cc: linux-security-module@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
security/apparmor/lsm.c | 4 +++-
security/tomoyo/tomoyo.c | 3 ++-
2 files changed, 5 insertions(+), 2 deletions(-)
Comments
On Wed, 2024-01-24 at 11:22 -0800, Kees Cook wrote: > After commit 978ffcbf00d8 ("execve: open the executable file before > doing anything else"), current->in_execve was no longer in sync with the > open(). This broke AppArmor and TOMOYO which depend on this flag to > distinguish "open" operations from being "exec" operations. > > Instead of moving around in_execve, switch to using __FMODE_EXEC, which > is where the "is this an exec?" intent is stored. Note that TOMOYO still > uses in_execve around cred handling. It solves the AppArmor issue I was experiencing and I don't notice any other issues. Tested-by: Kevin Locke <kevin@kevinlocke.name> Thanks! Kevin
On Wed, Jan 24, 2024 at 12:39:38PM -0700, Kevin Locke wrote: > On Wed, 2024-01-24 at 11:22 -0800, Kees Cook wrote: > > After commit 978ffcbf00d8 ("execve: open the executable file before > > doing anything else"), current->in_execve was no longer in sync with the > > open(). This broke AppArmor and TOMOYO which depend on this flag to > > distinguish "open" operations from being "exec" operations. > > > > Instead of moving around in_execve, switch to using __FMODE_EXEC, which > > is where the "is this an exec?" intent is stored. Note that TOMOYO still > > uses in_execve around cred handling. > > It solves the AppArmor issue I was experiencing and I don't notice any > other issues. > > Tested-by: Kevin Locke <kevin@kevinlocke.name> Thanks! Sounds like Linus has taken the patch directly, and I'll send a follow-up PR with other clean-ups. -Kees
On Wed, Jan 24, 2024 at 8:22 PM Kees Cook <keescook@chromium.org> wrote: > After commit 978ffcbf00d8 ("execve: open the executable file before > doing anything else"), current->in_execve was no longer in sync with the > open(). This broke AppArmor and TOMOYO which depend on this flag to > distinguish "open" operations from being "exec" operations. > > Instead of moving around in_execve, switch to using __FMODE_EXEC, which > is where the "is this an exec?" intent is stored. Note that TOMOYO still > uses in_execve around cred handling. I think this is wrong. When CONFIG_USELIB is enabled, the uselib() syscall will open a file with __FMODE_EXEC but without going through execve(). From what I can tell, there are no bprm hooks on this path. I don't know if it _matters_ much, given that it'll only let you read/execute stuff from files with valid ELF headers, but still.
On Wed, Jan 24, 2024 at 08:58:55PM +0100, Jann Horn wrote: > On Wed, Jan 24, 2024 at 8:22 PM Kees Cook <keescook@chromium.org> wrote: > > After commit 978ffcbf00d8 ("execve: open the executable file before > > doing anything else"), current->in_execve was no longer in sync with the > > open(). This broke AppArmor and TOMOYO which depend on this flag to > > distinguish "open" operations from being "exec" operations. > > > > Instead of moving around in_execve, switch to using __FMODE_EXEC, which > > is where the "is this an exec?" intent is stored. Note that TOMOYO still > > uses in_execve around cred handling. > > I think this is wrong. When CONFIG_USELIB is enabled, the uselib() > syscall will open a file with __FMODE_EXEC but without going through > execve(). From what I can tell, there are no bprm hooks on this path. Hrm, that's true. We've been trying to remove uselib for at least 10 years[1]. :( > I don't know if it _matters_ much, given that it'll only let you > read/execute stuff from files with valid ELF headers, but still. Hmpf, and frustratingly Ubuntu (and Debian) still builds with CONFIG_USELIB, even though it was reported[2] to them almost 4 years ago. -Kees [1] https://lore.kernel.org/lkml/20140221181103.GA5773@jtriplet-mobl1/ [2] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1879454
On Wed, 24 Jan 2024 at 12:15, Kees Cook <keescook@chromium.org> wrote: > > Hmpf, and frustratingly Ubuntu (and Debian) still builds with > CONFIG_USELIB, even though it was reported[2] to them almost 4 years ago. Well, we could just remove the __FMODE_EXEC from uselib. It's kind of wrong anyway. Unlike a real execve(), where the target executable actually takes control and you can't actually control it (except with ptrace, of course), 'uselib()' really is just a wrapper around a special mmap. And you can see it in the "acc_mode" flags: uselib already requires MAY_READ for that reason. So you cannot uselib() a non-readable file, unlike execve(). So I think just removing __FMODE_EXEC would just do the RightThing(tm), and changes nothing for any sane situation. In fact, I don't think __FMODE_EXEC really ever did anything for the uselib() case, so removing it *really* shouldn't matter, and only fix the new AppArmor / Tomoyo use. Of course, as you say, not having CONFIG_USELIB enabled at all is the _truly_ sane thing, but the only thing that used the FMODE_EXEC bit were landlock and some special-case nfs stuff. And at least the nfs stuff was about "don't require read permissions for exec", which was already wrong for the uselib() case as per above. So I think the simple oneliner is literally just --- a/fs/exec.c +++ b/fs/exec.c @@ -128,7 +128,7 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) struct filename *tmp = getname(library); int error = PTR_ERR(tmp); static const struct open_flags uselib_flags = { - .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC, + .open_flag = O_LARGEFILE | O_RDONLY, .acc_mode = MAY_READ | MAY_EXEC, .intent = LOOKUP_OPEN, .lookup_flags = LOOKUP_FOLLOW, but I obviously have nothing that uses uselib(). I don't see how it really *could* break anything, though, exactly because of that .acc_mode = MAY_READ | MAY_EXEC, that means that the *regular* permission checks already require the file to be readable. Never mind any LSM checks that might be confused. Linus
On Wed, Jan 24, 2024 at 9:47 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Wed, 24 Jan 2024 at 12:15, Kees Cook <keescook@chromium.org> wrote: > > > > Hmpf, and frustratingly Ubuntu (and Debian) still builds with > > CONFIG_USELIB, even though it was reported[2] to them almost 4 years ago. > > Well, we could just remove the __FMODE_EXEC from uselib. > > It's kind of wrong anyway. > > Unlike a real execve(), where the target executable actually takes > control and you can't actually control it (except with ptrace, of > course), 'uselib()' really is just a wrapper around a special mmap. > > And you can see it in the "acc_mode" flags: uselib already requires > MAY_READ for that reason. So you cannot uselib() a non-readable file, > unlike execve(). > > So I think just removing __FMODE_EXEC would just do the > RightThing(tm), and changes nothing for any sane situation. Sounds like a good idea. That makes this codepath behave more as if userspace had done the same steps manually...
On Wed, Jan 24, 2024 at 12:47:34PM -0800, Linus Torvalds wrote: > On Wed, 24 Jan 2024 at 12:15, Kees Cook <keescook@chromium.org> wrote: > > > > Hmpf, and frustratingly Ubuntu (and Debian) still builds with > > CONFIG_USELIB, even though it was reported[2] to them almost 4 years ago. For completeness, Fedora hasn't had CONFIG_USELIB for a while now. > Well, we could just remove the __FMODE_EXEC from uselib. > > It's kind of wrong anyway. Yeah. > So I think just removing __FMODE_EXEC would just do the > RightThing(tm), and changes nothing for any sane situation. Agreed about these: - fs/fcntl.c is just doing a bitfield sanity check. - nfs_open_permission_mask(), as you say, is only checking for unreadable case. - fsnotify would also see uselib() as a read, but afaict, that's what it would see for an mmap(), so this should be functionally safe. This one, though, I need some more time to examine: - AppArmor, TOMOYO, and LandLock will see uselib() as an open-for-read, so that might still be a problem? As you say, it's more of a mmap() call, but that would mean adding something a call like security_mmap_file() into uselib()... The issue isn't an insane "support uselib() under AppArmor" case, but rather "Can uselib() be used to bypass exec/mmap checks?" This totally untested patch might give appropriate coverage: diff --git a/fs/exec.c b/fs/exec.c index d179abb78a1c..0c9265312c8d 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -143,6 +143,10 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) if (IS_ERR(file)) goto out; + error = security_mmap_file(file, PROT_READ | PROT_EXEC, MAP_FIXED | MAP_SHARED); + if (error) + goto exit; + /* * may_open() has already checked for this, so it should be * impossible to trip now. But we need to be extra cautious > Of course, as you say, not having CONFIG_USELIB enabled at all is the > _truly_ sane thing, but the only thing that used the FMODE_EXEC bit > were landlock and some special-case nfs stuff. Do we want to attempt deprecation again? This was suggested last time: https://lore.kernel.org/lkml/20200518130251.zih2s32q2rxhxg6f@wittgenstein/ -Kees
On Wed, Jan 24, 2024 at 01:32:02PM -0800, Kees Cook wrote: > On Wed, Jan 24, 2024 at 12:47:34PM -0800, Linus Torvalds wrote: > > On Wed, 24 Jan 2024 at 12:15, Kees Cook <keescook@chromium.org> wrote: > > > > > > Hmpf, and frustratingly Ubuntu (and Debian) still builds with > > > CONFIG_USELIB, even though it was reported[2] to them almost 4 years ago. > > For completeness, Fedora hasn't had CONFIG_USELIB for a while now. > > > Well, we could just remove the __FMODE_EXEC from uselib. > > > > It's kind of wrong anyway. > > Yeah. > > > So I think just removing __FMODE_EXEC would just do the > > RightThing(tm), and changes nothing for any sane situation. > > Agreed about these: > > - fs/fcntl.c is just doing a bitfield sanity check. > > - nfs_open_permission_mask(), as you say, is only checking for > unreadable case. > > - fsnotify would also see uselib() as a read, but afaict, > that's what it would see for an mmap(), so this should > be functionally safe. > > This one, though, I need some more time to examine: > > - AppArmor, TOMOYO, and LandLock will see uselib() as an > open-for-read, so that might still be a problem? As you > say, it's more of a mmap() call, but that would mean > adding something a call like security_mmap_file() into > uselib()... > > The issue isn't an insane "support uselib() under AppArmor" case, but > rather "Can uselib() be used to bypass exec/mmap checks?" > > This totally untested patch might give appropriate coverage: > > diff --git a/fs/exec.c b/fs/exec.c > index d179abb78a1c..0c9265312c8d 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -143,6 +143,10 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) > if (IS_ERR(file)) > goto out; > > + error = security_mmap_file(file, PROT_READ | PROT_EXEC, MAP_FIXED | MAP_SHARED); Actually, this should probably match was load_shlib() uses: PROT_READ | PROT_WRITE | PROT_EXEC, MAP_FIXED_NOREPLACE | MAP_PRIVATE,
On Wed, Jan 24, 2024 at 10:32 PM Kees Cook <keescook@chromium.org> wrote: > > On Wed, Jan 24, 2024 at 12:47:34PM -0800, Linus Torvalds wrote: > > On Wed, 24 Jan 2024 at 12:15, Kees Cook <keescook@chromium.org> wrote: > > > > > > Hmpf, and frustratingly Ubuntu (and Debian) still builds with > > > CONFIG_USELIB, even though it was reported[2] to them almost 4 years ago. > > For completeness, Fedora hasn't had CONFIG_USELIB for a while now. > > > Well, we could just remove the __FMODE_EXEC from uselib. > > > > It's kind of wrong anyway. > > Yeah. > > > So I think just removing __FMODE_EXEC would just do the > > RightThing(tm), and changes nothing for any sane situation. > > Agreed about these: > > - fs/fcntl.c is just doing a bitfield sanity check. > > - nfs_open_permission_mask(), as you say, is only checking for > unreadable case. > > - fsnotify would also see uselib() as a read, but afaict, > that's what it would see for an mmap(), so this should > be functionally safe. > > This one, though, I need some more time to examine: > > - AppArmor, TOMOYO, and LandLock will see uselib() as an > open-for-read, so that might still be a problem? As you > say, it's more of a mmap() call, but that would mean > adding something a call like security_mmap_file() into > uselib()... > > The issue isn't an insane "support uselib() under AppArmor" case, but > rather "Can uselib() be used to bypass exec/mmap checks?" > > This totally untested patch might give appropriate coverage: > > diff --git a/fs/exec.c b/fs/exec.c > index d179abb78a1c..0c9265312c8d 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -143,6 +143,10 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) > if (IS_ERR(file)) > goto out; > > + error = security_mmap_file(file, PROT_READ | PROT_EXEC, MAP_FIXED | MAP_SHARED); > + if (error) > + goto exit; Call path from here is: sys_uselib -> load_elf_library -> elf_load -> elf_map -> vm_mmap -> vm_mmap_pgoff Call path for normal mmap is: sys_mmap_pgoff -> ksys_mmap_pgoff -> vm_mmap_pgoff So I think the call paths converge before any real security checks happen, and the check you're suggesting should be superfluous. (There is some weird audit call in ksys_mmap_pgoff() but that's just to record the FD number, so I guess that doesn't matter.)
On Wed, Jan 24, 2024 at 10:40:49PM +0100, Jann Horn wrote: > On Wed, Jan 24, 2024 at 10:32 PM Kees Cook <keescook@chromium.org> wrote: > > > > On Wed, Jan 24, 2024 at 12:47:34PM -0800, Linus Torvalds wrote: > > > On Wed, 24 Jan 2024 at 12:15, Kees Cook <keescook@chromium.org> wrote: > > > > > > > > Hmpf, and frustratingly Ubuntu (and Debian) still builds with > > > > CONFIG_USELIB, even though it was reported[2] to them almost 4 years ago. > > > > For completeness, Fedora hasn't had CONFIG_USELIB for a while now. > > > > > Well, we could just remove the __FMODE_EXEC from uselib. > > > > > > It's kind of wrong anyway. > > > > Yeah. > > > > > So I think just removing __FMODE_EXEC would just do the > > > RightThing(tm), and changes nothing for any sane situation. > > > > Agreed about these: > > > > - fs/fcntl.c is just doing a bitfield sanity check. > > > > - nfs_open_permission_mask(), as you say, is only checking for > > unreadable case. > > > > - fsnotify would also see uselib() as a read, but afaict, > > that's what it would see for an mmap(), so this should > > be functionally safe. > > > > This one, though, I need some more time to examine: > > > > - AppArmor, TOMOYO, and LandLock will see uselib() as an > > open-for-read, so that might still be a problem? As you > > say, it's more of a mmap() call, but that would mean > > adding something a call like security_mmap_file() into > > uselib()... > > > > The issue isn't an insane "support uselib() under AppArmor" case, but > > rather "Can uselib() be used to bypass exec/mmap checks?" > > > > This totally untested patch might give appropriate coverage: > > > > diff --git a/fs/exec.c b/fs/exec.c > > index d179abb78a1c..0c9265312c8d 100644 > > --- a/fs/exec.c > > +++ b/fs/exec.c > > @@ -143,6 +143,10 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) > > if (IS_ERR(file)) > > goto out; > > > > + error = security_mmap_file(file, PROT_READ | PROT_EXEC, MAP_FIXED | MAP_SHARED); > > + if (error) > > + goto exit; > > Call path from here is: > > sys_uselib -> load_elf_library -> elf_load -> elf_map -> vm_mmap -> > vm_mmap_pgoff > > Call path for normal mmap is: > > sys_mmap_pgoff -> ksys_mmap_pgoff -> vm_mmap_pgoff > > So I think the call paths converge before any real security checks > happen, and the check you're suggesting should be superfluous. (There > is some weird audit call in ksys_mmap_pgoff() but that's just to > record the FD number, so I guess that doesn't matter.) Yeah, I was just noticing this. I was over thinking. :) It does look like all that is needed is to remove __FMODE_EXEC.
On 1/25/24 08:38, Mickaël Salaün wrote: > On Wed, Jan 24, 2024 at 01:32:02PM -0800, Kees Cook wrote: >> On Wed, Jan 24, 2024 at 12:47:34PM -0800, Linus Torvalds wrote: >>> On Wed, 24 Jan 2024 at 12:15, Kees Cook <keescook@chromium.org> wrote: >>>> >>>> Hmpf, and frustratingly Ubuntu (and Debian) still builds with >>>> CONFIG_USELIB, even though it was reported[2] to them almost 4 years ago. >> >> For completeness, Fedora hasn't had CONFIG_USELIB for a while now. >> >>> Well, we could just remove the __FMODE_EXEC from uselib. >>> >>> It's kind of wrong anyway. >> >> Yeah. >> >>> So I think just removing __FMODE_EXEC would just do the >>> RightThing(tm), and changes nothing for any sane situation. >> >> Agreed about these: >> >> - fs/fcntl.c is just doing a bitfield sanity check. >> >> - nfs_open_permission_mask(), as you say, is only checking for >> unreadable case. >> >> - fsnotify would also see uselib() as a read, but afaict, >> that's what it would see for an mmap(), so this should >> be functionally safe. >> >> This one, though, I need some more time to examine: >> >> - AppArmor, TOMOYO, and LandLock will see uselib() as an >> open-for-read, so that might still be a problem? As you >> say, it's more of a mmap() call, but that would mean >> adding something a call like security_mmap_file() into >> uselib()... > > If user space can emulate uselib() without opening a file with > __FMODE_EXEC, then there is no security reason to keep __FMODE_EXEC for > uselib(). > agreed > Removing __FMODE_EXEC from uselib() looks OK for Landlock. We use > __FMODE_EXEC to infer if a file is being open for execution i.e., by > execve(2). > apparmor the hint should be to avoid doing permission work again that we are doing in exec. That it regressed anything more than performance here is a bug, that will get fixed. > If __FMODE_EXEC is removed from uselib(), I think it should also be > backported to all stable kernels for consistency though. > hrmmm, I am not opposed to it being backported but I don't know that it should be backported. Consistency is good but its not a serious bug fix either > >> >> The issue isn't an insane "support uselib() under AppArmor" case, but >> rather "Can uselib() be used to bypass exec/mmap checks?" >> >> This totally untested patch might give appropriate coverage: >> >> diff --git a/fs/exec.c b/fs/exec.c >> index d179abb78a1c..0c9265312c8d 100644 >> --- a/fs/exec.c >> +++ b/fs/exec.c >> @@ -143,6 +143,10 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) >> if (IS_ERR(file)) >> goto out; >> >> + error = security_mmap_file(file, PROT_READ | PROT_EXEC, MAP_FIXED | MAP_SHARED); >> + if (error) >> + goto exit; >> + >> /* >> * may_open() has already checked for this, so it should be >> * impossible to trip now. But we need to be extra cautious >> >>> Of course, as you say, not having CONFIG_USELIB enabled at all is the >>> _truly_ sane thing, but the only thing that used the FMODE_EXEC bit >>> were landlock and some special-case nfs stuff. >> >> Do we want to attempt deprecation again? This was suggested last time: >> https://lore.kernel.org/lkml/20200518130251.zih2s32q2rxhxg6f@wittgenstein/ >> >> -Kees >> >> -- >> Kees Cook >>
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 7717354ce095..98e1150bee9d 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -469,8 +469,10 @@ static int apparmor_file_open(struct file *file) * Cache permissions granted by the previous exec check, with * implicit read and executable mmap which are required to * actually execute the image. + * + * Illogically, FMODE_EXEC is in f_flags, not f_mode. */ - if (current->in_execve) { + if (file->f_flags & __FMODE_EXEC) { fctx->allow = MAY_EXEC | MAY_READ | AA_EXEC_MMAP; return 0; } diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c index 3c3af149bf1c..04a92c3d65d4 100644 --- a/security/tomoyo/tomoyo.c +++ b/security/tomoyo/tomoyo.c @@ -328,7 +328,8 @@ static int tomoyo_file_fcntl(struct file *file, unsigned int cmd, static int tomoyo_file_open(struct file *f) { /* Don't check read permission here if called from execve(). */ - if (current->in_execve) + /* Illogically, FMODE_EXEC is in f_flags, not f_mode. */ + if (f->f_flags & __FMODE_EXEC) return 0; return tomoyo_check_open_permission(tomoyo_domain(), &f->f_path, f->f_flags);