Suggestion for Capability Check Refinement in check_syslog_permissions()

Suggestion for Capability Check Refinement in check_syslog_permissions()

[孟敬姿]

Hi, we suggest revisiting the capability checks in check_syslog_permissions(). Currently CAP_SYSLOG is checked first, and if it’s not there but there is a CAP_SYS_ADMIN, it can also pass the check. We recommend refining this check to exclusively use CAP_SYSLOG. Here's our reasoning for this suggestion:

(1) Independence of CAP_SYSLOG and CAP_SYS_ADMIN: Since the introduction of CAP_SYSLOG in Linux 2.6.37, it has been a distinct capability from CAP_SYS_ADMIN. For compatibility reasons, it might make sense to keep CAP_SYS_ADMIN at the beginning. However, now that 13 years have passed，we think maybe it's appropriate to update the code to reflect their separate roles. 

(2) Maintaining Least Privilege Principle: CAP_SYS_ADMIN is overloaded and known as the new "root"[1]. And according to the manual page[2] “Don't choose CAP_SYS_ADMIN if you can possibly avoid it!”, it's beneficial to use the most specific capability required for a given task.

This issue exists in several kernel versions and we have checked it on the latest stable release(Linux 6.6.9). 

Your feedback and insights on this proposed modification would be highly appreciated. Thank you for your time and consideration.

Best regards,
Jingzi

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

Re: Suggestion for Capability Check Refinement in check_syslog_permissions()

[Greg KH]

On Wed, Jan 03, 2024 at 01:00:58PM +0800, 孟敬姿 wrote:
> Hi, we suggest revisiting the capability checks in
> check_syslog_permissions(). Currently CAP_SYSLOG is checked first, and
> if it’s not there but there is a CAP_SYS_ADMIN, it can also pass the
> check. We recommend refining this check to exclusively use CAP_SYSLOG.
> Here's our reasoning for this suggestion:

Again, have you tested this?

thanks,

greg k-h

Re: Suggestion for Capability Check Refinement in check_syslog_permissions()

[Petr Mladek]

On Wed 2024-01-03 07:59:18, Greg KH wrote:
> On Wed, Jan 03, 2024 at 01:00:58PM +0800, 孟敬姿 wrote:
> > Hi, we suggest revisiting the capability checks in
> > check_syslog_permissions(). Currently CAP_SYSLOG is checked first, and
> > if it’s not there but there is a CAP_SYS_ADMIN, it can also pass the
> > check. We recommend refining this check to exclusively use CAP_SYSLOG.
> > Here's our reasoning for this suggestion:
> 
> Again, have you tested this?

I guess that Meng is right. The current code looks like:

static int check_syslog_permissions(int type, int source)
{
	/*
	 * If this is from /proc/kmsg and we've already opened it, then we've
	 * already done the capabilities checks at open time.
	 */
	if (source == SYSLOG_FROM_PROC && type != SYSLOG_ACTION_OPEN)
		goto ok;

	if (syslog_action_restricted(type)) {
		if (capable(CAP_SYSLOG))
			goto ok;
		/*
		 * For historical reasons, accept CAP_SYS_ADMIN too, with
		 * a warning.
		 */
		if (capable(CAP_SYS_ADMIN)) {
			pr_warn_once("%s (%d): Attempt to access syslog with "
				     "CAP_SYS_ADMIN but no CAP_SYSLOG "
				     "(deprecated).\n",
				 current->comm, task_pid_nr(current));
			goto ok;
		}
		return -EPERM;
	}
ok:
	return security_syslog(type);
}

And CAP_SYS_ADMIN has really been deprecated last 13 years, see the
commit ee24aebffb75a7f940cf ("cap_syslog: accept CAP_SYS_ADMIN for now").

Maybe, it is really time to remove it.

Best Regards,
Petr

Re: Suggestion for Capability Check Refinement in check_syslog_permissions()

[Steven Rostedt]

