[bug#44549] etc: updates for the guix-daemon SELinux policy

Daniel Brooks <db48x@db48x.net> writes:

>>>> +  (allow guix_daemon_t
>>>> +         guix_daemon_socket_t
>>>> +         (sock_file (unlink)))
>>>
>>> That shouldn't be a problem, though we don't have any other rules for
>>> guix_daemon_socket_t. Possibly that is because my socket file is labeled
>>> guix_daemon_conf_t, for unknown reasons. Perhaps it was not labeled
>>> correctly when created, and hasn't been relabeled since.
>>
>> It could also be an artifact from my ancient experiments with Guix and
>> SELinux on this system.  Perhaps we should test on a "clean" system to
>> verify, I can do that next week.
>
> Ok, I figured this one out. When the socket file is created it is
> labeled at guix_daemon_conf_t, but the filecon rules will cause that to
> be relabeled to guix_daemon_socket_t at some point in the future. When
> the guix-daemon process stops it tries to delete the socket file, but
> can't. I'll go ahead and include the rule.

OK.

>> As a side note, I've seen a couple other audit messages from
>> guix-daemon, although though they don't seem to cause a problem in
>> practice.
>>
>> type=AVC msg=audit(1605189801.627:8637388): avc: denied { read } for
>> pid=2312896 comm="guix-daemon" path="socket:[74336318]" dev="sockfs"
>> ino=74336318 scontext=system_u:system_r:guix_daemon.guix_daemon_t:s0
>> tcontext=system_u:system_r:init_t:s0 tclass=unix_stream_socket
>> permissive=0
>> type=AVC msg=audit(1605189801.627:8637388): avc: denied { read } for
>> pid=2312896 comm="guix-daemon" path="socket:[74336318]" dev="sockfs"
>> ino=74336318 scontext=system_u:system_r:guix_daemon.guix_daemon_t:s0
>> tcontext=system_u:system_r:init_t:s0 tclass=unix_stream_socket
>> permissive=0
>> type=AVC msg=audit(1605189801.627:8637388): avc: denied { siginh } for
>> pid=2312896 comm="guix-daemon" scontext=system_u:system_r:init_t:s0
>> tcontext=system_u:system_r:guix_daemon.guix_daemon_t:s0 tclass=process
>> permissive=0
>
> The first two are already covered by the new policy, and the third is
> inconsequential. The kernel checks on our behalf to see if our child
> processes are allowed to inherit our signal state. That's usually
> disallowed, so that rule is marked 'dontaudit' so that it doesn't spam
> the logs; you probably had that disabled. I'm not going to add a rule
> allowing that one; It would just cause accidents.

Thanks for investigating.

Interestingly, after updating the system (both RHEL8 and Guix) and
rebooting, I got new SELinux troubles!

I had to add these additional rules to make guix-daemon start again:
Do these look sane to you?  I can squash them into the commit if so.

Marius Bakke <marius@gnu.org> writes:

> Interestingly, after updating the system (both RHEL8 and Guix) and
> rebooting, I got new SELinux troubles!
>
> I had to add these additional rules to make guix-daemon start again:
>
> diff --git a/etc/guix-daemon.cil.in b/etc/guix-daemon.cil.in
> index 47fd12a214..3e254a2187 100644
> --- a/etc/guix-daemon.cil.in
> +++ b/etc/guix-daemon.cil.in
> @@ -86,12 +86,15 @@
>    (allow init_t
>           guix_daemon_t
>           (process (transition)))
> +  (allow init_t
> +         self
> +         (process (execmem)))

At some point we should track down why that one is necessary, perhaps
Guile has a JIT compiler or something?

>    (allow init_t
>           guix_store_content_t
> -         (file (open read execute)))
> +         (file (open read execute execute_no_trans map)))

This one looks pretty suspicious. I think it would allow any file
labeled guix_store_content_t to run in the init_t domain? We wouldn't
want that.

db48x

Daniel Brooks <db48x@db48x.net> writes:

> Marius Bakke <marius@gnu.org> writes:
>
>> Interestingly, after updating the system (both RHEL8 and Guix) and
>> rebooting, I got new SELinux troubles!
>>
>> I had to add these additional rules to make guix-daemon start again:
>>
>> diff --git a/etc/guix-daemon.cil.in b/etc/guix-daemon.cil.in
>> index 47fd12a214..3e254a2187 100644
>> --- a/etc/guix-daemon.cil.in
>> +++ b/etc/guix-daemon.cil.in
>> @@ -86,12 +86,15 @@
>>    (allow init_t
>>           guix_daemon_t
>>           (process (transition)))
>> +  (allow init_t
>> +         self
>> +         (process (execmem)))
>
> At some point we should track down why that one is necessary, perhaps
> Guile has a JIT compiler or something?

Ding ding ding.

  https://wingolog.org/archives/2019/05/24/lightening-run-time-code-generation

>>    (allow init_t
>>           guix_store_content_t
>> -         (file (open read execute)))
>> +         (file (open read execute execute_no_trans map)))
>
> This one looks pretty suspicious. I think it would allow any file
> labeled guix_store_content_t to run in the init_t domain? We wouldn't
> want that.

Right.  The guix_store_content_t file in question was 'guile', which I
suppose is a kind of special case.  Can you think of any workarounds
for this?

Are you testing with the latest version of guix-daemon?