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

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

[Mikhail Novosyolov]

/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.

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

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

[Petr Lautrbach]

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