Received-SPF: pass (google.com: best guess record for domain of selinux-refpolicy-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   "Sugar, David" <dsugar@tresys.com>
To:     "selinux-refpolicy@vger.kernel.org" 
        <selinux-refpolicy@vger.kernel.org>
Subject: Re: [PATCH] Allow systemd to getattr configfile
Thread-Topic: [PATCH] Allow systemd to getattr configfile
Thread-Index: AQHVqsCK9y2wMKKqwU6WJMp7oqJPM6eqMfgA//+zi4CAAFm5gIAA63YAgABc5wA=
Date:   Thu, 5 Dec 2019 13:19:06 +0000
Message-ID: <394bf12e-7833-d4b9-1554-4f2755794152@tresys.com>
References: <20191204163306.16545-1-dsugar@tresys.com>
 <20191204163306.16545-2-dsugar@tresys.com>
 <20191204165614.GA1321684@brutus.lan>
 <e3d4c752-899f-5454-37f5-851378426e8a@tresys.com>
 <20191204174343.GC1321684@brutus.lan> <20191205074628.GA1734091@brutus.lan>
In-Reply-To: <20191205074628.GA1734091@brutus.lan>
Accept-Language: en-US
Content-Language: en-US
user-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.1.1
x-ms-exchange-messagesentrepresentingtype: 1
x-ms-publictraffictype: Email
x-ms-office365-filtering-correlation-id: b4839f55-66b2-4995-8792-08d77985b45d
x-ms-traffictypediagnostic: MW2PR1501MB2092:
x-microsoft-antispam-prvs: <MW2PR1501MB20923524ABDD0B1C0AEDF078B95C0@MW2PR1501MB2092.namprd15.prod.outlook.com>
x-ms-oob-tlc-oobclassifiers: OLM:7691;
x-forefront-prvs: 02426D11FE
x-forefront-antispam-report: SFV:NSPM;SFS:(10001)(10019020)(4636009)(39840400004)(396003)(376002)(136003)(346002)(366004)(199004)(189003)(14454004)(2906002)(229853002)(6486002)(58126008)(8676002)(508600001)(6506007)(6916009)(316002)(102836004)(65956001)(5660300002)(186003)(81156014)(86362001)(31696002)(36756003)(53546011)(8936002)(66556008)(31686004)(66946007)(99286004)(66446008)(81166006)(66476007)(966005)(26005)(64756008)(6512007)(25786009)(11346002)(2616005)(71200400001)(14444005)(71190400001)(5640700003)(52116002)(305945005)(76176011);DIR:OUT;SFP:1102;SCL:1;SRVR:MW2PR1501MB2092;H:MW2PR1501MB1978.namprd15.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;A:1;MX:1;
received-spf: None (protection.outlook.com: tresys.com does not designate
 permitted sender hosts)
x-ms-exchange-senderadcheck: 1
x-microsoft-antispam: BCL:0;
x-microsoft-antispam-message-info: EdBExL/qP4yF00h1/RVGnqUWHVWRBQjWBll+gd2nq2IdOqkIYM7eQGgEfUH5DZZxvjttkWN85ZkQcQq1XSgQw3KRsAUGg14Lni2iwJ+vJ8aDKHEaoKCfXZHW5d0NkJgtm4uAb2VG8KIs6io+a58sGJazxlcxmwNGPLf/JmvA7V1aEBAAUQ9xEKwLB7xg1MtBYkaSnagBraPYuS11mbNV8+UAydS+sv+xX/ixC3EtSowgmxJMqGF7QpXUyTbjZwVyVgMaYsJsNi/GvVkwrQLrYxFiMrt6SJqFvaArUH2D8TaYKWs4iUSqUQzSBsIG6Ir2XE+FcCSmCjgVaKz6vjWqFYiJcxeBdgarGWA30priTOmeIX10HhsWA0tWwKtDR0Znp8L2RrHcMhRl21gdxMo4rffd6BEKa+I0fI6+e0XjMgApgtuKV+9w1fqlOClZ8wtSTMOvOOOXWg5OJcAbKWjXuhTofXPrq6rRXahTpDnIW1r4ejuklERpGHf4Fs0hYEhnZ1+qpbGfP0B8oUXOR+UXqDYL7pmyfdUyoIb+PsrplkA=
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="Windows-1252"
Content-ID: <A7060B5BFB81654EBCFD4C08573DDC42@namprd15.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: tresys.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b4839f55-66b2-4995-8792-08d77985b45d
X-MS-Exchange-CrossTenant-originalarrivaltime: 05 Dec 2019 13:19:06.2502
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: a0d45667-6c07-4e88-868f-4ac9af95c7ed
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 8PCK6zlfNO/iKrvCn09bPBK3fO1IYcYDvGbxn4UaF3h+21Zq1TKD9ml207oYYBnd2+d4NPjMFP47dknERKKKzQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW2PR1501MB2092
Sender: selinux-refpolicy-owner@vger.kernel.org
Precedence: bulk
List-ID: <selinux-refpolicy.vger.kernel.org>
X-Mailing-List: selinux-refpolicy@vger.kernel.org


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 ex=
ists to control starting a service.  But this requires getattr permissions =
on the file.  This is generally for configuration files.  We are mostly see=
ing this is in our own policy.  But this lvm denial also fits the example.
>>>>>
>>>>> type=3DAVC msg=3Daudit(1575427946.229:1624): avc:  denied  { getattr =
} for  pid=3D1 comm=3D"systemd" path=3D"/etc/lvm/lvm.conf" dev=3D"dm-0" ino=
=3D51799 scontext=3Dsystem_u:system_r:init_t:s0 tcontext=3Dsystem_u:object_=
r:lvm_etc_t:s0 tclass=3Dfile permissive=3D0
>>>>>
>>>>> This second example is from chronyd, but it is happening becuase I ad=
ded the conditional in a drop-in file. Note that chronyd_conf_t is already =
a 'configfile'.
>>>>>
>>>>> type=3DAVC msg=3Daudit(1575427959.882:1901): avc:  denied  { getattr =
} for  pid=3D1 comm=3D"systemd" path=3D"/etc/chrony.conf" dev=3D"dm-0" ino=
=3D53824 scontext=3Dsystem_u:system_r:init_t:s0 tcontext=3Dsystem_u:object_=
r:chronyd_conf_t:s0 tclass=3Dfile permissive=3D1
>>>>
>>>> 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 want=
s
>>> 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 uni=
t
>>> 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=3D
>>         ConditionPathExistsGlob=3D
>>         ConditionPathIsDirectory=3D
>>         ConditionPathIsMountPoint=3D
>>         ConditionPathIsReadWrite=3D
>>         ConditionPathIsSymbolicLink=3D
>=20
> 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 enviro=
nment config files in /etc/sysconfig (EnvironmentFile=3D)
> Ideally I would also narrow this down and maybe one day i will (or maybe =
not)
>=20
> The point i am trying to make wrt to this patch though, is that Condition=
Path* is not limited to config files
>=20

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

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

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

Then an interface like the following (which is just an idea) where $1 is=20
the type that init_t needs to getattr for a Conditional* and $2 is one=20
(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=20
'systemd_conditional' which init.te has getattr permission on and then=20
types can be added to that attribute that need to be used in a=20
Condition*.  Granted that is very similar to what you initial proposed=20
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/f=
iles.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)
>>>>>    ')
>>>>>   =20
>>>>> +########################################
>>>>> +## <summary>
>>>>> +##	Getattr config files in /etc.
>>>>> +## </summary>
>>>>> +## <param name=3D"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/in=
it.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)
>>>>>   =20
>>>>> +	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;
>>>>>   =20
>>>>>    type lvm_etc_t;
>>>>> -files_type(lvm_etc_t)
>>>>> +files_config_file(lvm_etc_t)
>>>>>   =20
>>>>>    type lvm_lock_t;
>>>>>    files_lock_file(lvm_lock_t)
>>>>> --=20
>>>>> 2.21.0
>>>>>
>>>>
>>
>> --=20
>> Key fingerprint =3D 5F4D 3CDB D3F8 3652 FBD8 02D5 3B6C 5F1D 2C7B 6B02
>> https://sks-keyservers.net/pks/lookup?op=3Dget&search=3D0x3B6C5F1D2C7B6B=
02
>> Dominick Grift
>=20
>=20
>=20