GPM & Emacs broken in Linux 6.7 -- ok to relax check?

GPM & Emacs broken in Linux 6.7 -- ok to relax check?

[Jared Finder]

The change to restrict access to TIOCLINUX that was added in Linux 6.7 
breaks Emacs rendering of the mouse pointer. This change was previous 
discussed in 
https://lwn.net/ml/kernel-hardening/20230402160815.74760f87.hanno@hboeck.de/. 
An associated Emacs bug report, bug #74220, is discussed at 
https://lists.gnu.org/archive/html/bug-gnu-emacs/2024-11/msg00275.html.

I wanted to ask if it made sense for the restriction to not apply to the 
following three selection modes for TIOCL_SETSEL:

TIOCL_SELPOINTER   3 /* show the pointer */
TIOCL_SELCLEAR   4 /* clear visibility of selection */
TIOCL_SELMOUSEREPORT   16 /* report beginning of selection */

On a glance over the selection code, none of these interact with 
vc_sel.buffer and therefore are unrelated to the exploit linked in the 
original report. Only SELPOINTER is necessary to be available to fix 
Emacs bug #74220. I imagine such a change would involve moving the 
capability check from tioclinux(), case TIOCL_SETSEL to inside 
vc_do_selection().

Note: This is my first time emailing a Linux kernel mailing list, so 
please let me know if there's any additional conventions I should be 
following here.

Thank you for your time.

   -- MJF

Re: GPM & Emacs broken in Linux 6.7 -- ok to relax check?

[Jann Horn]

+regression list, LKML, maintainers, patch authors

On Fri, Nov 29, 2024 at 8:38 PM Jared Finder <jared@finder.org> wrote:
> The change to restrict access to TIOCLINUX that was added in Linux 6.7
> breaks Emacs rendering of the mouse pointer. This change was previous
> discussed in
> https://lwn.net/ml/kernel-hardening/20230402160815.74760f87.hanno@hboeck.de/.

