qubes-core-agent postinst script selinux related issues

[adrelanos]

There are various problems with the following code.

https://github.com/marmarek/qubes-core-agent-linux/blob/f95c3990baf2acbe72d5214841e078139ceb5102/debian/qubes-core-agent.postinst#L198-L206

                # Disable SELinux"
                /etc/selinux/config)
                    echo "Disabling SELinux..."
                    if [ -e /etc/selinux/config ]; then
                        sed -e s/^SELINUX=.*$/SELINUX=disabled/ </etc/selinux/config >/etc/selinux/config.processed
                        mv /etc/selinux/config.processed /etc/selinux/config
                        setenforce 0 2>/dev/null
                    fi
                    ;;

• minor: extraneous quote in comment
• minor: a comment is redundant when the code is self explanatory
• minor: no need to comment something, if it's explained in an above echo
• It would fail when file /etc/selinux/config exists while setenforce is not installed. I think installing and removing (not purging) the selinux package should trigger this, while I haven't actually tested this. It's not guarded by command -v.

if command -v setenforce >/dev/null 2>&1 ; then
   ...
fi 

• Actually, perhaps alternatively, that whole code block could be safely removed.
  • 'apt-get install'ing selinux is not enough to enable it as long as Debian feature request Please automatically enable AppArmor when the userspace tools are installed is not implemented.
  • Even if that was implemented, selinux still would not be enabled, because kernel parameters cannot be influenced from within the VM with Qubes.
• Why do we force disable selinux for the user anyway? This is something that could use a comment in the code. If the user wants to install selinux, experiment with it. More power to them. No reason to interfere with it? Or would enabling selinux break that Qubes VM so badly that the user could not even disable selinux again?

I am happily be corrected if I suffer from any misconceptions.

[marmarek]

• Even if that was implemented, selinux still would not be enabled, because kernel parameters cannot be influenced from within the VM with Qubes.

Unfortunately not true, because it is possible to enable it by default
in kernel configuration. And kernel from Fedora (which can be easily
installed in Qubes) has this option enabled
(CONFIG_SECURITY_SELINUX_BOOTPARAM_VALUE=1).

• Why do we force disable selinux for the user anyway? This is something that could use a comment in the code. If the user wants to install selinux, experiment with it. More power to them. No reason to interfere with it? Or would enabling selinux break that Qubes VM so badly that the user could not even disable selinux again?

Some time ago enabling SELinux makes Fedora VMs unbootable. But this was
few years ago. Checked just now and at least it doesn't breaks that
badly. Only some minor problems (denials) with rtkit-daemon.

But all the user processes are running as
system_u:system_r:unconfined_service_t:s0. So to have some meaningful
SELinux support, it would require some work on gui-agent/qrexec-agent[1] to
properly switch selinux context (including writing some policy for
that). Not something we want to do ourself.

[1] Some starting point could be 'pam' branch in my
'qubes-core-agent-linux' repo:
https://github.com/marmarek/qubes-core-agent-linux/commits/pam

Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[marmarek]

• Even if that was implemented, selinux still would not be enabled, because kernel parameters cannot be influenced from within the VM with Qubes.

Unfortunately not true, because it is possible to enable it by default
in kernel configuration. And kernel from Fedora (which can be easily
installed in Qubes) has this option enabled
(CONFIG_SECURITY_SELINUX_BOOTPARAM_VALUE=1).

• Why do we force disable selinux for the user anyway? This is something that could use a comment in the code. If the user wants to install selinux, experiment with it. More power to them. No reason to interfere with it? Or would enabling selinux break that Qubes VM so badly that the user could not even disable selinux again?

Some time ago enabling SELinux makes Fedora VMs unbootable. But this was
few years ago. Checked just now and at least it doesn't breaks that
badly. Only some minor problems (denials) with rtkit-daemon.

