Re: [PATCH] procfs: block chmod on /proc/thread-self/comm

Re: [PATCH] procfs: block chmod on /proc/thread-self/comm

[Christian Brauner]

> > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > index 486334981e60..08f0969208eb 100644
> > --- a/tools/testing/selftests/nolibc/nolibc-test.c
> > +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> > @@ -580,6 +580,10 @@ int run_syscall(int min, int max)
> >  		CASE_TEST(chmod_net);         EXPECT_SYSZR(proc, chmod("/proc/self/net", 0555)); break;
> >  		CASE_TEST(chmod_self);        EXPECT_SYSER(proc, chmod("/proc/self", 0555), -1, EPERM); break;
> >  		CASE_TEST(chown_self);        EXPECT_SYSER(proc, chown("/proc/self", 0, 0), -1, EPERM); break;
> > +		CASE_TEST(chmod_self_comm);   EXPECT_SYSER(proc, chmod("/proc/self/comm", 0777), -1, EPERM); break;
> > +		CASE_TEST(chmod_tid_comm);    EXPECT_SYSER(proc, chmod("/proc/thread-self/comm", 0777), -1, EPERM); break;
> > +		CASE_TEST(chmod_self_environ);EXPECT_SYSER(proc, chmod("/proc/self/environ", 0777), -1, EPERM); break;
> > +		CASE_TEST(chmod_tid_environ); EXPECT_SYSER(proc, chmod("/proc/thread-self/environ", 0777), -1, EPERM); break;

> 
> I'm not a big fan of this, it abuses the nolibc testsuite to test core
> kernel functionality.

Yes, this should be dropped.
We need a minimal patch to fix this. This just makes backporting harder
and any test doesn't need to be backported.

Re: [PATCH] procfs: block chmod on /proc/thread-self/comm

[Aleksa Sarai]

On 2023-07-13, Christian Brauner <brauner@kernel.org> wrote:
> > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > > index 486334981e60..08f0969208eb 100644
> > > --- a/tools/testing/selftests/nolibc/nolibc-test.c
> > > +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> > > @@ -580,6 +580,10 @@ int run_syscall(int min, int max)
> > >  		CASE_TEST(chmod_net);         EXPECT_SYSZR(proc, chmod("/proc/self/net", 0555)); break;
> > >  		CASE_TEST(chmod_self);        EXPECT_SYSER(proc, chmod("/proc/self", 0555), -1, EPERM); break;
> > >  		CASE_TEST(chown_self);        EXPECT_SYSER(proc, chown("/proc/self", 0, 0), -1, EPERM); break;
> > > +		CASE_TEST(chmod_self_comm);   EXPECT_SYSER(proc, chmod("/proc/self/comm", 0777), -1, EPERM); break;
> > > +		CASE_TEST(chmod_tid_comm);    EXPECT_SYSER(proc, chmod("/proc/thread-self/comm", 0777), -1, EPERM); break;
> > > +		CASE_TEST(chmod_self_environ);EXPECT_SYSER(proc, chmod("/proc/self/environ", 0777), -1, EPERM); break;
> > > +		CASE_TEST(chmod_tid_environ); EXPECT_SYSER(proc, chmod("/proc/thread-self/environ", 0777), -1, EPERM); break;
> 
> > 
> > I'm not a big fan of this, it abuses the nolibc testsuite to test core
> > kernel functionality.
> 
> Yes, this should be dropped.
> We need a minimal patch to fix this. This just makes backporting harder
> and any test doesn't need to be backported.

Alright, I'll drop it in v2 (though I'm not sure why there are tests for
/proc/self and /proc/self/net then).

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

Re: [PATCH] procfs: block chmod on /proc/thread-self/comm

[Christian Brauner]

On Fri, Jul 14, 2023 at 12:00:51AM +1000, Aleksa Sarai wrote:
> On 2023-07-13, Christian Brauner <brauner@kernel.org> wrote:
> > > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > > > index 486334981e60..08f0969208eb 100644
> > > > --- a/tools/testing/selftests/nolibc/nolibc-test.c
> > > > +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> > > > @@ -580,6 +580,10 @@ int run_syscall(int min, int max)
> > > >  		CASE_TEST(chmod_net);         EXPECT_SYSZR(proc, chmod("/proc/self/net", 0555)); break;
> > > >  		CASE_TEST(chmod_self);        EXPECT_SYSER(proc, chmod("/proc/self", 0555), -1, EPERM); break;
> > > >  		CASE_TEST(chown_self);        EXPECT_SYSER(proc, chown("/proc/self", 0, 0), -1, EPERM); break;
> > > > +		CASE_TEST(chmod_self_comm);   EXPECT_SYSER(proc, chmod("/proc/self/comm", 0777), -1, EPERM); break;
> > > > +		CASE_TEST(chmod_tid_comm);    EXPECT_SYSER(proc, chmod("/proc/thread-self/comm", 0777), -1, EPERM); break;
> > > > +		CASE_TEST(chmod_self_environ);EXPECT_SYSER(proc, chmod("/proc/self/environ", 0777), -1, EPERM); break;
> > > > +		CASE_TEST(chmod_tid_environ); EXPECT_SYSER(proc, chmod("/proc/thread-self/environ", 0777), -1, EPERM); break;
> > 
> > > 
> > > I'm not a big fan of this, it abuses the nolibc testsuite to test core
> > > kernel functionality.
> > 
> > Yes, this should be dropped.
> > We need a minimal patch to fix this. This just makes backporting harder
> > and any test doesn't need to be backported.
> 
> Alright, I'll drop it in v2 (though I'm not sure why there are tests for
> /proc/self and /proc/self/net then).

If you wanted to add tests as a separate patch they should very likely
go into tools/testing/selftets/proc.

Re: [PATCH] procfs: block chmod on /proc/thread-self/comm

[Thomas Weißschuh]

On 2023-07-14 00:00:51+1000, Aleksa Sarai wrote:
> On 2023-07-13, Christian Brauner <brauner@kernel.org> wrote:
> > > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > > > index 486334981e60..08f0969208eb 100644
> > > > --- a/tools/testing/selftests/nolibc/nolibc-test.c
> > > > +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> > > > @@ -580,6 +580,10 @@ int run_syscall(int min, int max)
> > > >  		CASE_TEST(chmod_net);         EXPECT_SYSZR(proc, chmod("/proc/self/net", 0555)); break;
> > > >  		CASE_TEST(chmod_self);        EXPECT_SYSER(proc, chmod("/proc/self", 0555), -1, EPERM); break;
> > > >  		CASE_TEST(chown_self);        EXPECT_SYSER(proc, chown("/proc/self", 0, 0), -1, EPERM); break;
> > > > +		CASE_TEST(chmod_self_comm);   EXPECT_SYSER(proc, chmod("/proc/self/comm", 0777), -1, EPERM); break;
> > > > +		CASE_TEST(chmod_tid_comm);    EXPECT_SYSER(proc, chmod("/proc/thread-self/comm", 0777), -1, EPERM); break;
> > > > +		CASE_TEST(chmod_self_environ);EXPECT_SYSER(proc, chmod("/proc/self/environ", 0777), -1, EPERM); break;
> > > > +		CASE_TEST(chmod_tid_environ); EXPECT_SYSER(proc, chmod("/proc/thread-self/environ", 0777), -1, EPERM); break;
> > 
> > > 
> > > I'm not a big fan of this, it abuses the nolibc testsuite to test core
> > > kernel functionality.
> > 
> > Yes, this should be dropped.
> > We need a minimal patch to fix this. This just makes backporting harder
> > and any test doesn't need to be backported.
> 
> Alright, I'll drop it in v2 (though I'm not sure why there are tests for
> /proc/self and /proc/self/net then).

To test the functionality of the implementations of chown() and chmod()
in nolibc. procfs is used used as a test fixture to provide diverse file
and directories that are (nearly) always available.

The system under test is nolibc, not the kernel itself.

Thomas

Re: [PATCH] procfs: block chmod on /proc/thread-self/comm

[Willy Tarreau]

On Fri, Jul 14, 2023 at 12:00:51AM +1000, Aleksa Sarai wrote:
> On 2023-07-13, Christian Brauner <brauner@kernel.org> wrote:
> > > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > > > index 486334981e60..08f0969208eb 100644
> > > > --- a/tools/testing/selftests/nolibc/nolibc-test.c
> > > > +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> > > > @@ -580,6 +580,10 @@ int run_syscall(int min, int max)
> > > >  		CASE_TEST(chmod_net);         EXPECT_SYSZR(proc, chmod("/proc/self/net", 0555)); break;
> > > >  		CASE_TEST(chmod_self);        EXPECT_SYSER(proc, chmod("/proc/self", 0555), -1, EPERM); break;
> > > >  		CASE_TEST(chown_self);        EXPECT_SYSER(proc, chown("/proc/self", 0, 0), -1, EPERM); break;
> > > > +		CASE_TEST(chmod_self_comm);   EXPECT_SYSER(proc, chmod("/proc/self/comm", 0777), -1, EPERM); break;
> > > > +		CASE_TEST(chmod_tid_comm);    EXPECT_SYSER(proc, chmod("/proc/thread-self/comm", 0777), -1, EPERM); break;
> > > > +		CASE_TEST(chmod_self_environ);EXPECT_SYSER(proc, chmod("/proc/self/environ", 0777), -1, EPERM); break;
> > > > +		CASE_TEST(chmod_tid_environ); EXPECT_SYSER(proc, chmod("/proc/thread-self/environ", 0777), -1, EPERM); break;
> > 
> > > 
> > > I'm not a big fan of this, it abuses the nolibc testsuite to test core
> > > kernel functionality.
> > 
> > Yes, this should be dropped.
> > We need a minimal patch to fix this. This just makes backporting harder
> > and any test doesn't need to be backported.
> 
> Alright, I'll drop it in v2 (though I'm not sure why there are tests for
> /proc/self and /proc/self/net then).

In fact the goal was to rely on existing entries that were certain to
return certain errors, as we are testing nolibc syscalls in limited
environments, such as not being able to create a new file due to another
syscall not being available yet. /proc is convenient to make a number
of syscalls fail. That's how the problem was detected by the way :-)

I personally don't mind that much that tests would be added, provided
they really test a new syscall+error combination each. As Thomas said,
here we already have other tests for chmod+EPERM so these ones do not
bring value here for the purpose of this specific test.

With that in mind, if there is some perceived value in adding such
tests, that's something we could discuss, either in this file as another
category or (preferably) in a separate one, because the framework makes
this easy. We could for example have a "proc-test" sub-project forked
from this one to run various tests on /proc file permissions. This would
respect a clean split, with nolibc-test assuming a valid kernel to test
a libc, and proc-test assuming a valid libc to test the kernel. Just an
idea.

Regards,
willy

[PATCH] procfs: block chmod on /proc/thread-self/comm

[Aleksa Sarai]

