Re: [06/07] [PATCH] SCSI tape security: require CAP_ADMIN for SG_IO etc.

Re: [06/07] [PATCH] SCSI tape security: require CAP_ADMIN for SG_IO etc.

[Alan Cox]

On Mer, 2005-04-27 at 18:16, Greg KH wrote:
> -stable review patch.  If anyone has any objections, please let us know.

This patch is just wrong on so many different levels its hard to know
where to begin.

1. The auth for arbitary commands is CAP_SYS_RAWIO
2. "The SCSI command permissions were discussed widely on the linux
lists but this did not result in any useful refinement of the
permissions." - this is false. The process was refined, a table setup
was added and debugged. Someone even wrote an fs for managing it that is
not yet merged. Perhaps the patch author would care to re-read the
archives and submit a new patch if one is even needed
3. Pleas explain *what* the specific consistency problems are

And then please fix the same mess in 12rc.

Alan

[06/07] [PATCH] SCSI tape security: require CAP_ADMIN for SG_IO etc.

[Greg KH]

-stable review patch.  If anyone has any objections, please let us know.

------------------

The kernel currently allows any user permitted to access the tape device file
to send the tape drive commands that may either make the tape drivers internal
state inconsistent or to change the drive parameters so that other users find
the drive to be unusable. This patch changes ioctl handling so that SG_IO,
SCSI_IOCTL_COMMAND, etc. require CAP_ADMIN. This solves the consistency
problems for SCSI tapes. The st driver provides user-accessible commands to
change the drive parameters that users may need to access.

The SCSI command permissions were discussed widely on the linux lists but this
did not result in any useful refinement of the permissions. It may very well
be that the tape drives are the only devices that users are sometimes given
permissions to access and that have security problems with the current command
filtering. This patch solves the problem for tapes and no more elaborate
patches are needed.

Signed-off-by: Kai Makisara <kai.makisara@kolumbus.fi>
Signed-off-by: James Bottomley <James.Bottomley@SteelEye.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>


diff -Naru a/drivers/scsi/st.c b/drivers/scsi/st.c
--- a/drivers/scsi/st.c	2005-04-27 09:50:24 -07:00
+++ b/drivers/scsi/st.c	2005-04-27 09:50:24 -07:00
@@ -3461,11 +3461,17 @@
 		case SCSI_IOCTL_GET_BUS_NUMBER:
 			break;
 		default:
-			i = scsi_cmd_ioctl(file, STp->disk, cmd_in, p);
+			if (!capable(CAP_SYS_ADMIN))
+				i = -EPERM;
+			else
+				i = scsi_cmd_ioctl(file, STp->disk, cmd_in, p);
 			if (i != -ENOTTY)
 				return i;
 			break;
 	}
+	if (!capable(CAP_SYS_ADMIN) &&
+	    (cmd_in == SCSI_IOCTL_START_UNIT || cmd_in == SCSI_IOCTL_STOP_UNIT))
+		return -EPERM;
 	return scsi_ioctl(STp->device, cmd_in, p);
 
  out:

Re: [06/07] [PATCH] SCSI tape security: require CAP_ADMIN for SG_IO etc.

[Alan Cox]

On Mer, 2005-04-27 at 19:26, Greg KH wrote:
> > This patch is just wrong on so many different levels its hard to know
> > where to begin.
> 
> But that is what is now in mainline, right?  If so, all of these
> questions still pertain to the current tree...

Correct, the 12rc code is also completely wrong. Good job we caught it
since its apparently not been reviewed properly until now.

Re: [06/07] [PATCH] SCSI tape security: require CAP_ADMIN for SG_IO etc.

[Greg KH]

On Wed, Apr 27, 2005 at 05:38:49PM +0100, Alan Cox wrote:
> On Mer, 2005-04-27 at 18:16, Greg KH wrote:
> > -stable review patch.  If anyone has any objections, please let us know.
> 
> This patch is just wrong on so many different levels its hard to know
> where to begin.

But that is what is now in mainline, right?  If so, all of these
questions still pertain to the current tree...

> 1. The auth for arbitary commands is CAP_SYS_RAWIO
> 2. "The SCSI command permissions were discussed widely on the linux
> lists but this did not result in any useful refinement of the
> permissions." - this is false. The process was refined, a table setup
> was added and debugged. Someone even wrote an fs for managing it that is
> not yet merged. Perhaps the patch author would care to re-read the
> archives and submit a new patch if one is even needed
> 3. Pleas explain *what* the specific consistency problems are
> 
> And then please fix the same mess in 12rc.

thanks,

greg k-h

Re: [06/07] [PATCH] SCSI tape security: require CAP_ADMIN for SG_IO etc.

[Kai Makisara]

On Wed, 27 Apr 2005, Alan Cox wrote:

> On Mer, 2005-04-27 at 18:16, Greg KH wrote:
> > -stable review patch.  If anyone has any objections, please let us know.
> 
> This patch is just wrong on so many different levels its hard to know
> where to begin.
> 
> 1. The auth for arbitary commands is CAP_SYS_RAWIO