But all the user processes are running as
system_u:system_r:unconfined_service_t:s0. So to have some meaningful
SELinux support, it would require some work on gui-agent/qrexec-agent[1] to
properly switch selinux context (including writing some policy for
that). Not something we want to do ourself.

[1] Some starting point could be 'pam' branch in my
'qubes-core-agent-linux' repo:
https://github.com/marmarek/qubes-core-agent-linux/commits/pam

Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[nrgaway]

On 08/05/2015 02:51 PM, Patrick Schleizer wrote:

There are various problems with the following code.

https://github.com/marmarek/qubes-core-agent-linux/blob/f95c3990baf2acbe72d5214841e078139ceb5102/debian/qubes-core-agent.postinst#L198-L206

                # Disable SELinux"
                /etc/selinux/config)
                    echo "Disabling SELinux..."
                    if [ -e /etc/selinux/config ]; then
                        sed -e s/^SELINUX=.*$/SELINUX=disabled/ </etc/selinux/config >/etc/selinux/config.processed
                        mv /etc/selinux/config.processed /etc/selinux/config
                        setenforce 0 2>/dev/null
                    fi
                    ;;

• minor: extraneous quote in comment
• minor: a comment is redundant when the code is self explanatory
• minor: no need to comment something, if it's explained in an above echo
• It would fail when file /etc/selinux/config exists while setenforce is not installed. I think installing and removing (not purging) the selinux package should trigger this, while I haven't actually tested this. It's not guarded by command -v.

if command -v setenforce >/dev/null 2>&1 ; then
   ...
fi 

I will look at correct this with next qubes-core-agent PR update
(r3-xdg-config branch) which will be within a day or so. You may want
to look at the postinst script now as it won't change too much with
update. I have simplified xdg-config and modified qubes-desktop-run to
function with DBUS; that's what will be in the update when I have
completed testing.

* Actually, perhaps alternatively, that whole code block could be safely removed.
 * 'apt-get install'ing selinux is not enough to enable it as long as Debian feature request [Please automatically enable AppArmor when the userspace tools are installed](https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=702030) is not implemented.
  * Even if that was implemented, selinux still would not be enabled, because kernel parameters cannot be influenced from within the VM with Qubes.
* Why do we force disable selinux for the user anyway? This is something that could use a comment in the code. If the user wants to install selinux, experiment with it. More power to them. No reason to interfere with it? Or would enabling selinux break that Qubes VM so badly that the user could not even disable selinux again?

I am happily be corrected if I suffer from any misconceptions.

---
Reply to this email directly or view it on GitHub:
https://github.com/QubesOS/qubes-issues/issues/1103

[nrgaway]

On 08/05/2015 02:51 PM, Patrick Schleizer wrote:

There are various problems with the following code.

https://github.com/marmarek/qubes-core-agent-linux/blob/f95c3990baf2acbe72d5214841e078139ceb5102/debian/qubes-core-agent.postinst#L198-L206

                # Disable SELinux"
                /etc/selinux/config)
                    echo "Disabling SELinux..."
                    if [ -e /etc/selinux/config ]; then
                        sed -e s/^SELINUX=.*$/SELINUX=disabled/ </etc/selinux/config >/etc/selinux/config.processed
                        mv /etc/selinux/config.processed /etc/selinux/config
                        setenforce 0 2>/dev/null
                    fi
                    ;;

• minor: extraneous quote in comment
• minor: a comment is redundant when the code is self explanatory
• minor: no need to comment something, if it's explained in an above echo
• It would fail when file /etc/selinux/config exists while setenforce is not installed. I think installing and removing (not purging) the selinux package should trigger this, while I haven't actually tested this. It's not guarded by command -v.

if command -v setenforce >/dev/null 2>&1 ; then
   ...
fi 

I will look at correct this with next qubes-core-agent PR update
(r3-xdg-config branch) which will be within a day or so. You may want
to look at the postinst script now as it won't change too much with
update. I have simplified xdg-config and modified qubes-desktop-run to
function with DBUS; that's what will be in the update when I have
completed testing.

