linux-kernel - Re: [PATCH] tty: Permit some TIOCL_SETSEL modes without CAP_SYS

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z2BKetPygDM36X-X@google.com>
Date: Mon, 16 Dec 2024 16:42:50 +0100
From: "Günther Noack" <gnoack@...gle.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Jann Horn <jannh@...gle.com>, "Hanno Böck" <hanno@...eck.de>, Jiri Slaby <jirislaby@...nel.org>, 
	linux-hardening@...r.kernel.org, regressions@...ts.linux.dev, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN

Hello!

On Mon, Dec 16, 2024 at 04:17:15PM +0100, Greg Kroah-Hartman wrote:
> On Mon, Dec 16, 2024 at 03:07:23PM +0000, Günther Noack wrote:
> > With this, processes without CAP_SYS_ADMIN are able to use TIOCLINUX with
> > subcode TIOCL_SETSEL, in the selection modes TIOCL_SETPOINTER,
> > TIOCL_SELCLEAR and TIOCL_SELMOUSEREPORT.
> > 
> > TIOCL_SETSEL was previously changed to require CAP_SYS_ADMIN, as this IOCTL
> > let callers change the selection buffer and could be used to simulate
> > keypresses.  These three TIOCL_SETSEL selection modes, however, are safe to
> > use, as they do not modify the selection buffer.
> > 
> > This fixes a mouse support regression that affected Emacs (invisible mouse
> > cursor).
> > 
> > Link: https://lore.kernel.org/r/ee3ec63269b43b34e1c90dd8c9743bf8@finder.org
> > Fixes: 8d1b43f6a6df ("tty: Restrict access to TIOCLINUX' copy-and-paste subcommands")
> > Signed-off-by: Günther Noack <gnoack@...gle.com>
> > ---
> >  drivers/tty/vt/selection.c | 14 ++++++++++++++
> >  drivers/tty/vt/vt.c        |  3 +--
> >  2 files changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
> > index 564341f1a74f..0bd6544e30a6 100644
> > --- a/drivers/tty/vt/selection.c
> > +++ b/drivers/tty/vt/selection.c
> > @@ -192,6 +192,20 @@ int set_selection_user(const struct tiocl_selection __user *sel,
> >  	if (copy_from_user(&v, sel, sizeof(*sel)))
> >  		return -EFAULT;
> >  
> > +	/*
> > +	 * TIOCL_SELCLEAR, TIOCL_SELPOINTER and TIOCL_SELMOUSEREPORT are OK to
> > +	 * use without CAP_SYS_ADMIN as they do not modify the selection.
> > +	 */
> > +	switch (v.sel_mode) {
> > +	case TIOCL_SELCLEAR:
> > +	case TIOCL_SELPOINTER:
> > +	case TIOCL_SELMOUSEREPORT:
> > +		break;
> > +	default:
> > +		if (!capable(CAP_SYS_ADMIN))
> > +			return -EPERM;
> > +	}
> 
> Shouldn't you check this _before_ doing the copy_from_user() to emulate
> the previous logic properly?

You are right, I believe this can technically return a different error code.

There is a data dependency between the two though - the capability check should
only happen for certain values of v.sel_mode, and v is only populated by
copy_from_user().

As far as I can tell, this only makes a difference in scenarios where both
copy_from_user() and the capability check would fail.

Do you think we have to emulate it down to this level of detail?


As background, we have only introduced the CAP_SYS_ADMIN check recently in
8d1b43f6a6df ("tty: Restrict access to TIOCLINUX' copy-and-paste subcommands")
The patch landed in Linux 6.7 and was discussed in
https://lore.kernel.org/all/20230828164117.3608812-1-gnoack@google.com/.

I suspect that existing callers of the TIOCL_SETSEL IOCTL are probably all
written at a time before the capability check existed.  ((a) These IOCTLs are
used for the console mouse support, and (b) these "modes" for the TIOCL_SETSEL
subcode for the TIOCLINUX IOCTL are not documented in the man page either.)

> 
> > +
> >  	return set_selection_kernel(&v, tty);
> >  }
> >  
> > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > index 96842ce817af..ed65b3b80fbd 100644
> > --- a/drivers/tty/vt/vt.c
> > +++ b/drivers/tty/vt/vt.c
> > @@ -3345,8 +3345,7 @@ int tioclinux(struct tty_struct *tty, unsigned long arg)
> >  
> >  	switch (type) {
> >  	case TIOCL_SETSEL:
> > -		if (!capable(CAP_SYS_ADMIN))
> > -			return -EPERM;
> > +		/* CAP_SYS_ADMIN check happens in set_selection_user(). */
> 
> No need to mention this here, it's obvious in the function itself.

OK, thanks.  I will remove it in the next version.

—Günther