Bug in libselinux/src/setrans_client.c

Bug in libselinux/src/setrans_client.c

[Nicolas Iooss]

Hi,

After upgrading to coreutils 8.22, cp is crashing when building
policycoreutils package (see gdb backtrace below). A segmentation
fault occurred in libselinux/src/lsetfilecon.c line 12 [1], when
calling "strlen(context)" with a NULL context. This code path has been
possible because selinux_trans_to_raw_context(0, &rcontext) returns 0
even though rcontext is NULL, in libselinux/src/setrans_client.c lines
287-290 [2]. I think this function should return a non-null value like
-1 on line 289. Could you please fix this bug?

System information:
I'm running SELinux on Archlinux using packages from
https://github.com/fishilico/siosm-selinux/ and a policy patched from
the Reference Policy. I'm using coreutils 8.22, libselinux 2.2,
libsepol 2.2 and glibc 2.18.

Thanks,

Nicolas
(IooNag on irc.freenode.net)

[1] http://userspace.selinuxproject.org/trac/browser/libselinux/src/fsetfilecon.c?rev=51d9a078c260b230f65863766e73e6db0b2c2d3a
[2] http://userspace.selinuxproject.org/trac/browser/libselinux/src/setrans_client.c?rev=aa62cd60f7192123b509c2518e7a2083e34a65a2#L284

GDB Coredump:

# systemd-coredumpctl gdb
TIME                                         PID   UID   GID SIG EXE
             sam. 2013-12-21 14:23:00 CET   2872  1000   100  11 /usr/bin/cp
GNU gdb (GDB) 7.6.2
Copyright (C) 2013 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-unknown-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /usr/bin/cp...done.
[New LWP 2872]

warning: Could not load shared library symbols for linux-vdso.so.1.
Do you need "set solib-search-path" or "set sysroot"?
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/usr/lib/libthread_db.so.1".

warning: no loadable sections found in added symbol-file
system-supplied DSO at 0x7fff82d84000
Core was generated by `cp -af setfiles.8 setfiles.8.man'.
Program terminated with signal 11, Segmentation fault.
#0  0x00007fb34934c9ba in strlen () from /usr/lib/libc.so.6
(gdb) bt
#0  0x00007fb34934c9ba in strlen () from /usr/lib/libc.so.6
#1  0x00007fb349a9b1db in lsetfilecon_raw_internal
(path=0x7fff82c3bea9 "setfiles.8.man", context=0x0) at
lsetfilecon.c:12
#2  0x00007fb349a9b2b6 in lsetfilecon (path=0x7fff82c3bea9
"setfiles.8.man", context=0x0) at lsetfilecon.c:38
#3  0x0000000000409a55 in restorecon_private (path=0x7fff82c3bea9
"setfiles.8.man", local=local@entry=true) at src/selinux.c:195
#4  0x0000000000409f68 in restorecon (path=0x7fff82c3bea9
"setfiles.8.man", recurse=<optimized out>, local=<optimized out>) at
src/selinux.c:301
#5  0x0000000000405c0b in set_file_security_ctx
(dst_name=0x7fff82c3bea9 "setfiles.8.man", process_local=<optimized
out>, recurse=<optimized out>, x=<optimized out>) at src/copy.c:835
#6  0x000000000040893b in copy_reg (src_sb=0x7fff82c39df0,
new_dst=<synthetic pointer>, omitted_permissions=36,
dst_mode=<optimized out>, x=0x7fff82c3a210,
    dst_name=0x7fff82c3bea9 "setfiles.8.man", src_name=0x7fff82c3be9e
"setfiles.8") at src/copy.c:952
#7  copy_internal (src_name=src_name@entry=0x7fff82c3be9e
"setfiles.8", dst_name=dst_name@entry=0x7fff82c3bea9 "setfiles.8.man",
new_dst=<optimized out>, new_dst@entry=false,
    device=device@entry=0, ancestors=ancestors@entry=0x0,
x=x@entry=0x7fff82c3a210,
command_line_arg=command_line_arg@entry=true,
    first_dir_created_per_command_line_arg=first_dir_created_per_command_line_arg@entry=0x7fff82c3a0af,
copy_into_self=copy_into_self@entry=0x7fff82c3a0f8,
    rename_succeeded=rename_succeeded@entry=0x0) at src/copy.c:2503
#8  0x00000000004094bc in copy (src_name=src_name@entry=0x7fff82c3be9e
"setfiles.8", dst_name=dst_name@entry=0x7fff82c3bea9 "setfiles.8.man",
nonexistent_dst=nonexistent_dst@entry=false,
    options=options@entry=0x7fff82c3a210,
copy_into_self=copy_into_self@entry=0x7fff82c3a0f8,
rename_succeeded=rename_succeeded@entry=0x0) at src/copy.c:2809
#9  0x0000000000404fb0 in do_copy (n_files=<optimized out>,
file=0x7fff82c3a418, target_directory=<optimized out>,
target_directory@entry=0x0,
no_target_directory=no_target_directory@entry=false,
    x=x@entry=0x7fff82c3a210) at src/cp.c:765
#10 0x0000000000403ba9 in main (argc=4, argv=0x7fff82c3a408) at src/cp.c:1212

Re: Bug in libselinux/src/setrans_client.c

[Nicolas Iooss]

My first message was not so clear. The check in
libselinux/src/lsetfilecon.c line 35 [1] doesn't work because
selinux_trans_to_raw_context(context, &rcontext) returns 0 and sets
rcontext to NULL. This is why I'm asking to change the return value to
something else if you want "cp -a" working. This fix is not to
introduce a new feature but to fix an existing one.

Nicolas

[1] http://userspace.selinuxproject.org/trac/browser/libselinux/src/lsetfilecon.c?rev=51d9a078c260b230f65863766e73e6db0b2c2d3a

2013/12/21 Frank C <frankc@networkcrypt.com>:
> Why not make your own function prototype to return if NULL whatever your
> heart desires?
>
> On Dec 21, 2013 6:07 AM, "Nicolas Iooss" <nicolas.iooss@m4x.org> wrote:
>>
>> Hi,
>>
>> After upgrading to coreutils 8.22, cp is crashing when building
>> policycoreutils package (see gdb backtrace below). A segmentation
>> fault occurred in libselinux/src/lsetfilecon.c line 12 [1], when
>> calling "strlen(context)" with a NULL context. This code path has been
>> possible because selinux_trans_to_raw_context(0, &rcontext) returns 0
>> even though rcontext is NULL, in libselinux/src/setrans_client.c lines
>> 287-290 [2]. I think this function should return a non-null value like
>> -1 on line 289. Could you please fix this bug?
>>
>> System information:
>> I'm running SELinux on Archlinux using packages from
>> https://github.com/fishilico/siosm-selinux/ and a policy patched from
>> the Reference Policy. I'm using coreutils 8.22, libselinux 2.2,
>> libsepol 2.2 and glibc 2.18.
>>
>> Thanks,
>>
>> Nicolas
>> (IooNag on irc.freenode.net)
>>
>> [1]
>> http://userspace.selinuxproject.org/trac/browser/libselinux/src/fsetfilecon.c?rev=51d9a078c260b230f65863766e73e6db0b2c2d3a
>> [2]
>> http://userspace.selinuxproject.org/trac/browser/libselinux/src/setrans_client.c?rev=aa62cd60f7192123b509c2518e7a2083e34a65a2#L284
>>
>> GDB Coredump:
>>
>> # systemd-coredumpctl gdb
>> TIME                                         PID   UID   GID SIG EXE
>>              sam. 2013-12-21 14:23:00 CET   2872  1000   100  11
>> /usr/bin/cp
>> GNU gdb (GDB) 7.6.2
>> Copyright (C) 2013 Free Software Foundation, Inc.
>> License GPLv3+: GNU GPL version 3 or later
>> <http://gnu.org/licenses/gpl.html>
>> This is free software: you are free to change and redistribute it.
>> There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
>> and "show warranty" for details.
>> This GDB was configured as "x86_64-unknown-linux-gnu".
>> For bug reporting instructions, please see:
>> <http://www.gnu.org/software/gdb/bugs/>...
>> Reading symbols from /usr/bin/cp...done.
>> [New LWP 2872]
>>
>> warning: Could not load shared library symbols for linux-vdso.so.1.
>> Do you need "set solib-search-path" or "set sysroot"?
>> [Thread debugging using libthread_db enabled]
>> Using host libthread_db library "/usr/lib/libthread_db.so.1".
>>
>> warning: no loadable sections found in added symbol-file
>> system-supplied DSO at 0x7fff82d84000
>> Core was generated by `cp -af setfiles.8 setfiles.8.man'.
>> Program terminated with signal 11, Segmentation fault.
>> #0  0x00007fb34934c9ba in strlen () from /usr/lib/libc.so.6
>> (gdb) bt
>> #0  0x00007fb34934c9ba in strlen () from /usr/lib/libc.so.6
>> #1  0x00007fb349a9b1db in lsetfilecon_raw_internal
>> (path=0x7fff82c3bea9 "setfiles.8.man", context=0x0) at
>> lsetfilecon.c:12
>> #2  0x00007fb349a9b2b6 in lsetfilecon (path=0x7fff82c3bea9
>> "setfiles.8.man", context=0x0) at lsetfilecon.c:38
>> #3  0x0000000000409a55 in restorecon_private (path=0x7fff82c3bea9
>> "setfiles.8.man", local=local@entry=true) at src/selinux.c:195
>> #4  0x0000000000409f68 in restorecon (path=0x7fff82c3bea9
>> "setfiles.8.man", recurse=<optimized out>, local=<optimized out>) at
>> src/selinux.c:301
>> #5  0x0000000000405c0b in set_file_security_ctx
>> (dst_name=0x7fff82c3bea9 "setfiles.8.man", process_local=<optimized
>> out>, recurse=<optimized out>, x=<optimized out>) at src/copy.c:835
>> #6  0x000000000040893b in copy_reg (src_sb=0x7fff82c39df0,
>> new_dst=<synthetic pointer>, omitted_permissions=36,
>> dst_mode=<optimized out>, x=0x7fff82c3a210,
>>     dst_name=0x7fff82c3bea9 "setfiles.8.man", src_name=0x7fff82c3be9e
>> "setfiles.8") at src/copy.c:952
>> #7  copy_internal (src_name=src_name@entry=0x7fff82c3be9e
>> "setfiles.8", dst_name=dst_name@entry=0x7fff82c3bea9 "setfiles.8.man",
>> new_dst=<optimized out>, new_dst@entry=false,
>>     device=device@entry=0, ancestors=ancestors@entry=0x0,
>> x=x@entry=0x7fff82c3a210,
>> command_line_arg=command_line_arg@entry=true,
>>
>> first_dir_created_per_command_line_arg=first_dir_created_per_command_line_arg@entry=0x7fff82c3a0af,
>> copy_into_self=copy_into_self@entry=0x7fff82c3a0f8,
>>     rename_succeeded=rename_succeeded@entry=0x0) at src/copy.c:2503
>> #8  0x00000000004094bc in copy (src_name=src_name@entry=0x7fff82c3be9e
>> "setfiles.8", dst_name=dst_name@entry=0x7fff82c3bea9 "setfiles.8.man",
>> nonexistent_dst=nonexistent_dst@entry=false,
>>     options=options@entry=0x7fff82c3a210,
>> copy_into_self=copy_into_self@entry=0x7fff82c3a0f8,
>> rename_succeeded=rename_succeeded@entry=0x0) at src/copy.c:2809
>> #9  0x0000000000404fb0 in do_copy (n_files=<optimized out>,
>> file=0x7fff82c3a418, target_directory=<optimized out>,
>> target_directory@entry=0x0,
>> no_target_directory=no_target_directory@entry=false,
>>     x=x@entry=0x7fff82c3a210) at src/cp.c:765
>> #10 0x0000000000403ba9 in main (argc=4, argv=0x7fff82c3a408) at
>> src/cp.c:1212
>>
>> _______________________________________________
>> Selinux mailing list
>> Selinux@tycho.nsa.gov
>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>> To get help, send an email containing "help" to
>> Selinux-request@tycho.nsa.gov.

