On 2024/01/27 16:04, Tetsuo Handa wrote:
> If we can accept revival of security_bprm_free(), we can "get rid of current->in_execve flag"
> and "stop saving things across two *independent* execve() calls".
Oops, I found a bug in TOMOYO (and possibly in AppArmor as well).
TOMOYO has to continue depending on current->in_execve flag even if
security_bprm_free() is revived.
v6.7 and current linux.git check the following permissions.
----------------------------------------
<kernel> /usr/lib/systemd/systemd /etc/mail/make /usr/bin/grep
0: file getattr /etc/ld.so.cache
1: file getattr /etc/mail/sendmail.cf
2: file getattr /usr/lib/locale/locale-archive
3: file getattr /usr/lib64/gconv/gconv-modules.cache
4: file getattr /usr/lib64/libc-2.17.so
5: file getattr /usr/lib64/libpcre.so.1.2.0
6: file getattr /usr/lib64/libpthread-2.17.so
7: file getattr pipe:[24810]
8: file ioctl /etc/mail/sendmail.cf 0x5401
9: file ioctl pipe:[24810] 0x5401
10: file read /etc/ld.so.cache
11: file read /etc/mail/sendmail.cf
12: file read /usr/lib/locale/locale-archive
13: file read /usr/lib64/gconv/gconv-modules.cache
14: file read /usr/lib64/libc-2.17.so
15: file read /usr/lib64/libpcre.so.1.2.0
16: file read /usr/lib64/libpthread-2.17.so
17: misc env LANG
18: misc env OLDPWD
19: misc env PATH
20: misc env PWD
21: misc env SENDMAIL_OPTS
22: misc env SHLVL
23: misc env _
24: use_group 0
----------------------------------------
But due to "if (f->f_flags & __FMODE_EXEC)" test in current linux.git (or
"if (current->in_execve)" test until v6.7), currently permission for the ELF
loader file (i.e. /lib64/ld-linux-x86-64.so.2 shown below) is not checked.
$ ldd /usr/bin/grep
linux-vdso.so.1 => (0x00007ffc079ac000)
libpcre.so.1 => /lib64/libpcre.so.1 (0x00007fdcfb000000)
libc.so.6 => /lib64/libc.so.6 (0x00007fdcfac00000)
libpthread.so.0 => /lib64/libpthread.so.0 (0x00007fdcfa800000)
/lib64/ld-linux-x86-64.so.2 (0x00007fdcfb400000)
If I make below change
----------------------------------------
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 04a92c3d65d4..34739e4ba5a4 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -329,8 +329,8 @@ static int tomoyo_file_open(struct file *f)
{
/* Don't check read permission here if called from execve(). */
/* Illogically, FMODE_EXEC is in f_flags, not f_mode. */
- if (f->f_flags & __FMODE_EXEC)
- return 0;
+ //if (f->f_flags & __FMODE_EXEC)
+ // return 0;
return tomoyo_check_open_permission(tomoyo_domain(), &f->f_path,
f->f_flags);
}
----------------------------------------
permission for the ELF loader file is checked, but causes the caller to
require read permission for /usr/bin/grep in addition to execute permission.
----------------------------------------
<kernel> /usr/lib/systemd/systemd /etc/mail/make /usr/bin/grep
0: file getattr /etc/ld.so.cache
1: file getattr /etc/mail/sendmail.cf
2: file getattr /usr/lib/locale/locale-archive
3: file getattr /usr/lib64/gconv/gconv-modules.cache
4: file getattr /usr/lib64/libc-2.17.so
5: file getattr /usr/lib64/libpcre.so.1.2.0
6: file getattr /usr/lib64/libpthread-2.17.so
7: file getattr pipe:[22370]
8: file ioctl /etc/mail/sendmail.cf 0x5401
9: file ioctl pipe:[22370] 0x5401
10: file read /etc/ld.so.cache
11: file read /etc/mail/sendmail.cf
12: file read /usr/lib/locale/locale-archive
13: file read /usr/lib64/gconv/gconv-modules.cache
14: file read /usr/lib64/ld-2.17.so
15: file read /usr/lib64/libc-2.17.so
16: file read /usr/lib64/libpcre.so.1.2.0
17: file read /usr/lib64/libpthread-2.17.so
18: misc env LANG
19: misc env OLDPWD
20: misc env PATH
21: misc env PWD
22: misc env SENDMAIL_OPTS
23: misc env SHLVL
24: misc env _
25: use_group 0
----------------------------------------
To make it possible to check permission for the ELF loader file without requiring
the caller of execve() read permission of a program specified in execve() request,
I need to
----------------------------------------
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 04a92c3d65d4..942c08a36027 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -104,11 +104,6 @@ static int tomoyo_bprm_check_security(struct linux_binprm *bprm)
tomoyo_read_unlock(idx);
return err;
}
- /*
- * Read permission is checked against interpreters using next domain.
- */
- return tomoyo_check_open_permission(s->domain_info,
- &bprm->file->f_path, O_RDONLY);
}
/**
@@ -327,9 +322,13 @@ 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(). */
- /* Illogically, FMODE_EXEC is in f_flags, not f_mode. */
- if (f->f_flags & __FMODE_EXEC)
+ /*
+ * Don't check read permission here if called from execve() for
+ * the first time of that execve() request, for execute permission
+ * will be checked at tomoyo_bprm_check_security() with argv/envp
+ * taken into account.
+ */
+ if (current->in_execve && !tomoyo_task(current)->old_domain_info)
return 0;
return tomoyo_check_open_permission(tomoyo_domain(), &f->f_path,
f->f_flags);
----------------------------------------
in addition to moving around in_execve and reviving security_bprm_free().
Apparmor uses similar approach, which is based on an assumption that
permission check is done by bprm hooks.
----------------------------------------
static int apparmor_file_open(struct file *file)
{
struct aa_file_ctx *fctx = file_ctx(file);
struct aa_label *label;
int error = 0;
if (!path_mediated_fs(file->f_path.dentry))
return 0;
/* If in exec, permission is handled by bprm hooks.
* 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 (file->f_flags & __FMODE_EXEC) {
fctx->allow = MAY_EXEC | MAY_READ | AA_EXEC_MMAP;
return 0;
}
---------------------------------------
https://lkml.kernel.org/r/4bb5dd09-9e09-477b-9ea8-d7b9d2fb4760@xxxxxxxxxxxxx :
> 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.
But I can't find apparmor_bprm_check_security() callback.
AppArmor uses apparmor_bprm_creds_for_exec() callback, but
security_bprm_creds_for_exec() is called for only once for each execve() request.
That is, apparmor_bprm_creds_for_exec() is not suitable for checking permission
for interpreter or ELF loader, is it?
https://lkml.kernel.org/r/ff9a525e-8c39-4590-9ace-57f4426cbe74@xxxxxxxxxxxxx :
> that this even tripped a regression is a bug that I am going to
> have to chase down. The file check at this point should just be
> redundant.
Then, how does AppArmor check permissions for files opened for interpreter or
ELF loader? AppArmor does not check permissions for files opened for interpreter
and ELF loader (i.e. accepting any malicious binary file being specified)?