This landed as commit 8d1b43f6a6df ("tty: Restrict access to
TIOCLINUX' copy-and-paste subcommands").

#regzbot introduced: 8d1b43f6a6df

> An associated Emacs bug report, bug #74220, is discussed at
> https://lists.gnu.org/archive/html/bug-gnu-emacs/2024-11/msg00275.html.
>
> I wanted to ask if it made sense for the restriction to not apply to the
> following three selection modes for TIOCL_SETSEL:
>
> TIOCL_SELPOINTER   3 /* show the pointer */
> TIOCL_SELCLEAR   4 /* clear visibility of selection */
> TIOCL_SELMOUSEREPORT   16 /* report beginning of selection */
>
> On a glance over the selection code, none of these interact with
> vc_sel.buffer and therefore are unrelated to the exploit linked in the
> original report. Only SELPOINTER is necessary to be available to fix
> Emacs bug #74220. I imagine such a change would involve moving the
> capability check from tioclinux(), case TIOCL_SETSEL to inside
> vc_do_selection().
>
> Note: This is my first time emailing a Linux kernel mailing list, so
> please let me know if there's any additional conventions I should be
> following here.

FYI, for next time: There is some documentation at
https://docs.kernel.org/admin-guide/reporting-regressions.html
specifically for reporting regressions (with a focus on what to do so
that your regression report doesn't get lost).

> Thank you for your time.
>
>    -- MJF
>

Re: GPM & Emacs broken in Linux 6.7 -- ok to relax check?

[Günther Noack]

Hello!

On Fri, Nov 29, 2024 at 08:50:38PM +0100, Jann Horn wrote:
> +regression list, LKML, maintainers, patch authors
> 
> On Fri, Nov 29, 2024 at 8:38 PM Jared Finder <jared@finder.org> wrote:
> > The change to restrict access to TIOCLINUX that was added in Linux 6.7
> > breaks Emacs rendering of the mouse pointer. This change was previous
> > discussed in
> > https://lwn.net/ml/kernel-hardening/20230402160815.74760f87.hanno@hboeck.de/.
> 
> This landed as commit 8d1b43f6a6df ("tty: Restrict access to
> TIOCLINUX' copy-and-paste subcommands").
> 
> #regzbot introduced: 8d1b43f6a6df

Thank you for reporting the bug, and thanks for forwarding, Jann!

> > An associated Emacs bug report, bug #74220, is discussed at
> > https://lists.gnu.org/archive/html/bug-gnu-emacs/2024-11/msg00275.html.
> >
> > I wanted to ask if it made sense for the restriction to not apply to the
> > following three selection modes for TIOCL_SETSEL:
> >
> > TIOCL_SELPOINTER   3 /* show the pointer */
> > TIOCL_SELCLEAR   4 /* clear visibility of selection */
> > TIOCL_SELMOUSEREPORT   16 /* report beginning of selection */
> >
> > On a glance over the selection code, none of these interact with
> > vc_sel.buffer and therefore are unrelated to the exploit linked in the
> > original report. Only SELPOINTER is necessary to be available to fix
> > Emacs bug #74220. I imagine such a change would involve moving the
> > capability check from tioclinux(), case TIOCL_SETSEL to inside
> > vc_do_selection().

We did indeed miss that Emacs is using these IOCTLs directly.

To paraphrase what is happening, so that we are on the same page:

* Emacs includes the GPM header gpm.h and calls Gpm_DrawPointer(x, y, fd).
* Gpm_DrawPointer is implemented as a macro, which hardcodes all IOCTL constants
  (as it is the case in the entire GPM codebase), and it invokes ioctl(2)
  directly from the macro.

The Gpm_DrawPointer also gets called from other packages, including mc (Midnight
Commander), elinks and libt3widget (which supports the Tilde text editor).

https://codesearch.debian.net/search?q=Gpm_DrawPointer&literal=1&perpkg=1

* Midnight Commander and the Tilde text editor display the mouse cursor fine,
  also as a regular user.

* Elinks does not display the mouse cursor at all, independent of whether it is
  being run as root or not.  (Which makes me suspect that it might be an
  independent bug.)

* Emacs does not display the mouse cursor when run as normal user,
  but it works as root.

I can see how the three selection modes TIOCL_SELPOINTER, TIOCL_SELCLEAR and
TIOCL_SELMOUSEREPORT are related - TIOCL_SELPOINTER is the one that we need, but
TIOCL_SELPOINTER and TIOCL_SELCLEAR are the other two which can happen leading
up to that at the top of vc_selection()?

I also believe that what we *actually* want to guard here is the change to
vc_sel, so to propose a (somewhat simple-minded) patch, I guess we could put the
CAP_SYS_ADMIN check in vc_do_selection() right before the place where
vc_sel.start and vc_sel.end are being assigned?

Hanno, you are the original author of this patch and you have done a more
detailed analysis on the TIOCLINUX problems than me -- do you agree that this
weakened check would still be sufficient to protect against the TIOCLINUX
problems?  (Or in other words, if we permitted TIOCL_SELPOINTER, TIOCL_SELCLEAR
and TIOCL_SELMOUSEREPORT for non-CAP_SYS_ADMIN processes, would you still see a
way to misuse that functionality?)

Thanks,
—Günther

Re: GPM & Emacs broken in Linux 6.7 -- ok to relax check?

[Günther Noack]

On Tue, Dec 03, 2024 at 02:53:27PM +0100, Günther Noack wrote:
> Hanno, you are the original author of this patch and you have done a more
> detailed analysis on the TIOCLINUX problems than me -- do you agree that this
> weakened check would still be sufficient to protect against the TIOCLINUX
> problems?  (Or in other words, if we permitted TIOCL_SELPOINTER, TIOCL_SELCLEAR
> and TIOCL_SELMOUSEREPORT for non-CAP_SYS_ADMIN processes, would you still see a
> way to misuse that functionality?)

P.S.: For reference, some more detailed reasoning why I think that that proposal
would be OK:

It would protect at least against the "minittyjack.c" example that was attached
to https://www.openwall.com/lists/oss-security/2023/03/14/3

The trick used there was:

* ioctl() with TIOCLINUX with TIOCL_SETSEL with TIOCL_SELLINE,
  to make a selection (a.k.a. changing the contents of vc_sel)
* ioctl() with TIOCLINUX and TIOCL_PASTESEL to paste the selection.
  (The implementation for that is in selection.c/paste_selection()
  and is just copying from vc_sel.)

So as long as we are protecting the change to vc_sel, that should be OK in my
mind.

—Günther

Re: GPM & Emacs broken in Linux 6.7 -- ok to relax check?

[Jared Finder]

On 2024-12-03 06:07, Günther Noack wrote:
> On Tue, Dec 03, 2024 at 02:53:27PM +0100, Günther Noack wrote:
>> Hanno, you are the original author of this patch and you have done a 
>> more
>> detailed analysis on the TIOCLINUX problems than me -- do you agree 
>> that this
>> weakened check would still be sufficient to protect against the 
>> TIOCLINUX
>> problems?  (Or in other words, if we permitted TIOCL_SELPOINTER, 
>> TIOCL_SELCLEAR
>> and TIOCL_SELMOUSEREPORT for non-CAP_SYS_ADMIN processes, would you 
>> still see a
>> way to misuse that functionality?)
> 
> P.S.: For reference, some more detailed reasoning why I think that that 
> proposal
> would be OK:
> 
> It would protect at least against the "minittyjack.c" example that was 
> attached
> to https://www.openwall.com/lists/oss-security/2023/03/14/3
> 
> The trick used there was:
> 
> * ioctl() with TIOCLINUX with TIOCL_SETSEL with TIOCL_SELLINE,
>   to make a selection (a.k.a. changing the contents of vc_sel)
> * ioctl() with TIOCLINUX and TIOCL_PASTESEL to paste the selection.
>   (The implementation for that is in selection.c/paste_selection()
>   and is just copying from vc_sel.)
> 
> So as long as we are protecting the change to vc_sel, that should be OK 
> in my
> mind.

It's been silent for about a week and a half here. How long do we wait 
before creating a patch here? This is a regression so I'm assuming we 
shouldn't wait too long.

I'm happy to create the patch if that's helpful. I'd need help with 
process for submitting it for inclusion in the latest kernel as well as 
backports to earlier impacted versions. I've never submitted code to 
Linux before.

   -- MJF

Re: GPM & Emacs broken in Linux 6.7 -- ok to relax check?

[Greg Kroah-Hartman]

On Fri, Dec 13, 2024 at 09:13:54PM -0800, Jared Finder wrote:
> On 2024-12-03 06:07, Günther Noack wrote:
> > On Tue, Dec 03, 2024 at 02:53:27PM +0100, Günther Noack wrote:
> > > Hanno, you are the original author of this patch and you have done a
> > > more
> > > detailed analysis on the TIOCLINUX problems than me -- do you agree
> > > that this
> > > weakened check would still be sufficient to protect against the
> > > TIOCLINUX
> > > problems?  (Or in other words, if we permitted TIOCL_SELPOINTER,
> > > TIOCL_SELCLEAR
> > > and TIOCL_SELMOUSEREPORT for non-CAP_SYS_ADMIN processes, would you
> > > still see a
> > > way to misuse that functionality?)
> > 
> > P.S.: For reference, some more detailed reasoning why I think that that
> > proposal
> > would be OK:
> > 
> > It would protect at least against the "minittyjack.c" example that was
> > attached
> > to https://www.openwall.com/lists/oss-security/2023/03/14/3
> > 
> > The trick used there was:
> > 
> > * ioctl() with TIOCLINUX with TIOCL_SETSEL with TIOCL_SELLINE,
> >   to make a selection (a.k.a. changing the contents of vc_sel)
> > * ioctl() with TIOCLINUX and TIOCL_PASTESEL to paste the selection.
> >   (The implementation for that is in selection.c/paste_selection()
> >   and is just copying from vc_sel.)
> > 
> > So as long as we are protecting the change to vc_sel, that should be OK
> > in my
> > mind.
> 
> It's been silent for about a week and a half here. How long do we wait
> before creating a patch here? This is a regression so I'm assuming we
> shouldn't wait too long.
> 
> I'm happy to create the patch if that's helpful. I'd need help with process
> for submitting it for inclusion in the latest kernel as well as backports to
> earlier impacted versions. I've never submitted code to Linux before.

Sure, make a patch and we will be glad to help you out in getting it
accepted if it looks good.

thanks,

greg k-h

[PATCH] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN

[Günther Noack]

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@google.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;
+	}
+
 	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(). */
 		return set_selection_user(param, tty);
 	case TIOCL_PASTESEL:
 		if (!capable(CAP_SYS_ADMIN))
-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN

[Greg Kroah-Hartman]

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@google.com>
> ---
>  drivers/tty/vt/selection.c | 14 ++++++++++++++
>  drivers/tty/vt/vt.c        |  3 +--
>  2 files changed, 15 insertions(+), 2 deletions(-)

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- Your patch breaks the build.

- Your patch contains warnings and/or errors noticed by the
  scripts/checkpatch.pl tool.

- You have marked a patch with a "Fixes:" tag for a commit that is in an
  older released kernel, yet you do not have a cc: stable line in the
  signed-off-by area at all, which means that the patch will not be
  applied to any older kernel releases.  To properly fix this, please
  follow the documented rules in the
  Documentation/process/stable-kernel-rules.rst file for how to resolve
  this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

Re: [PATCH] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN

[Greg Kroah-Hartman]

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@google.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?

> +
>  	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.

thanks,

greg k-h

Re: [PATCH] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN

[Günther Noack]

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@google.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

Re: GPM & Emacs broken in Linux 6.7 -- ok to relax check?

[Hanno Böck]

Hello,

On Tue, 3 Dec 2024 14:53:27 +0100
"Günther Noack" <gnoack@google.com> wrote:

> Hanno, you are the original author of this patch and you have done a
> more detailed analysis on the TIOCLINUX problems than me -- do you
> agree that this weakened check would still be sufficient to protect
> against the TIOCLINUX problems?  (Or in other words, if we permitted
> TIOCL_SELPOINTER, TIOCL_SELCLEAR and TIOCL_SELMOUSEREPORT for
> non-CAP_SYS_ADMIN processes, would you still see a way to misuse that
> functionality?)

Sorry for the late feedback.

I believe that this is correct, and permitting these functionalities
still preserves the security fix. I also checked with Jakub Wilk, who
was the original author of the exploit.
The patch you posted in the meantime[1] should be fine.

https://lore.kernel.org/linux-hardening/Z2BKetPygDM36X-X@google.com/T/#u

-- 
Hanno Böck
https://hboeck.de/

Re: GPM & Emacs broken in Linux 6.7 -- ok to relax check?

[Greg Kroah-Hartman]

On Tue, Dec 17, 2024 at 09:47:23AM +0100, Hanno Böck wrote:
> Hello,
> 
> On Tue, 3 Dec 2024 14:53:27 +0100
> "Günther Noack" <gnoack@google.com> wrote:
> 
> > Hanno, you are the original author of this patch and you have done a
> > more detailed analysis on the TIOCLINUX problems than me -- do you
> > agree that this weakened check would still be sufficient to protect
> > against the TIOCLINUX problems?  (Or in other words, if we permitted
> > TIOCL_SELPOINTER, TIOCL_SELCLEAR and TIOCL_SELMOUSEREPORT for
> > non-CAP_SYS_ADMIN processes, would you still see a way to misuse that
> > functionality?)
> 
> Sorry for the late feedback.
> 
> I believe that this is correct, and permitting these functionalities
> still preserves the security fix. I also checked with Jakub Wilk, who
> was the original author of the exploit.
> The patch you posted in the meantime[1] should be fine.
> 
> https://lore.kernel.org/linux-hardening/Z2BKetPygDM36X-X@google.com/T/#u

Great, can you test that and if it works for you, provide a tested-by
line?

thanks,

greg k-h

Re: [PATCH] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN

[Günther Noack]

FYI, I have tested this locally - it makes the mouse curser show up again in
Emacs.  (I do also believe that is it fine security-wise, but will also very
happily add a Reviewed-By or Tested-By from Hanno or others in the next
iteration :))

—Günther

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@google.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;
> +	}
> +
>  	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(). */
>  		return set_selection_user(param, tty);
>  	case TIOCL_PASTESEL:
>  		if (!capable(CAP_SYS_ADMIN))
> -- 
> 2.47.1.613.gc27f4b7a9f-goog
> 