On Thu, 4 Jan 2024 10:43:53 +0100
Petr Mladek <pmladek@suse.com> wrote:

> And CAP_SYS_ADMIN has really been deprecated last 13 years, see the
> commit ee24aebffb75a7f940cf ("cap_syslog: accept CAP_SYS_ADMIN for now").
> 
> Maybe, it is really time to remove it.

Perhaps what we should do is to remove it in a simple change that can be
easily reverted if someone complains.

The point is, Linus always says "Don't break user space", but he has even
said that we have broken user API before, but as long as no user space
breaks, it's OK. It's the "tree falls in the forest" analogy. If we break
user space API, but no user space application uses it, did it really break?

If nothing is using CAP_SYS_ADMIN why keep it?

If we remove it, and nobody complains, it means that nothing broke. If we
remove it and someone complains, then we know it's still needed and we
revert the removal just like Linus would do with any other commit that
broke user space.

-- Steve

[PATCH] cap_syslog: remove CAP_SYS_ADMIN when dmesg_restrict

[Jingzi Meng]

CAP_SYSLOG was separated from CAP_SYS_ADMIN and introduced in Linux
2.6.37 (2010-11). For a long time, certain syslog actions required
CAP_SYS_ADMIN or CAP_SYSLOG. Maybe it’s time to officially remove
CAP_SYS_ADMIN for more fine-grained control.