Re: Bug in libselinux/src/setrans_client.c

[Daniel J Walsh]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/21/2013 09:27 AM, Nicolas Iooss wrote:
> My first message was not so clear. The check in 
> libselinux/src/lsetfilecon.c line 35 [1] doesn't work because 
> selinux_trans_to_raw_context(context, &rcontext) returns 0 and sets 
> rcontext to NULL. This is why I'm asking to change the return value to 
> something else if you want "cp -a" working. This fix is not to introduce a
> new feature but to fix an existing one.
> 
> Nicolas
> 

How about if we add a check on lsetfilecon_raw?  Changing the behaviour on
selinux_trans_to_raw_context might cause other problems.


diff --git a/libselinux/src/lsetfilecon.c b/libselinux/src/lsetfilecon.c
index 461e3f7..af3775e 100644
- --- a/libselinux/src/lsetfilecon.c
+++ b/libselinux/src/lsetfilecon.c
@@ -9,6 +9,10 @@

 int lsetfilecon_raw(const char *path, const security_context_t context)
 {
+       if (! context) {
+               errno=EINVAL;
+               return -1;
+       }
        return lsetxattr(path, XATTR_NAME_SELINUX, context, strlen(context) + 1
                         0);
 }

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlK4TN8ACgkQrlYvE4MpobNstACfcVXS9KZVDW9gc7PQrG7xUgVs
foIAoOe8r4LO0CoyzwGW3+TWsX2oaRKq
=BgSq
-----END PGP SIGNATURE-----

Re: Bug in libselinux/src/setrans_client.c

[Stephen Smalley]

Calling *setfilecon() with a NULL context is a bug in the caller.  Not
opposed to having it return an error, but what do you intend for the
caller to do in that case?  It never should have called it with a NULL
context in the first place.

On Mon, Dec 23, 2013 at 9:46 AM, Daniel J Walsh <dwalsh@redhat.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 12/21/2013 09:27 AM, Nicolas Iooss wrote:
>> My first message was not so clear. The check in
>> libselinux/src/lsetfilecon.c line 35 [1] doesn't work because
>> selinux_trans_to_raw_context(context, &rcontext) returns 0 and sets
>> rcontext to NULL. This is why I'm asking to change the return value to
>> something else if you want "cp -a" working. This fix is not to introduce a
>> new feature but to fix an existing one.
>>
>> Nicolas
>>
>
> How about if we add a check on lsetfilecon_raw?  Changing the behaviour on
> selinux_trans_to_raw_context might cause other problems.
>
>
> diff --git a/libselinux/src/lsetfilecon.c b/libselinux/src/lsetfilecon.c
> index 461e3f7..af3775e 100644
> - --- a/libselinux/src/lsetfilecon.c
> +++ b/libselinux/src/lsetfilecon.c
> @@ -9,6 +9,10 @@
>
>  int lsetfilecon_raw(const char *path, const security_context_t context)
>  {
> +       if (! context) {
> +               errno=EINVAL;
> +               return -1;
> +       }
>         return lsetxattr(path, XATTR_NAME_SELINUX, context, strlen(context) + 1
>                          0);
>  }
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iEYEARECAAYFAlK4TN8ACgkQrlYvE4MpobNstACfcVXS9KZVDW9gc7PQrG7xUgVs
> foIAoOe8r4LO0CoyzwGW3+TWsX2oaRKq
> =BgSq
> -----END PGP SIGNATURE-----
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.

Re: Bug in libselinux/src/setrans_client.c

[Nicolas Iooss]

I've recompiled libselinux with the proposed patch and it looks like
it fixed the bug. Thanks!

2013/12/23 Stephen Smalley <stephen.smalley@gmail.com>:
> Calling *setfilecon() with a NULL context is a bug in the caller.  Not
> opposed to having it return an error, but what do you intend for the
> caller to do in that case?  It never should have called it with a NULL
> context in the first place.

The caller here is cp program, from coreutils project, in function
restorecon_private(path="setfiles.8.man", local=true), line 195 of
src/selinux.c [1]. The code runs like this (according to the gdb
backtrace I get):

      if (getfscreatecon (&tcon) < 0)  /* getfscreatecon sets tcon to
NULL and returns 0 */
        return rc;
      rc = lsetfilecon (path, tcon); /* call
lsetfilecon("setfiles.8.man", NULL) */
      freecon (tcon);
      return rc;

If you think this code is doing wrong, there may be a missing check to
test whether tcon is NULL before calling lsetfilecon. Anyway I don't
know what "cp" does if this call to restorecon_private fails, but it
seems to work fine on my system (at least it no longer segfaults).

Nicolas


[1] http://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=blob;f=src/selinux.c;h=680bc492e5ef2d1a7abd443df7141114b1bc0704;hb=HEAD#l195
[2] http://userspace.selinuxproject.org/trac/browser/libselinux/src/setrans_client.c?rev=aa62cd60f7192123b509c2518e7a2083e34a65a2#L321

