[PATCH] Allow syslog to write to the runtime socket

[PATCH] Allow syslog to write to the runtime socket

[Sugar, David]

This is realted to my previous patch for logging, I just didn't notice it before.

type=AVC msg=audit(1575426773.635:2469): avc:  denied  { write } for  pid=1213 comm="systemd-journal" name="syslog" dev="tmpfs" ino=22683 scontext=system_u:system_r:syslogd_t:s0 tcontext=system_u:object_r:syslogd_runtime_t:s0 tclass=sock_file permissive=0

Signed-off-by: Dave Sugar <dsugar@tresys.com>
---
 policy/modules/system/logging.te | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/policy/modules/system/logging.te b/policy/modules/system/logging.te
index 73ca3042..eee6bc18 100644
--- a/policy/modules/system/logging.te
+++ b/policy/modules/system/logging.te
@@ -427,7 +427,7 @@ files_search_var_lib(syslogd_t)
 
 # manage runtime files
 allow syslogd_t syslogd_runtime_t:dir create_dir_perms;
-allow syslogd_t syslogd_runtime_t:sock_file { create setattr };
+allow syslogd_t syslogd_runtime_t:sock_file { create setattr write };
 allow syslogd_t syslogd_runtime_t:file map;
 manage_files_pattern(syslogd_t, syslogd_runtime_t, syslogd_runtime_t)
 files_pid_filetrans(syslogd_t, syslogd_runtime_t, file)
-- 
2.21.0

[PATCH] Allow systemd to getattr configfile

[Sugar, David]

Systemd has ConditionalPathExists which is used to check if a path exists to control starting a service.  But this requires getattr permissions on the file.  This is generally for configuration files.  We are mostly seeing this is in our own policy.  But this lvm denial also fits the example.

type=AVC msg=audit(1575427946.229:1624): avc:  denied  { getattr } for  pid=1 comm="systemd" path="/etc/lvm/lvm.conf" dev="dm-0" ino=51799 scontext=system_u:system_r:init_t:s0 tcontext=system_u:object_r:lvm_etc_t:s0 tclass=file permissive=0

This second example is from chronyd, but it is happening becuase I added the conditional in a drop-in file. Note that chronyd_conf_t is already a 'configfile'.

type=AVC msg=audit(1575427959.882:1901): avc:  denied  { getattr } for  pid=1 comm="systemd" path="/etc/chrony.conf" dev="dm-0" ino=53824 scontext=system_u:system_r:init_t:s0 tcontext=system_u:object_r:chronyd_conf_t:s0 tclass=file permissive=1