CAP_SYS_ADMIN was once removed but added back for backwards
compatibility reasons. In commit 38ef4c2e437d ("syslog: check cap_syslog
when dmesg_restrict") (2010-12), CAP_SYS_ADMIN was no longer needed. And
in commit ee24aebffb75 ("cap_syslog: accept CAP_SYS_ADMIN for now")
(2011-02), it was accepted again. Since then, CAP_SYS_ADMIN has been
preserved.

Now that almost 13 years have passed, the legacy application may have
had enough time to be updated.

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

In addition, we examined the kernel code and identified that the user api
affected by this change is the syslog system call, which we tested on
the patched kernel to make sure the effect is as expected.

 kernel/printk/printk.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index be95a6851164..6e1153d7de21 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -598,17 +598,6 @@ static int check_syslog_permissions(int type, int source)
 	if (syslog_action_restricted(type)) {
 		if (capable(CAP_SYSLOG))
 			goto ok;
-		/*
-		 * For historical reasons, accept CAP_SYS_ADMIN too, with
-		 * a warning.
-		 */
-		if (capable(CAP_SYS_ADMIN)) {
-			pr_warn_once("%s (%d): Attempt to access syslog with "
-				     "CAP_SYS_ADMIN but no CAP_SYSLOG "
-				     "(deprecated).\n",
-				 current->comm, task_pid_nr(current));
-			goto ok;
-		}
 		return -EPERM;
 	}
 ok:
-- 
2.20.1

Re: [PATCH] cap_syslog: remove CAP_SYS_ADMIN when dmesg_restrict

[Theodore Ts'o]

On Fri, Jan 05, 2024 at 02:20:07PM +0800, Jingzi Meng wrote:
> CAP_SYSLOG was separated from CAP_SYS_ADMIN and introduced in Linux
> 2.6.37 (2010-11). For a long time, certain syslog actions required
> CAP_SYS_ADMIN or CAP_SYSLOG. Maybe it’s time to officially remove
> CAP_SYS_ADMIN for more fine-grained control.
> 
> CAP_SYS_ADMIN was once removed but added back for backwards
> compatibility reasons. In commit 38ef4c2e437d ("syslog: check cap_syslog
> when dmesg_restrict") (2010-12), CAP_SYS_ADMIN was no longer needed. And
> in commit ee24aebffb75 ("cap_syslog: accept CAP_SYS_ADMIN for now")
> (2011-02), it was accepted again. Since then, CAP_SYS_ADMIN has been
> preserved.
> 
> Now that almost 13 years have passed, the legacy application may have
> had enough time to be updated.

What testing have you done to make sure that this is OK?  "May have
had enough time"?  That's not very reassuring?

Also, note that we can't actually reuse the bit position of
CAP_SYS_ADMIN since it's likely that there may be pre-existing
capability masks that are still using that position.  So there isn't
all that much upside in trying to retire CAP_SYS_ADMIN --- if you as a
system administrator think it's not too course, then just don't use
it.

It's unclear to me what goal you have in trying to mess with the
capability definitions?  Perhaps it might be useful if you were to
explicitly state your goals in these proposals?

Thanks,

						- Ted

Re: [PATCH] cap_syslog: remove CAP_SYS_ADMIN when dmesg_restrict

[Petr Mladek]

On Fri 2024-01-05 09:49:44, Theodore Ts'o wrote:
> On Fri, Jan 05, 2024 at 02:20:07PM +0800, Jingzi Meng wrote:
> > CAP_SYSLOG was separated from CAP_SYS_ADMIN and introduced in Linux
> > 2.6.37 (2010-11). For a long time, certain syslog actions required
> > CAP_SYS_ADMIN or CAP_SYSLOG. Maybe it’s time to officially remove
> > CAP_SYS_ADMIN for more fine-grained control.
> > 
> > CAP_SYS_ADMIN was once removed but added back for backwards
> > compatibility reasons. In commit 38ef4c2e437d ("syslog: check cap_syslog
> > when dmesg_restrict") (2010-12), CAP_SYS_ADMIN was no longer needed. And
> > in commit ee24aebffb75 ("cap_syslog: accept CAP_SYS_ADMIN for now")
> > (2011-02), it was accepted again. Since then, CAP_SYS_ADMIN has been
> > preserved.
> > 
> > Now that almost 13 years have passed, the legacy application may have
> > had enough time to be updated.
> 
> What testing have you done to make sure that this is OK?  "May have
> had enough time"?  That's not very reassuring?
> 
> Also, note that we can't actually reuse the bit position of
> CAP_SYS_ADMIN since it's likely that there may be pre-existing
> capability masks that are still using that position.  So there isn't
> all that much upside in trying to retire CAP_SYS_ADMIN --- if you as a
> system administrator think it's not too course, then just don't use
> it.
> 
> It's unclear to me what goal you have in trying to mess with the
> capability definitions?  Perhaps it might be useful if you were to
> explicitly state your goals in these proposals?

My understanding is that this patch is about reducing overlap of
capabilities.

Allowing the same thing with more capabilities seems to go against
the idea of separate capabilities.

Kernel has printed the warning for 13 years. It is a long
time to fix configuration for newly installed systems. And I doubt
that anyone is installing a new kernel on 13 year's old system.

IMHO, this fits into the category that it should be OK until
anyone complains. But I might miss something.

Best Regards,
Petr

Re: [PATCH] cap_syslog: remove CAP_SYS_ADMIN when dmesg_restrict

[孟敬姿]

On Fri 2024-01-05 09:49:44, Theodore Ts'o wrote:
> It's unclear to me what goal you have in trying to mess with the
> capability definitions?  Perhaps it might be useful if you were to
> explicitly state your goals in these proposals?

Petr is right, we are trying to resolve the overlap problem of capability. 

Capability is about to separate superuser privileges into distinct units for 
finer-grained access control. It’s effective work requires the kernel to use 
proper capability for sensitive resources and the user programs to choose the 
right capability instead of ROOT to execute. And we want to promote the 
standardized use of capability.


> So there isn't all that much upside in trying to retire CAP_SYS_ADMIN 

We are not going to retire CAP_SYS_ADMIN, but saying that CAP_SYSLOG is the
more appropriate capability when it comes to protecting syslog functionality. 
CAP_SYS_ADMIN is already overloaded, check out the link[1], narrowing down its 
definitions will facilitate the implementation of least privilege. This adjustment 
makes it clearer for a user program requiring syslog access to specify only the 
necessary capability, CAP_SYSLOG, instead of relying on the broader CAP_SYS_ADMIN.

> What testing have you done to make sure that this is OK?

We booted the modified kernel and confirmed that CAP_SYS_ADMIN no longer has access 
to syslog when dmesg is set.

While certain user programs relying on CAP_SYS_ADMIN for syslog access might be impacted, 
they can adjust their capability configuration using the ‘setcap’ command. Decreasing the 
reliance on CAP_SYS_ADMIN across applications contributes to minimizing security risks in 
the system. Currently, it's uncertain if any such programs exist.


Best regrads,
Jingzi

[1]: https://lwn.net/Articles/486306/

Re: [PATCH] cap_syslog: remove CAP_SYS_ADMIN when dmesg_restrict

[Kees Cook]

On Fri, Jan 05, 2024 at 02:20:07PM +0800, Jingzi Meng wrote:
> CAP_SYSLOG was separated from CAP_SYS_ADMIN and introduced in Linux
> 2.6.37 (2010-11). For a long time, certain syslog actions required
> CAP_SYS_ADMIN or CAP_SYSLOG. Maybe it’s time to officially remove
> CAP_SYS_ADMIN for more fine-grained control.
> 
> CAP_SYS_ADMIN was once removed but added back for backwards
> compatibility reasons. In commit 38ef4c2e437d ("syslog: check cap_syslog
> when dmesg_restrict") (2010-12), CAP_SYS_ADMIN was no longer needed. And
> in commit ee24aebffb75 ("cap_syslog: accept CAP_SYS_ADMIN for now")
> (2011-02), it was accepted again. Since then, CAP_SYS_ADMIN has been
> preserved.
> 
> Now that almost 13 years have passed, the legacy application may have
> had enough time to be updated.

I can't find anything mention this warning in recent times except this:
https://sysdig.com/blog/container-escape-capabilities-falco-detection/
which is mainly an example about how to apply Capabilities, not a
software failure.

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

Reviewed-by: Kees Cook <keescook@chromium.org>

If you can boot a few stock modern distros and verify nothing emits this
warning any more should be sufficient evidence that this can be dropped.

-Kees

-- 
Kees Cook

Re: [PATCH] cap_syslog: remove CAP_SYS_ADMIN when dmesg_restrict

[孟敬姿]

I tested on ubuntu 22.04.3 LTS kernel 6.2.16 and Fedora 39 kernel 6.6.11,
the warning is gone, and CAP_SYS_ADMIN is no longer authorized to bypass 
check_syslog_permissions().

Best regards,
Jingzi

Re: [PATCH] cap_syslog: remove CAP_SYS_ADMIN when dmesg_restrict

[Kees Cook]

On Fri, 05 Jan 2024 14:20:07 +0800, Jingzi Meng wrote:
> CAP_SYSLOG was separated from CAP_SYS_ADMIN and introduced in Linux
> 2.6.37 (2010-11). For a long time, certain syslog actions required
> CAP_SYS_ADMIN or CAP_SYSLOG. Maybe it’s time to officially remove
> CAP_SYS_ADMIN for more fine-grained control.
> 
> CAP_SYS_ADMIN was once removed but added back for backwards
> compatibility reasons. In commit 38ef4c2e437d ("syslog: check cap_syslog
> when dmesg_restrict") (2010-12), CAP_SYS_ADMIN was no longer needed. And
> in commit ee24aebffb75 ("cap_syslog: accept CAP_SYS_ADMIN for now")
> (2011-02), it was accepted again. Since then, CAP_SYS_ADMIN has been
> preserved.
> 
> [...]

Applied to for-next/hardening, thanks!

[1/1] cap_syslog: remove CAP_SYS_ADMIN when dmesg_restrict
      https://git.kernel.org/kees/c/09ce61e27db8

Take care,

-- 
Kees Cook