Valid complaint.

> 2. "The SCSI command permissions were discussed widely on the linux
> lists but this did not result in any useful refinement of the
> permissions." - this is false. The process was refined, a table setup
> was added and debugged.

Any user having write access to the device is still allowed to send MODE 
SELECT (and some other commands useful for CD/DVD writers but being 
potentially dangerous to other). The assumption that _any_ command needed 
for burning CDs/DVDs is safe for all device types is ridiculous. This is 
why I don't consider the refinements useful.

Controlling high-level access and pass-through with the same permissions 
is not a very good model.

> Someone even wrote an fs for managing it that is
> not yet merged.

Yes. So we should wait for a miracle happen and it get merged? My patch 
is meant to fix the problems at low level until a more general fix is 
merged (if that ever happens).

> Perhaps the patch author would care to re-read the
> archives and submit a new patch if one is even needed
> 3. Pleas explain *what* the specific consistency problems are

OK. Once again....

Using MODE SELECT you can change the drive behaviour so that it may become 
practically useless for other users (and even break very quickly). A 
simple method is to disable drive buffering. The inconsistency here is 
that to the users the drive status is not what the system managers meant 
it to be.

Another inconsistency comes from using commands moving the tape so that 
the tape driver does not know it. The tape driver provides users some 
status information about the tape head location (file and block number, 
BOT, EOF, etc.). This information is not valid if tape is moved using 
pass-through. The basic problem is that the driver for this kind of a 
sequential access device has to maintain some state information and it 
gets distorted if pass-through is used. One solution would, of course, be 
to interpret the commands and statuses going to/coming from pass-through 
but this is a quite heavy solution.


OK. If the Linux solution to these kind of security problems in the not so 
central areas of kernel is to wait and see if the problem disappears 
without any action, I have to accept that. But I have tried...

-- 
Kai

Re: [06/07] [PATCH] SCSI tape security: require CAP_ADMIN for SG_IO etc.

[Arjan van de Ven]

On Thu, 2005-04-28 at 08:43 +0300, Kai Makisara wrote:
> On Wed, 27 Apr 2005, Alan Cox wrote:
> 
> > On Mer, 2005-04-27 at 18:16, Greg KH wrote:
> > > -stable review patch.  If anyone has any objections, please let us know.
> > 
> > This patch is just wrong on so many different levels its hard to know
> > where to begin.
> > 
> > 1. The auth for arbitary commands is CAP_SYS_RAWIO
> 
> Valid complaint.
> 
> > 2. "The SCSI command permissions were discussed widely on the linux
> > lists but this did not result in any useful refinement of the
> > permissions." - this is false. The process was refined, a table setup
> > was added and debugged.
> 
> Any user having write access to the device is still allowed to send MODE 
> SELECT (and some other commands useful for CD/DVD writers but being 
> potentially dangerous to other). 

If you give your user *WRITE ACCESS* to the tape you expect him to be
able to do a lot of writing, right? The restrictions for *READ* are
obviously more clear...

> OK. If the Linux solution to these kind of security problems in the not so 
> central areas of kernel is to wait and see if the problem disappears 
> without any action, I have to accept that. But I have tried...

the security problem is giving someone write access to a device and then
somehow expect that to mean "selective write" ?

Re: [06/07] [PATCH] SCSI tape security: require CAP_ADMIN for SG_IO etc.

[Alan Cox]

On Iau, 2005-04-28 at 06:43, Kai Makisara wrote:
> Any user having write access to the device is still allowed to send MODE 
> SELECT (and some other commands useful for CD/DVD writers but being 
> potentially dangerous to other). The assumption that _any_ command needed 
> for burning CDs/DVDs is safe for all device types is ridiculous. This is 
> why I don't consider the refinements useful.

Ok thats the bit I needed to know

> Using MODE SELECT you can change the drive behaviour so that it may become 
> practically useless for other users (and even break very quickly). A 
> simple method is to disable drive buffering. The inconsistency here is 
> that to the users the drive status is not what the system managers meant 
> it to be.

Equally users will want to change the drive status and in some
situations will be changing the drive status habitually when using the
tape devices. That seems to have tape level ioctls anyway.

> Another inconsistency comes from using commands moving the tape so that 
> the tape driver does not know it. The tape driver provides users some 
> status information about the tape head location (file and block number, 
> BOT, EOF, etc.). This information is not valid if tape is moved using 
> pass-through. The basic problem is that the driver for this kind of a 
> sequential access device has to maintain some state information and it 
> gets distorted if pass-through is used. One solution would, of course, be 
> to interpret the commands and statuses going to/coming from pass-through 
> but this is a quite heavy solution.

That isn't a problem. The other drivers and O_DIRECT all take the same
basic approach that users doing raw I/O commands can shoot themselves in
the feet. Anything else stops people doing clever things they need to.
What it must not allow (as you explained in your example with mode
select) is let people shoot each other in the feet.