Re: [PATCH] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN

[Günther Noack]

Hello Greg, Hanno and everyone else!

On Mon, Dec 16, 2024 at 04:42:50PM +0100, Günther Noack wrote:
> 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@google.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?

Another observation which makes it clearer that copy_from_user() failing here
should really not happen in practice:

The 'argp' pointer that gets passed to ioctl(2) here is a pointer to a region in
memory whose first byte is a TIOCLINUX subcommand, and at the time we do the
copy_from_user(), this subcommand byte has already been successfully copied over
from user space.  The region that we are now copying with copy_from_user() here
is directly after this byte, and should under normal circumstances work as well
-- I believe that callers would have to construct a specifically crafted ioctl()
so that the first copy-from-userspace works and the second one fails (referring
to a byte just before a page boundary, or something like that...?)

I'll send a V2 of this patch with the comment removed, the 'Cc: stable' added,
but where the CAP_SYS_ADMIN check is still done after copy_from_user(), with the
reasoning that:

 1. A previous get_user() from an adjacent memory region already worked
    (making this a very unlikely failure)
 2. I do not see a good alternative to reorder the code here, because we need
    the data from copy_from_user() in order to know whether the CAP_SYS_ADMIN
    check even needs to be performed.

Hanno: If you are OK with this patch, I'd still appreciate a more formal
"Reviewed-By" or "Tested-By" from you. :)

