Re: [PATCH v3 1/1] tty: Restrict access to TIOCLINUX' copy-and-paste subcommands

["Mickaël Salaün" <mic@digikod.net>]

On Mon, Aug 28, 2023 at 06:41:17PM +0200, Günther Noack wrote:
> From: Hanno Böck <hanno@hboeck.de>
> 
> TIOCLINUX can be used for privilege escalation on virtual terminals when
> code is executed via tools like su/sudo and sandboxing tools.
> 
> By abusing the selection features, a lower-privileged application can
> write content to the console, select and copy/paste that content and
> thereby executing code on the privileged account. See also the poc
> here:
> 
>   https://www.openwall.com/lists/oss-security/2023/03/14/3
> 
> Selection is usually used by tools like gpm that provide mouse features
> on the virtual console. gpm already runs as root (due to earlier
> changes that restrict access to a user on the current TTY), therefore
> it will still work with this change.
> 
> With this change, the following TIOCLINUX subcommands require
> CAP_SYS_ADMIN:
> 
>  * TIOCL_SETSEL - setting the selected region on the terminal
>  * TIOCL_PASTESEL - pasting the contents of the selected region into
>    the input buffer
>  * TIOCL_SELLOADLUT - changing word-by-word selection behaviour
> 
> The security problem mitigated is similar to the security risks caused
> by TIOCSTI, which, since kernel 6.2, can be disabled with
> CONFIG_LEGACY_TIOCSTI=n.
> 
> Signed-off-by: Hanno Böck <hanno@hboeck.de>
> Signed-off-by: Günther Noack <gnoack@google.com>

The SoB rules are tricky, you cannot have a Signed-off-by if you are not
in the From/Author or Committer or Co-Developed-by fields:
https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

It should be:

Co-Developed-by: Günther Noack <gnoack@google.com>
Signed-off-by: Günther Noack <gnoack@google.com>
Signed-off-by: Hanno Böck <hanno@hboeck.de>

> Tested-by: Günther Noack <gnoack@google.com>

This Tested-by should not be required anymore because of your SoB,
which should implicitly stipulate that you tested this patch.

I'm not sure if it's worth sending another version with only this fix
though, if there is no more issue I guess the maintainer picking it
could fix it.


> ---
>  drivers/tty/vt/vt.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 1e8e57b45688..1eb30ed1118d 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -3156,9 +3156,13 @@ 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((struct tiocl_selection
>  					 __user *)(p+1), tty);
>  	case TIOCL_PASTESEL:
> +		if (!capable(CAP_SYS_ADMIN))
> +			return -EPERM;
>  		return paste_selection(tty);
>  	case TIOCL_UNBLANKSCREEN:
>  		console_lock();
> @@ -3166,6 +3170,8 @@ int tioclinux(struct tty_struct *tty, unsigned long arg)
>  		console_unlock();
>  		break;
>  	case TIOCL_SELLOADLUT:
> +		if (!capable(CAP_SYS_ADMIN))
> +			return -EPERM;
>  		console_lock();
>  		ret = sel_loadlut(p);
>  		console_unlock();
> -- 
> 2.42.0.rc2.253.gd59a3bf2b4-goog
>