Signed-off-by: Dave Sugar <dsugar@tresys.com>
---
 policy/modules/kernel/files.if | 20 ++++++++++++++++++++
 policy/modules/system/init.te  |  1 +
 policy/modules/system/lvm.te   |  2 +-
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/policy/modules/kernel/files.if b/policy/modules/kernel/files.if
index f1c94411..87be07ae 100644
--- a/policy/modules/kernel/files.if
+++ b/policy/modules/kernel/files.if
@@ -1562,6 +1562,26 @@ interface(`files_relabel_config_dirs',`
 	relabel_dirs_pattern($1, configfile, configfile)
 ')
 
+########################################
+## <summary>
+##	Getattr config files in /etc.
+## </summary>
+## <param name="domain">
+##	<summary>
+##	Domain allowed access.
+##	</summary>
+## </param>
+#
+interface(`files_getattr_config_files',`
+	gen_require(`
+		attribute configfile;
+	')
+
+	allow $1 configfile:dir list_dir_perms;
+	getattr_files_pattern($1, configfile, configfile)
+	read_lnk_files_pattern($1, configfile, configfile)
+')
+
 ########################################
 ## <summary>
 ##	Read config files in /etc.
diff --git a/policy/modules/system/init.te b/policy/modules/system/init.te
index 8973a622..747b696e 100644
--- a/policy/modules/system/init.te
+++ b/policy/modules/system/init.te
@@ -320,6 +320,7 @@ ifdef(`init_systemd',`
 	domain_subj_id_change_exemption(init_t)
 	domain_role_change_exemption(init_t)
 
+	files_getattr_config_files(init_t)
 	files_read_all_pids(init_t)
 	files_list_usr(init_t)
 	files_list_var(init_t)
diff --git a/policy/modules/system/lvm.te b/policy/modules/system/lvm.te
index ad4eb579..c05344e0 100644
--- a/policy/modules/system/lvm.te
+++ b/policy/modules/system/lvm.te
@@ -25,7 +25,7 @@ domain_obj_id_change_exemption(lvm_t)
 role system_r types lvm_t;
 
 type lvm_etc_t;
-files_type(lvm_etc_t)
+files_config_file(lvm_etc_t)
 
 type lvm_lock_t;
 files_lock_file(lvm_lock_t)
-- 
2.21.0

Re: [PATCH] Allow systemd to getattr configfile

[Dominick Grift]

[-- Attachment #1: Type: text/plain, Size: 3701 bytes --]

On Wed, Dec 04, 2019 at 04:33:20PM +0000, Sugar, David wrote:
> Systemd has ConditionalPathExists which is used to check if a path exists to control starting a service.  But this requires getattr permissions on the file.  This is generally for configuration files.  We are mostly seeing this is in our own policy.  But this lvm denial also fits the example.
> 
> type=AVC msg=audit(1575427946.229:1624): avc:  denied  { getattr } for  pid=1 comm="systemd" path="/etc/lvm/lvm.conf" dev="dm-0" ino=51799 scontext=system_u:system_r:init_t:s0 tcontext=system_u:object_r:lvm_etc_t:s0 tclass=file permissive=0
> 
> This second example is from chronyd, but it is happening becuase I added the conditional in a drop-in file. Note that chronyd_conf_t is already a 'configfile'.
> 
> type=AVC msg=audit(1575427959.882:1901): avc:  denied  { getattr } for  pid=1 comm="systemd" path="/etc/chrony.conf" dev="dm-0" ino=53824 scontext=system_u:system_r:init_t:s0 tcontext=system_u:object_r:chronyd_conf_t:s0 tclass=file permissive=1

how about something a little more general?

systemd_ConditionPath(`,'
	allow init_t $1:dir search_dir_perms;
	allow init_t $1:lnk_file read_lnk_file_perms;
	allow init_t $1:fifo_file getattr_fifo_file_perms;
	allow init_t $1:sock_file getattr_sock_file_perms;
	allow init_t $1:file getattr_file_perms;
	allow init_t $1:blk_file getattr_blk_file_perms;
	allow init_t $1:chr_file getattr_chr_file_perms;
')

> 
> Signed-off-by: Dave Sugar <dsugar@tresys.com>
> ---
>  policy/modules/kernel/files.if | 20 ++++++++++++++++++++
>  policy/modules/system/init.te  |  1 +
>  policy/modules/system/lvm.te   |  2 +-
>  3 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/policy/modules/kernel/files.if b/policy/modules/kernel/files.if
> index f1c94411..87be07ae 100644
> --- a/policy/modules/kernel/files.if
> +++ b/policy/modules/kernel/files.if
> @@ -1562,6 +1562,26 @@ interface(`files_relabel_config_dirs',`
>  	relabel_dirs_pattern($1, configfile, configfile)
>  ')
>  
> +########################################
> +## <summary>
> +##	Getattr config files in /etc.
> +## </summary>
> +## <param name="domain">
> +##	<summary>
> +##	Domain allowed access.
> +##	</summary>
> +## </param>
> +#
> +interface(`files_getattr_config_files',`
> +	gen_require(`
> +		attribute configfile;
> +	')
> +
> +	allow $1 configfile:dir list_dir_perms;
> +	getattr_files_pattern($1, configfile, configfile)
> +	read_lnk_files_pattern($1, configfile, configfile)
> +')
> +
>  ########################################
>  ## <summary>
>  ##	Read config files in /etc.
> diff --git a/policy/modules/system/init.te b/policy/modules/system/init.te
> index 8973a622..747b696e 100644
> --- a/policy/modules/system/init.te
> +++ b/policy/modules/system/init.te
> @@ -320,6 +320,7 @@ ifdef(`init_systemd',`
>  	domain_subj_id_change_exemption(init_t)
>  	domain_role_change_exemption(init_t)
>  
> +	files_getattr_config_files(init_t)
>  	files_read_all_pids(init_t)
>  	files_list_usr(init_t)
>  	files_list_var(init_t)
> diff --git a/policy/modules/system/lvm.te b/policy/modules/system/lvm.te
> index ad4eb579..c05344e0 100644
> --- a/policy/modules/system/lvm.te
> +++ b/policy/modules/system/lvm.te
> @@ -25,7 +25,7 @@ domain_obj_id_change_exemption(lvm_t)
>  role system_r types lvm_t;
>  
>  type lvm_etc_t;
> -files_type(lvm_etc_t)
> +files_config_file(lvm_etc_t)
>  
>  type lvm_lock_t;
>  files_lock_file(lvm_lock_t)
> -- 
> 2.21.0
> 

-- 
Key fingerprint = 5F4D 3CDB D3F8 3652 FBD8 02D5 3B6C 5F1D 2C7B 6B02
https://sks-keyservers.net/pks/lookup?op=get&search=0x3B6C5F1D2C7B6B02
Dominick Grift

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Allow systemd to getattr configfile

[Sugar, David]

On 12/4/19 11:56 AM, Dominick Grift wrote:
> On Wed, Dec 04, 2019 at 04:33:20PM +0000, Sugar, David wrote:
>> Systemd has ConditionalPathExists which is used to check if a path exists to control starting a service.  But this requires getattr permissions on the file.  This is generally for configuration files.  We are mostly seeing this is in our own policy.  But this lvm denial also fits the example.
>>
>> type=AVC msg=audit(1575427946.229:1624): avc:  denied  { getattr } for  pid=1 comm="systemd" path="/etc/lvm/lvm.conf" dev="dm-0" ino=51799 scontext=system_u:system_r:init_t:s0 tcontext=system_u:object_r:lvm_etc_t:s0 tclass=file permissive=0
>>
>> This second example is from chronyd, but it is happening becuase I added the conditional in a drop-in file. Note that chronyd_conf_t is already a 'configfile'.
>>
>> type=AVC msg=audit(1575427959.882:1901): avc:  denied  { getattr } for  pid=1 comm="systemd" path="/etc/chrony.conf" dev="dm-0" ino=53824 scontext=system_u:system_r:init_t:s0 tcontext=system_u:object_r:chronyd_conf_t:s0 tclass=file permissive=1
> 
> how about something a little more general?
> 
> systemd_ConditionPath(`,'
> 	allow init_t $1:dir search_dir_perms;
> 	allow init_t $1:lnk_file read_lnk_file_perms;
> 	allow init_t $1:fifo_file getattr_fifo_file_perms;
> 	allow init_t $1:sock_file getattr_sock_file_perms;
> 	allow init_t $1:file getattr_file_perms;
> 	allow init_t $1:blk_file getattr_blk_file_perms;
> 	allow init_t $1:chr_file getattr_chr_file_perms;
> ')
> 
I think you are suggesting an interface 'systemd_conditionpath' that 
would exist in init.if and then need to be used by any module that wants 
to grant access to a particular type to getattr?

So, for this case, I would need to modify chronyd.te and lvm.te to use 
this interface?

I think you are also suggesting that ConditionPathExists usage in a unit 
file could be trying to check for the existence of something other than 
a configuration file.

Taking it to the extreme, a unit file could be checking for the 
existence of a file that is in a different SELinux domain.  Does it 
instead make sense to use the 'files_getattr_all_files', 
'files_getattr_all_sockets', 'files_getattr_all_pipes', etc... in init.te?


>>
>> Signed-off-by: Dave Sugar <dsugar@tresys.com>
>> ---
>>   policy/modules/kernel/files.if | 20 ++++++++++++++++++++
>>   policy/modules/system/init.te  |  1 +
>>   policy/modules/system/lvm.te   |  2 +-
>>   3 files changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/policy/modules/kernel/files.if b/policy/modules/kernel/files.if
>> index f1c94411..87be07ae 100644
>> --- a/policy/modules/kernel/files.if
>> +++ b/policy/modules/kernel/files.if
>> @@ -1562,6 +1562,26 @@ interface(`files_relabel_config_dirs',`
>>   	relabel_dirs_pattern($1, configfile, configfile)
>>   ')
>>   
>> +########################################
>> +## <summary>
>> +##	Getattr config files in /etc.
>> +## </summary>
>> +## <param name="domain">
>> +##	<summary>
>> +##	Domain allowed access.
>> +##	</summary>
>> +## </param>
>> +#
>> +interface(`files_getattr_config_files',`
>> +	gen_require(`
>> +		attribute configfile;
>> +	')
>> +
>> +	allow $1 configfile:dir list_dir_perms;
>> +	getattr_files_pattern($1, configfile, configfile)
>> +	read_lnk_files_pattern($1, configfile, configfile)
>> +')
>> +
>>   ########################################
>>   ## <summary>
>>   ##	Read config files in /etc.
>> diff --git a/policy/modules/system/init.te b/policy/modules/system/init.te
>> index 8973a622..747b696e 100644
>> --- a/policy/modules/system/init.te
>> +++ b/policy/modules/system/init.te
>> @@ -320,6 +320,7 @@ ifdef(`init_systemd',`
>>   	domain_subj_id_change_exemption(init_t)
>>   	domain_role_change_exemption(init_t)
>>   
>> +	files_getattr_config_files(init_t)
>>   	files_read_all_pids(init_t)
>>   	files_list_usr(init_t)
>>   	files_list_var(init_t)
>> diff --git a/policy/modules/system/lvm.te b/policy/modules/system/lvm.te
>> index ad4eb579..c05344e0 100644
>> --- a/policy/modules/system/lvm.te
>> +++ b/policy/modules/system/lvm.te
>> @@ -25,7 +25,7 @@ domain_obj_id_change_exemption(lvm_t)
>>   role system_r types lvm_t;
>>   
>>   type lvm_etc_t;
>> -files_type(lvm_etc_t)
>> +files_config_file(lvm_etc_t)
>>   
>>   type lvm_lock_t;
>>   files_lock_file(lvm_lock_t)
>> -- 
>> 2.21.0
>>
> 

Re: [PATCH] Allow systemd to getattr configfile

[Dominick Grift]

[-- Attachment #1: Type: text/plain, Size: 5163 bytes --]

On Wed, Dec 04, 2019 at 05:22:55PM +0000, Sugar, David wrote:
> 
> 
> On 12/4/19 11:56 AM, Dominick Grift wrote:
> > On Wed, Dec 04, 2019 at 04:33:20PM +0000, Sugar, David wrote:
> >> Systemd has ConditionalPathExists which is used to check if a path exists to control starting a service.  But this requires getattr permissions on the file.  This is generally for configuration files.  We are mostly seeing this is in our own policy.  But this lvm denial also fits the example.
> >>
> >> type=AVC msg=audit(1575427946.229:1624): avc:  denied  { getattr } for  pid=1 comm="systemd" path="/etc/lvm/lvm.conf" dev="dm-0" ino=51799 scontext=system_u:system_r:init_t:s0 tcontext=system_u:object_r:lvm_etc_t:s0 tclass=file permissive=0
> >>
> >> This second example is from chronyd, but it is happening becuase I added the conditional in a drop-in file. Note that chronyd_conf_t is already a 'configfile'.
> >>
> >> type=AVC msg=audit(1575427959.882:1901): avc:  denied  { getattr } for  pid=1 comm="systemd" path="/etc/chrony.conf" dev="dm-0" ino=53824 scontext=system_u:system_r:init_t:s0 tcontext=system_u:object_r:chronyd_conf_t:s0 tclass=file permissive=1
> > 
> > how about something a little more general?
> > 
> > systemd_ConditionPath(`,'
> > 	allow init_t $1:dir search_dir_perms;
> > 	allow init_t $1:lnk_file read_lnk_file_perms;
> > 	allow init_t $1:fifo_file getattr_fifo_file_perms;
> > 	allow init_t $1:sock_file getattr_sock_file_perms;
> > 	allow init_t $1:file getattr_file_perms;
> > 	allow init_t $1:blk_file getattr_blk_file_perms;
> > 	allow init_t $1:chr_file getattr_chr_file_perms;
> > ')
> > 
> I think you are suggesting an interface 'systemd_conditionpath' that 
> would exist in init.if and then need to be used by any module that wants 
> to grant access to a particular type to getattr?

Yes

> 
> So, for this case, I would need to modify chronyd.te and lvm.te to use 
> this interface?

Yes

> 
> I think you are also suggesting that ConditionPathExists usage in a unit 
> file could be trying to check for the existence of something other than 
> a configuration file.

Yes, and on top of that there are other "conditions" but generally it boils down to systemd "statting" the target

> 
> Taking it to the extreme, a unit file could be checking for the 
> existence of a file that is in a different SELinux domain.  Does it 
> instead make sense to use the 'files_getattr_all_files', 
> 'files_getattr_all_sockets', 'files_getattr_all_pipes', etc... in init.te?

I would argue that this would be too broad/generic, not to mention that it could also apply to a device node (basicallu anything)

> 
> 
> >>
> >> Signed-off-by: Dave Sugar <dsugar@tresys.com>
> >> ---
> >>   policy/modules/kernel/files.if | 20 ++++++++++++++++++++
> >>   policy/modules/system/init.te  |  1 +
> >>   policy/modules/system/lvm.te   |  2 +-
> >>   3 files changed, 22 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/policy/modules/kernel/files.if b/policy/modules/kernel/files.if
> >> index f1c94411..87be07ae 100644
> >> --- a/policy/modules/kernel/files.if
> >> +++ b/policy/modules/kernel/files.if
> >> @@ -1562,6 +1562,26 @@ interface(`files_relabel_config_dirs',`
> >>   	relabel_dirs_pattern($1, configfile, configfile)
> >>   ')
> >>   
> >> +########################################
> >> +## <summary>
> >> +##	Getattr config files in /etc.
> >> +## </summary>
> >> +## <param name="domain">
> >> +##	<summary>
> >> +##	Domain allowed access.
> >> +##	</summary>
> >> +## </param>
> >> +#
> >> +interface(`files_getattr_config_files',`
> >> +	gen_require(`
> >> +		attribute configfile;
> >> +	')
> >> +
> >> +	allow $1 configfile:dir list_dir_perms;
> >> +	getattr_files_pattern($1, configfile, configfile)
> >> +	read_lnk_files_pattern($1, configfile, configfile)
> >> +')
> >> +
> >>   ########################################
> >>   ## <summary>
> >>   ##	Read config files in /etc.
> >> diff --git a/policy/modules/system/init.te b/policy/modules/system/init.te
> >> index 8973a622..747b696e 100644
> >> --- a/policy/modules/system/init.te
> >> +++ b/policy/modules/system/init.te
> >> @@ -320,6 +320,7 @@ ifdef(`init_systemd',`
> >>   	domain_subj_id_change_exemption(init_t)
> >>   	domain_role_change_exemption(init_t)
> >>   
> >> +	files_getattr_config_files(init_t)
> >>   	files_read_all_pids(init_t)
> >>   	files_list_usr(init_t)
> >>   	files_list_var(init_t)
> >> diff --git a/policy/modules/system/lvm.te b/policy/modules/system/lvm.te
> >> index ad4eb579..c05344e0 100644
> >> --- a/policy/modules/system/lvm.te
> >> +++ b/policy/modules/system/lvm.te
> >> @@ -25,7 +25,7 @@ domain_obj_id_change_exemption(lvm_t)
> >>   role system_r types lvm_t;
> >>   
> >>   type lvm_etc_t;
> >> -files_type(lvm_etc_t)
> >> +files_config_file(lvm_etc_t)
> >>   
> >>   type lvm_lock_t;
> >>   files_lock_file(lvm_lock_t)
> >> -- 
> >> 2.21.0
> >>
> > 

-- 
Key fingerprint = 5F4D 3CDB D3F8 3652 FBD8 02D5 3B6C 5F1D 2C7B 6B02
https://sks-keyservers.net/pks/lookup?op=get&search=0x3B6C5F1D2C7B6B02
Dominick Grift

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Allow systemd to getattr configfile

[Dominick Grift]

[-- Attachment #1: Type: text/plain, Size: 5147 bytes --]

On Wed, Dec 04, 2019 at 05:22:55PM +0000, Sugar, David wrote:
> 
> 
> On 12/4/19 11:56 AM, Dominick Grift wrote:
> > On Wed, Dec 04, 2019 at 04:33:20PM +0000, Sugar, David wrote:
> >> Systemd has ConditionalPathExists which is used to check if a path exists to control starting a service.  But this requires getattr permissions on the file.  This is generally for configuration files.  We are mostly seeing this is in our own policy.  But this lvm denial also fits the example.
> >>
> >> type=AVC msg=audit(1575427946.229:1624): avc:  denied  { getattr } for  pid=1 comm="systemd" path="/etc/lvm/lvm.conf" dev="dm-0" ino=51799 scontext=system_u:system_r:init_t:s0 tcontext=system_u:object_r:lvm_etc_t:s0 tclass=file permissive=0
> >>
> >> This second example is from chronyd, but it is happening becuase I added the conditional in a drop-in file. Note that chronyd_conf_t is already a 'configfile'.
> >>
> >> type=AVC msg=audit(1575427959.882:1901): avc:  denied  { getattr } for  pid=1 comm="systemd" path="/etc/chrony.conf" dev="dm-0" ino=53824 scontext=system_u:system_r:init_t:s0 tcontext=system_u:object_r:chronyd_conf_t:s0 tclass=file permissive=1
> > 
> > how about something a little more general?
> > 
> > systemd_ConditionPath(`,'
> > 	allow init_t $1:dir search_dir_perms;
> > 	allow init_t $1:lnk_file read_lnk_file_perms;
> > 	allow init_t $1:fifo_file getattr_fifo_file_perms;
> > 	allow init_t $1:sock_file getattr_sock_file_perms;
> > 	allow init_t $1:file getattr_file_perms;
> > 	allow init_t $1:blk_file getattr_blk_file_perms;
> > 	allow init_t $1:chr_file getattr_chr_file_perms;
> > ')
> > 
> I think you are suggesting an interface 'systemd_conditionpath' that 
> would exist in init.if and then need to be used by any module that wants 
> to grant access to a particular type to getattr?
> 
> So, for this case, I would need to modify chronyd.te and lvm.te to use 
> this interface?
> 
> I think you are also suggesting that ConditionPathExists usage in a unit 
> file could be trying to check for the existence of something other than 
> a configuration file.
> 
> Taking it to the extreme, a unit file could be checking for the 
> existence of a file that is in a different SELinux domain.  Does it 
> instead make sense to use the 'files_getattr_all_files', 
> 'files_getattr_all_sockets', 'files_getattr_all_pipes', etc... in init.te?
> 

$ man systemd.directives | grep -i conditionpath
       ConditionPathExists=
       ConditionPathExistsGlob=
       ConditionPathIsDirectory=
       ConditionPathIsMountPoint=
       ConditionPathIsReadWrite=
       ConditionPathIsSymbolicLink=

> 
> >>
> >> Signed-off-by: Dave Sugar <dsugar@tresys.com>
> >> ---
> >>   policy/modules/kernel/files.if | 20 ++++++++++++++++++++
> >>   policy/modules/system/init.te  |  1 +
> >>   policy/modules/system/lvm.te   |  2 +-
> >>   3 files changed, 22 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/policy/modules/kernel/files.if b/policy/modules/kernel/files.if
> >> index f1c94411..87be07ae 100644
> >> --- a/policy/modules/kernel/files.if
> >> +++ b/policy/modules/kernel/files.if
> >> @@ -1562,6 +1562,26 @@ interface(`files_relabel_config_dirs',`
> >>   	relabel_dirs_pattern($1, configfile, configfile)
> >>   ')
> >>   
> >> +########################################
> >> +## <summary>
> >> +##	Getattr config files in /etc.
> >> +## </summary>
> >> +## <param name="domain">
> >> +##	<summary>
> >> +##	Domain allowed access.
> >> +##	</summary>
> >> +## </param>
> >> +#
> >> +interface(`files_getattr_config_files',`
> >> +	gen_require(`
> >> +		attribute configfile;
> >> +	')
> >> +
> >> +	allow $1 configfile:dir list_dir_perms;
> >> +	getattr_files_pattern($1, configfile, configfile)
> >> +	read_lnk_files_pattern($1, configfile, configfile)
> >> +')
> >> +
> >>   ########################################
> >>   ## <summary>
> >>   ##	Read config files in /etc.
> >> diff --git a/policy/modules/system/init.te b/policy/modules/system/init.te
> >> index 8973a622..747b696e 100644
> >> --- a/policy/modules/system/init.te
> >> +++ b/policy/modules/system/init.te
> >> @@ -320,6 +320,7 @@ ifdef(`init_systemd',`
> >>   	domain_subj_id_change_exemption(init_t)
> >>   	domain_role_change_exemption(init_t)
> >>   
> >> +	files_getattr_config_files(init_t)
> >>   	files_read_all_pids(init_t)
> >>   	files_list_usr(init_t)
> >>   	files_list_var(init_t)
> >> diff --git a/policy/modules/system/lvm.te b/policy/modules/system/lvm.te
> >> index ad4eb579..c05344e0 100644
> >> --- a/policy/modules/system/lvm.te
> >> +++ b/policy/modules/system/lvm.te
> >> @@ -25,7 +25,7 @@ domain_obj_id_change_exemption(lvm_t)
> >>   role system_r types lvm_t;
> >>   
> >>   type lvm_etc_t;
> >> -files_type(lvm_etc_t)
> >> +files_config_file(lvm_etc_t)
> >>   
> >>   type lvm_lock_t;
> >>   files_lock_file(lvm_lock_t)
> >> -- 
> >> 2.21.0
> >>
> > 

-- 
Key fingerprint = 5F4D 3CDB D3F8 3652 FBD8 02D5 3B6C 5F1D 2C7B 6B02
https://sks-keyservers.net/pks/lookup?op=get&search=0x3B6C5F1D2C7B6B02
Dominick Grift

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Allow systemd to getattr configfile

[Dominick Grift]

[-- Attachment #1: Type: text/plain, Size: 6082 bytes --]

On Wed, Dec 04, 2019 at 06:43:43PM +0100, Dominick Grift wrote:
> On Wed, Dec 04, 2019 at 05:22:55PM +0000, Sugar, David wrote:
> > 
> > 
> > On 12/4/19 11:56 AM, Dominick Grift wrote:
> > > On Wed, Dec 04, 2019 at 04:33:20PM +0000, Sugar, David wrote:
> > >> Systemd has ConditionalPathExists which is used to check if a path exists to control starting a service.  But this requires getattr permissions on the file.  This is generally for configuration files.  We are mostly seeing this is in our own policy.  But this lvm denial also fits the example.
> > >>
> > >> type=AVC msg=audit(1575427946.229:1624): avc:  denied  { getattr } for  pid=1 comm="systemd" path="/etc/lvm/lvm.conf" dev="dm-0" ino=51799 scontext=system_u:system_r:init_t:s0 tcontext=system_u:object_r:lvm_etc_t:s0 tclass=file permissive=0
> > >>
> > >> This second example is from chronyd, but it is happening becuase I added the conditional in a drop-in file. Note that chronyd_conf_t is already a 'configfile'.
> > >>
> > >> type=AVC msg=audit(1575427959.882:1901): avc:  denied  { getattr } for  pid=1 comm="systemd" path="/etc/chrony.conf" dev="dm-0" ino=53824 scontext=system_u:system_r:init_t:s0 tcontext=system_u:object_r:chronyd_conf_t:s0 tclass=file permissive=1
> > > 
> > > how about something a little more general?
> > > 
> > > systemd_ConditionPath(`,'
> > > 	allow init_t $1:dir search_dir_perms;
> > > 	allow init_t $1:lnk_file read_lnk_file_perms;
> > > 	allow init_t $1:fifo_file getattr_fifo_file_perms;
> > > 	allow init_t $1:sock_file getattr_sock_file_perms;
> > > 	allow init_t $1:file getattr_file_perms;
> > > 	allow init_t $1:blk_file getattr_blk_file_perms;
> > > 	allow init_t $1:chr_file getattr_chr_file_perms;
> > > ')
> > > 
> > I think you are suggesting an interface 'systemd_conditionpath' that 
> > would exist in init.if and then need to be used by any module that wants 
> > to grant access to a particular type to getattr?
> > 
> > So, for this case, I would need to modify chronyd.te and lvm.te to use 
> > this interface?
> > 
> > I think you are also suggesting that ConditionPathExists usage in a unit 
> > file could be trying to check for the existence of something other than 
> > a configuration file.
> > 
> > Taking it to the extreme, a unit file could be checking for the 
> > existence of a file that is in a different SELinux domain.  Does it 
> > instead make sense to use the 'files_getattr_all_files', 
> > 'files_getattr_all_sockets', 'files_getattr_all_pipes', etc... in init.te?
> > 
> 
> $ man systemd.directives | grep -i conditionpath
>        ConditionPathExists=
>        ConditionPathExistsGlob=
>        ConditionPathIsDirectory=
>        ConditionPathIsMountPoint=
>        ConditionPathIsReadWrite=
>        ConditionPathIsSymbolicLink=

Don't get me wrong though. In DSSP2 standard I allow systemd to read all config, which is also a less-than-optimal compromise.
I basically do this because systemd often needs to be able to read environment config files in /etc/sysconfig (EnvironmentFile=)
Ideally I would also narrow this down and maybe one day i will (or maybe not)

The point i am trying to make wrt to this patch though, is that ConditionPath* is not limited to config files

> 
> > 
> > >>
> > >> Signed-off-by: Dave Sugar <dsugar@tresys.com>
> > >> ---
> > >>   policy/modules/kernel/files.if | 20 ++++++++++++++++++++
> > >>   policy/modules/system/init.te  |  1 +
> > >>   policy/modules/system/lvm.te   |  2 +-
> > >>   3 files changed, 22 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/policy/modules/kernel/files.if b/policy/modules/kernel/files.if
> > >> index f1c94411..87be07ae 100644
> > >> --- a/policy/modules/kernel/files.if
> > >> +++ b/policy/modules/kernel/files.if
> > >> @@ -1562,6 +1562,26 @@ interface(`files_relabel_config_dirs',`
> > >>   	relabel_dirs_pattern($1, configfile, configfile)
> > >>   ')
> > >>   
> > >> +########################################
> > >> +## <summary>
> > >> +##	Getattr config files in /etc.
> > >> +## </summary>
> > >> +## <param name="domain">
> > >> +##	<summary>
> > >> +##	Domain allowed access.
> > >> +##	</summary>
> > >> +## </param>
> > >> +#
> > >> +interface(`files_getattr_config_files',`
> > >> +	gen_require(`
> > >> +		attribute configfile;
> > >> +	')
> > >> +
> > >> +	allow $1 configfile:dir list_dir_perms;
> > >> +	getattr_files_pattern($1, configfile, configfile)
> > >> +	read_lnk_files_pattern($1, configfile, configfile)
> > >> +')
> > >> +
> > >>   ########################################
> > >>   ## <summary>
> > >>   ##	Read config files in /etc.
> > >> diff --git a/policy/modules/system/init.te b/policy/modules/system/init.te
> > >> index 8973a622..747b696e 100644
> > >> --- a/policy/modules/system/init.te
> > >> +++ b/policy/modules/system/init.te
> > >> @@ -320,6 +320,7 @@ ifdef(`init_systemd',`
> > >>   	domain_subj_id_change_exemption(init_t)
> > >>   	domain_role_change_exemption(init_t)
> > >>   
> > >> +	files_getattr_config_files(init_t)
> > >>   	files_read_all_pids(init_t)
> > >>   	files_list_usr(init_t)
> > >>   	files_list_var(init_t)
> > >> diff --git a/policy/modules/system/lvm.te b/policy/modules/system/lvm.te
> > >> index ad4eb579..c05344e0 100644
> > >> --- a/policy/modules/system/lvm.te
> > >> +++ b/policy/modules/system/lvm.te
> > >> @@ -25,7 +25,7 @@ domain_obj_id_change_exemption(lvm_t)
> > >>   role system_r types lvm_t;
> > >>   
> > >>   type lvm_etc_t;
> > >> -files_type(lvm_etc_t)
> > >> +files_config_file(lvm_etc_t)
> > >>   
> > >>   type lvm_lock_t;
> > >>   files_lock_file(lvm_lock_t)
> > >> -- 
> > >> 2.21.0
> > >>
> > > 
> 
> -- 
> Key fingerprint = 5F4D 3CDB D3F8 3652 FBD8 02D5 3B6C 5F1D 2C7B 6B02
> https://sks-keyservers.net/pks/lookup?op=get&search=0x3B6C5F1D2C7B6B02
> Dominick Grift



-- 
Key fingerprint = 5F4D 3CDB D3F8 3652 FBD8 02D5 3B6C 5F1D 2C7B 6B02
https://sks-keyservers.net/pks/lookup?op=get&search=0x3B6C5F1D2C7B6B02
Dominick Grift

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Allow systemd to getattr configfile

[Sugar, David]

On 12/5/19 2:46 AM, Dominick Grift wrote:
> On Wed, Dec 04, 2019 at 06:43:43PM +0100, Dominick Grift wrote:
>> On Wed, Dec 04, 2019 at 05:22:55PM +0000, Sugar, David wrote:
>>>
>>>
>>> On 12/4/19 11:56 AM, Dominick Grift wrote:
>>>> On Wed, Dec 04, 2019 at 04:33:20PM +0000, Sugar, David wrote:
>>>>> Systemd has ConditionalPathExists which is used to check if a path exists to control starting a service.  But this requires getattr permissions on the file.  This is generally for configuration files.  We are mostly seeing this is in our own policy.  But this lvm denial also fits the example.
>>>>>
>>>>> type=AVC msg=audit(1575427946.229:1624): avc:  denied  { getattr } for  pid=1 comm="systemd" path="/etc/lvm/lvm.conf" dev="dm-0" ino=51799 scontext=system_u:system_r:init_t:s0 tcontext=system_u:object_r:lvm_etc_t:s0 tclass=file permissive=0
>>>>>
>>>>> This second example is from chronyd, but it is happening becuase I added the conditional in a drop-in file. Note that chronyd_conf_t is already a 'configfile'.
>>>>>
>>>>> type=AVC msg=audit(1575427959.882:1901): avc:  denied  { getattr } for  pid=1 comm="systemd" path="/etc/chrony.conf" dev="dm-0" ino=53824 scontext=system_u:system_r:init_t:s0 tcontext=system_u:object_r:chronyd_conf_t:s0 tclass=file permissive=1
>>>>
>>>> how about something a little more general?
>>>>
>>>> systemd_ConditionPath(`,'
>>>> 	allow init_t $1:dir search_dir_perms;
>>>> 	allow init_t $1:lnk_file read_lnk_file_perms;
>>>> 	allow init_t $1:fifo_file getattr_fifo_file_perms;
>>>> 	allow init_t $1:sock_file getattr_sock_file_perms;
>>>> 	allow init_t $1:file getattr_file_perms;
>>>> 	allow init_t $1:blk_file getattr_blk_file_perms;
>>>> 	allow init_t $1:chr_file getattr_chr_file_perms;
>>>> ')
>>>>
>>> I think you are suggesting an interface 'systemd_conditionpath' that
>>> would exist in init.if and then need to be used by any module that wants
>>> to grant access to a particular type to getattr?
>>>
>>> So, for this case, I would need to modify chronyd.te and lvm.te to use
>>> this interface?
>>>
>>> I think you are also suggesting that ConditionPathExists usage in a unit
>>> file could be trying to check for the existence of something other than
>>> a configuration file.
>>>
>>> Taking it to the extreme, a unit file could be checking for the
>>> existence of a file that is in a different SELinux domain.  Does it
>>> instead make sense to use the 'files_getattr_all_files',
>>> 'files_getattr_all_sockets', 'files_getattr_all_pipes', etc... in init.te?
>>>
>>
>> $ man systemd.directives | grep -i conditionpath
>>         ConditionPathExists=
>>         ConditionPathExistsGlob=
>>         ConditionPathIsDirectory=
>>         ConditionPathIsMountPoint=
>>         ConditionPathIsReadWrite=
>>         ConditionPathIsSymbolicLink=
> 
> Don't get me wrong though. In DSSP2 standard I allow systemd to read all config, which is also a less-than-optimal compromise.
> I basically do this because systemd often needs to be able to read environment config files in /etc/sysconfig (EnvironmentFile=)
> Ideally I would also narrow this down and maybe one day i will (or maybe not)
> 
> The point i am trying to make wrt to this patch though, is that ConditionPath* is not limited to config files
> 

I agree with you, and I'm very much in favor of a more generic solution 
that will cover as much of this type of stuff as possible.  I just don't 
have any other ideas yet.

Looking at current refpolicy, it looks like most (but not all) of the 
files in /etc/sysconfig are labeled etc_t (at least on my system) and 
init_t can read etc_t files already.

Maybe we need a mix of solutions here.  Something added to init.te to 
grant most of the obvious access needed to deal with many of these cases 
and then an interface in init.if to give an ability to grant additional 
access when the generic case isn't enough for some reason.  Maybe grant 
getattr access to all 'non_security_file_type'.

Then an interface like the following (which is just an idea) where $1 is 
the type that init_t needs to getattr for a Conditional* and $2 is one 
(or more) object classes.

interface(`init_systemd_conditional','
	gen_require(`
		type init_t;
	')

	allow init_t $1:dir list_dir_perms;
	allow init_t $1:{ $2 } { getattr };
	read_lnk_files_pattern(init_t, $1, $1)
')

Alternatively add a new attribute in init.te called 
'systemd_conditional' which init.te has getattr permission on and then 
types can be added to that attribute that need to be used in a 
Condition*.  Granted that is very similar to what you initial proposed 
just using an attribute instead.

I'm also hoping that Chris will chime in with some opinion on this topic.

>>
>>>
>>>>>
>>>>> Signed-off-by: Dave Sugar <dsugar@tresys.com>
>>>>> ---
>>>>>    policy/modules/kernel/files.if | 20 ++++++++++++++++++++
>>>>>    policy/modules/system/init.te  |  1 +
>>>>>    policy/modules/system/lvm.te   |  2 +-
>>>>>    3 files changed, 22 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/policy/modules/kernel/files.if b/policy/modules/kernel/files.if
>>>>> index f1c94411..87be07ae 100644
>>>>> --- a/policy/modules/kernel/files.if
>>>>> +++ b/policy/modules/kernel/files.if
>>>>> @@ -1562,6 +1562,26 @@ interface(`files_relabel_config_dirs',`
>>>>>    	relabel_dirs_pattern($1, configfile, configfile)
>>>>>    ')
>>>>>    
>>>>> +########################################
>>>>> +## <summary>
>>>>> +##	Getattr config files in /etc.
>>>>> +## </summary>
>>>>> +## <param name="domain">
>>>>> +##	<summary>
>>>>> +##	Domain allowed access.
>>>>> +##	</summary>
>>>>> +## </param>
>>>>> +#
>>>>> +interface(`files_getattr_config_files',`
>>>>> +	gen_require(`
>>>>> +		attribute configfile;
>>>>> +	')
>>>>> +
>>>>> +	allow $1 configfile:dir list_dir_perms;
>>>>> +	getattr_files_pattern($1, configfile, configfile)
>>>>> +	read_lnk_files_pattern($1, configfile, configfile)
>>>>> +')
>>>>> +
>>>>>    ########################################
>>>>>    ## <summary>
>>>>>    ##	Read config files in /etc.
>>>>> diff --git a/policy/modules/system/init.te b/policy/modules/system/init.te
>>>>> index 8973a622..747b696e 100644
>>>>> --- a/policy/modules/system/init.te
>>>>> +++ b/policy/modules/system/init.te
>>>>> @@ -320,6 +320,7 @@ ifdef(`init_systemd',`
>>>>>    	domain_subj_id_change_exemption(init_t)
>>>>>    	domain_role_change_exemption(init_t)
>>>>>    
>>>>> +	files_getattr_config_files(init_t)
>>>>>    	files_read_all_pids(init_t)
>>>>>    	files_list_usr(init_t)
>>>>>    	files_list_var(init_t)
>>>>> diff --git a/policy/modules/system/lvm.te b/policy/modules/system/lvm.te
>>>>> index ad4eb579..c05344e0 100644
>>>>> --- a/policy/modules/system/lvm.te
>>>>> +++ b/policy/modules/system/lvm.te
>>>>> @@ -25,7 +25,7 @@ domain_obj_id_change_exemption(lvm_t)
>>>>>    role system_r types lvm_t;
>>>>>    
>>>>>    type lvm_etc_t;
>>>>> -files_type(lvm_etc_t)
>>>>> +files_config_file(lvm_etc_t)
>>>>>    
>>>>>    type lvm_lock_t;
>>>>>    files_lock_file(lvm_lock_t)
>>>>> -- 
>>>>> 2.21.0
>>>>>
>>>>
>>
>> -- 
>> Key fingerprint = 5F4D 3CDB D3F8 3652 FBD8 02D5 3B6C 5F1D 2C7B 6B02
>> https://sks-keyservers.net/pks/lookup?op=get&search=0x3B6C5F1D2C7B6B02
>> Dominick Grift
> 
> 
> 

Re: [PATCH] Allow systemd to getattr configfile

[Chris PeBenito]

On 12/5/19 8:19 AM, Sugar, David wrote:
> 
> 
> On 12/5/19 2:46 AM, Dominick Grift wrote:
>> On Wed, Dec 04, 2019 at 06:43:43PM +0100, Dominick Grift wrote:
>>> On Wed, Dec 04, 2019 at 05:22:55PM +0000, Sugar, David wrote:
>>>>
>>>>
>>>> On 12/4/19 11:56 AM, Dominick Grift wrote:
>>>>> On Wed, Dec 04, 2019 at 04:33:20PM +0000, Sugar, David wrote:
>>>>>> Systemd has ConditionalPathExists which is used to check if a path exists to control starting a service.  But this requires getattr permissions on the file.  This is generally for configuration files.  We are mostly seeing this is in our own policy.  But this lvm denial also fits the example.
>>>>>>
>>>>>> type=AVC msg=audit(1575427946.229:1624): avc:  denied  { getattr } for  pid=1 comm="systemd" path="/etc/lvm/lvm.conf" dev="dm-0" ino=51799 scontext=system_u:system_r:init_t:s0 tcontext=system_u:object_r:lvm_etc_t:s0 tclass=file permissive=0
>>>>>>
>>>>>> This second example is from chronyd, but it is happening becuase I added the conditional in a drop-in file. Note that chronyd_conf_t is already a 'configfile'.
>>>>>>
>>>>>> type=AVC msg=audit(1575427959.882:1901): avc:  denied  { getattr } for  pid=1 comm="systemd" path="/etc/chrony.conf" dev="dm-0" ino=53824 scontext=system_u:system_r:init_t:s0 tcontext=system_u:object_r:chronyd_conf_t:s0 tclass=file permissive=1
>>>>>
>>>>> how about something a little more general?
>>>>>
>>>>> systemd_ConditionPath(`,'
>>>>> 	allow init_t $1:dir search_dir_perms;
>>>>> 	allow init_t $1:lnk_file read_lnk_file_perms;
>>>>> 	allow init_t $1:fifo_file getattr_fifo_file_perms;
>>>>> 	allow init_t $1:sock_file getattr_sock_file_perms;
>>>>> 	allow init_t $1:file getattr_file_perms;
>>>>> 	allow init_t $1:blk_file getattr_blk_file_perms;
>>>>> 	allow init_t $1:chr_file getattr_chr_file_perms;
>>>>> ')
>>>>>
>>>> I think you are suggesting an interface 'systemd_conditionpath' that
>>>> would exist in init.if and then need to be used by any module that wants
>>>> to grant access to a particular type to getattr?
>>>>
>>>> So, for this case, I would need to modify chronyd.te and lvm.te to use
>>>> this interface?
>>>>
>>>> I think you are also suggesting that ConditionPathExists usage in a unit
>>>> file could be trying to check for the existence of something other than
>>>> a configuration file.
>>>>
>>>> Taking it to the extreme, a unit file could be checking for the
>>>> existence of a file that is in a different SELinux domain.  Does it
>>>> instead make sense to use the 'files_getattr_all_files',
>>>> 'files_getattr_all_sockets', 'files_getattr_all_pipes', etc... in init.te?
>>>>
>>>
>>> $ man systemd.directives | grep -i conditionpath
>>>          ConditionPathExists=
>>>          ConditionPathExistsGlob=
>>>          ConditionPathIsDirectory=
>>>          ConditionPathIsMountPoint=
>>>          ConditionPathIsReadWrite=
>>>          ConditionPathIsSymbolicLink=
>>
>> Don't get me wrong though. In DSSP2 standard I allow systemd to read all config, which is also a less-than-optimal compromise.
>> I basically do this because systemd often needs to be able to read environment config files in /etc/sysconfig (EnvironmentFile=)
>> Ideally I would also narrow this down and maybe one day i will (or maybe not)
>>
>> The point i am trying to make wrt to this patch though, is that ConditionPath* is not limited to config files
>>
> 
> I agree with you, and I'm very much in favor of a more generic solution
> that will cover as much of this type of stuff as possible.  I just don't
> have any other ideas yet.
> 
> Looking at current refpolicy, it looks like most (but not all) of the
> files in /etc/sysconfig are labeled etc_t (at least on my system) and
> init_t can read etc_t files already.
> 
> Maybe we need a mix of solutions here.  Something added to init.te to
> grant most of the obvious access needed to deal with many of these cases
> and then an interface in init.if to give an ability to grant additional
> access when the generic case isn't enough for some reason.  Maybe grant
> getattr access to all 'non_security_file_type'.
> 
> Then an interface like the following (which is just an idea) where $1 is
> the type that init_t needs to getattr for a Conditional* and $2 is one
> (or more) object classes.
> 
> interface(`init_systemd_conditional','
> 	gen_require(`
> 		type init_t;
> 	')
> 
> 	allow init_t $1:dir list_dir_perms;
> 	allow init_t $1:{ $2 } { getattr };
> 	read_lnk_files_pattern(init_t, $1, $1)
> ')
> 
> Alternatively add a new attribute in init.te called
> 'systemd_conditional' which init.te has getattr permission on and then
> types can be added to that attribute that need to be used in a
> Condition*.  Granted that is very similar to what you initial proposed
> just using an attribute instead.
> 
> I'm also hoping that Chris will chime in with some opinion on this topic.

My interface preference is what Dominick is suggesting though it should 
be an init interface.


-- 
Chris PeBenito