Re: [PATCH] gui: do not recreate /etc/selinux/config

[Petr Lautrbach <plautrba@redhat.com>]

Mikhail Novosyolov <m.novosyolov@rosalinux.ru> writes:

> /etc/selinux/config.bck was created and then replaced /etc/selinux/config.
> /etc/selinux/config is often read by libselinux from non-root,
> it must have mode 0644, but, when umask is 077, it became not world-readable
> after running system-config-gui.
>
> Overwrite the existing file instead of creating a new one.
>
> Unfortunately, we may get a corrupted file if the GUI is closed when writing it,
> but writing takes only a bit of time, plus we save a backup for manual restoring in such case.
>

I don't think it's a good idea. The final operation needs to be atomic.

If you want to preserve mode and other attributes you can use
shutil.copystat(), what about this:

        fd = open(backup_path, "w")
        ...
        fd.close()
        shutil.copystat(path, backup_path)    


> At Github: https://github.com/SELinuxProject/selinux/pull/345
>
> Signed-off-by: Mikhail Novosyolov <m.novosyolov@rosalinux.ru>
> ---
>  gui/statusPage.py | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/gui/statusPage.py b/gui/statusPage.py
> index 766854b1..ded3929d 100644
> --- a/gui/statusPage.py
> +++ b/gui/statusPage.py
> @@ -18,6 +18,7 @@
>  ## Author: Dan Walsh
>  import os
>  import sys
> +import tempfile
>  from gi.repository import Gtk
>  import selinux
>  
> @@ -162,12 +163,20 @@ class statusPage:
>          self.enabled = enabled
>  
>      def write_selinux_config(self, enforcing, type):
> -        path = selinux.selinux_path() + "config"
> -        backup_path = path + ".bck"
> -        fd = open(path)
> -        lines = fd.readlines()
> -        fd.close()
> -        fd = open(backup_path, "w")
> +        selinux_path = selinux.selinux_path()
> +        path = selinux_path + "config"
> +        # Make a backup /etc/selinux/config.*.bck
> +        backup_path = tempfile.mkstemp(prefix="config.", dir=selinux_path, suffix=".bck")[1]
> +        fd1 = open(path, "r")
> +        lines = fd1.readlines()
> +        fd1.close()
> +        fd2 = open(backup_path, "a")
> +        for l in lines:
> +            fd2.write(l)
> +        fd2.close()
> +        # Write to path, not backup_path, to guarantee that file metadata
> +        # (permissions, xattrs, including SELinux labels etc.) is not lost.
> +        fd = open(path, "w")
>          for l in lines:
>              if l.startswith("SELINUX="):
>                  fd.write("SELINUX=%s\n" % enforcing)
> @@ -177,7 +186,9 @@ class statusPage:
>                  continue
>              fd.write(l)
>          fd.close()
> -        os.rename(backup_path, path)
> +        # Here we are sure that we are deleting our backup,
> +        # not another file or directory
> +        os.unlink(backup_path)
>  
>      def read_selinux_config(self):
>          self.initialtype = selinux.selinux_getpolicytype()[1]
> -- 
> 2.31.1