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