* Actually, perhaps alternatively, that whole code block could be safely removed.
 * 'apt-get install'ing selinux is not enough to enable it as long as Debian feature request [Please automatically enable AppArmor when the userspace tools are installed](https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=702030) is not implemented.
  * Even if that was implemented, selinux still would not be enabled, because kernel parameters cannot be influenced from within the VM with Qubes.
* Why do we force disable selinux for the user anyway? This is something that could use a comment in the code. If the user wants to install selinux, experiment with it. More power to them. No reason to interfere with it? Or would enabling selinux break that Qubes VM so badly that the user could not even disable selinux again?

I am happily be corrected if I suffer from any misconceptions.

---
Reply to this email directly or view it on GitHub:
https://github.com/QubesOS/qubes-issues/issues/1103

[marmarek]

What do we want to do with this issue? "minor" comments above are already fixed by @nrgaway. There is still possible issue with missing setenforce.
Anyway, do we want to remove this code? What is Debian default regarding SELinux - I guess disabled and even not installed, right? In such a case I think we don't need this code in Debian package.
We still need it in Fedora package, where (AFAIR) SELinux is enabled by default and of course we don't want to have it enabled by default when it is broken.

[marmarek]

What do we want to do with this issue? "minor" comments above are already fixed by @nrgaway. There is still possible issue with missing setenforce.
Anyway, do we want to remove this code? What is Debian default regarding SELinux - I guess disabled and even not installed, right? In such a case I think we don't need this code in Debian package.
We still need it in Fedora package, where (AFAIR) SELinux is enabled by default and of course we don't want to have it enabled by default when it is broken.

[adrelanos]

Debian: selinux not installed by default. Not enabled by default even if you install it, you still need to manually enable it in kernel parameters.

I think it's safe to remove that code for Debian. Premature suite upgrades (jessie -> stretch or jessie -> sid) are for advanced users only anyhow. And before you officially start supporting a new suite, the next stable, let's say Debian stretch in some time, then you need to check for such stuff anyhow. Beforehand. There is no way to preemptively handle everything they could change in future. Especially not for stuff that is not planned or even likely at this time.

[adrelanos]

Debian: selinux not installed by default. Not enabled by default even if you install it, you still need to manually enable it in kernel parameters.

I think it's safe to remove that code for Debian. Premature suite upgrades (jessie -> stretch or jessie -> sid) are for advanced users only anyhow. And before you officially start supporting a new suite, the next stable, let's say Debian stretch in some time, then you need to check for such stuff anyhow. Beforehand. There is no way to preemptively handle everything they could change in future. Especially not for stuff that is not planned or even likely at this time.

[marmarek]

Regarding kernel parameter - it may be enabled by default. For example
user is free to use kernel from Fedora repositories, which have those
options:

CONFIG_SECURITY_SELINUX=y
CONFIG_SECURITY_SELINUX_BOOTPARAM=y
CONFIG_SECURITY_SELINUX_BOOTPARAM_VALUE=1  # selinux=1 by default
CONFIG_DEFAULT_SECURITY_SELINUX=y
CONFIG_DEFAULT_SECURITY="selinux"  # security=selinux by default

Anyway if selinux userspace tools are not installed by default (and
enforcing is disabled), I think it shouldn't matter.

Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[marmarek]

Regarding kernel parameter - it may be enabled by default. For example
user is free to use kernel from Fedora repositories, which have those
options:

CONFIG_SECURITY_SELINUX=y
CONFIG_SECURITY_SELINUX_BOOTPARAM=y
CONFIG_SECURITY_SELINUX_BOOTPARAM_VALUE=1  # selinux=1 by default
CONFIG_DEFAULT_SECURITY_SELINUX=y
CONFIG_DEFAULT_SECURITY="selinux"  # security=selinux by default

Anyway if selinux userspace tools are not installed by default (and
enforcing is disabled), I think it shouldn't matter.

Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?