Re: [PATCH] exec: Check __FMODE_EXEC instead of in_execve for LSMs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxx> 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






[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux