linux-kernel - Re: [PATCH] tty: replace capable() with file_ns

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAH4c4jLRcjceC2tynp5h9zFv9ev+usUrKO2Titnup1SPe_aNBg@mail.gmail.com>
Date: Fri, 13 Jun 2025 19:23:13 +0530
From: Pranav Tyagi <pranav.tyagi03@...il.com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: jirislaby@...nel.org, kees@...nel.org, skhan@...uxfoundation.org, 
	linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org, 
	linux-kernel-mentees@...ts.linux.dev
Subject: Re: [PATCH] tty: replace capable() with file_ns_capable()

On Sun, Jun 8, 2025 at 3:55 PM Greg KH <gregkh@...uxfoundation.org> wrote:
>
> On Sat, Jun 07, 2025 at 07:11:14PM +0530, Pranav Tyagi wrote:
> > The TIOCCONS ioctl currently uses capable(CAP_SYS_ADMIN) to check for
> > privileges, which validates the current task's credentials. Since this
> > ioctl acts on an open file descriptor, the check should instead use the
> > file opener's credentials.
> >
> > Replace capable() with file_ns_capable() to ensure the capability is
> > checked against file->f_cred in the correct user namespace. This
> > prevents unintended privilege escalation and aligns with best practices
> > for secure ioctl implementations.
> >
> > Signed-off-by: Pranav Tyagi <pranav.tyagi03@...il.com>
> > Link: https://github.com/KSPP/linux/issues/156
> > ---
> >  drivers/tty/tty_io.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> > index e2d92cf70eb7..ee0df35d65c3 100644
> > --- a/drivers/tty/tty_io.c
> > +++ b/drivers/tty/tty_io.c
> > @@ -102,6 +102,9 @@
> >  #include <linux/uaccess.h>
> >  #include <linux/termios_internal.h>
> >  #include <linux/fs.h>
> > +#include <linux/cred.h>
> > +#include <linux/user_namespace.h>
> > +#include <linux/capability.h>
> >
> >  #include <linux/kbd_kern.h>
> >  #include <linux/vt_kern.h>
> > @@ -2379,7 +2382,7 @@ static int tiocswinsz(struct tty_struct *tty, struct winsize __user *arg)
> >   */
> >  static int tioccons(struct file *file)
> >  {
> > -     if (!capable(CAP_SYS_ADMIN))
> > +     if (!file_ns_capable(file, file->f_cred->user_ns, CAP_SYS_ADMIN))
>
> As you now are affecting the user/kernel api here, how was this tested
> and are you _SURE_ this is the correct thing to be doing?  Did you audit
> all userspace users of this ioctl that you can find (i.e. a Debian code
> search) to verify that they can handle this change in behaviour?
>
> I need a lot of assurances before being able to take a change like this,
> for obvious reasons.
>
> thanks,
>
> greg k-h

Hi,

Thank you for the feedback.

I could not find any existing selftests specifically targeting
TIOCCONS. If there are any related ones, I’d appreciate it if you
could point me in the right direction.

Given that, I had to write a standalone functional test to exercise
this ioctl and validate the permission behavior with the proposed
change. I’ll share the test code and output shortly.

I'm also in the process of auditing userspace tools that use
TIOCCONS, using Debian Code Search as suggested. That is taking a
bit of time, but I’ll include the findings in my next update.

In addition, I plan to run integration tests with a few known tools
that rely on TIOCCONS and will report those results as well.

I’d appreciate a bit more time to complete this work and ensure all
bases are covered before resubmitting.

Regards
Pranav Tyagi