Due to an oversight in commit 1b3044e39a89 ("procfs: fix pthread
cross-thread naming if !PR_DUMPABLE") in switching from REG to NOD,
chmod operations on /proc/thread-self/comm were no longer blocked as
they are on almost all other procfs files.

A very similar situation with /proc/self/environ was used to as a root
exploit a long time ago, but procfs has SB_I_NOEXEC so this is simply a
correctness issue.

Ref: https://lwn.net/Articles/191954/
Ref: 6d76fa58b050 ("Don't allow chmod() on the /proc/<pid>/ files")
Fixes: 1b3044e39a89 ("procfs: fix pthread cross-thread naming if !PR_DUMPABLE")
Cc: stable@vger.kernel.org # v4.7+
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
 fs/proc/base.c                               | 3 ++-
 tools/testing/selftests/nolibc/nolibc-test.c | 4 ++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 05452c3b9872..7394229816f3 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -3583,7 +3583,8 @@ static int proc_tid_comm_permission(struct mnt_idmap *idmap,
 }
 
 static const struct inode_operations proc_tid_comm_inode_operations = {
-		.permission = proc_tid_comm_permission,
+		.setattr	= proc_setattr,
+		.permission	= proc_tid_comm_permission,
 };
 
 /*
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index 486334981e60..08f0969208eb 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -580,6 +580,10 @@ int run_syscall(int min, int max)
 		CASE_TEST(chmod_net);         EXPECT_SYSZR(proc, chmod("/proc/self/net", 0555)); break;
 		CASE_TEST(chmod_self);        EXPECT_SYSER(proc, chmod("/proc/self", 0555), -1, EPERM); break;
 		CASE_TEST(chown_self);        EXPECT_SYSER(proc, chown("/proc/self", 0, 0), -1, EPERM); break;
+		CASE_TEST(chmod_self_comm);   EXPECT_SYSER(proc, chmod("/proc/self/comm", 0777), -1, EPERM); break;
+		CASE_TEST(chmod_tid_comm);    EXPECT_SYSER(proc, chmod("/proc/thread-self/comm", 0777), -1, EPERM); break;
+		CASE_TEST(chmod_self_environ);EXPECT_SYSER(proc, chmod("/proc/self/environ", 0777), -1, EPERM); break;
+		CASE_TEST(chmod_tid_environ); EXPECT_SYSER(proc, chmod("/proc/thread-self/environ", 0777), -1, EPERM); break;
 		CASE_TEST(chroot_root);       EXPECT_SYSZR(euid0, chroot("/")); break;
 		CASE_TEST(chroot_blah);       EXPECT_SYSER(1, chroot("/proc/self/blah"), -1, ENOENT); break;
 		CASE_TEST(chroot_exe);        EXPECT_SYSER(proc, chroot("/proc/self/exe"), -1, ENOTDIR); break;
-- 
2.41.0

Re: [PATCH] procfs: block chmod on /proc/thread-self/comm

[Willy Tarreau]

+Cc Thomas Weißschuh <thomas@t-8ch.de> as this seems quite related to
his finding about /proc/self/net:

  https://lore.kernel.org/lkml/20230624-proc-net-setattr-v1-0-73176812adee@weissschuh.net/#b

On Thu, Jul 13, 2023 at 10:19:04PM +1000, Aleksa Sarai wrote:
> Due to an oversight in commit 1b3044e39a89 ("procfs: fix pthread
> cross-thread naming if !PR_DUMPABLE") in switching from REG to NOD,
> chmod operations on /proc/thread-self/comm were no longer blocked as
> they are on almost all other procfs files.
> 
> A very similar situation with /proc/self/environ was used to as a root
> exploit a long time ago, but procfs has SB_I_NOEXEC so this is simply a
> correctness issue.
> 
> Ref: https://lwn.net/Articles/191954/
> Ref: 6d76fa58b050 ("Don't allow chmod() on the /proc/<pid>/ files")
> Fixes: 1b3044e39a89 ("procfs: fix pthread cross-thread naming if !PR_DUMPABLE")
> Cc: stable@vger.kernel.org # v4.7+
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
>  fs/proc/base.c                               | 3 ++-
>  tools/testing/selftests/nolibc/nolibc-test.c | 4 ++++
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 05452c3b9872..7394229816f3 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -3583,7 +3583,8 @@ static int proc_tid_comm_permission(struct mnt_idmap *idmap,
>  }
>  
>  static const struct inode_operations proc_tid_comm_inode_operations = {
> -		.permission = proc_tid_comm_permission,
> +		.setattr	= proc_setattr,
> +		.permission	= proc_tid_comm_permission,
>  };
>  
>  /*
> diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> index 486334981e60..08f0969208eb 100644
> --- a/tools/testing/selftests/nolibc/nolibc-test.c
> +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> @@ -580,6 +580,10 @@ int run_syscall(int min, int max)
>  		CASE_TEST(chmod_net);         EXPECT_SYSZR(proc, chmod("/proc/self/net", 0555)); break;
>  		CASE_TEST(chmod_self);        EXPECT_SYSER(proc, chmod("/proc/self", 0555), -1, EPERM); break;
>  		CASE_TEST(chown_self);        EXPECT_SYSER(proc, chown("/proc/self", 0, 0), -1, EPERM); break;
> +		CASE_TEST(chmod_self_comm);   EXPECT_SYSER(proc, chmod("/proc/self/comm", 0777), -1, EPERM); break;
> +		CASE_TEST(chmod_tid_comm);    EXPECT_SYSER(proc, chmod("/proc/thread-self/comm", 0777), -1, EPERM); break;
> +		CASE_TEST(chmod_self_environ);EXPECT_SYSER(proc, chmod("/proc/self/environ", 0777), -1, EPERM); break;
> +		CASE_TEST(chmod_tid_environ); EXPECT_SYSER(proc, chmod("/proc/thread-self/environ", 0777), -1, EPERM); break;
>  		CASE_TEST(chroot_root);       EXPECT_SYSZR(euid0, chroot("/")); break;
>  		CASE_TEST(chroot_blah);       EXPECT_SYSER(1, chroot("/proc/self/blah"), -1, ENOENT); break;
>  		CASE_TEST(chroot_exe);        EXPECT_SYSER(proc, chroot("/proc/self/exe"), -1, ENOTDIR); break;
> -- 
> 2.41.0

Re: [PATCH] procfs: block chmod on /proc/thread-self/comm

[Aleksa Sarai]

On 2023-07-13, Willy Tarreau <w@1wt.eu> wrote:
> +Cc Thomas Weißschuh <thomas@t-8ch.de> as this seems quite related to
> his finding about /proc/self/net:
> 
>   https://lore.kernel.org/lkml/20230624-proc-net-setattr-v1-0-73176812adee@weissschuh.net/#b

Yeah I saw this patch and (along with an earlier discussion with
Christian on the topic of chmod on symlinks -- see [1]) lead us to find
that there were three other cases where this happens unintentionally:

 * /proc/self (on the symlink itself)
 * /proc/thread-self (on the symlink itself)
 * /proc/thread-self/comm

The first two will be fixed by [1] so fixing them isn't necessary.

[1]: https://lore.kernel.org/linux-fsdevel/20230712-vfs-chmod-symlinks-v2-1-08cfb92b61dd@kernel.org/

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

Re: [PATCH] procfs: block chmod on /proc/thread-self/comm

[Thomas Weißschuh]

On 2023-07-13 22:19:04+1000, Aleksa Sarai wrote:
> Due to an oversight in commit 1b3044e39a89 ("procfs: fix pthread
> cross-thread naming if !PR_DUMPABLE") in switching from REG to NOD,
> chmod operations on /proc/thread-self/comm were no longer blocked as
> they are on almost all other procfs files.
> 
> A very similar situation with /proc/self/environ was used to as a root
> exploit a long time ago, but procfs has SB_I_NOEXEC so this is simply a
> correctness issue.
> 
> Ref: https://lwn.net/Articles/191954/
> Ref: 6d76fa58b050 ("Don't allow chmod() on the /proc/<pid>/ files")
> Fixes: 1b3044e39a89 ("procfs: fix pthread cross-thread naming if !PR_DUMPABLE")
> Cc: stable@vger.kernel.org # v4.7+
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
>  fs/proc/base.c                               | 3 ++-
>  tools/testing/selftests/nolibc/nolibc-test.c | 4 ++++
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 05452c3b9872..7394229816f3 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -3583,7 +3583,8 @@ static int proc_tid_comm_permission(struct mnt_idmap *idmap,
>  }
>  
>  static const struct inode_operations proc_tid_comm_inode_operations = {
> -		.permission = proc_tid_comm_permission,
> +		.setattr	= proc_setattr,
> +		.permission	= proc_tid_comm_permission,
>  };

Given that this seems to be a recurring theme a more systematic
aproach would help.

Something like the following (untested) patch:

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 05452c3b9872..b90f2e9cda66 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2649,6 +2649,7 @@ static struct dentry *proc_pident_instantiate(struct dentry *dentry,
 		set_nlink(inode, 2);	/* Use getattr to fix if necessary */
 	if (p->iop)
 		inode->i_op = p->iop;
+	WARN_ON(!inode->i_op->setattr);
 	if (p->fop)
 		inode->i_fop = p->fop;
 	ei->op = p->op;

>  /*
> diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> index 486334981e60..08f0969208eb 100644
> --- a/tools/testing/selftests/nolibc/nolibc-test.c
> +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> @@ -580,6 +580,10 @@ int run_syscall(int min, int max)
>  		CASE_TEST(chmod_net);         EXPECT_SYSZR(proc, chmod("/proc/self/net", 0555)); break;
>  		CASE_TEST(chmod_self);        EXPECT_SYSER(proc, chmod("/proc/self", 0555), -1, EPERM); break;
>  		CASE_TEST(chown_self);        EXPECT_SYSER(proc, chown("/proc/self", 0, 0), -1, EPERM); break;
> +		CASE_TEST(chmod_self_comm);   EXPECT_SYSER(proc, chmod("/proc/self/comm", 0777), -1, EPERM); break;
> +		CASE_TEST(chmod_tid_comm);    EXPECT_SYSER(proc, chmod("/proc/thread-self/comm", 0777), -1, EPERM); break;
> +		CASE_TEST(chmod_self_environ);EXPECT_SYSER(proc, chmod("/proc/self/environ", 0777), -1, EPERM); break;
> +		CASE_TEST(chmod_tid_environ); EXPECT_SYSER(proc, chmod("/proc/thread-self/environ", 0777), -1, EPERM); break;

I'm not a big fan of this, it abuses the nolibc testsuite to test core
kernel functionality.
If this needs to be tested explicitly there is hopefully a better place.

Those existing tests focus on testing functionality provided by nolibc.
The test chmod_net just got removed because it suffered from the same
bug as /proc/thread-self/comm.

>  		CASE_TEST(chroot_root);       EXPECT_SYSZR(euid0, chroot("/")); break;
>  		CASE_TEST(chroot_blah);       EXPECT_SYSER(1, chroot("/proc/self/blah"), -1, ENOENT); break;
>  		CASE_TEST(chroot_exe);        EXPECT_SYSER(proc, chroot("/proc/self/exe"), -1, ENOTDIR); break;
> -- 
> 2.41.0
> 

Re: [PATCH] procfs: block chmod on /proc/thread-self/comm

[Christian Brauner]

On Thu, Jul 13, 2023 at 03:01:24PM +0200, Thomas Weißschuh wrote:
> On 2023-07-13 22:19:04+1000, Aleksa Sarai wrote:
> > Due to an oversight in commit 1b3044e39a89 ("procfs: fix pthread
> > cross-thread naming if !PR_DUMPABLE") in switching from REG to NOD,
> > chmod operations on /proc/thread-self/comm were no longer blocked as
> > they are on almost all other procfs files.
> > 
> > A very similar situation with /proc/self/environ was used to as a root
> > exploit a long time ago, but procfs has SB_I_NOEXEC so this is simply a
> > correctness issue.
> > 
> > Ref: https://lwn.net/Articles/191954/
> > Ref: 6d76fa58b050 ("Don't allow chmod() on the /proc/<pid>/ files")
> > Fixes: 1b3044e39a89 ("procfs: fix pthread cross-thread naming if !PR_DUMPABLE")
> > Cc: stable@vger.kernel.org # v4.7+
> > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > ---
> >  fs/proc/base.c                               | 3 ++-
> >  tools/testing/selftests/nolibc/nolibc-test.c | 4 ++++
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 05452c3b9872..7394229816f3 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -3583,7 +3583,8 @@ static int proc_tid_comm_permission(struct mnt_idmap *idmap,
> >  }
> >  
> >  static const struct inode_operations proc_tid_comm_inode_operations = {
> > -		.permission = proc_tid_comm_permission,
> > +		.setattr	= proc_setattr,
> > +		.permission	= proc_tid_comm_permission,
> >  };
> 
> Given that this seems to be a recurring theme a more systematic
> aproach would help.
> 
> Something like the following (untested) patch:
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 05452c3b9872..b90f2e9cda66 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2649,6 +2649,7 @@ static struct dentry *proc_pident_instantiate(struct dentry *dentry,
>  		set_nlink(inode, 2);	/* Use getattr to fix if necessary */
>  	if (p->iop)
>  		inode->i_op = p->iop;
> +	WARN_ON(!inode->i_op->setattr);

Hm, no. This is hacky.

To fix this properly we will need to wean off notify_change() from
falling back to simple_setattr() when no i_op->setattr() method is
defined. To do that we will have to go through every filesystem and port
all that rely on this fallback to set simple_setattr() explicitly as
their i_op->setattr() method.

Christoph and I just discussed this in relation to another patch.

This is a bugfix so it should be as minimal as possible for easy
backport.

[PATCH] procfs: block chmod on /proc/thread-self/comm

[Aleksa Sarai]

Due to an oversight in commit 1b3044e39a89 ("procfs: fix pthread
cross-thread naming if !PR_DUMPABLE") in switching from REG to NOD,
chmod operations on /proc/thread-self/comm were no longer blocked as
they are on almost all other procfs files.

A very similar situation with /proc/self/environ was used to as a root
exploit a long time ago, but procfs has SB_I_NOEXEC so this is simply a
correctness issue.

Ref: https://lwn.net/Articles/191954/
Ref: 6d76fa58b050 ("Don't allow chmod() on the /proc/<pid>/ files")
Fixes: 1b3044e39a89 ("procfs: fix pthread cross-thread naming if !PR_DUMPABLE")
Cc: stable@vger.kernel.org # v4.7+
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
 fs/proc/base.c                               | 3 ++-
 tools/testing/selftests/nolibc/nolibc-test.c | 4 ++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 05452c3b9872..7394229816f3 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -3583,7 +3583,8 @@ static int proc_tid_comm_permission(struct mnt_idmap *idmap,
 }
 
 static const struct inode_operations proc_tid_comm_inode_operations = {
-		.permission = proc_tid_comm_permission,
+		.setattr	= proc_setattr,
+		.permission	= proc_tid_comm_permission,
 };
 
 /*
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index 486334981e60..08f0969208eb 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -580,6 +580,10 @@ int run_syscall(int min, int max)
 		CASE_TEST(chmod_net);         EXPECT_SYSZR(proc, chmod("/proc/self/net", 0555)); break;
 		CASE_TEST(chmod_self);        EXPECT_SYSER(proc, chmod("/proc/self", 0555), -1, EPERM); break;
 		CASE_TEST(chown_self);        EXPECT_SYSER(proc, chown("/proc/self", 0, 0), -1, EPERM); break;
+		CASE_TEST(chmod_self_comm);   EXPECT_SYSER(proc, chmod("/proc/self/comm", 0777), -1, EPERM); break;
+		CASE_TEST(chmod_tid_comm);    EXPECT_SYSER(proc, chmod("/proc/thread-self/comm", 0777), -1, EPERM); break;
+		CASE_TEST(chmod_self_environ);EXPECT_SYSER(proc, chmod("/proc/self/environ", 0777), -1, EPERM); break;
+		CASE_TEST(chmod_tid_environ); EXPECT_SYSER(proc, chmod("/proc/thread-self/environ", 0777), -1, EPERM); break;
 		CASE_TEST(chroot_root);       EXPECT_SYSZR(euid0, chroot("/")); break;
 		CASE_TEST(chroot_blah);       EXPECT_SYSER(1, chroot("/proc/self/blah"), -1, ENOENT); break;
 		CASE_TEST(chroot_exe);        EXPECT_SYSER(proc, chroot("/proc/self/exe"), -1, ENOTDIR); break;
-- 
2.41.0