Thanks, and Happy Holidays!
—Günther

[PATCH v2] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN

[Günther Noack]

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).

Cc: stable@vger.kernel.org
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@google.com>
---
 drivers/tty/vt/selection.c | 14 ++++++++++++++
 drivers/tty/vt/vt.c        |  2 --
 2 files changed, 14 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;
+	}
+
 	return set_selection_kernel(&v, tty);
 }
 
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 96842ce817af..be5564ed8c01 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3345,8 +3345,6 @@ int tioclinux(struct tty_struct *tty, unsigned long arg)
 
 	switch (type) {
 	case TIOCL_SETSEL:
-		if (!capable(CAP_SYS_ADMIN))
-			return -EPERM;
 		return set_selection_user(param, tty);
 	case TIOCL_PASTESEL:
 		if (!capable(CAP_SYS_ADMIN))
-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH v2] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN

[Greg Kroah-Hartman]

On Sat, Dec 21, 2024 at 11:10:45AM +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).
> 
> Cc: stable@vger.kernel.org
> 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@google.com>
> ---
>  drivers/tty/vt/selection.c | 14 ++++++++++++++
>  drivers/tty/vt/vt.c        |  2 --
>  2 files changed, 14 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;
> +	}
> +
>  	return set_selection_kernel(&v, tty);
>  }
>  
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 96842ce817af..be5564ed8c01 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -3345,8 +3345,6 @@ int tioclinux(struct tty_struct *tty, unsigned long arg)
>  
>  	switch (type) {
>  	case TIOCL_SETSEL:
> -		if (!capable(CAP_SYS_ADMIN))
> -			return -EPERM;
>  		return set_selection_user(param, tty);
>  	case TIOCL_PASTESEL:
>  		if (!capable(CAP_SYS_ADMIN))
> -- 
> 2.47.1.613.gc27f4b7a9f-goog
> 
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/process/submitting-patches.rst for what
  needs to be done here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

[PATCH v2] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN

[Günther Noack]

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).

Cc: stable@vger.kernel.org
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@google.com>
---
Changes in V2:

 * Removed comment in vt.c (per Greg's suggestion)

 * CC'd stable@

 * I *kept* the CAP_SYS_ADMIN check *after* copy_from_user(),
   with the reasoning that:

   1. I do not see a good alternative to reorder the code here.
      We need the data from copy_from_user() in order to know whether
      the CAP_SYS_ADMIN check even needs to be performed.
   2. A previous get_user() from an adjacent memory region already worked
      (making this a very unlikely failure)

I would still appreciate a more formal Tested-by from Hanno (hint, hint) :)

---
 drivers/tty/vt/selection.c | 14 ++++++++++++++
 drivers/tty/vt/vt.c        |  2 --
 2 files changed, 14 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;
+	}
+
 	return set_selection_kernel(&v, tty);
 }
 
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 96842ce817af..be5564ed8c01 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3345,8 +3345,6 @@ int tioclinux(struct tty_struct *tty, unsigned long arg)
 
 	switch (type) {
 	case TIOCL_SETSEL:
-		if (!capable(CAP_SYS_ADMIN))
-			return -EPERM;
 		return set_selection_user(param, tty);
 	case TIOCL_PASTESEL:
 		if (!capable(CAP_SYS_ADMIN))
-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH v2] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN

[Kees Cook]

On Fri, Jan 10, 2025 at 02:21:22PM +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).
> 
> Cc: stable@vger.kernel.org
> 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@google.com>

Reviewed-by: Kees Cook <kees@kernel.org>

-- 
Kees Cook

Re: [PATCH v2] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN

[Greg Kroah-Hartman]

On Fri, Jan 10, 2025 at 02:21:22PM +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).
> 
> Cc: stable@vger.kernel.org
> 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@google.com>
> ---
> Changes in V2:
> 
>  * Removed comment in vt.c (per Greg's suggestion)
> 
>  * CC'd stable@
> 
>  * I *kept* the CAP_SYS_ADMIN check *after* copy_from_user(),
>    with the reasoning that:
> 
>    1. I do not see a good alternative to reorder the code here.
>       We need the data from copy_from_user() in order to know whether
>       the CAP_SYS_ADMIN check even needs to be performed.
>    2. A previous get_user() from an adjacent memory region already worked
>       (making this a very unlikely failure)
> 
> I would still appreciate a more formal Tested-by from Hanno (hint, hint) :)

This really is v3, as you sent a v2 last year, right?

