bug#16335: Segmentation fault when using cp -a with SELinux and fakeroot

[Nicolas Iooss]

On 04/01/2014 04:03, Pádraig Brady wrote :
> On 01/04/2014 01:42 AM, Pádraig Brady wrote:
>> On 01/03/2014 10:08 PM, Nicolas Iooss wrote:
>>> Hello,
>>>
>>> After upgrading to coreutils 8.22 I can no longer build packages which
>>> uses "cp -a" to copy files due to a segmentation fault happening in
>>> libselinux.
>>>
>>> I've tried to reproduce this bug with few commands, in a directory which
>>> doesn't have any default context:
>>>
>>>     $ mkdir /tmp/foobar
>>>     $ matchpathcon
>>>     /tmp/foobar     <<none>>
>>>     $ touch /tmp/foobar/a
>>>     $ fakeroot cp -a /tmp/foobar/a /tmp/foobar/b
>>>     $ fakeroot cp -a /tmp/foobar/a /tmp/foobar/b
>>>     /usr/bin/fakeroot: line 181:  9207 Segmentation fault
>>>
>>> Without fakeroot there is no segmentation fault.
>>>
>>> Even if the message says "/usr/bin/fakeroot", a coredump has been
>>> created for cp. I've analyzed this dump using gdb and after some
>>> debugging, I found out that restorecon_private (from src/selinux.c) was
>>> calling lsetfilecon with a NULL security context which was obtained by
>>> getfscreatecon (case "local = true" in the code [1]). This causes a null
>>> pointer dereference in libselinux and so a SIGSEGV.
>>>
>>> I've reported this bug to libselinux maintainers [2] and got the reply
>>> that calling lsetfilecon with a NULL security context was like calling
>>> strlen with a NULL string and that this was a problem in caller's code [3].
>>>
>>> Hence I propose the attached patch to fix the segmentation fault. Could
>>> you please accept it?
>>>
>>> When you reply, please Cc me as I'm not subscribed.
>>>
>>> Thanks,
>>>
>>> Nicolas Iooss
>>>
>>> -----------
>>>
>>> System configuration during my tests:
>>>
>>> * distro: ArchLinux which SELinux packages
>>> * CPU arch: x86_64
>>> * SELinux in permissive mode
>>> * coreutils 8.22
>>> * libselinux 2.2.1
>>> * fakeroot 1.20
>>>
>>> [1]
>>> http://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=blob;f=src/selinux.c;hb=v8.22#l191
>>> [2] http://marc.info/?l=selinux&m=138763485330568&w=2
>>> [3] http://marc.info/?l=selinux&m=138842015508829&w=2
>>
>> Thanks for the very thorough analysis and patch.
>> The patch looks correct as getfscreatecon() is
>> documented to return a NULL context in some cases.
>> I'll see if I can add a robust test and will apply
>> this in your name.

Thanks for your quick reply.

> 
> Actually what's errno set to with tcon is NULL.
> If if was 0 you might get the classic "error success" message
> if using the --preserve=context option rather than -a for example.

According to libselinux code [4], when "fscreate" attribute is empty,
getfscreatecon sets the security context to NULL and returns 0 without
setting errno. Hence if it remains zero, set_file_security_ctx from
src/copy.c will report the "error success" message.

> 
> I.E. the following might be more appropriate.

I agree. With your patch I get this (as expected):

    $ fakeroot cp --preserve=context a b
    cp: failed to get security context of 'a': No data available

> Note neither Fedora 15 or 20 here produce a NULL value with fakeroot.

On my system, fakeroot (version 1.20) doesn't seem to support xattr:

   $ fakeroot getfattr -m - -d /tmp/foobar/a
   $ getfattr -m - -d /tmp/foobar/a
   getfattr: Suppression des « / » en tête des chemins absolus
   # file: tmp/foobar/a
   security.selinux="unconfined_u:object_r:user_tmp_t:s0"

> 
> thanks,
> Pádraig.
> 
> diff --git a/src/selinux.c b/src/selinux.c
> index cd38a81..016db16 100644
> --- a/src/selinux.c
> +++ b/src/selinux.c
> @@ -192,6 +192,11 @@ restorecon_private (char const *path, bool local)
>      {
>        if (getfscreatecon (&tcon) < 0)
>          return rc;
> +      if (!tcon)
> +        {
> +          errno = ENODATA;
> +          return rc;
> +        }
>        rc = lsetfilecon (path, tcon);
>        freecon (tcon);
>        return rc;
> 

Nicolas

[4]
http://userspace.selinuxproject.org/trac/browser/libselinux/src/procattr.c?rev=edc2e99687b050d5be21a78a66d038aa1fc068d9#L176