[RFC] Livecd-creator and selinux, we can play nice

Jeremy Katz katzj at redhat.com
Thu May 29 12:51:12 UTC 2008


Eric Paris wrote:
> So I've spent a fair bit of time the last 2 weeks trying to get
> livecd-creator and an selinux enforcing machine to play nicely together.
> It doesn't look like much, but from the point of view of the livecd
> creator I think the following patch is all we need.  Working with
> rawhide as the host system I was able to build F8, F9 and rawhide
> livecd's with an enforcing machine.
> 
> I wouldn't suggest jumping into enfocing builds just yet as there are
> still some policy issues I need to work out with the selinux people but
> I would like comments.  Basically its quite simple, if selinux is on the
> host we create a fake /selinux which tells the install chroot lies.
> I've had to make some changes to some selinux libraries to support all
> this, but I think we are just about there.

Very cool and definitely long needed.  Thanks for taking the time to 
really dive into this.  And this is far simpler than the approach I had 
started looking at once upon a time (... which involved fuse)

A few comments on the patch

> diff -Naupr /usr/lib/python2.5/site-packages/imgcreate/creator.py /root/imgcreate-5-28-08/creator.py
> --- /usr/lib/python2.5/site-packages/imgcreate/creator.py	2008-05-06 12:16:08.000000000 -0400
> +++ /root/imgcreate-5-28-08/creator.py	2008-05-28 15:48:30.000000000 -0400
> @@ -460,6 +457,37 @@ class ImageCreator(object):
>          os.symlink('/proc/self/fd/2', self._instroot + "/dev/stderr")
>          os.umask(origumask)
>  
> +        # if selinux exists on the host we need to lie to the chroot
> +        if os.path.exists("/selinux/enforce"):
> +            selinux_dir = self._instroot + "/selinux"
> +
> +            # enforce=0 tells the chroot selinux is not enforcing
> +            # policyvers=99 tell the chroot to make the highest version of policy it can
> +            files = [('/enforce', '0'),
> +                     ('/policyvers', '99')]

Does the kernel guarantee that 99 will be the highest version of policy? 
  Not that it likely matters much.  Also, having this as a tuple rather 
than a list makes it marginally faster since we're never going to modify it

> +            for (file, value) in files:
> +                fd = os.open(selinux_dir + file, os.O_WRONLY | os.O_TRUNC | os.O_CREAT)
> +                os.write(fd, value)
> +                os.close(fd)
> +
> +            # we steal mls from the host system for now, might be best to always set it to 1????
> +            files = ["/selinux/mls"]
> +            for file in files:
> +                shutil.copyfile(file, self._instroot + file)
> +
> +            # make /load -> /dev/null so chroot policy loads don't hurt anything
> +            os.mknod(selinux_dir + "/load", 0666 | stat.S_IFCHR, os.makedev(1, 3))

This being the big win :)

> +        # selinux is on whoo hooo
> +        if kickstart.selinux_enabled(self.ks):
> +            # label the fs like it is a root before the bind mounting
> +            cmd = "/sbin/setfiles -F -r %s %s %s" % (self._instroot, selinux.selinux_file_context_path(), self._instroot)
> +            os.system(cmd)
> +            # these dumb things don't get magically fixed, so make the user generic
> +            for f in ["/proc", "/sys", "/selinux"]:
> +                cmd = "chcon -u system_u %s" % (self._instroot + f)
> +                os.system(cmd)

os.system is generally not preferred -- using the subprocess module is a 
lot safer.

Also, overall it might be nice to encapsulate the /selinux creation here 
into its own __create_selinuxfs() method that gets called.  /me makes a 
note to do that to the /dev creation too.

> @@ -853,6 +881,18 @@ class LoopImageCreator(ImageCreator):
>                                 (self._image, e))
>  
>      def _unmount_instroot(self):
> +        # if the system was running selinux clean up our lies
> +        if os.path.exists("/selinux/enforce"):
> +            files = ['/enforce',
> +                     '/policyvers',
> +                     '/mls',
> +                     '/load']

Again a tuple versus a list

> +            for file in files:
> +                try:
> +                    os.unlink(self._instroot + "/selinux" + file)
> +                except OSError:
> +                    pass

And again having it in a method is probably the nice thing to do.  And I 
know I said to stick it into _unmount_instroot, but seeing where you've 
put the mount side, it probably makes more sense in unmount() instead

Jeremy




More information about the fedora-selinux-list mailing list