b4 got really confused, but I think I figured it out.  Whenever you
resend, bump the version number please, otherwise it causes problems.
Remember, some of use deal with thousands of patches a week...

thanks,

greg k-h

Re: [PATCH v2] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN

[Jared Finder]

Hi, I'm the original reporter of this regression (noticed because it 
impacted GNU Emacs) and I'm wondering if there's any traction on 
creating an updated patch? This thread appears to have stalled out. I 
haven't seen any reply for three weeks.

   -- MJF

Re: [PATCH v2] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN

[Greg KH]

On Sat, Feb 08, 2025 at 07:18:22AM -0800, Jared Finder wrote:
> Hi, I'm the original reporter of this regression (noticed because it
> impacted GNU Emacs) and I'm wondering if there's any traction on creating an
> updated patch? This thread appears to have stalled out. I haven't seen any
> reply for three weeks.

It's already in 6.14-rc1 as commit 2f83e38a095f ("tty: Permit some
TIOCL_SETSEL modes without CAP_SYS_ADMIN").

thanks,

greg k-h

Re: [PATCH v2] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN

[Jared Finder]

On 2025-02-08 07:28, Greg KH wrote:
> On Sat, Feb 08, 2025 at 07:18:22AM -0800, Jared Finder wrote:
>> Hi, I'm the original reporter of this regression (noticed because it
>> impacted GNU Emacs) and I'm wondering if there's any traction on 
>> creating an
>> updated patch? This thread appears to have stalled out. I haven't seen 
>> any
>> reply for three weeks.
> 
> It's already in 6.14-rc1 as commit 2f83e38a095f ("tty: Permit some
> TIOCL_SETSEL modes without CAP_SYS_ADMIN").

Great! Is this expected to get backported to 6.7 through 6.13? I would 
like to note the expected resolution correctly in Emacs' bug tracker.

   -- MJF

Re: [PATCH v2] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN

[Greg KH]

On Sat, Feb 08, 2025 at 08:03:27AM -0800, Jared Finder wrote:
> On 2025-02-08 07:28, Greg KH wrote:
> > On Sat, Feb 08, 2025 at 07:18:22AM -0800, Jared Finder wrote:
> > > Hi, I'm the original reporter of this regression (noticed because it
> > > impacted GNU Emacs) and I'm wondering if there's any traction on
> > > creating an
> > > updated patch? This thread appears to have stalled out. I haven't
> > > seen any
> > > reply for three weeks.
> > 
> > It's already in 6.14-rc1 as commit 2f83e38a095f ("tty: Permit some
> > TIOCL_SETSEL modes without CAP_SYS_ADMIN").
> 
> Great! Is this expected to get backported to 6.7 through 6.13? I would like
> to note the expected resolution correctly in Emacs' bug tracker.

Yes, it should show up in the next round of stable kernel updates later
this week.

But note, it will only show up in the 6.12.y and 6.13.y kernels, as all
others in your range are end-of-life and shouldn't be used by anyone at
this point in time.

thanks,

greg k-h

Re: [PATCH v2] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN

[Günther Noack]

Hello Jared and Hanno!

On Sat, Feb 08, 2025 at 07:18:22AM -0800, Jared Finder wrote:
> Hi, I'm the original reporter of this regression (noticed because it
> impacted GNU Emacs) and I'm wondering if there's any traction on creating an
> updated patch? This thread appears to have stalled out. I haven't seen any
> reply for three weeks.
> 
>   -- MJF

Jared, can you please confirm whether Emacs works now with this patch
in the kernel?

I am asking this because I realized that the patch had a bug.  We are
erring in the "secure" direction, but not all TIOCL_SELMOUSEREPORT
invocations work without CAP_SYS_ADMIN.

(TIOCL_SELMOUSEREPORT has to be put into the sel_mode field of the
argument struct, and that field looked like an enum to me, but as it
turns out, the TIOCL_SELMOUSEREPORT is 16 and the lower 4 bits of that
integer are used as an additional argument to indicate the mouse
button status and modifier keys.  I had overlooked that the
implementation was doing this.  As a result, TIOCL_SELMOUSEREPORT
works without CAP_SYS_ADMIN, but only if all four lower bits are 0.)

So, I apologize for the oversight.  -- Jared, can you please confirm
whether TIOCL_SELMOUSEREPORT is called directly from Emacs (rather
than from gpm)?  I tried to trace it with perf but could not reproduce
a scenario where Emacs called it.

If this specific selection mode is not needed by Emacs, I think *the
best thing would be to keep it guarded by CAP_SYS_ADMIN, after all*.

As it turns out, the following scenario is possible:

* A root shell invokes malicious program P and changes its UID to a
  less privileged user, but it passes a FD to the TTY.  (Instead of
  UID switch, it can also be a sandboxed program or another way of
  lowering privileges.)
