inappropriate capability checks in tty_ioctl()

inappropriate capability checks in tty_ioctl()

[孟敬姿]

Hi!

We would like to propose an adjustment to the capability checks in the tty_ioctl() function. Currently, the function uses CAP_SYS_ADMIN to protect three subcommands: TIOCCONS, TIOCSTI and TIOCVHANGUP. We propose updating this to use CAP_SYS_TTY_CONFIG instead for the following reasons:

(1) CAP_SYS_TTY_CONFIG is more relevant to the functions: The three subcommands are responsible for tty-related functions: redirecting console output (TIOCCONS), faking input to a terminal (TIOCSTI), and making the terminal be hung up (TIOCVHANGUP). As the definitions in the capability manual page[1], CAP_SYS_TTY_CONFIG is specifically designed for "employing various privileged ioctl(2) operations on virtual terminals." This aligns more closely with the intended usage scenario compared to CAP_SYS_ADMIN.

(2) Consistency: CAP_SYS_TTY_CONFIG is already employed in other parts of the kernel to protect TIOCVHANGUP-like functionality. For instance, in tty_ioctl() CAP_SYS_ADMIN is used before tty_vhangup(), while in SYSCALL_DEFINE0(vhangup), which located in fs/open.c, the check is done with CAP_SYS_TTY_CONFIG before tty_vhangup().

(3) Maintaining Least Privilege: CAP_SYS_ASMIN is already overloaded and known as the new "root"[2]. According to the manual page[1] “don't choose CAP_SYS_ADMIN if you can possibly avoid it”, switching to CAP_SYS_TTY_CONFIG could be helpful for standardizing the use of capabilities and implementing least privileges. 

This issue exists in several kernel versions and we have checked it on the latest stable release(Linux 6.6.9). We would appreciate your thoughts and feedback on this proposal. Thank you for your time and consideration.

Best regards,
Jingzi

reference:
[1] https://www.man7.org/linux/man-pages/man7/capabilities.7.html
[2] https://lwn.net/Articles/486306/

Re: inappropriate capability checks in tty_ioctl()

[Greg KH]

On Tue, Jan 02, 2024 at 07:38:31PM +0800, 孟敬姿 wrote:
> Hi!
> 
> We would like to propose an adjustment to the capability checks in the tty_ioctl() function. Currently, the function uses CAP_SYS_ADMIN to protect three subcommands: TIOCCONS, TIOCSTI and TIOCVHANGUP. We propose updating this to use CAP_SYS_TTY_CONFIG instead for the following reasons:
> 
> (1) CAP_SYS_TTY_CONFIG is more relevant to the functions: The three subcommands are responsible for tty-related functions: redirecting console output (TIOCCONS), faking input to a terminal (TIOCSTI), and making the terminal be hung up (TIOCVHANGUP). As the definitions in the capability manual page[1], CAP_SYS_TTY_CONFIG is specifically designed for "employing various privileged ioctl(2) operations on virtual terminals." This aligns more closely with the intended usage scenario compared to CAP_SYS_ADMIN.
> 
> (2) Consistency: CAP_SYS_TTY_CONFIG is already employed in other parts of the kernel to protect TIOCVHANGUP-like functionality. For instance, in tty_ioctl() CAP_SYS_ADMIN is used before tty_vhangup(), while in SYSCALL_DEFINE0(vhangup), which located in fs/open.c, the check is done with CAP_SYS_TTY_CONFIG before tty_vhangup().
> 
> (3) Maintaining Least Privilege: CAP_SYS_ASMIN is already overloaded and known as the new "root"[2]. According to the manual page[1] “don't choose CAP_SYS_ADMIN if you can possibly avoid it”, switching to CAP_SYS_TTY_CONFIG could be helpful for standardizing the use of capabilities and implementing least privileges. 
> 
> This issue exists in several kernel versions and we have checked it on the latest stable release(Linux 6.6.9). We would appreciate your thoughts and feedback on this proposal. Thank you for your time and consideration.

What would break if you made such a change?  Have you tried it and
tested it out?

Also, if you wish to have a change accepted, we need an actual patch to
apply, that shows you did the work and research to ensure that it will
work properly.

thanks,

greg k-h

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

[Jingzi Meng]

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

[Greg KH]

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

[孟敬姿]

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


> Why did you just change these 3 usages, and not all of them?  Why are
> 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.

> 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?

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

[Greg KH]

On Mon, Jan 15, 2024 at 05:55:22PM +0800, 孟敬姿 wrote:
> > 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

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

[Greg KH]

On Mon, Jan 15, 2024 at 11:10:29PM +0800, 孟敬姿 wrote:
> 
> 在 2024-1-15 16:35, Greg KH 写道:
> > 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?
> > 
> First of all, this change is not about functionality, only about permissions.

I'm confused, permissions dictate functionality.

If I do not have permissions to do something, the functionality is
suddenly not there.  That's what you are doing here, you are changing
the requirement of previously one capability was required to achive a
function in the kernel, to a different capability.

So it is ALL ABOUT functionality here.

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

I'm referring to existing programs that are not modified to have the new
capability that you are now requiring.  Who is going to do that work?
Where are they?  How have you searched to find them?

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

That's not the issue, the issue is "what existing software is going to
break with this change in permission handling."

That needs to be answered first, and I suggest some research into how we
HAVE TO keep backwards compability and can not break it.

Without some more proof, this type of change can never be accepted, nor
would you want it to be.

good luck!

greg k-h