---------------- gdb session with "cp" coredump ----------------
Core was generated by `cp -af setfiles.8 setfiles.8.man'.
Program terminated with signal 11, Segmentation fault.
#0  0x00007f31be0b39ba in strlen () from /usr/lib/libc.so.6
(gdb) bt
#0  0x00007f31be0b39ba in strlen () from /usr/lib/libc.so.6
#1  0x00007f31be801be8 in lsetfilecon_raw_internal
(path=path@entry=0x7fffaeb3cea9 "setfiles.8.man", context=0x0) at
lsetfilecon.c:12
#2  0x00007f31be801cab in lsetfilecon (path=path@entry=0x7fffaeb3cea9
"setfiles.8.man", context=<optimized out>) at lsetfilecon.c:38
#3  0x0000000000409a55 in restorecon_private (path=0x7fffaeb3cea9
"setfiles.8.man", local=local@entry=true) at src/selinux.c:195
#4  0x0000000000409f68 in restorecon (path=0x7fffaeb3cea9
"setfiles.8.man", recurse=<optimized out>, local=<optimized out>) at
src/selinux.c:301
#5  0x0000000000405c0b in set_file_security_ctx
(dst_name=0x7fffaeb3cea9 "setfiles.8.man", process_local=<optimized
out>, recurse=<optimized out>, x=<optimized out>) at src/copy.c:835
#6  0x000000000040893b in copy_reg (src_sb=0x7fffaeb3acb0,
new_dst=<synthetic pointer>, omitted_permissions=36,
dst_mode=<optimized out>, x=0x7fffaeb3b0d0,
    dst_name=0x7fffaeb3cea9 "setfiles.8.man", src_name=0x7fffaeb3ce9e
"setfiles.8") at src/copy.c:952
#7  copy_internal (src_name=src_name@entry=0x7fffaeb3ce9e
"setfiles.8", dst_name=dst_name@entry=0x7fffaeb3cea9 "setfiles.8.man",
new_dst=<optimized out>, new_dst@entry=false,
    device=device@entry=0, ancestors=ancestors@entry=0x0,
x=x@entry=0x7fffaeb3b0d0,
command_line_arg=command_line_arg@entry=true,
    first_dir_created_per_command_line_arg=first_dir_created_per_command_line_arg@entry=0x7fffaeb3af6f,
copy_into_self=copy_into_self@entry=0x7fffaeb3afb8,
    rename_succeeded=rename_succeeded@entry=0x0) at src/copy.c:2503
#8  0x00000000004094bc in copy (src_name=src_name@entry=0x7fffaeb3ce9e
"setfiles.8", dst_name=dst_name@entry=0x7fffaeb3cea9 "setfiles.8.man",
nonexistent_dst=nonexistent_dst@entry=false,
    options=options@entry=0x7fffaeb3b0d0,
copy_into_self=copy_into_self@entry=0x7fffaeb3afb8,
rename_succeeded=rename_succeeded@entry=0x0) at src/copy.c:2809
#9  0x0000000000404fb0 in do_copy (n_files=<optimized out>,
file=0x7fffaeb3b2d8, target_directory=<optimized out>,
target_directory@entry=0x0,
no_target_directory=no_target_directory@entry=false,
    x=x@entry=0x7fffaeb3b0d0) at src/cp.c:765
#10 0x0000000000403ba9 in main (argc=4, argv=0x7fffaeb3b2c8) at src/cp.c:1212
(gdb) f 3
#3  0x0000000000409a55 in restorecon_private (path=0x7fffaeb3cea9
"setfiles.8.man", local=local@entry=true) at src/selinux.c:195
195      rc = lsetfilecon (path, tcon);
(gdb) info locals
rc = -1
sb = {st_dev = 139851618037504, st_ino = 139851632465104, st_nlink =
0, st_mode = 2931011792, st_uid = 32767, st_gid = 2931019422, __pad0 =
32767, st_rdev = 140736124407465,
  st_size = 140736124399408, st_blksize = 139851630542055, st_blocks =
140733193388033, st_atim = {tv_sec = 0, tv_nsec = 140736124407465},
st_mtim = {tv_sec = 139851618056848, tv_nsec = 420},
  st_ctim = {tv_sec = 6434904, tv_nsec = 0}, __unused =
{140736124399824, 140736124407454, 140736124407465}}
scon = 0x0
tcon = 0x0
scontext = 0x0
tcontext = 0x0
contype = <optimized out>
constr = <optimized out>

Re: Bug in libselinux/src/setrans_client.c

[Nicolas Iooss]

2013/12/23 Daniel J Walsh wrote:
>
> On 12/21/2013 09:27 AM, Nicolas Iooss wrote:
> > My first message was not so clear. The check in
> > libselinux/src/lsetfilecon.c line 35 [1] doesn't work because
> > selinux_trans_to_raw_context(context, &rcontext) returns 0 and sets
> > rcontext to NULL. This is why I'm asking to change the return value to
> > something else if you want "cp -a" working. This fix is not to introduce a
> > new feature but to fix an existing one.
> >
> > Nicolas
> >
>
> How about if we add a check on lsetfilecon_raw?  Changing the behaviour on
> selinux_trans_to_raw_context might cause other problems.

I agree. I've found
http://selinuxproject.org/page/LibselinuxAPISummary which says
precisely for selinux_trans_to_raw_context: "If passed NULL, sets the
returned context to NULL and returns 0." As this feature is
documented, callers may rely on it and changing this behavior is
likely to break things.

Moreover setfilecon_raw and fsetfilecon_raw have the same NULL-pointer
dereference issue. Do these functions need a patch too?

By the way, other callers of selinux_trans_to_raw_context may also
share this bug: avc_context_to_sid, security_canonicalize_context,
security_check_context, etc. Is doing a segmentation fault the
expected way to tell the caller it used a NULL pointer and should have
manually checked every parameter before calling any libselinux
function?

Thanks and merry Christmas!

Nicolas

>
>
> diff --git a/libselinux/src/lsetfilecon.c b/libselinux/src/lsetfilecon.c
> index 461e3f7..af3775e 100644
> - --- a/libselinux/src/lsetfilecon.c
> +++ b/libselinux/src/lsetfilecon.c
> @@ -9,6 +9,10 @@
>
>  int lsetfilecon_raw(const char *path, const security_context_t context)
>  {
> +       if (! context) {
> +               errno=EINVAL;
> +               return -1;
> +       }
>         return lsetxattr(path, XATTR_NAME_SELINUX, context, strlen(context) + 1
>                          0);
>  }

Re: Bug in libselinux/src/setrans_client.c

[Francis Cunnane]

[-- Attachment #1: Type: text/plain, Size: 2365 bytes --]

Merry Christmas!

On 12/25/2013 6:36 AM, Nicolas Iooss wrote:
> 2013/12/23 Daniel J Walsh wrote:
>> On 12/21/2013 09:27 AM, Nicolas Iooss wrote:
>>> My first message was not so clear. The check in
>>> libselinux/src/lsetfilecon.c line 35 [1] doesn't work because
>>> selinux_trans_to_raw_context(context, &rcontext) returns 0 and sets
>>> rcontext to NULL. This is why I'm asking to change the return value to
>>> something else if you want "cp -a" working. This fix is not to introduce a
>>> new feature but to fix an existing one.
>>>
>>> Nicolas
>>>
>> How about if we add a check on lsetfilecon_raw?  Changing the behaviour on
>> selinux_trans_to_raw_context might cause other problems.
> I agree. I've found
> http://selinuxproject.org/page/LibselinuxAPISummary which says
> precisely for selinux_trans_to_raw_context: "If passed NULL, sets the
> returned context to NULL and returns 0." As this feature is
> documented, callers may rely on it and changing this behavior is
> likely to break things.
>
> Moreover setfilecon_raw and fsetfilecon_raw have the same NULL-pointer
> dereference issue. Do these functions need a patch too?
>
> By the way, other callers of selinux_trans_to_raw_context may also
> share this bug: avc_context_to_sid, security_canonicalize_context,
> security_check_context, etc. Is doing a segmentation fault the
> expected way to tell the caller it used a NULL pointer and should have
> manually checked every parameter before calling any libselinux
> function?
>
> Thanks and merry Christmas!
>
> Nicolas
>
>>
>> diff --git a/libselinux/src/lsetfilecon.c b/libselinux/src/lsetfilecon.c
>> index 461e3f7..af3775e 100644
>> - --- a/libselinux/src/lsetfilecon.c
>> +++ b/libselinux/src/lsetfilecon.c
>> @@ -9,6 +9,10 @@
>>
>>   int lsetfilecon_raw(const char *path, const security_context_t context)
>>   {
>> +       if (! context) {
>> +               errno=EINVAL;
>> +               return -1;
>> +       }
>>          return lsetxattr(path, XATTR_NAME_SELINUX, context, strlen(context) + 1
>>                           0);
>>   }
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3756 bytes --]

Re: Bug in libselinux/src/setrans_client.c

[Stephen Smalley]

Calling *setfilecon() with a NULL context is a bug in the caller, just
like calling strlen() with a NULL string.
Fix the callers, please.

On Wed, Dec 25, 2013 at 9:36 AM, Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> 2013/12/23 Daniel J Walsh wrote:
>>
>> On 12/21/2013 09:27 AM, Nicolas Iooss wrote:
>> > My first message was not so clear. The check in
>> > libselinux/src/lsetfilecon.c line 35 [1] doesn't work because
>> > selinux_trans_to_raw_context(context, &rcontext) returns 0 and sets
>> > rcontext to NULL. This is why I'm asking to change the return value to
>> > something else if you want "cp -a" working. This fix is not to introduce a
>> > new feature but to fix an existing one.
>> >
>> > Nicolas
>> >
>>
>> How about if we add a check on lsetfilecon_raw?  Changing the behaviour on
>> selinux_trans_to_raw_context might cause other problems.
>
> I agree. I've found
> http://selinuxproject.org/page/LibselinuxAPISummary which says
> precisely for selinux_trans_to_raw_context: "If passed NULL, sets the
> returned context to NULL and returns 0." As this feature is
> documented, callers may rely on it and changing this behavior is
> likely to break things.
>
> Moreover setfilecon_raw and fsetfilecon_raw have the same NULL-pointer
> dereference issue. Do these functions need a patch too?
>
> By the way, other callers of selinux_trans_to_raw_context may also
> share this bug: avc_context_to_sid, security_canonicalize_context,
> security_check_context, etc. Is doing a segmentation fault the
> expected way to tell the caller it used a NULL pointer and should have
> manually checked every parameter before calling any libselinux
> function?
>
> Thanks and merry Christmas!
>
> Nicolas
>
>>
>>
>> diff --git a/libselinux/src/lsetfilecon.c b/libselinux/src/lsetfilecon.c
>> index 461e3f7..af3775e 100644
>> - --- a/libselinux/src/lsetfilecon.c
>> +++ b/libselinux/src/lsetfilecon.c
>> @@ -9,6 +9,10 @@
>>
>>  int lsetfilecon_raw(const char *path, const security_context_t context)
>>  {
>> +       if (! context) {
>> +               errno=EINVAL;
>> +               return -1;
>> +       }
>>         return lsetxattr(path, XATTR_NAME_SELINUX, context, strlen(context) + 1
>>                          0);
>>  }
>
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.

Re: Bug in libselinux/src/setrans_client.c

[Matthew Thode]

[-- Attachment #1: Type: text/plain, Size: 3016 bytes --]

On 12/30/2013 10:11 AM, Stephen Smalley wrote:
> Calling *setfilecon() with a NULL context is a bug in the caller, just
> like calling strlen() with a NULL string.
> Fix the callers, please.
> 
> On Wed, Dec 25, 2013 at 9:36 AM, Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>> 2013/12/23 Daniel J Walsh wrote:
>>>
>>> On 12/21/2013 09:27 AM, Nicolas Iooss wrote:
>>>> My first message was not so clear. The check in
>>>> libselinux/src/lsetfilecon.c line 35 [1] doesn't work because
>>>> selinux_trans_to_raw_context(context, &rcontext) returns 0 and sets
>>>> rcontext to NULL. This is why I'm asking to change the return value to
>>>> something else if you want "cp -a" working. This fix is not to introduce a
>>>> new feature but to fix an existing one.
>>>>
>>>> Nicolas
>>>>
>>>
>>> How about if we add a check on lsetfilecon_raw?  Changing the behaviour on
>>> selinux_trans_to_raw_context might cause other problems.
>>
>> I agree. I've found
>> http://selinuxproject.org/page/LibselinuxAPISummary which says
>> precisely for selinux_trans_to_raw_context: "If passed NULL, sets the
>> returned context to NULL and returns 0." As this feature is
>> documented, callers may rely on it and changing this behavior is
>> likely to break things.
>>
>> Moreover setfilecon_raw and fsetfilecon_raw have the same NULL-pointer
>> dereference issue. Do these functions need a patch too?
>>
>> By the way, other callers of selinux_trans_to_raw_context may also
>> share this bug: avc_context_to_sid, security_canonicalize_context,
>> security_check_context, etc. Is doing a segmentation fault the
>> expected way to tell the caller it used a NULL pointer and should have
>> manually checked every parameter before calling any libselinux
>> function?
>>
>> Thanks and merry Christmas!
>>
>> Nicolas
>>
>>>
>>>
>>> diff --git a/libselinux/src/lsetfilecon.c b/libselinux/src/lsetfilecon.c
>>> index 461e3f7..af3775e 100644
>>> - --- a/libselinux/src/lsetfilecon.c
>>> +++ b/libselinux/src/lsetfilecon.c
>>> @@ -9,6 +9,10 @@
>>>
>>>  int lsetfilecon_raw(const char *path, const security_context_t context)
>>>  {
>>> +       if (! context) {
>>> +               errno=EINVAL;
>>> +               return -1;
>>> +       }
>>>         return lsetxattr(path, XATTR_NAME_SELINUX, context, strlen(context) + 1
>>>                          0);
>>>  }
>>
>> _______________________________________________
>> Selinux mailing list
>> Selinux@tycho.nsa.gov
>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
> 
I think I may have hit this bug as well.

https://bugs.gentoo.org/show_bug.cgi?id=495274

-- 
-- Matthew Thode


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Bug in libselinux/src/setrans_client.c

[Francis Cunnane]

[-- Attachment #1.1: Type: text/plain, Size: 3415 bytes --]

What do you propose....  This is free software.... Don't be a Jew.

   On 12/30/2013 7:11 PM, Matthew Thode wrote:
> On 12/30/2013 10:11 AM, Stephen Smalley wrote:
>> Calling *setfilecon() with a NULL context is a bug in the caller, just
>> like calling strlen() with a NULL string.
>> Fix the callers, please.
>>
>> On Wed, Dec 25, 2013 at 9:36 AM, Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>>> 2013/12/23 Daniel J Walsh wrote:
>>>> On 12/21/2013 09:27 AM, Nicolas Iooss wrote:
>>>>> My first message was not so clear. The check in
>>>>> libselinux/src/lsetfilecon.c line 35 [1] doesn't work because
>>>>> selinux_trans_to_raw_context(context, &rcontext) returns 0 and sets
>>>>> rcontext to NULL. This is why I'm asking to change the return value to
>>>>> something else if you want "cp -a" working. This fix is not to introduce a
>>>>> new feature but to fix an existing one.
>>>>>
>>>>> Nicolas
>>>>>
>>>> How about if we add a check on lsetfilecon_raw?  Changing the behaviour on
>>>> selinux_trans_to_raw_context might cause other problems.
>>> I agree. I've found
>>> http://selinuxproject.org/page/LibselinuxAPISummary which says
>>> precisely for selinux_trans_to_raw_context: "If passed NULL, sets the
>>> returned context to NULL and returns 0." As this feature is
>>> documented, callers may rely on it and changing this behavior is
>>> likely to break things.
>>>
>>> Moreover setfilecon_raw and fsetfilecon_raw have the same NULL-pointer
>>> dereference issue. Do these functions need a patch too?
>>>
>>> By the way, other callers of selinux_trans_to_raw_context may also
>>> share this bug: avc_context_to_sid, security_canonicalize_context,
>>> security_check_context, etc. Is doing a segmentation fault the
>>> expected way to tell the caller it used a NULL pointer and should have
>>> manually checked every parameter before calling any libselinux
>>> function?
>>>
>>> Thanks and merry Christmas!
>>>
>>> Nicolas
>>>
>>>>
>>>> diff --git a/libselinux/src/lsetfilecon.c b/libselinux/src/lsetfilecon.c
>>>> index 461e3f7..af3775e 100644
>>>> - --- a/libselinux/src/lsetfilecon.c
>>>> +++ b/libselinux/src/lsetfilecon.c
>>>> @@ -9,6 +9,10 @@
>>>>
>>>>   int lsetfilecon_raw(const char *path, const security_context_t context)
>>>>   {
>>>> +       if (! context) {
>>>> +               errno=EINVAL;
>>>> +               return -1;
>>>> +       }
>>>>          return lsetxattr(path, XATTR_NAME_SELINUX, context, strlen(context) + 1
>>>>                           0);
>>>>   }
>>> _______________________________________________
>>> Selinux mailing list
>>> Selinux@tycho.nsa.gov
>>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>>> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
>> _______________________________________________
>> Selinux mailing list
>> Selinux@tycho.nsa.gov
>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
>>
> I think I may have hit this bug as well.
>
> https://bugs.gentoo.org/show_bug.cgi?id=495274
>
>
>
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.


[-- Attachment #1.2: Type: text/html, Size: 5216 bytes --]

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3756 bytes --]

Re: Bug in libselinux/src/setrans_client.c

[Daniel J Walsh]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/25/2013 09:36 AM, Nicolas Iooss wrote:
> 2013/12/23 Daniel J Walsh wrote:
>> 
>> On 12/21/2013 09:27 AM, Nicolas Iooss wrote:
>>> My first message was not so clear. The check in 
>>> libselinux/src/lsetfilecon.c line 35 [1] doesn't work because 
>>> selinux_trans_to_raw_context(context, &rcontext) returns 0 and sets 
>>> rcontext to NULL. This is why I'm asking to change the return value to 
>>> something else if you want "cp -a" working. This fix is not to
>>> introduce a new feature but to fix an existing one.
>>> 
>>> Nicolas
>>> 
>> 
>> How about if we add a check on lsetfilecon_raw?  Changing the behaviour
>> on selinux_trans_to_raw_context might cause other problems.
> 
> I agree. I've found http://selinuxproject.org/page/LibselinuxAPISummary
> which says precisely for selinux_trans_to_raw_context: "If passed NULL,
> sets the returned context to NULL and returns 0." As this feature is 
> documented, callers may rely on it and changing this behavior is likely to
> break things.
> 
> Moreover setfilecon_raw and fsetfilecon_raw have the same NULL-pointer 
> dereference issue. Do these functions need a patch too?
> 
I think so, I think we should protect libselinux from segfaults to check
proper input if at all possible.
> By the way, other callers of selinux_trans_to_raw_context may also share
> this bug: avc_context_to_sid, security_canonicalize_context, 
> security_check_context, etc. Is doing a segmentation fault the expected way
> to tell the caller it used a NULL pointer and should have manually checked
> every parameter before calling any libselinux function?
> 
> Thanks and merry Christmas!
> 
> Nicolas
> 
>> 
>> 
>> diff --git a/libselinux/src/lsetfilecon.c b/libselinux/src/lsetfilecon.c 
>> index 461e3f7..af3775e 100644 - --- a/libselinux/src/lsetfilecon.c +++
>> b/libselinux/src/lsetfilecon.c @@ -9,6 +9,10 @@
>> 
>> int lsetfilecon_raw(const char *path, const security_context_t context) 
>> { +       if (! context) { +               errno=EINVAL; +
>> return -1; +       } return lsetxattr(path, XATTR_NAME_SELINUX, context,
>> strlen(context) + 1 0); }
> 
> _______________________________________________ Selinux mailing list 
> Selinux@tycho.nsa.gov To unsubscribe, send email to
> Selinux-leave@tycho.nsa.gov. To get help, send an email containing "help"
> to Selinux-request@tycho.nsa.gov.
> 
> 

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlLC4+IACgkQrlYvE4MpobMFAwCeLj9dtCqPd91lyiujHn71FUSl
DjcAoKHO0xbSAmAMGF1SZMlyn7g9wRTF
=P+o/
-----END PGP SIGNATURE-----

Re: Bug in libselinux/src/setrans_client.c

[Daniel J Walsh]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

There is a bug in the coreutils patch, which should check if the
getfscreatecon() returns a null field, it should not call the setfilecon.

getfscreatecon(&scon)  Returning scon == NULL, means the systems default
labeling should take effect.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlLC5HEACgkQrlYvE4MpobPMKACgmNTPS43v+0RsrJk3vTrxFeQV
Ke0AoKScjaUS8Foslumq9bxhie8su5uQ
=RchO
-----END PGP SIGNATURE-----

Re: Bug in libselinux/src/setrans_client.c

[Matthew Thode]

[-- Attachment #1: Type: text/plain, Size: 4053 bytes --]

On 12/31/2013 01:33 AM, Francis Cunnane wrote:
> What do you propose....  This is free software.... Don't be a Jew.
> 
>   On 12/30/2013 7:11 PM, Matthew Thode wrote:
>> On 12/30/2013 10:11 AM, Stephen Smalley wrote:
>>> Calling *setfilecon() with a NULL context is a bug in the caller, just
>>> like calling strlen() with a NULL string.
>>> Fix the callers, please.
>>>
>>> On Wed, Dec 25, 2013 at 9:36 AM, Nicolas Iooss
>>> <nicolas.iooss@m4x.org> wrote:
>>>> 2013/12/23 Daniel J Walsh wrote:
>>>>> On 12/21/2013 09:27 AM, Nicolas Iooss wrote:
>>>>>> My first message was not so clear. The check in
>>>>>> libselinux/src/lsetfilecon.c line 35 [1] doesn't work because
>>>>>> selinux_trans_to_raw_context(context, &rcontext) returns 0 and sets
>>>>>> rcontext to NULL. This is why I'm asking to change the return
>>>>>> value to
>>>>>> something else if you want "cp -a" working. This fix is not to
>>>>>> introduce a
>>>>>> new feature but to fix an existing one.
>>>>>>
>>>>>> Nicolas
>>>>>>
>>>>> How about if we add a check on lsetfilecon_raw?  Changing the
>>>>> behaviour on
>>>>> selinux_trans_to_raw_context might cause other problems.
>>>> I agree. I've found
>>>> http://selinuxproject.org/page/LibselinuxAPISummary which says
>>>> precisely for selinux_trans_to_raw_context: "If passed NULL, sets the
>>>> returned context to NULL and returns 0." As this feature is
>>>> documented, callers may rely on it and changing this behavior is
>>>> likely to break things.
>>>>
>>>> Moreover setfilecon_raw and fsetfilecon_raw have the same NULL-pointer
>>>> dereference issue. Do these functions need a patch too?
>>>>
>>>> By the way, other callers of selinux_trans_to_raw_context may also
>>>> share this bug: avc_context_to_sid, security_canonicalize_context,
>>>> security_check_context, etc. Is doing a segmentation fault the
>>>> expected way to tell the caller it used a NULL pointer and should have
>>>> manually checked every parameter before calling any libselinux
>>>> function?
>>>>
>>>> Thanks and merry Christmas!
>>>>
>>>> Nicolas
>>>>
>>>>>
>>>>> diff --git a/libselinux/src/lsetfilecon.c
>>>>> b/libselinux/src/lsetfilecon.c
>>>>> index 461e3f7..af3775e 100644
>>>>> - --- a/libselinux/src/lsetfilecon.c
>>>>> +++ b/libselinux/src/lsetfilecon.c
>>>>> @@ -9,6 +9,10 @@
>>>>>
>>>>>   int lsetfilecon_raw(const char *path, const security_context_t
>>>>> context)
>>>>>   {
>>>>> +       if (! context) {
>>>>> +               errno=EINVAL;
>>>>> +               return -1;
>>>>> +       }
>>>>>          return lsetxattr(path, XATTR_NAME_SELINUX, context,
>>>>> strlen(context) + 1
>>>>>                           0);
>>>>>   }
>>>> _______________________________________________
>>>> Selinux mailing list
>>>> Selinux@tycho.nsa.gov
>>>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>>>> To get help, send an email containing "help" to
>>>> Selinux-request@tycho.nsa.gov.
>>> _______________________________________________
>>> Selinux mailing list
>>> Selinux@tycho.nsa.gov
>>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>>> To get help, send an email containing "help" to
>>> Selinux-request@tycho.nsa.gov.
>>>
>> I think I may have hit this bug as well.
>>
>> https://bugs.gentoo.org/show_bug.cgi?id=495274
>>
>>
>>
>> _______________________________________________
>> Selinux mailing list
>> Selinux@tycho.nsa.gov
>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>> To get help, send an email containing "help" to
>> Selinux-request@tycho.nsa.gov.
> 
> 
> 
> 
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
> 
If I had any more info in the bug report then what was mentioned here,
it was meant to help.  Also, on vacation, so won't be of much help this
week :P

-- 
-- Matthew Thode


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Bug in libselinux/src/setrans_client.c

[Francis Cunnane]

[-- Attachment #1.1: Type: text/plain, Size: 4478 bytes --]

tHERE IS ONE MARINE AT nsa, WHY DON'T YOU ASK HIM?

On 12/31/2013 10:52 AM, Matthew Thode wrote:
> On 12/31/2013 01:33 AM, Francis Cunnane wrote:
>> What do you propose....  This is free software.... Don't be a Jew.
>>
>>    On 12/30/2013 7:11 PM, Matthew Thode wrote:
>>> On 12/30/2013 10:11 AM, Stephen Smalley wrote:
>>>> Calling *setfilecon() with a NULL context is a bug in the caller, just
>>>> like calling strlen() with a NULL string.
>>>> Fix the callers, please.
>>>>
>>>> On Wed, Dec 25, 2013 at 9:36 AM, Nicolas Iooss
>>>> <nicolas.iooss@m4x.org> wrote:
>>>>> 2013/12/23 Daniel J Walsh wrote:
>>>>>> On 12/21/2013 09:27 AM, Nicolas Iooss wrote:
>>>>>>> My first message was not so clear. The check in
>>>>>>> libselinux/src/lsetfilecon.c line 35 [1] doesn't work because
>>>>>>> selinux_trans_to_raw_context(context, &rcontext) returns 0 and sets
>>>>>>> rcontext to NULL. This is why I'm asking to change the return
>>>>>>> value to
>>>>>>> something else if you want "cp -a" working. This fix is not to
>>>>>>> introduce a
>>>>>>> new feature but to fix an existing one.
>>>>>>>
>>>>>>> Nicolas
>>>>>>>
>>>>>> How about if we add a check on lsetfilecon_raw?  Changing the
>>>>>> behaviour on
>>>>>> selinux_trans_to_raw_context might cause other problems.
>>>>> I agree. I've found
>>>>> http://selinuxproject.org/page/LibselinuxAPISummary which says
>>>>> precisely for selinux_trans_to_raw_context: "If passed NULL, sets the
>>>>> returned context to NULL and returns 0." As this feature is
>>>>> documented, callers may rely on it and changing this behavior is
>>>>> likely to break things.
>>>>>
>>>>> Moreover setfilecon_raw and fsetfilecon_raw have the same NULL-pointer
>>>>> dereference issue. Do these functions need a patch too?
>>>>>
>>>>> By the way, other callers of selinux_trans_to_raw_context may also
>>>>> share this bug: avc_context_to_sid, security_canonicalize_context,
>>>>> security_check_context, etc. Is doing a segmentation fault the
>>>>> expected way to tell the caller it used a NULL pointer and should have
>>>>> manually checked every parameter before calling any libselinux
>>>>> function?
>>>>>
>>>>> Thanks and merry Christmas!
>>>>>
>>>>> Nicolas
>>>>>
>>>>>> diff --git a/libselinux/src/lsetfilecon.c
>>>>>> b/libselinux/src/lsetfilecon.c
>>>>>> index 461e3f7..af3775e 100644
>>>>>> - --- a/libselinux/src/lsetfilecon.c
>>>>>> +++ b/libselinux/src/lsetfilecon.c
>>>>>> @@ -9,6 +9,10 @@
>>>>>>
>>>>>>    int lsetfilecon_raw(const char *path, const security_context_t
>>>>>> context)
>>>>>>    {
>>>>>> +       if (! context) {
>>>>>> +               errno=EINVAL;
>>>>>> +               return -1;
>>>>>> +       }
>>>>>>           return lsetxattr(path, XATTR_NAME_SELINUX, context,
>>>>>> strlen(context) + 1
>>>>>>                            0);
>>>>>>    }
>>>>> _______________________________________________
>>>>> Selinux mailing list
>>>>> Selinux@tycho.nsa.gov
>>>>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>>>>> To get help, send an email containing "help" to
>>>>> Selinux-request@tycho.nsa.gov.
>>>> _______________________________________________
>>>> Selinux mailing list
>>>> Selinux@tycho.nsa.gov
>>>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>>>> To get help, send an email containing "help" to
>>>> Selinux-request@tycho.nsa.gov.
>>>>
>>> I think I may have hit this bug as well.
>>>
>>> https://bugs.gentoo.org/show_bug.cgi?id=495274
>>>
>>>
>>>
>>> _______________________________________________
>>> Selinux mailing list
>>> Selinux@tycho.nsa.gov
>>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>>> To get help, send an email containing "help" to
>>> Selinux-request@tycho.nsa.gov.
>>
>>
>>
>> _______________________________________________
>> Selinux mailing list
>> Selinux@tycho.nsa.gov
>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
>>
> If I had any more info in the bug report then what was mentioned here,
> it was meant to help.  Also, on vacation, so won't be of much help this
> week :P
>
>
>
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.


[-- Attachment #1.2: Type: text/html, Size: 6787 bytes --]

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3756 bytes --]

Re: Bug in libselinux/src/setrans_client.c

[Nicolas Iooss]

I've reported this bug to coreutils' bug tracker and the caller is going
to be fixed: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=16335

Thanks,

Nicolas

On 30/12/2013 17:11, Stephen Smalley wrote :
> Calling *setfilecon() with a NULL context is a bug in the caller, just
> like calling strlen() with a NULL string.
> Fix the callers, please.
> 
> On Wed, Dec 25, 2013 at 9:36 AM, Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>> 2013/12/23 Daniel J Walsh wrote:
>>>
>>> On 12/21/2013 09:27 AM, Nicolas Iooss wrote:
>>>> My first message was not so clear. The check in
>>>> libselinux/src/lsetfilecon.c line 35 [1] doesn't work because
>>>> selinux_trans_to_raw_context(context, &rcontext) returns 0 and sets
>>>> rcontext to NULL. This is why I'm asking to change the return value to
>>>> something else if you want "cp -a" working. This fix is not to introduce a
>>>> new feature but to fix an existing one.
>>>>
>>>> Nicolas
>>>>
>>>
>>> How about if we add a check on lsetfilecon_raw?  Changing the behaviour on
>>> selinux_trans_to_raw_context might cause other problems.
>>
>> I agree. I've found
>> http://selinuxproject.org/page/LibselinuxAPISummary which says
>> precisely for selinux_trans_to_raw_context: "If passed NULL, sets the
>> returned context to NULL and returns 0." As this feature is
>> documented, callers may rely on it and changing this behavior is
>> likely to break things.
>>
>> Moreover setfilecon_raw and fsetfilecon_raw have the same NULL-pointer
>> dereference issue. Do these functions need a patch too?
>>
>> By the way, other callers of selinux_trans_to_raw_context may also
>> share this bug: avc_context_to_sid, security_canonicalize_context,
>> security_check_context, etc. Is doing a segmentation fault the
>> expected way to tell the caller it used a NULL pointer and should have
>> manually checked every parameter before calling any libselinux
>> function?
>>
>> Thanks and merry Christmas!
>>
>> Nicolas
>>
>>>
>>>
>>> diff --git a/libselinux/src/lsetfilecon.c b/libselinux/src/lsetfilecon.c
>>> index 461e3f7..af3775e 100644
>>> - --- a/libselinux/src/lsetfilecon.c
>>> +++ b/libselinux/src/lsetfilecon.c
>>> @@ -9,6 +9,10 @@
>>>
>>>  int lsetfilecon_raw(const char *path, const security_context_t context)
>>>  {
>>> +       if (! context) {
>>> +               errno=EINVAL;
>>> +               return -1;
>>> +       }
>>>         return lsetxattr(path, XATTR_NAME_SELINUX, context, strlen(context) + 1
>>>                          0);
>>>  }
>>
>> _______________________________________________
>> Selinux mailing list
>> Selinux@tycho.nsa.gov
>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.

Re: Bug in libselinux/src/setrans_client.c

[Daniel J Walsh]

[-- Attachment #1: Type: text/plain, Size: 4525 bytes --]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/31/2013 01:52 PM, Matthew Thode wrote:
> On 12/31/2013 01:33 AM, Francis Cunnane wrote:
>> What do you propose....  This is free software.... Don't be a Jew.
>> 
>> On 12/30/2013 7:11 PM, Matthew Thode wrote:
>>> On 12/30/2013 10:11 AM, Stephen Smalley wrote:
>>>> Calling *setfilecon() with a NULL context is a bug in the caller,
>>>> just like calling strlen() with a NULL string. Fix the callers,
>>>> please.
>>>> 
>>>> On Wed, Dec 25, 2013 at 9:36 AM, Nicolas Iooss 
>>>> <nicolas.iooss@m4x.org> wrote:
>>>>> 2013/12/23 Daniel J Walsh wrote:
>>>>>> On 12/21/2013 09:27 AM, Nicolas Iooss wrote:
>>>>>>> My first message was not so clear. The check in 
>>>>>>> libselinux/src/lsetfilecon.c line 35 [1] doesn't work because 
>>>>>>> selinux_trans_to_raw_context(context, &rcontext) returns 0 and
>>>>>>> sets rcontext to NULL. This is why I'm asking to change the
>>>>>>> return value to something else if you want "cp -a" working.
>>>>>>> This fix is not to introduce a new feature but to fix an
>>>>>>> existing one.
>>>>>>> 
>>>>>>> Nicolas
>>>>>>> 
>>>>>> How about if we add a check on lsetfilecon_raw?  Changing the 
>>>>>> behaviour on selinux_trans_to_raw_context might cause other
>>>>>> problems.
>>>>> I agree. I've found 
>>>>> http://selinuxproject.org/page/LibselinuxAPISummary which says 
>>>>> precisely for selinux_trans_to_raw_context: "If passed NULL, sets
>>>>> the returned context to NULL and returns 0." As this feature is 
>>>>> documented, callers may rely on it and changing this behavior is 
>>>>> likely to break things.
>>>>> 
>>>>> Moreover setfilecon_raw and fsetfilecon_raw have the same
>>>>> NULL-pointer dereference issue. Do these functions need a patch
>>>>> too?
>>>>> 
>>>>> By the way, other callers of selinux_trans_to_raw_context may also 
>>>>> share this bug: avc_context_to_sid, security_canonicalize_context, 
>>>>> security_check_context, etc. Is doing a segmentation fault the 
>>>>> expected way to tell the caller it used a NULL pointer and should
>>>>> have manually checked every parameter before calling any
>>>>> libselinux function?
>>>>> 
>>>>> Thanks and merry Christmas!
>>>>> 
>>>>> Nicolas
>>>>> 
>>>>>> 
>>>>>> diff --git a/libselinux/src/lsetfilecon.c 
>>>>>> b/libselinux/src/lsetfilecon.c index 461e3f7..af3775e 100644 -
>>>>>> --- a/libselinux/src/lsetfilecon.c +++
>>>>>> b/libselinux/src/lsetfilecon.c @@ -9,6 +9,10 @@
>>>>>> 
>>>>>> int lsetfilecon_raw(const char *path, const security_context_t 
>>>>>> context) { +       if (! context) { +
>>>>>> errno=EINVAL; +               return -1; +       } return
>>>>>> lsetxattr(path, XATTR_NAME_SELINUX, context, strlen(context) + 1 
>>>>>> 0); }
>>>>> _______________________________________________ Selinux mailing
>>>>> list Selinux@tycho.nsa.gov To unsubscribe, send email to
>>>>> Selinux-leave@tycho.nsa.gov. To get help, send an email containing
>>>>> "help" to Selinux-request@tycho.nsa.gov.
>>>> _______________________________________________ Selinux mailing list 
>>>> Selinux@tycho.nsa.gov To unsubscribe, send email to
>>>> Selinux-leave@tycho.nsa.gov. To get help, send an email containing
>>>> "help" to Selinux-request@tycho.nsa.gov.
>>>> 
>>> I think I may have hit this bug as well.
>>> 
>>> https://bugs.gentoo.org/show_bug.cgi?id=495274
>>> 
>>> 
>>> 
>>> _______________________________________________ Selinux mailing list 
>>> Selinux@tycho.nsa.gov To unsubscribe, send email to
>>> Selinux-leave@tycho.nsa.gov. To get help, send an email containing
>>> "help" to Selinux-request@tycho.nsa.gov.
>> 
>> 
>> 
>> 
>> _______________________________________________ Selinux mailing list 
>> Selinux@tycho.nsa.gov To unsubscribe, send email to
>> Selinux-leave@tycho.nsa.gov. To get help, send an email containing "help"
>> to Selinux-request@tycho.nsa.gov.
>> 
> If I had any more info in the bug report then what was mentioned here, it
> was meant to help.  Also, on vacation, so won't be of much help this week
> :P
> 
> 
> 
> _______________________________________________ Selinux mailing list 
> Selinux@tycho.nsa.gov To unsubscribe, send email to
> Selinux-leave@tycho.nsa.gov. To get help, send an email containing "help"
> to Selinux-request@tycho.nsa.gov.
> 

How about this patch?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlLK0jcACgkQrlYvE4MpobN0RgCfafbSstZ1FK+PDvqIsEjxYo3O
iz8AoLadj5UAqzD4Nw3+qyfK+s/vfThu
=GLwc
-----END PGP SIGNATURE-----

[-- Attachment #2: 0001-Verify-context-input-to-funtions-to-make-sure-the-co.patch --]
[-- Type: text/x-patch, Size: 6104 bytes --]

>From 1a9ef9cd4fe84f590ff9b0d7b402d68220922d7a Mon Sep 17 00:00:00 2001
From: Dan Walsh <dwalsh@redhat.com>
Date: Mon, 23 Dec 2013 09:50:54 -0500
Subject: [PATCH] Verify context input to funtions to make sure the context
 field is not null.

Return errno EINVAL, to prevent segfault.
---
 libselinux/src/avc_sidtab.c           | 5 +++++
 libselinux/src/canonicalize_context.c | 5 +++++
 libselinux/src/check_context.c        | 5 +++++
 libselinux/src/compute_av.c           | 5 +++++
 libselinux/src/compute_create.c       | 5 +++++
 libselinux/src/compute_member.c       | 5 +++++
 libselinux/src/compute_relabel.c      | 5 +++++
 libselinux/src/compute_user.c         | 5 +++++
 libselinux/src/fsetfilecon.c          | 8 ++++++--
 libselinux/src/lsetfilecon.c          | 9 +++++++--
 libselinux/src/setfilecon.c           | 8 ++++++--
 11 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/libselinux/src/avc_sidtab.c b/libselinux/src/avc_sidtab.c
index 0b696bb..506e236 100644
--- a/libselinux/src/avc_sidtab.c
+++ b/libselinux/src/avc_sidtab.c
@@ -81,6 +81,11 @@ sidtab_context_to_sid(struct sidtab *s,
 	int hvalue, rc = 0;
 	struct sidtab_node *cur;
 
+	if (! ctx) {
+		errno=EINVAL;
+		return -1;
+	}
+
 	*sid = NULL;
 	hvalue = sidtab_hash(ctx);
 
diff --git a/libselinux/src/canonicalize_context.c b/libselinux/src/canonicalize_context.c
index 176c45a..6075025 100644
--- a/libselinux/src/canonicalize_context.c
+++ b/libselinux/src/canonicalize_context.c
@@ -17,6 +17,11 @@ int security_canonicalize_context_raw(const security_context_t con,
 	size_t size;
 	int fd, ret;
 
+	if (! con) {
+		errno=EINVAL;
+		return -1;
+	}
+
 	if (!selinux_mnt) {
 		errno = ENOENT;
 		return -1;
diff --git a/libselinux/src/check_context.c b/libselinux/src/check_context.c
index 33ab5e3..1277bdd 100644
--- a/libselinux/src/check_context.c
+++ b/libselinux/src/check_context.c
@@ -14,6 +14,11 @@ int security_check_context_raw(const security_context_t con)
 	char path[PATH_MAX];
 	int fd, ret;
 
+	if (! con) {
+		errno=EINVAL;
+		return -1;
+	}
+
 	if (!selinux_mnt) {
 		errno = ENOENT;
 		return -1;
diff --git a/libselinux/src/compute_av.c b/libselinux/src/compute_av.c
index 5962c0b..61ea454 100644
--- a/libselinux/src/compute_av.c
+++ b/libselinux/src/compute_av.c
@@ -26,6 +26,11 @@ int security_compute_av_flags_raw(const security_context_t scon,
 		return -1;
 	}
 
+	if ((! scon) || (! tcon)) {
+		errno=EINVAL;
+		return -1;
+	}
+
 	snprintf(path, sizeof path, "%s/access", selinux_mnt);
 	fd = open(path, O_RDWR);
 	if (fd < 0)
diff --git a/libselinux/src/compute_create.c b/libselinux/src/compute_create.c
index 3c05be3..34a1ccd 100644
--- a/libselinux/src/compute_create.c
+++ b/libselinux/src/compute_create.c
@@ -64,6 +64,11 @@ int security_compute_create_name_raw(const security_context_t scon,
 		return -1;
 	}
 
+	if ((! scon) || (! tcon)) {
+		errno=EINVAL;
+		return -1;
+	}
+
 	snprintf(path, sizeof path, "%s/create", selinux_mnt);
 	fd = open(path, O_RDWR);
 	if (fd < 0)
diff --git a/libselinux/src/compute_member.c b/libselinux/src/compute_member.c
index dad0a77..7850986 100644
--- a/libselinux/src/compute_member.c
+++ b/libselinux/src/compute_member.c
@@ -25,6 +25,11 @@ int security_compute_member_raw(const security_context_t scon,
 		return -1;
 	}
 
+	if ((! scon) || (! tcon)) {
+		errno=EINVAL;
+		return -1;
+	}
+
 	snprintf(path, sizeof path, "%s/member", selinux_mnt);
 	fd = open(path, O_RDWR);
 	if (fd < 0)
diff --git a/libselinux/src/compute_relabel.c b/libselinux/src/compute_relabel.c
index 656f00a..2560e78 100644
--- a/libselinux/src/compute_relabel.c
+++ b/libselinux/src/compute_relabel.c
@@ -25,6 +25,11 @@ int security_compute_relabel_raw(const security_context_t scon,
 		return -1;
 	}
 
+	if ((! scon) || (! tcon)) {
+		errno=EINVAL;
+		return -1;
+	}
+
 	snprintf(path, sizeof path, "%s/relabel", selinux_mnt);
 	fd = open(path, O_RDWR);
 	if (fd < 0)
diff --git a/libselinux/src/compute_user.c b/libselinux/src/compute_user.c
index 3b39ddd..af20735 100644
--- a/libselinux/src/compute_user.c
+++ b/libselinux/src/compute_user.c
@@ -24,6 +24,11 @@ int security_compute_user_raw(const security_context_t scon,
 		return -1;
 	}
 
+	if (! scon) {
+		errno=EINVAL;
+		return -1;
+	}
+
 	snprintf(path, sizeof path, "%s/user", selinux_mnt);
 	fd = open(path, O_RDWR);
 	if (fd < 0)
diff --git a/libselinux/src/fsetfilecon.c b/libselinux/src/fsetfilecon.c
index 9963f7a..37f9d74 100644
--- a/libselinux/src/fsetfilecon.c
+++ b/libselinux/src/fsetfilecon.c
@@ -9,8 +9,12 @@
 
 int fsetfilecon_raw(int fd, const security_context_t context)
 {
-	int rc = fsetxattr(fd, XATTR_NAME_SELINUX, context, strlen(context) + 1,
-			 0);
+	int rc;
+	if (! context) {
+		errno=EINVAL;
+		return -1;
+	}
+	rc = fsetxattr(fd, XATTR_NAME_SELINUX, context, strlen(context) + 1, 0);
 	if (rc < 0 && errno == ENOTSUP) {
 		security_context_t ccontext = NULL;
 		int err = errno;
diff --git a/libselinux/src/lsetfilecon.c b/libselinux/src/lsetfilecon.c
index fd9bb26..af2d88c 100644
--- a/libselinux/src/lsetfilecon.c
+++ b/libselinux/src/lsetfilecon.c
@@ -9,8 +9,13 @@
 
 int lsetfilecon_raw(const char *path, const security_context_t context)
 {
-	int rc = lsetxattr(path, XATTR_NAME_SELINUX, context, strlen(context) + 1,
-			 0);
+	int rc;
+	if (! context) {
+		errno=EINVAL;
+		return -1;
+	}
+
+	rc = lsetxattr(path, XATTR_NAME_SELINUX, context, strlen(context) + 1, 0);
 	if (rc < 0 && errno == ENOTSUP) {
 		security_context_t ccontext = NULL;
 		int err = errno;
diff --git a/libselinux/src/setfilecon.c b/libselinux/src/setfilecon.c
index 50cb228..e617039 100644
--- a/libselinux/src/setfilecon.c
+++ b/libselinux/src/setfilecon.c
@@ -9,8 +9,12 @@
 
 int setfilecon_raw(const char *path, const security_context_t context)
 {
-	int rc = setxattr(path, XATTR_NAME_SELINUX, context, strlen(context) + 1,
-			0);
+	int rc;
+	if (! context) {
+		errno=EINVAL;
+		return -1;
+	}
+	rc = setxattr(path, XATTR_NAME_SELINUX, context, strlen(context) + 1, 0);
 	if (rc < 0 && errno == ENOTSUP) {
 		security_context_t ccontext = NULL;
 		int err = errno;
-- 
1.8.5.2

Re: Bug in libselinux/src/setrans_client.c

[Stephen Smalley]

On 01/06/2014 10:56 AM, Daniel J Walsh wrote:
> How about this patch?

I'm not in favor.  We shouldn't hide bugs in the callers.

Re: Bug in libselinux/src/setrans_client.c

[Daniel J Walsh]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/06/2014 12:49 PM, Stephen Smalley wrote:
> On 01/06/2014 10:56 AM, Daniel J Walsh wrote:
>> How about this patch?
> 
> I'm not in favor.  We shouldn't hide bugs in the callers.
> 
> 
> _______________________________________________ Selinux mailing list 
> Selinux@tycho.nsa.gov To unsubscribe, send email to
> Selinux-leave@tycho.nsa.gov. To get help, send an email containing "help"
> to Selinux-request@tycho.nsa.gov.
> 
> 
How is returning a ERRNO/ERROR for bad values hiding bugs from callers?  We
are actually telling the callers what is bad.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlLK7zkACgkQrlYvE4MpobNRvACfShksLPm7FOSeZqvHipEcqLFT
TfwAoNblhfZOU+wtA0i5B/AkE62CNQkB
=rRfh
-----END PGP SIGNATURE-----

Re: Bug in libselinux/src/setrans_client.c

[Stephen Smalley]

On 01/06/2014 01:00 PM, Daniel J Walsh wrote:
> How is returning a ERRNO/ERROR for bad values hiding bugs from callers?  We
> are actually telling the callers what is bad.

Not really.  Stack trace is more informative than EINVAL, as the one
will show you exactly where you failed and what the arguments were,
whereas EINVAL could be a result of any of the arguments or a side
effect elsewhere in the function.  And the original functions that
prompted this discussion have never allowed you to pass NULL as
contexts.  Would you add a NULL check inside of every string.h function
in libc?

Re: Bug in libselinux/src/setrans_client.c

[Daniel J Walsh]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/06/2014 01:11 PM, Stephen Smalley wrote:
> On 01/06/2014 01:00 PM, Daniel J Walsh wrote:
>> How is returning a ERRNO/ERROR for bad values hiding bugs from callers?
>> We are actually telling the callers what is bad.
> 
> Not really.  Stack trace is more informative than EINVAL, as the one will
> show you exactly where you failed and what the arguments were, whereas
> EINVAL could be a result of any of the arguments or a side effect elsewhere
> in the function.  And the original functions that prompted this discussion
> have never allowed you to pass NULL as contexts.  Would you add a NULL
> check inside of every string.h function in libc?
> 
> 
> 
> _______________________________________________ Selinux mailing list 
> Selinux@tycho.nsa.gov To unsubscribe, send email to
> Selinux-leave@tycho.nsa.gov. To get help, send an email containing "help"
> to Selinux-request@tycho.nsa.gov.
> 
> 
- From a programmer point of view yes, but from an enterprise point of view no.
 I would prefer the apps to have a chance to reasonably fail.  Our functions
define return codes, while functions like strlen, strcmp and strcp do not have
failure modes.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlLLGSgACgkQrlYvE4MpobOb/ACg5bJKo8bGUHXpWVZyA6NpUo/v
jBIAn0OUcGiVnVLiSibgssvOLOqjem6o
=uKuI
-----END PGP SIGNATURE-----

Mailing list etiquette

[Stephen Smalley]

Hi,

In light of a recent unfortunate posting to the list, we'd like to
remind everyone that posting content that demeans, ridicules or
discriminates against any individual on the basis of sex, age, national
origin, race, disability, religion, sexual orientation, or color is a
violation of the list rules and grounds for removal from the list.

If you encounter such a posting on the list, please bring it to the
attention of the list owners (selinux-owner@tycho.nsa.gov) and do not
reply to it on the list.

Thanks.