On the info you give making it CAP_SYS_RAWIO for now does appear to make
sense. A tape safe command or filter list might be better eventually
(and/or making the driver put the state back right on open - which may
also be neccessary when drives get powered off independantly of the
host...)

Alan

Re: [06/07] [PATCH] SCSI tape security: require CAP_ADMIN for SG_IO etc.

[Greg KH]

On Thu, Apr 28, 2005 at 02:21:52PM +0100, Alan Cox wrote:
> On Iau, 2005-04-28 at 06:43, Kai Makisara wrote:
> > Any user having write access to the device is still allowed to send MODE 
> > SELECT (and some other commands useful for CD/DVD writers but being 
> > potentially dangerous to other). The assumption that _any_ command needed 
> > for burning CDs/DVDs is safe for all device types is ridiculous. This is 
> > why I don't consider the refinements useful.
> 
> Ok thats the bit I needed to know

So, do you still object to this patch being accepted?

thanks,

greg k-h

Re: [06/07] [PATCH] SCSI tape security: require CAP_ADMIN for SG_IO etc.

[Alan Cox]

On Gwe, 2005-04-29 at 05:20, Greg KH wrote:
> > Ok thats the bit I needed to know
> 
> So, do you still object to this patch being accepted?

Switched to CAP_SYS_RAWIO I don't. Its the wrong answer long term I
suspect but its definitely a good answer for now.

Alan

Re: [06/07] [PATCH] SCSI tape security: require CAP_ADMIN for SG_IO etc.

[Greg KH]

On Fri, Apr 29, 2005 at 09:16:27PM +0100, Alan Cox wrote:
> On Gwe, 2005-04-29 at 05:20, Greg KH wrote:
> > > Ok thats the bit I needed to know
> > 
> > So, do you still object to this patch being accepted?
> 
> Switched to CAP_SYS_RAWIO I don't. Its the wrong answer long term I
> suspect but its definitely a good answer for now.

Switch it in both capable() calls in the patch?  Kai, is this acceptable
to you also?

thanks,

greg k-h

Re: [06/07] [PATCH] SCSI tape security: require CAP_ADMIN for SG_IO etc.

[Greg KH]

On Sat, Apr 30, 2005 at 08:52:31AM +0300, Kai Makisara wrote:
> On Fri, 29 Apr 2005, Greg KH wrote:
> 
> > On Fri, Apr 29, 2005 at 09:16:27PM +0100, Alan Cox wrote:
> > > On Gwe, 2005-04-29 at 05:20, Greg KH wrote:
> > > > > Ok thats the bit I needed to know
> > > > 
> > > > So, do you still object to this patch being accepted?
> > > 
> > > Switched to CAP_SYS_RAWIO I don't. Its the wrong answer long term I
> > > suspect but its definitely a good answer for now.
> > 
> > Switch it in both capable() calls in the patch?  Kai, is this acceptable
> > to you also?
> > 
> Yes. Using CAP_SYS_ADMIN here was wrong.

Ok, care to send a new patch that I can use for the next -stable kernel
release?

thanks,

greg k-h

Re: [06/07] [PATCH] SCSI tape security: require CAP_ADMIN for SG_IO etc.

[Kai Makisara]

On Fri, 29 Apr 2005, Greg KH wrote:

> On Fri, Apr 29, 2005 at 09:16:27PM +0100, Alan Cox wrote:
> > On Gwe, 2005-04-29 at 05:20, Greg KH wrote:
> > > > Ok thats the bit I needed to know
> > > 
> > > So, do you still object to this patch being accepted?
> > 
> > Switched to CAP_SYS_RAWIO I don't. Its the wrong answer long term I
> > suspect but its definitely a good answer for now.
> 
> Switch it in both capable() calls in the patch?  Kai, is this acceptable
> to you also?
> 
Yes. Using CAP_SYS_ADMIN here was wrong.

-- 
Kai

Re: [06/07] [PATCH] SCSI tape security: require CAP_ADMIN for SG_IO etc.

[Kai Makisara]

On Fri, 29 Apr 2005, Greg KH wrote:

> On Sat, Apr 30, 2005 at 08:52:31AM +0300, Kai Makisara wrote:
> > On Fri, 29 Apr 2005, Greg KH wrote:
> > 
...
> > > Switch it in both capable() calls in the patch?  Kai, is this acceptable
> > > to you also?
> > > 
> > Yes. Using CAP_SYS_ADMIN here was wrong.
> 
> Ok, care to send a new patch that I can use for the next -stable kernel
> release?
> 
Sent in a different message. This patch does not restrict 
usage of SCSI_IOCTL_START_UNIT and SCSI_IOCTL_STOP_UNIT. For tapes, those 
mean load and unload commands. The drive status changes resulting from 
these commands seem to be caught by st otherwise. I will do a patch for 
-12rc later today or tomorrow. It may add changing some st status bits if 
these commands succeed but that is not -stable material.

Thanks for the review for all participants.

-- 
Kai