Subject: Re: [PATCH v2] Setup attribute for fixed_disk_device and
 removable_device
To:     "Sugar, David" <dsugar@tresys.com>,
        "selinux-refpolicy@vger.kernel.org" 
        <selinux-refpolicy@vger.kernel.org>
References: <20190313181804.10224-1-dsugar@tresys.com>
 <20190313181804.10224-2-dsugar@tresys.com>
From:   Chris PeBenito <pebenito@ieee.org>
Message-ID: <06720853-7f0a-7ed4-b180-7d6b6b4d4a3f@ieee.org>
Date:   Thu, 14 Mar 2019 18:06:32 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.3.0
MIME-Version: 1.0
In-Reply-To: <20190313181804.10224-2-dsugar@tresys.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: selinux-refpolicy-owner@vger.kernel.org
Precedence: bulk

On 3/13/19 2:18 PM, Sugar, David wrote:
> I am having trouble with some denials due to the fact I am setting
> up specific private types for media attached to my system.  This
> changes to use an attribute for media and interfaces to add types
> to the newly created attribute

What you implemented doesn't seem consistent with what you have in the 
commit message.  sr0 is in your example denials, so these aren't all 
fixed disk devices, so the interface names and the attribute names 
should be related to all storage devices, it would seem.


> Updates based on feedback to create 'all' interfaces rather than
> updating existing interfaces.
> 
> I'm trying to resolve issues with denials like the following:
> type=AVC msg=audit(1551461957.121:9050): avc:  denied  { open } for  pid=30877 comm="cryptsetup" path="/dev/sdb" dev="devtmpfs" ino=110450 scontext=system_u:system_r:lvm_t:s0-s0:c0.c1023 tcontext=system_u:object_r:private_device_t:s0 tclass=blk_file permissive=1
> type=AVC msg=audit(1551461957.121:9051): avc:  denied  { ioctl } for  pid=30877 comm="cryptsetup" path="/dev/sdb" dev="devtmpfs" ino=110450 ioctlcmd=1268 scontext=system_u:system_r:lvm_t:s0-s0:c0.c1023 tcontext=system_u:object_r:private_device_t:s0 tclass=blk_file permissive=1
> type=AVC msg=audit(1551461973.149:9101): avc:  denied  { getattr } for  pid=28 comm="kdevtmpfs" path="/sdb1" dev="devtmpfs" ino=110461 scontext=system_u:system_r:kernel_t:s0 tcontext=system_u:object_r:private_device_t:s0 tclass=blk_file permissive=1
> type=AVC msg=audit(1551461973.149:9102): avc:  denied  { setattr } for  pid=28 comm="kdevtmpfs" name="sdb1" dev="devtmpfs" ino=110461 scontext=system_u:system_r:kernel_t:s0 tcontext=system_u:object_r:private_device_t:s0 tclass=blk_file permissive=1
> type=AVC msg=audit(1551461973.149:9103): avc:  denied  { unlink } for  pid=28 comm="kdevtmpfs" name="sdb1" dev="devtmpfs" ino=110461 scontext=system_u:system_r:kernel_t:s0 tcontext=system_u:object_r:private_device_t:s0 tclass=blk_file permissive=1
> 
> type=AVC msg=audit(1552228893.128:472): avc:  denied  { getattr } for  pid=8141 comm="systemd-logind" name="sg1" dev="devtmpfs" ino=31873 scontext=system_u:system_r:systemd_logind_t:s0 tcontext=system_u:object_r:private_device_t:s0 tclass=chr_file permissive=1
> type=AVC msg=audit(1552228893.128:473): avc:  denied  { setattr } for  pid=8141 comm="systemd-logind" name="sg1" dev="devtmpfs" ino=31873 scontext=system_u:system_r:systemd_logind_t:s0 tcontext=system_u:object_r:private_device_t:s0 tclass=chr_file permissive=1
> type=AVC msg=audit(1552228893.128:474): avc:  denied  { getattr } for  pid=8141 comm="systemd-logind" name="sr0" dev="devtmpfs" ino=14570 scontext=system_u:system_r:systemd_logind_t:s0 tcontext=system_u:object_r:private_device_t:s0 tclass=blk_file permissive=1
> type=AVC msg=audit(1552228893.128:475): avc:  denied  { setattr } for  pid=8141 comm="systemd-logind" name="sr0" dev="devtmpfs" ino=14570 scontext=system_u:system_r:systemd_logind_t:s0 tcontext=system_u:object_r:private_device_t:s0 tclass=blk_file permissive=1
> 
> Signed-off-by: Dave Sugar <dsugar@tresys.com>
> ---
>   policy/modules/kernel/kernel.te  |   7 +-
>   policy/modules/kernel/storage.if | 320 +++++++++++++++++++++++++++++++
>   policy/modules/kernel/storage.te |   7 +
>   policy/modules/system/lvm.te     |   2 +-
>   policy/modules/system/systemd.te |   8 +-
>   5 files changed, 336 insertions(+), 8 deletions(-)
> 
> diff --git a/policy/modules/kernel/kernel.te b/policy/modules/kernel/kernel.te
> index e971c533..acc8a88c 100644
> --- a/policy/modules/kernel/kernel.te
> +++ b/policy/modules/kernel/kernel.te
> @@ -352,9 +352,10 @@ ifdef(`init_systemd',`
>   
>   	optional_policy(`
>   		storage_dev_filetrans_fixed_disk(kernel_t)
> -		storage_setattr_fixed_disk_dev(kernel_t)
> -		storage_create_fixed_disk_dev(kernel_t)
> -		storage_delete_fixed_disk_dev(kernel_t)
> +		storage_getattr_all_scsi_generic_dev(kernel_t)
> +		storage_setattr_all_fixed_disk_dev(kernel_t)
> +		storage_create_all_fixed_disk_dev(kernel_t)
> +		storage_delete_all_fixed_disk_dev(kernel_t)
>   	')
>   ')
>   
> diff --git a/policy/modules/kernel/storage.if b/policy/modules/kernel/storage.if
> index 0292eee4..93208835 100644
> --- a/policy/modules/kernel/storage.if
> +++ b/policy/modules/kernel/storage.if
> @@ -1,5 +1,25 @@
>   ## <summary>Policy controlling access to storage devices</summary>
>   
> +########################################
> +## <summary>
> +##	Allow the caller to get the attributes of
> +##	all fixed disk device nodes.
> +## </summary>
> +## <param name="domain">
> +##	<summary>
> +##	Domain allowed access.
> +##	</summary>
> +## </param>
> +#
> +interface(`storage_getattr_all_fixed_disk_dev',`
> +	gen_require(`
> +		attribute fixed_disk_device;
> +	')
> +
> +	dev_list_all_dev_nodes($1)
> +	allow $1 fixed_disk_device:blk_file getattr;
> +')
> +
>   ########################################
>   ## <summary>
>   ##	Allow the caller to get the attributes of fixed disk
> @@ -20,6 +40,26 @@ interface(`storage_getattr_fixed_disk_dev',`
>   	allow $1 fixed_disk_device_t:blk_file getattr;
>   ')
>   
> +########################################
> +## <summary>
> +##	Do not audit attempts made by the caller to get
> +##	the attributes of all fixed disk device nodes.
> +## </summary>
> +## <param name="domain">
> +##	<summary>
> +##	Domain to not audit.
> +##	</summary>
> +## </param>
> +#
> +interface(`storage_dontaudit_all_getattr_fixed_disk_dev',`
> +	gen_require(`
> +		attribute fixed_disk_device;
> +	')
> +
> +	dontaudit $1 fixed_disk_device:blk_file getattr;
> +	dontaudit $1 fixed_disk_device:chr_file getattr; # /dev/rawctl
> +')
> +
>   ########################################
>   ## <summary>
>   ##	Do not audit attempts made by the caller to get
> @@ -40,6 +80,26 @@ interface(`storage_dontaudit_getattr_fixed_disk_dev',`
>   	dontaudit $1 fixed_disk_device_t:chr_file getattr; # /dev/rawctl
>   ')
>   
> +########################################
> +## <summary>
> +##	Allow the caller to set the attributes of all
> +##	fixed disk device nodes.
> +## </summary>
> +## <param name="domain">
> +##	<summary>
> +##	Domain allowed access.
> +##	</summary>
> +## </param>
> +#
> +interface(`storage_setattr_all_fixed_disk_dev',`
> +	gen_require(`
> +		attribute fixed_disk_device;
> +	')
> +
> +	dev_list_all_dev_nodes($1)
> +	allow $1 fixed_disk_device:blk_file setattr;
> +')
> +
>   ########################################
>   ## <summary>
>   ##	Allow the caller to set the attributes of fixed disk
> @@ -79,6 +139,31 @@ interface(`storage_dontaudit_setattr_fixed_disk_dev',`
>   	dontaudit $1 fixed_disk_device_t:blk_file setattr;
>   ')
>   
> +########################################
> +## <summary>
> +##	Allow the caller to directly read from all fixed disks.
> +##	This is extremely dangerous as it can bypass the
> +##	SELinux protections for filesystem objects, and
> +##	should only be used by trusted domains.
> +## </summary>
> +## <param name="domain">
> +##	<summary>
> +##	Domain allowed access.
> +##	</summary>
> +## </param>
> +#
> +interface(`storage_raw_read_all_fixed_disk',`
> +	gen_require(`
> +		attribute fixed_disk_raw_read;
> +		attribute fixed_disk_device;
> +	')
> +
> +	dev_list_all_dev_nodes($1)
> +	allow $1 fixed_disk_device:blk_file read_blk_file_perms;
> +	allow $1 fixed_disk_device:chr_file read_chr_file_perms;
> +	typeattribute $1 fixed_disk_raw_read;
> +')
> +
>   ########################################
>   ## <summary>
>   ##	Allow the caller to directly read from a fixed disk.
> @@ -188,6 +273,26 @@ interface(`storage_raw_rw_fixed_disk',`
>   	storage_raw_write_fixed_disk($1)
>   ')
>   
> +########################################
> +## <summary>
> +##	Allow the caller to create all fixed disk device nodes.
> +## </summary>
> +## <param name="domain">
> +##	<summary>
> +##	Domain allowed access.
> +##	</summary>
> +## </param>
> +#
> +interface(`storage_create_all_fixed_disk_dev',`
> +	gen_require(`
> +		attribute fixed_disk_device;
> +	')
> +
> +	allow $1 self:capability mknod;
> +	allow $1 fixed_disk_device:blk_file create_blk_file_perms;
> +	dev_add_entry_generic_dirs($1)
> +')
> +
>   ########################################
>   ## <summary>
>   ##	Allow the caller to create fixed disk device nodes.
> @@ -208,6 +313,25 @@ interface(`storage_create_fixed_disk_dev',`
>   	dev_add_entry_generic_dirs($1)
>   ')
>   
> +########################################
> +## <summary>
> +##	Allow the caller to delete all fixed disk device nodes.
> +## </summary>
> +## <param name="domain">
> +##	<summary>
> +##	Domain allowed access.
> +##	</summary>
> +## </param>
> +#
> +interface(`storage_delete_all_fixed_disk_dev',`
> +	gen_require(`
> +		attribute fixed_disk_device;
> +	')
> +
> +	allow $1 fixed_disk_device:blk_file delete_blk_file_perms;
> +	dev_remove_entry_generic_dirs($1)
> +')
> +
>   ########################################
>   ## <summary>
>   ##	Allow the caller to delete fixed disk device nodes.
> @@ -227,6 +351,29 @@ interface(`storage_delete_fixed_disk_dev',`
>   	dev_remove_entry_generic_dirs($1)
>   ')
>   
> +########################################
> +## <summary>
> +##	Create, read, write, and delete fixed disk device nodes.
> +## </summary>
> +## <param name="domain">
> +##	<summary>
> +##	Domain allowed access.
> +##	</summary>
> +## </param>
> +#
> +interface(`storage_manage_all_fixed_disk',`
> +	gen_require(`
> +		attribute fixed_disk_raw_read, fixed_disk_raw_write;
> +		attribute fixed_disk_device;
> +	')
> +
> +	dev_list_all_dev_nodes($1)
> +	allow $1 self:capability mknod;
> +	allow $1 fixed_disk_device:blk_file manage_blk_file_perms;
> +	allow $1 fixed_disk_device:chr_file manage_chr_file_perms;
> +	typeattribute $1 fixed_disk_raw_read, fixed_disk_raw_write;
> +')
> +
>   ########################################
>   ## <summary>
>   ##	Create, read, write, and delete fixed disk device nodes.
> @@ -293,6 +440,25 @@ interface(`storage_tmpfs_filetrans_fixed_disk',`
>   	fs_tmpfs_filetrans($1, fixed_disk_device_t, blk_file)
>   ')
>   
> +########################################
> +## <summary>
> +##	Relabel all fixed disk device nodes.
> +## </summary>
> +## <param name="domain">
> +##	<summary>
> +##	Domain allowed access.
> +##	</summary>
> +## </param>
> +#
> +interface(`storage_relabel_all_fixed_disk',`
> +	gen_require(`
> +		attribute fixed_disk_device;
> +	')
> +
> +	dev_list_all_dev_nodes($1)
> +	allow $1 fixed_disk_device:blk_file relabel_blk_file_perms;
> +')
> +
>   ########################################
>   ## <summary>
>   ##	Relabel fixed disk device nodes.
> @@ -388,6 +554,26 @@ interface(`storage_dontaudit_rw_fuse',`
>   	dontaudit $1 fuse_device_t:chr_file rw_file_perms;
>   ')
>   
> +########################################
> +## <summary>
> +##	Allow the caller to get the attributes of
> +##	all generic SCSI interface device nodes.
> +## </summary>
> +## <param name="domain">
> +##	<summary>
> +##	Domain allowed access.
> +##	</summary>
> +## </param>
> +#
> +interface(`storage_getattr_all_scsi_generic_dev',`
> +	gen_require(`
> +		attribute scsi_generic_device;
> +	')
> +
> +	dev_list_all_dev_nodes($1)
> +	allow $1 scsi_generic_device:chr_file getattr;
> +')
> +
>   ########################################
>   ## <summary>
>   ##	Allow the caller to get the attributes of
> @@ -408,6 +594,26 @@ interface(`storage_getattr_scsi_generic_dev',`
>   	allow $1 scsi_generic_device_t:chr_file getattr;
>   ')
>   
> +########################################
> +## <summary>
> +##	Allow the caller to set the attributes of
> +##	all generic SCSI interface device nodes.
> +## </summary>
> +## <param name="domain">
> +##	<summary>
> +##	Domain allowed access.
> +##	</summary>
> +## </param>
> +#
> +interface(`storage_setattr_all_scsi_generic_dev',`
> +	gen_require(`
> +		attribute scsi_generic_device;
> +	')
> +
> +	dev_list_all_dev_nodes($1)
> +	allow $1 scsi_generic_device:chr_file setattr;
> +')
> +
>   ########################################
>   ## <summary>
>   ##	Allow the caller to set the attributes of
> @@ -517,6 +723,26 @@ interface(`storage_dontaudit_rw_scsi_generic',`
>   	dontaudit $1 scsi_generic_device_t:chr_file rw_file_perms;
>   ')
>   
> +########################################
> +## <summary>
> +##	Allow the caller to get the attributes of all removable
> +##	devices device nodes.
> +## </summary>
> +## <param name="domain">
> +##	<summary>
> +##	Domain allowed access.
> +##	</summary>
> +## </param>
> +#
> +interface(`storage_getattr_all_removable_dev',`
> +	gen_require(`
> +		attribute removable_device;
> +	')
> +
> +	dev_list_all_dev_nodes($1)
> +	allow $1 removable_device:blk_file getattr;
> +')
> +
>   ########################################
>   ## <summary>
>   ##	Allow the caller to get the attributes of removable
> @@ -556,6 +782,25 @@ interface(`storage_dontaudit_getattr_removable_dev',`
>   	dontaudit $1 removable_device_t:blk_file getattr;
>   ')
>   
> +########################################
> +## <summary>
> +##	Do not audit attempts made by the caller to get
> +##	the attributes of all removable devices device nodes.
> +## </summary>
> +## <param name="domain">
> +##	<summary>
> +##	Domain to not audit.
> +##	</summary>
> +## </param>
> +#
> +interface(`storage_dontaudit_getattr_all_removable_dev',`
> +	gen_require(`
> +		attribute removable_device;
> +	')
> +
> +	dontaudit $1 removable_device:blk_file getattr;
> +')
> +
>   ########################################
>   ## <summary>
>   ##	Do not audit attempts made by the caller to read
> @@ -595,6 +840,26 @@ interface(`storage_dontaudit_write_removable_device',`
>   	dontaudit $1 removable_device_t:blk_file write_blk_file_perms;
>   ')
>   
> +########################################
> +## <summary>
> +##	Allow the caller to set the attributes of all removable
> +##	devices device nodes.
> +## </summary>
> +## <param name="domain">
> +##	<summary>
> +##	Domain allowed access.
> +##	</summary>
> +## </param>
> +#
> +interface(`storage_setattr_all_removable_dev',`
> +	gen_require(`
> +		attribute removable_device;
> +	')
> +
> +	dev_list_all_dev_nodes($1)
> +	allow $1 removable_device:blk_file setattr;
> +')
> +
>   ########################################
>   ## <summary>
>   ##	Allow the caller to set the attributes of removable
> @@ -796,6 +1061,61 @@ interface(`storage_setattr_tape_dev',`
>   	allow $1 tape_device_t:chr_file setattr;
>   ')
>   
> +########################################
> +## <summary>
> +##	Mark a type as a fixed disk device type.
> +## </summary>
> +## <param name="domain">
> +##	<summary>
> +##	Type to associate
> +##	</summary>
> +## </param>
> +#
> +interface(`storage_fixed_disk_device_type',`
> +	gen_require(`
> +		attribute fixed_disk_device;
> +	')
> +
> +	typeattribute $1 fixed_disk_device;
> +')
> +
> +########################################
> +## <summary>
> +##	Mark a type as a removable device type.
> +## </summary>
> +## <param name="domain">
> +##	<summary>
> +##	Type to associate.
> +##	</summary>
> +## </param>
> +#
> +interface(`storage_removable_device_type',`
> +	gen_require(`
> +		attribute removable_device;
> +	')
> +
> +	typeattribute $1 removable_device;
> +')
> +
> +
> +########################################
> +## <summary>
> +##	Mark a type as a scsi generic device type.
> +## </summary>
> +## <param name="domain">
> +##	<summary>
> +##	Type to associate.
> +##	</summary>
> +## </param>
> +#
> +interface(`storage_scsi_generic_device_type',`
> +	gen_require(`
> +		attribute scsi_generic_device;
> +	')
> +
> +	typeattribute $1 scsi_generic_device;
> +')
> +
>   ########################################
>   ## <summary>
>   ##	Unconfined access to storage devices.
> diff --git a/policy/modules/kernel/storage.te b/policy/modules/kernel/storage.te
> index c10290c0..cc59380d 100644
> --- a/policy/modules/kernel/storage.te
> +++ b/policy/modules/kernel/storage.te
> @@ -11,12 +11,17 @@ attribute scsi_generic_read;
>   attribute scsi_generic_write;
>   attribute storage_unconfined_type;
>   
> +attribute fixed_disk_device;
> +attribute removable_device;
> +attribute scsi_generic_device;
> +
>   #
>   # fixed_disk_device_t is the type of
>   # /dev/hd* and /dev/sd*.
>   #
>   type fixed_disk_device_t;
>   dev_node(fixed_disk_device_t)
> +storage_fixed_disk_device_type(fixed_disk_device_t)
>   
>   neverallow ~{ fixed_disk_raw_read storage_unconfined_type } fixed_disk_device_t:{ chr_file blk_file } read;
>   neverallow ~{ fixed_disk_raw_write storage_unconfined_type } fixed_disk_device_t:{ chr_file blk_file } { append write };
> @@ -33,6 +38,7 @@ dev_node(fuse_device_t)
>   #
>   type scsi_generic_device_t;
>   dev_node(scsi_generic_device_t)
> +storage_scsi_generic_device_type(scsi_generic_device_t)
>   
>   neverallow ~{ scsi_generic_read storage_unconfined_type } scsi_generic_device_t:{ chr_file blk_file } read;
>   neverallow ~{ scsi_generic_write storage_unconfined_type } scsi_generic_device_t:{ chr_file blk_file } { append write };
> @@ -43,6 +49,7 @@ neverallow ~{ scsi_generic_write storage_unconfined_type } scsi_generic_device_t
>   #
>   type removable_device_t;
>   dev_node(removable_device_t)
> +storage_removable_device_type(removable_device_t)
>   
>   #
>   # tape_device_t is the type of
> diff --git a/policy/modules/system/lvm.te b/policy/modules/system/lvm.te
> index ba64c39d..0bb7177c 100644
> --- a/policy/modules/system/lvm.te
> +++ b/policy/modules/system/lvm.te
> @@ -300,7 +300,7 @@ storage_dontaudit_read_removable_device(lvm_t)
>   # cjp: needs to create an interface here for fixed disk create
>   storage_dev_filetrans_fixed_disk(lvm_t)
>   # Access raw devices and old /dev/lvm (c 109,0).  Is this needed?
> -storage_manage_fixed_disk(lvm_t)
> +storage_manage_all_fixed_disk(lvm_t)
>   
>   term_use_all_terms(lvm_t)
>   
> diff --git a/policy/modules/system/systemd.te b/policy/modules/system/systemd.te
> index 07529a5d..83f85a9d 100644
> --- a/policy/modules/system/systemd.te
> +++ b/policy/modules/system/systemd.te
> @@ -472,10 +472,10 @@ fs_unmount_tmpfs(systemd_logind_t)
>   
>   selinux_get_enforce_mode(systemd_logind_t)
>   
> -storage_getattr_removable_dev(systemd_logind_t)
> -storage_getattr_scsi_generic_dev(systemd_logind_t)
> -storage_setattr_removable_dev(systemd_logind_t)
> -storage_setattr_scsi_generic_dev(systemd_logind_t)
> +storage_getattr_all_removable_dev(systemd_logind_t)
> +storage_getattr_all_scsi_generic_dev(systemd_logind_t)
> +storage_setattr_all_removable_dev(systemd_logind_t)
> +storage_setattr_all_scsi_generic_dev(systemd_logind_t)
>   
>   term_setattr_unallocated_ttys(systemd_logind_t)
>   term_use_unallocated_ttys(systemd_logind_t)
> 


-- 
Chris PeBenito