tty: change the privilege required for tty operarions

[PATCH] tty: change the privilege required for tty operarions

Posted by Jingzi Meng 1 year, 11 months ago

Currently, CAP_SYS_ADMIN is responsible for tty-related functions in
tty_ioctl(): TIOCSTI, TIOCCONS, TIOCVHANGUP. CAP_SYS_ADMIN is already
overloaded, change it to CAP_SYS_TTY_CONFIG for a more fine-grained
and accurate access control.

Signed-off-by: Jingzi Meng <mengjingzi@iie.ac.cn>
---

The userland api affected by this change is the ioctl system call,
especially when the second argument is TIOCSTI, TIOCCONS, TIOCVHANGUP,
which now requires sys_tty_config instead of sys_admin. Tested on Debian
with kernel 6.7.0-rc5.

 drivers/tty/tty_io.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index f3ca2105b66d..c81479366317 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2286,7 +2286,7 @@ static int tiocsti(struct tty_struct *tty, u8 __user *p)
 	if (!tty_legacy_tiocsti && !capable(CAP_SYS_ADMIN))
 		return -EIO;
 
-	if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN))
+	if ((current->signal->tty != tty) && !capable(CAP_SYS_TTY_CONFIG))
 		return -EPERM;
 	if (get_user(ch, p))
 		return -EFAULT;
@@ -2390,7 +2390,7 @@ static int tiocswinsz(struct tty_struct *tty, struct winsize __user *arg)
  */
 static int tioccons(struct file *file)
 {
-	if (!capable(CAP_SYS_ADMIN))
+	if (!capable(CAP_SYS_TTY_CONFIG))
 		return -EPERM;
 	if (file->f_op->write_iter == redirected_tty_write) {
 		struct file *f;
@@ -2719,7 +2719,7 @@ long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case TIOCSETD:
 		return tiocsetd(tty, p);
 	case TIOCVHANGUP:
-		if (!capable(CAP_SYS_ADMIN))
+		if (!capable(CAP_SYS_TTY_CONFIG))
 			return -EPERM;
 		tty_vhangup(tty);
 		return 0;
-- 
2.20.1

Re: [PATCH] tty: change the privilege required for tty operarions

Posted by Greg KH 1 year, 11 months ago

On Mon, Jan 15, 2024 at 04:24:20PM +0800, Jingzi Meng wrote:
> Currently, CAP_SYS_ADMIN is responsible for tty-related functions in
> tty_ioctl(): TIOCSTI, TIOCCONS, TIOCVHANGUP. CAP_SYS_ADMIN is already
> overloaded, change it to CAP_SYS_TTY_CONFIG for a more fine-grained
> and accurate access control.
> 
> Signed-off-by: Jingzi Meng <mengjingzi@iie.ac.cn>
> ---
> 
> The userland api affected by this change is the ioctl system call,
> especially when the second argument is TIOCSTI, TIOCCONS, TIOCVHANGUP,
> which now requires sys_tty_config instead of sys_admin. Tested on Debian
> with kernel 6.7.0-rc5.

Tested how?  You are changing the permissions of a kernel operation,
which is arguably, going to break userspace in lots of interesting ways
unless you can prove that this is functionally the same as the existing
code.

And not all the world is Debian (although lots of it is, yes.)  But
actually running programs that exercise this kernel codepath is going to
be the key, did you do that?




> 
>  drivers/tty/tty_io.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index f3ca2105b66d..c81479366317 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -2286,7 +2286,7 @@ static int tiocsti(struct tty_struct *tty, u8 __user *p)
>  	if (!tty_legacy_tiocsti && !capable(CAP_SYS_ADMIN))
>  		return -EIO;
>  
> -	if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN))
> +	if ((current->signal->tty != tty) && !capable(CAP_SYS_TTY_CONFIG))
>  		return -EPERM;
>  	if (get_user(ch, p))
>  		return -EFAULT;
> @@ -2390,7 +2390,7 @@ static int tiocswinsz(struct tty_struct *tty, struct winsize __user *arg)
>   */
>  static int tioccons(struct file *file)
>  {
> -	if (!capable(CAP_SYS_ADMIN))
> +	if (!capable(CAP_SYS_TTY_CONFIG))
>  		return -EPERM;
>  	if (file->f_op->write_iter == redirected_tty_write) {
>  		struct file *f;
> @@ -2719,7 +2719,7 @@ long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  	case TIOCSETD:
>  		return tiocsetd(tty, p);
>  	case TIOCVHANGUP:
> -		if (!capable(CAP_SYS_ADMIN))
> +		if (!capable(CAP_SYS_TTY_CONFIG))
>  			return -EPERM;
>  		tty_vhangup(tty);
>  		return 0;

Why did you just change these 3 usages, and not all of them?  Why are
these "safe" but the others not?

And most importantly of all, why make this change at all?  Who is using
capabilities these days in a fine-grained way to warrent this type of
modification?

thanks,

greg k-h

Re: [PATCH] tty: change the privilege required for tty operarions

Posted by 孟敬姿 1 year, 11 months ago

&gt; Tested how?  

First of all, this change is not about functionality, only about permissions. 
I wrote 3 testcases which calls ioctl() with TIOCSTI, TIOCCONS, TIOCVHANGUP 
respectively. Then execute them on the origin kernel and patched kernel. 
Running it on both sets of kernels gives the same result. However, through
the system error message, and the kernel log output I added, I confirmed 
that the relevant functionality under the origin kernel requires sys_admin, 
and under the patched kernel requires sys_tty_config.

Indeed, it doesn't have much to do with the distro either, I just tested it 
on Debian, and similar tests can be done on other distros.


&gt; Why did you just change these 3 usages, and not all of them?  Why are
&gt; these "safe" but the others not?

There are 5 capability checks in this file, all using sys_admin. The first 
one is in the tiocsti function, in commit 690c8b804ad2 ("TIOCSTI: always 
enable for CAP_SYS_ADMIN"), sys_admin was introduced for a special 
application BRLTTY, so I hesitated to change it. In fact, BRLTTY has both 
sys_admin and sys_tty_config, so modify the kernel's permission check, will 
not affect BRLTTY's function. The other permission check is located in a 
different function, not triggered by ioctl, so it's not written together.

&gt; And most importantly of all, why make this change at all?  Who is using
&gt; capabilities these days in a fine-grained way to warrent this type of
&gt; modification?

I guess you are right, there's not a lot of people using capabilities. 
But the idea of a least privilege design is still tempting from a 
security point of view. The scarce use of capabilities is related to 
its problematic implementation, and we hope to promote its use by 
improving its implementation. sys_admin is a relatively large privilege, 
which may pose a risk to the system （check the cve list:
https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=CAP_SYS_ADMIN ), and by 
reducing the unnecessary use of sys_admin, we can avoid some of the risks. 
In particular, the linux capability has been designed with sys_tty_config, 
so it should take over the privileges originally managed by sys_admin 
related to TTY.

Best regards,
Jingzi

Re: [PATCH] tty: change the privilege required for tty operarions

Posted by Greg KH 1 year, 11 months ago

On Mon, Jan 15, 2024 at 05:55:22PM +0800, 孟敬姿 wrote:
> &gt; Tested how?  

I have no context here at all, sorry.

Please properly quote your emails when responding, remember some of us
get 1000+ emails a day to deal with.

And are you trying to type html formatting somehow?

confused,

greg k-h