Re: [PATCH v2 2/2] libselinux: retain LIFO order for path substitutions

[Dominick Grift <dominick.grift@defensec.nl>]

Petr Lautrbach <lautrbach@redhat.com> writes:

> Christian Göttsche <cgoettsche@seltendoof.de> writes:
>
>> From: Christian Göttsche <cgzones@googlemail.com>
>>
>> Store the path substitutions in LIFO order as in previous versions.
>>
>> Fixes: 4d436e4b ("libselinux: use vector instead of linked list for substitutions")
>> Reported-by: Dominick Grift <dominick.grift@defensec.nl>
>> Link: https://lore.kernel.org/selinux/87ldqftsxd.fsf@defensec.nl/
>> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
>> ---
>> v2: add patch
>> ---
>>  libselinux/src/label_file.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
>> index 61a9ddb6..b785eab6 100644
>> --- a/libselinux/src/label_file.c
>> +++ b/libselinux/src/label_file.c
>> @@ -1350,6 +1350,15 @@ static int selabel_subs_init(const char *path, struct selabel_digest *digest,
>>  	if (digest_add_specfile(digest, cfg, NULL, sb.st_size, path) < 0)
>>  		goto err;
>>  
>> +	/* LIFO order for backward compatibility */
>> +	for (uint32_t i = 0; i < tmp_num/2; i++) {
>> +		struct selabel_sub swap;
>> +
>> +		swap = tmp[i];
>> +		tmp[i] = tmp[tmp_num - i - 1];
>> +		tmp[tmp_num - i - 1] = swap;
>> +	}
>> +
>>  	*out_subs = tmp;
>>  	*out_num = tmp_num;
>>  	*out_alloc = tmp_alloc;
>> -- 
>> 2.49.0
>
> It fixes the original reproducer:
>
> root@fedora:/# cat /etc/selinux/fedora-selinux/contexts/files/file_contexts.subs
> /new_root /
> /new_root/foo /usr/bin
> root@fedora:/# matchpathcon /new_root /new_root/foo
> /new_root       system_u:object_r:root_t:s0
> /new_root/foo   system_u:object_r:bin_t:s0
>
> but it's important to say it depends on the order of entries in .subst:
>
> root@fedora:/# cat /etc/selinux/fedora-selinux/contexts/files/file_contexts.subs
> /new_root/foo /usr/bin
> /new_root /
> root@fedora:/# matchpathcon /new_root /new_root/foo
> /new_root       system_u:object_r:root_t:s0
> /new_root/foo   system_u:object_r:etc_runtime_t:s0
>
>
> it looks like it was like that even before with 3.8:
>
> [root@fedora-41 /]# cat /etc/selinux/targeted/contexts/files/file_contexts.subs
> /new_root /
> /new_root/foo /usr/bin
> [root@fedora-41 /]# matchpathcon /new_root /new_root/foo
> /new_root       system_u:object_r:root_t:s0
> /new_root/foo   system_u:object_r:bin_t:s0
>
> [root@fedora-41 /]# cat /etc/selinux/targeted/contexts/files/file_contexts.subs
> /new_root/foo /usr/bin
> /new_root /
> [root@fedora-41 /]# matchpathcon /new_root /new_root/foo
> /new_root       system_u:object_r:root_t:s0
> /new_root/foo   system_u:object_r:etc_runtime_t:s0
>
> It would be great to have this behavior documented, but it's a different
> issue.
>
> If there's no other objection I'll merge tomorrow before 3.9-rc1.
>
> Acked-by: Petr Lautrbach <lautrbach@redhat.com>
>

I applied this patch and it addresses my issue (restores compatiblity).
I can also confirm that ordering the specs indeed also addresses the
issue at least in a simple test I did.

I do prefer that ordering shouldnt matter.

Thanks!

-- 
gpg --locate-keys dominick.grift@defensec.nl (wkd)
Key fingerprint = FCD2 3660 5D6B 9D27 7FC6  E0FF DA7E 521F 10F6 4098
Dominick Grift
Mastodon: @kcinimod@defensec.nl