* Program P enables mouse tracking mode by writing "\033[?1000h".
* Program P sends IOCTL TIOCLINUX with TIOCL_SETSEL with
  TIOCL_SELMOUSEREPORT and passes suitable mouse coordinate and button
  press arguments.  As a response, the terminal writes the escape
  sequence \033[MBXY, where B, X and Y are bytes indicating the button
  press mask and the 1-based mouse X and Y coordinates, all added to
  0x20 (space).

It is an obscure scenario and probably requires a console with a
character width and height above 256 (to control the full byte range),
but it seems that this can in principle be used to simulate short
keyboard inputs to programs (like bash) that are not expecting this
old mouse protocol. - This sort of keypress-simulation is exactly why
we created the CAP_SYS_ADMIN restriction for TIOCL_SETSEL in the first
place.

For background on this mouse tracking mechanism, I had to read up on
it myself, but found the following two links helpful:
https://invisible-island.net/xterm/ctlseqs/ctlseqs.html#h3-Normal-tracking-mode
https://www.ibm.com/docs/en/aix/7.2?topic=x-xterm-command#xterm__mouse

(Remark on the side, the GPM client library normally gets its mouse
coordinates through a Unix Domain socket from the GPM daemon. It has
support for this xterm emulation mode as well, but it is only enabled
if $TERM starts with "xterm".)

In summary:

If it is not absolutely needed, I think it would be better to not
permit access to TIOCL_SELMOUSEREPORT after all.  It does not let
attackers simulate keypresses quite as easily as the other features,
but it does let them send such input with more limited control, and it
seems like an unnecessary risk if the feature is not needed by
anything but mouse daemons running as root, such as GPM and
Consolation.

Does this seem reasonable?  (Hanno, do you agree with this
assessment?)  I am by no means an expert in this terminal-mouse
interaction, I am happy to stand corrected if I am wrong here.

–Günther

Re: [PATCH v2] tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN

[Jared Finder]

On 2025-02-20 16:10, Günther Noack wrote:
> 
> Jared, can you please confirm whether Emacs works now with this patch
> in the kernel?
> 
> I am asking this because I realized that the patch had a bug.  We are
> erring in the "secure" direction, but not all TIOCL_SELMOUSEREPORT
> invocations work without CAP_SYS_ADMIN.

I confirmed that Emacs worked fine with 6.14-rc1.  My understanding is 
that the Emacs process relies only on TIOCL_SELPOINTER which it needs to 
do to draw the mouse pointer after Emacs' redisplay.  It's fine for 
TIOCL_SELMOUSEREPORT to not work in an unpriviliged Emacs.

> If this specific selection mode is not needed by Emacs, I think *the
> best thing would be to keep it guarded by CAP_SYS_ADMIN, after all*.

This sounds good to me.

Reading over a documentation proposal for TIOCL_SELMOUSEREPORT 
(https://lkml.org/lkml/2020/7/6/249), I can not imagine how a userspace 
program that was not acting as the mouse daemon could successfully use 
SELMOUSEREPORT as the mouse daemon will be fighting with it.  Any 
legitimate setting of mouse state (for example, setting the mouse x/y 
coordinate) would need to be done with the mouse daemon in the loop, in 
which case the mouse daemon might as well send the message itself.

   -- MJF

[PATCH] tty: Require CAP_SYS_ADMIN for all usages of TIOCL_SELMOUSEREPORT

[Günther Noack]

This requirement was overeagerly loosened in commit 2f83e38a095f
("tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN"), but as
it turns out,

  (1) the logic I implemented there was inconsistent (apologies!),

  (2) TIOCL_SELMOUSEREPORT might actually be a small security risk
      after all, and

  (3) TIOCL_SELMOUSEREPORT is only meant to be used by the mouse
      daemon (GPM or Consolation), which runs as CAP_SYS_ADMIN
      already.

In more detail:

1. The previous patch has inconsistent logic:

   In commit 2f83e38a095f ("tty: Permit some TIOCL_SETSEL modes
   without CAP_SYS_ADMIN"), we checked for sel_mode ==
   TIOCL_SELMOUSEREPORT, but overlooked that the lower four bits of
   this "mode" parameter were actually used as an additional way to
   pass an argument.  So the patch did actually still require
   CAP_SYS_ADMIN, if any of the mouse button bits are set, but did not
   require it if none of the mouse buttons bits are set.

   This logic is inconsistent and was not intentional.  We should have
   the same policies for using TIOCL_SELMOUSEREPORT independent of the
   value of the "hidden" mouse button argument.

   I sent a separate documentation patch to the man page list with
   more details on TIOCL_SELMOUSEREPORT:
   https://lore.kernel.org/all/20250223091342.35523-2-gnoack3000@gmail.com/

2. TIOCL_SELMOUSEREPORT is indeed a potential security risk which can
   let an attacker simulate "keyboard" input to command line
   applications on the same terminal, like TIOCSTI and some other
   TIOCLINUX "selection mode" IOCTLs.

   By enabling mouse reporting on a terminal and then injecting mouse
   reports through TIOCL_SELMOUSEREPORT, an attacker can simulate
   mouse movements on the same terminal, similar to the TIOCSTI
   keystroke injection attacks that were previously possible with
   TIOCSTI and other TIOCL_SETSEL selection modes.

   Many programs (including libreadline/bash) are then prone to
   misinterpret these mouse reports as normal keyboard input because
   they do not expect input in the X11 mouse protocol form.  The
   attacker does not have complete control over the escape sequence,
   but they can at least control the values of two consecutive bytes
   in the binary mouse reporting escape sequence.

   I went into more detail on that in the discussion at
   https://lore.kernel.org/all/20250221.0a947528d8f3@gnoack.org/

   It is not equally trivial to simulate arbitrary keystrokes as it
   was with TIOCSTI (commit 83efeeeb3d04 ("tty: Allow TIOCSTI to be
   disabled")), but the general mechanism is there, and together with
   the small number of existing legit use cases (see below), it would
   be better to revert back to requiring CAP_SYS_ADMIN for
   TIOCL_SELMOUSEREPORT, as it was already the case before
   commit 2f83e38a095f ("tty: Permit some TIOCL_SETSEL modes without
   CAP_SYS_ADMIN").

3. TIOCL_SELMOUSEREPORT is only used by the mouse daemons (GPM or
   Consolation), and they are the only legit use case:

   To quote console_codes(4):

     The mouse tracking facility is intended to return
     xterm(1)-compatible mouse status reports.  Because the console
     driver has no way to know the device or type of the mouse, these
     reports are returned in the console input stream only when the
     virtual terminal driver receives a mouse update ioctl.  These
     ioctls must be generated by a mouse-aware user-mode application
     such as the gpm(8) daemon.

   Jared Finder has also confirmed in
   https://lore.kernel.org/all/491f3df9de6593df8e70dbe77614b026@finder.org/
   that Emacs does not call TIOCL_SELMOUSEREPORT directly, and it
   would be difficult to find good reasons for doing that, given that
   it would interfere with the reports that GPM is sending.

   More information on the interaction between GPM, terminals and the
   kernel with additional pointers is also available in this patch:
   https://lore.kernel.org/all/a773e48920aa104a65073671effbdee665c105fc.1603963593.git.tammo.block@gmail.com/

   For background on who else uses TIOCL_SELMOUSEREPORT: Debian Code
   search finds one page of results, the only two known callers are
   the two mouse daemons GPM and Consolation.  (GPM does not show up
   in the search results because it uses literal numbers to refer to
   TIOCLINUX-related enums.  I looked through GPM by hand instead.
   TIOCL_SELMOUSEREPORT is also not used from libgpm.)
   https://codesearch.debian.net/search?q=TIOCL_SELMOUSEREPORT

Cc: Jared Finder <jared@finder.org>
Cc: Jann Horn <jannh@google.com>
Cc: Hanno Böck <hanno@hboeck.de>
Cc: Jiri Slaby <jirislaby@kernel.org>
Cc: Kees Cook <kees@kernel.org>
Cc: stable@vger.kernel.org
Fixes: 2f83e38a095f ("tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN")
Signed-off-by: Günther Noack <gnoack3000@gmail.com>
---
 drivers/tty/vt/selection.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
index 0bd6544e30a6b..791e2f1f7c0b6 100644
--- a/drivers/tty/vt/selection.c
+++ b/drivers/tty/vt/selection.c
@@ -193,13 +193,12 @@ int set_selection_user(const struct tiocl_selection __user *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.
+	 * TIOCL_SELCLEAR and TIOCL_SELPOINTER 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))

base-commit: 27102b38b8ca7ffb1622f27bcb41475d121fb67f
-- 
2.48.1

Re: [PATCH] tty: Require CAP_SYS_ADMIN for all usages of TIOCL_SELMOUSEREPORT

[Günther Noack]

On Sun, Feb 23, 2025 at 09:54:50PM +0100, Günther Noack wrote:
> This requirement was overeagerly loosened in commit 2f83e38a095f
> ("tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN"), but as
> it turns out,
> 
>   (1) the logic I implemented there was inconsistent (apologies!),
> 
>   (2) TIOCL_SELMOUSEREPORT might actually be a small security risk
>       after all, and
> 
>   (3) TIOCL_SELMOUSEREPORT is only meant to be used by the mouse
>       daemon (GPM or Consolation), which runs as CAP_SYS_ADMIN
>       already.


Greg and Jared: Friendly ping on this patch.

Could you please have a look if you can find the time?



To maybe explain in an overview again why this is safe:

The TIOCLINUX ioctl has various subcommands (uapi/linux/tiocl.h),
and one of these has in turn more subcommands.  The structure is:

* TIOCLINUX, with "subcodes":
  * TIOCL_SETSEL, with "selection modes":
    * TIOCL_SELCHAR
    * TIOCL_SELWORD
    * TIOCL_SELLINE
    * TIOCL_SELPOINTER
    * TIOCL_SELCLEAR
    * TIOCL_SELMOUSEREPORT
  * TIOCL_PASTESEL
  * TIOCL_SELLOADLUT
  * ...

While securing TIOCLINUX, we restricted access to various subcommands
with CAP_SYS_ADMIN, but permitted different subcommands.

This table gives an overview of which TIOCL_SETSEL subcommands
required CAP_SYS_ADMIN at which point in time:

                          point in time
  TIOCL_SETSEL sel_mode | 0 | 1 | 2 | 3
  ----------------------|---|---|---|---
  TIOCL_SELCHAR         |   | x | x | x
  TIOCL_SELWORD         |   | x | x | x 
  TIOCL_SELLINE         |   | x | x | x 
  TIOCL_SELPOINTER      |   | x |   |
  TIOCL_SELCLEAR        |   | x |   | 
  TIOCL_SELMOUSEREPORT  |   | x | ? | x  <-- This is the change

  "x" means "requires CAP_SYS_ADMIN"
  "?" means "inconsistently requires CAP_SYS_ADMIN"

The points in time are:

 (0) before we required CAP_SYS_ADMIN on TIOCLINUX subcommands
 (1) after commit 8d1b43f6a6df ("tty: Restrict access to TIOCLINUX' copy-and-paste subcommands")
 (2) after commit 2f83e38a095f ("tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN
")
 (3) after this patch ("tty: Require CAP_SYS_ADMIN for all usages of TIOCL_SELMOUSEREPORT")

This patch **reverts the behaviour for TIOCL_SELMOUSEREPORT back to
what it was in phase (1)** after commit 8d1b43f6a6df ("tty: Restrict
access to TIOCLINUX' copy-and-paste subcommands").  We have double
checked this in Emacs and GPM's source code earlier in this mail
thread [1] and have confidence that this is better, because:

 (a) TIOCL_SELMOUSEREPORT can maybe be abused after all,
 (b) it is not required for Emacs as we thought in patch (2)
 (c) the behavior I implemented in patch (2) was accidentally
     inconsistent

Again, apologies for the pointless back-and-forth on this fix, but it
will be better after this iteration.  I hope that this summary helps
in the review.  Please let me know if you have further questions.

Thanks,
–Günther

[1] https://lore.kernel.org/all/491f3df9de6593df8e70dbe77614b026@finder.org/

Re: [PATCH] tty: Require CAP_SYS_ADMIN for all usages of TIOCL_SELMOUSEREPORT

[Greg Kroah-Hartman]

On Fri, Mar 07, 2025 at 11:16:21AM +0100, Günther Noack wrote:
> On Sun, Feb 23, 2025 at 09:54:50PM +0100, Günther Noack wrote:
> > This requirement was overeagerly loosened in commit 2f83e38a095f
> > ("tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN"), but as
> > it turns out,
> > 
> >   (1) the logic I implemented there was inconsistent (apologies!),
> > 
> >   (2) TIOCL_SELMOUSEREPORT might actually be a small security risk
> >       after all, and
> > 
> >   (3) TIOCL_SELMOUSEREPORT is only meant to be used by the mouse
> >       daemon (GPM or Consolation), which runs as CAP_SYS_ADMIN
> >       already.
> 
> 
> Greg and Jared: Friendly ping on this patch.

I think my bot found a problem with the v2 version so I was waiting for
a new one to meet the issues there, right?

Other than that I don't have a problem with this change.

thanks,

greg k-h

Re: [PATCH] tty: Require CAP_SYS_ADMIN for all usages of TIOCL_SELMOUSEREPORT

[Günther Noack]

Hello Greg!

On Fri, Mar 07, 2025 at 12:05:43PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Mar 07, 2025 at 11:16:21AM +0100, Günther Noack wrote:
> > On Sun, Feb 23, 2025 at 09:54:50PM +0100, Günther Noack wrote:
> > > This requirement was overeagerly loosened in commit 2f83e38a095f
> > > ("tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN"), but as
> > > it turns out,
> > > 
> > >   (1) the logic I implemented there was inconsistent (apologies!),
> > > 
> > >   (2) TIOCL_SELMOUSEREPORT might actually be a small security risk
> > >       after all, and
> > > 
> > >   (3) TIOCL_SELMOUSEREPORT is only meant to be used by the mouse
> > >       daemon (GPM or Consolation), which runs as CAP_SYS_ADMIN
> > >       already.
> > 
> > 
> > Greg and Jared: Friendly ping on this patch.
> 
> I think my bot found a problem with the v2 version so I was waiting for
> a new one to meet the issues there, right?

I made a submission mistake with the previous patch, which your bot
tripped over, but you already merged it into master and stable as
commit 2f83e38a095f ("tty: Permit some TIOCL_SETSEL modes without
CAP_SYS_ADMIN"):
https://lore.kernel.org/all/2025011205-spinout-rewrap-2dfa@gregkh/

The patch I am submitting here is a new bugfix on top, for which I am
seeking your approval, since the previous patch is already merged.  (I
should have sent it as a new mail thread, I guess. :-/)

(If that helps, I explained the relationship between these different
patches more visually in the table in
https://lore.kernel.org/all/20250307.9339126c0c96@gnoack.org/.)

Thanks,
–Günther

Re: [PATCH] tty: Require CAP_SYS_ADMIN for all usages of TIOCL_SELMOUSEREPORT

[Greg Kroah-Hartman]

On Fri, Mar 07, 2025 at 02:55:37PM +0100, Günther Noack wrote:
> Hello Greg!
> 
> On Fri, Mar 07, 2025 at 12:05:43PM +0100, Greg Kroah-Hartman wrote:
> > On Fri, Mar 07, 2025 at 11:16:21AM +0100, Günther Noack wrote:
> > > On Sun, Feb 23, 2025 at 09:54:50PM +0100, Günther Noack wrote:
> > > > This requirement was overeagerly loosened in commit 2f83e38a095f
> > > > ("tty: Permit some TIOCL_SETSEL modes without CAP_SYS_ADMIN"), but as
> > > > it turns out,
> > > > 
> > > >   (1) the logic I implemented there was inconsistent (apologies!),
> > > > 
> > > >   (2) TIOCL_SELMOUSEREPORT might actually be a small security risk
> > > >       after all, and
> > > > 
> > > >   (3) TIOCL_SELMOUSEREPORT is only meant to be used by the mouse
> > > >       daemon (GPM or Consolation), which runs as CAP_SYS_ADMIN
> > > >       already.
> > > 
> > > 
> > > Greg and Jared: Friendly ping on this patch.
> > 
> > I think my bot found a problem with the v2 version so I was waiting for
> > a new one to meet the issues there, right?
> 
> I made a submission mistake with the previous patch, which your bot
> tripped over, but you already merged it into master and stable as
> commit 2f83e38a095f ("tty: Permit some TIOCL_SETSEL modes without
> CAP_SYS_ADMIN"):
> https://lore.kernel.org/all/2025011205-spinout-rewrap-2dfa@gregkh/
> 
> The patch I am submitting here is a new bugfix on top, for which I am
> seeking your approval, since the previous patch is already merged.  (I
> should have sent it as a new mail thread, I guess. :-/)
> 
> (If that helps, I explained the relationship between these different
> patches more visually in the table in
> https://lore.kernel.org/all/20250307.9339126c0c96@gnoack.org/.)

Ok, I am totally lost.  Ah, I see this patch now in my queue, it's in my
"grab-bag" of patches to get to "last" as it wasn't cc: to the proper
list (hint, use scripts/get_maintainer.pl, it would have shown you that
the linux-serial list should have been cc:ed.)

So don't worry, it's not lost, just sitting next to a bunch of other
patches I need to review "soon".

thanks,

greg k-h