[Pkg-shadow-devel] acl, attr and selinux

[Peter Vrabec]

Hi Nicolas,


On Thursday, November 17, 2011 11:53:32 PM Nicolas François wrote:
> Hi Peter,
> 
> The 3 first patches are applied.
> I have limited acl, attr, semanage, selinux knowledge, so plese find some
> questions below.
> 
> shadow-upstream-acl.patch
> =========================
> I would just like to confirm that there is no need to call chmod()
> when perm_copy_file() returns != 0 with errno set to EOPNOTSUPP.
> Is this correct?
There is no need to call chmod. It worked correctly without chmod on my box. 
Please see the patch that I have attached. It seems we need to care about 
attr_copy_file/fd too.


> I've replaced EOPNOTSUPP by ENOTSUP,
> It's the code I could find on
> http://git.savannah.gnu.org/cgit/acl.git/tree/libacl/perm_copy_file.c (at
> least on my system they are defined to the same value anyway)
works for me:
bits/errno.h
/* Linux has no ENOTSUP error code.  */
# define ENOTSUP EOPNOTSUPP

> shadow-upstream-attr.patch
> ==========================
> Applied.
> 
> shadow-upstream-libsemanage.patch
> =================================
> In case of useradd -Z "":
> Is it an error? Or does it have a special meaning? (Is seems similar to
> not specifying the -Z option at all)
> (This is both maybe for documenting and understanding the use of
> 'Zflg && *user_selinux')
> Same question for usermod.
>  => in case of useradd, I would assume that the default SELinux user is
>     used (this is currently documented), as if -Z was not used. Maybe I
>     should reject such request?
>  => in case of usermod, I would assume that the default SELinux user needs
>     to be set (this is my understanding of the documentation). Should I
>     call del_seuser?
Hmm. I support that  'Zflg && *user_selinux' is not need, 'Zflg' should be 
enough.  useradd -Z "" does not have a special meening. Empty string "" is not 
a valid selinux user. If I remove '*user_selinux', so the condition is just  
'Zflg', I will get this message:

$ sudo ./useradd -Z "" foo12
[libsemanage]: selinux user  does not exist
[libsemanage]: seuser mapping [foo12 -> (, s0)] is invalid
[libsemanage]: could not iterate over records
Cannot commit SELinux transaction
useradd: warning: the user name foo12 to  SELinux user mapping failed.

Feel free to discard '*user_selinux'


> Should userdel require -Z to be explicitly set, or should it delete the
> SELinux user mapping in any case (if any)?
I would let it be set explicitly. In other words, sys admin is responsible for 
decision. 

> I changed userdel to set the long option --selinux-user to no_argument
> (this was already the case for short -Z option)
good.

> I removed the is_selinux_enabled check when Zflg is already set
> (is_selinux_enabled is already checked in the getopt switch)
> 
> I added a userdel -Z documentation
> 	Remove any SELinux user mapping for the user's login.
> 
> I did not really review lib/selinux.c
> 
> New return code still need to be documented.
> 
> shadow-4.1.4.3-selinux.patch
> ============================
> First, sorry, this issue was introduced by me in 2009.
> 
> I have an issue with the following sequence:
>  * commonio_open
>    -> scontext is set to the /etc/shadow context
>  * No changes by the program
>  * commonio_close
>    -> goto success because there were no changes
>    -> setfscreatecon (NULL)
>       because we did not pass through getfscreatecon (&old_context)
> 
> I would propose the attached patch. Is this fine?
> (I do not think there would really be issues because the shadow utils call
> close() shortly before exit())
> 
> Other sequence with possible issue:
>  * no context associated to /etc/shadow
>    -> scontext set to NULL
>  * restricted file creation context set to the shadow util
>  => Can this happen?
>  => Should the file creation context be set to NULL before creating
>     /etc/<file>+
> 
> Question regarding SELinux: The overall goal of scontext is to set
> the context of the temporary file /etc/shadow+ to the one of /etc/shadow.
> Is there a way to set this context in advance even if /etc/shadow did not
> exist (i.e. the context cannot be retrieved with fgetfilecon)?
> (does getfilecon provide a context even if the file does not exist?)
> Is it safe not to do it? Should the creation of /etc/shadow be forbidden
> in some cases to let the admin create the file correctly (with the right
> context)?
> 
> Best Regards,
-------------- next part --------------
A non-text attachment was scrubbed...
Name: shadow-upstream-attr_file.patch
Type: text/x-patch
Size: 2832 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/pkg-shadow-devel/attachments/20111121/c51e1a63/attachment.bin>