[PATCH v3] scsi: require CAP_SYS_ADMIN to write to procfs interface

[PATCH v3] scsi: require CAP_SYS_ADMIN to write to procfs interface

[Aleksa Sarai]

Previously, the only capability effectively required to operate on the
/proc/scsi interface was CAP_DAC_OVERRIDE (or for some other files,
having an fsuid of GLOBAL_ROOT_UID was enough). This means that
semi-privileged processes could interfere with core components of a
system (such as causing a DoS by removing the underlying SCSI device of
the host's / mount).

Cc: <stable@vger.kernel.org>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Aleksa Sarai <asarai@suse.de>
---
 drivers/scsi/scsi_proc.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_proc.c b/drivers/scsi/scsi_proc.c
index 480a597b3877..05d70e200c5f 100644
--- a/drivers/scsi/scsi_proc.c
+++ b/drivers/scsi/scsi_proc.c
@@ -27,6 +27,7 @@
 #include <linux/mutex.h>
 #include <linux/gfp.h>
 #include <linux/uaccess.h>
+#include <linux/capability.h>
 
 #include <scsi/scsi.h>
 #include <scsi/scsi_device.h>
@@ -51,7 +52,10 @@ static ssize_t proc_scsi_host_write(struct file *file, const char __user *buf,
 	struct Scsi_Host *shost = PDE_DATA(file_inode(file));
 	ssize_t ret = -ENOMEM;
 	char *page;
-    
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
 	if (count > PROC_BLOCK_SIZE)
 		return -EOVERFLOW;
 
@@ -313,6 +317,9 @@ static ssize_t proc_scsi_write(struct file *file, const char __user *buf,
 	char *buffer, *p;
 	int err;
 
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
 	if (!buf || length > PAGE_SIZE)
 		return -EINVAL;
 
-- 
2.14.3

Re: [PATCH v3] scsi: require CAP_SYS_ADMIN to write to procfs interface

[Aleksa Sarai]

On 11/05/2017 01:56 PM, Aleksa Sarai wrote:
> Previously, the only capability effectively required to operate on the
> /proc/scsi interface was CAP_DAC_OVERRIDE (or for some other files,
> having an fsuid of GLOBAL_ROOT_UID was enough). This means that
> semi-privileged processes could interfere with core components of a
> system (such as causing a DoS by removing the underlying SCSI device of
> the host's / mount).

An alternative to this patch would be to make the open(2) call fail, if 
you try to open it write-only or read-write. Not sure which would be 
preferred (should it be possible to pass /proc/scsi/scsi to a 
semi-privileged process to write to?).

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/

Re: [PATCH v3] scsi: require CAP_SYS_ADMIN to write to procfs interface

[Greg KH]

On Sun, Nov 05, 2017 at 01:56:35PM +1100, Aleksa Sarai wrote:
> Previously, the only capability effectively required to operate on the
> /proc/scsi interface was CAP_DAC_OVERRIDE (or for some other files,
> having an fsuid of GLOBAL_ROOT_UID was enough). This means that
> semi-privileged processes could interfere with core components of a
> system (such as causing a DoS by removing the underlying SCSI device of
> the host's / mount).

Given that the previous patch didn't even compile, I worry that you have
not tested this at all to see what breaks/changes in userspace with this
type of user-visable api change.

What did you do to test this?

thanks,

greg k-h

Re: [PATCH v3] scsi: require CAP_SYS_ADMIN to write to procfs interface

[Aleksa Sarai]

I've booted it on a few of my laptops, and nothing seemed to break. Is
there a particular test-suite you'd recommend that I run?

On Sun, Nov 5, 2017 at 6:31 PM, Greg KH <greg@kroah.com> wrote:
> On Sun, Nov 05, 2017 at 01:56:35PM +1100, Aleksa Sarai wrote:
>> Previously, the only capability effectively required to operate on the
>> /proc/scsi interface was CAP_DAC_OVERRIDE (or for some other files,
>> having an fsuid of GLOBAL_ROOT_UID was enough). This means that
>> semi-privileged processes could interfere with core components of a
>> system (such as causing a DoS by removing the underlying SCSI device of
>> the host's / mount).
>
> Given that the previous patch didn't even compile, I worry that you have
> not tested this at all to see what breaks/changes in userspace with this
> type of user-visable api change.
>
> What did you do to test this?
>
> thanks,
>
> greg k-h



-- 
Aleksa Sarai (cyphar)
www.cyphar.com

Re: [PATCH v3] scsi: require CAP_SYS_ADMIN to write to procfs interface

[Greg KH]

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Sun, Nov 05, 2017 at 08:11:20PM +1100, Aleksa Sarai wrote:
> I've booted it on a few of my laptops, and nothing seemed to break. Is
> there a particular test-suite you'd recommend that I run?

Workloads/tools that normally interact with this file would be the best
ones, right?  Odds are, your normal "laptop booting/running" mode does
not do anything with this file.

thanks,

greg k-h

Re: [PATCH v3] scsi: require CAP_SYS_ADMIN to write to procfs interface

[Eric W. Biederman]

Aleksa Sarai <asarai@suse.de> writes:

> On 11/05/2017 01:56 PM, Aleksa Sarai wrote:
>> Previously, the only capability effectively required to operate on the
>> /proc/scsi interface was CAP_DAC_OVERRIDE (or for some other files,
>> having an fsuid of GLOBAL_ROOT_UID was enough). This means that
>> semi-privileged processes could interfere with core components of a
>> system (such as causing a DoS by removing the underlying SCSI device of
>> the host's / mount).
>
> An alternative to this patch would be to make the open(2) call fail, if you try
> to open it write-only or read-write. Not sure which would be preferred (should
> it be possible to pass /proc/scsi/scsi to a semi-privileged process to write
> to?).

Making open fail is very much the preferred solution.

Testing for permission on write can be avoided by finding a suid root
application whose error output acts like a suid cat.

The best current practice for adding this kind of permission check is to
add the check in open.  For some older use cases where we made this
mistake we had to maintian a check during write to avoid breaking
userspace.  But as this check is new there is no reason to add a check
anywhere except in open.

Eric