[RFC] selinux: add a fallback to defcontext for native labeling

Commit Message

Jann Horn via Selinux Sept. 19, 2018, 4:52 p.m. UTC

When files on NFSv4 server are not properly labeled (label doesn't match
a policy on a client) they will end up with unlabeled_t type which is
too generic. We would like to be able to set a default context per
mount. 'defcontext' mount option looks like a nice solution, but it
doesn't seem to be fully implemented for native labeling. Default
context is stored, but is never used.

The patch adds a fallback to a default context if a received context is
invalid. If the inode context is already initialized, then it is left
untouched to preserve a context set locally on a client.

Signed-off-by: Taras Kondratiuk <[e-mail]>
---
 security/selinux/hooks.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

Comments

Paul Moore Sept. 19, 2018, 6:47 p.m. UTC | #1

On Wed, Sep 19, 2018 at 12:52 PM Taras Kondratiuk <[e-mail]> wrote:
> When files on NFSv4 server are not properly labeled (label doesn't match
> a policy on a client) they will end up with unlabeled_t type which is
> too generic. We would like to be able to set a default context per
> mount. 'defcontext' mount option looks like a nice solution, but it
> doesn't seem to be fully implemented for native labeling. Default
> context is stored, but is never used.
>
> The patch adds a fallback to a default context if a received context is
> invalid. If the inode context is already initialized, then it is left
> untouched to preserve a context set locally on a client.
>
> Signed-off-by: Taras Kondratiuk <[e-mail]>
> ---
>  security/selinux/hooks.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)

The idea seems reasonable to me; if we want to treat labeled NFS like
we treat local filesystems we should also support the defcontext mount
option.

However, I wonder if we are better off generalizing some of the
SECURITY_FS_USE_XATTR based code in inode_doinit_with_dentry() such
that it can be used both in selinux_inode_notifysecctx() and in
inode_doinit_with_dentry().  Or maybe we just need a sbsec->def_sid
variant of selinux_inode_setsecurity().  Regardless, the key is the
call to security_context_to_sid_default(), the
selinux_inode_setsecurity() function only calls
security_context_to_sid().

Just in case anyone is wondering, I'm not sure I want to update
selinux_inode_setsecurity() to call security_context_to_sid_default();
at least not without more investigation.

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index ad9a9b8e9979..f7debe798bf5 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6598,7 +6598,30 @@ static void selinux_inode_invalidate_secctx(struct inode *inode)
>   */
>  static int selinux_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen)
>  {
> -       return selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX, ctx, ctxlen, 0);
> +       struct superblock_security_struct *sbsec;
> +       struct inode_security_struct *isec;
> +       int rc;
> +
> +       rc = selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX, ctx, ctxlen, 0);
> +
> +       /*
> +        * In case of Native labeling with defcontext mount option fall back
> +        * to a default SID if received context is invalid.
> +        */
> +       if (rc == -EINVAL) {
> +               sbsec = inode->i_sb->s_security;
> +               if (sbsec->behavior == SECURITY_FS_USE_NATIVE &&
> +                   sbsec->flags & DEFCONTEXT_MNT) {
> +                       isec = inode->i_security;
> +                       if (!isec->initialized) {
> +                               isec->sclass = inode_mode_to_security_class(inode->i_mode);
> +                               isec->sid = sbsec->def_sid;
> +                               isec->initialized = 1;
> +                       }
> +                       rc = 0;
> +               }
> +       }
> +       return rc;
>  }
>
>  /*
> --
> 2.10.3.dirty
>

Stephen Smalley Sept. 19, 2018, 7 p.m. UTC | #2

On 09/19/2018 12:52 PM, Taras Kondratiuk wrote:
> When files on NFSv4 server are not properly labeled (label doesn't match
> a policy on a client) they will end up with unlabeled_t type which is
> too generic. We would like to be able to set a default context per
> mount. 'defcontext' mount option looks like a nice solution, but it
> doesn't seem to be fully implemented for native labeling. Default
> context is stored, but is never used.
> 
> The patch adds a fallback to a default context if a received context is
> invalid. If the inode context is already initialized, then it is left
> untouched to preserve a context set locally on a client.

Can you explain the use case further?  Why are you exporting a 
filesystem with security labeling enabled to a client that doesn't 
understand all of the labels used within it?  Why wouldn't you just 
disable NFSv4 security labeling and/or use a regular context= mount to 
assign a single context to all files in the mount?

To be clear, defcontext= doesn't work that way for local/FS_USE_XATTR 
filesystems. The context specified by it is only used for:
1) files that don't implement the xattr inode operations at all,
2) files that lack a security.selinux xattr,
3) the MLS portion of the context if it was missing (strictly as a 
legacy compatibility mechanism for RHEL4 which predated the enabling of 
the MLS field/logic).

A file with a security.selinux xattr that is invalid under policy for 
any reason other than a missing MLS field will be handled as having the 
unlabeled context.

So this would be a divergence in semantics for defcontext= between 
local/FS_USE_XATTR and NFS/FS_USE_NATIVE filesystems.

> 
> Signed-off-by: Taras Kondratiuk <[e-mail]>
> ---
>   security/selinux/hooks.c | 25 ++++++++++++++++++++++++-
>   1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index ad9a9b8e9979..f7debe798bf5 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6598,7 +6598,30 @@ static void selinux_inode_invalidate_secctx(struct inode *inode)
>    */
>   static int selinux_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen)
>   {
> -	return selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX, ctx, ctxlen, 0);
> +	struct superblock_security_struct *sbsec;
> +	struct inode_security_struct *isec;
> +	int rc;
> +
> +	rc = selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX, ctx, ctxlen, 0);

In this case, we likely don't gain much by reusing 
selinux_inode_setsecurity() here and could just inline the relevant 
portion of it if we were to make this change.  Logically they mean 
different things.

> +
> +	/*
> +	 * In case of Native labeling with defcontext mount option fall back
> +	 * to a default SID if received context is invalid.
> +	 */
> +	if (rc == -EINVAL) {
> +		sbsec = inode->i_sb->s_security;
> +		if (sbsec->behavior == SECURITY_FS_USE_NATIVE &&
> +		    sbsec->flags & DEFCONTEXT_MNT) {
> +			isec = inode->i_security;
> +			if (!isec->initialized) {
> +				isec->sclass = inode_mode_to_security_class(inode->i_mode);
> +				isec->sid = sbsec->def_sid;
> +				isec->initialized = 1;
> +			}
> +			rc = 0;
> +		}
> +	}
> +	return rc;
>   }
>   
>   /*
>

Jann Horn via Selinux Sept. 20, 2018, 2:41 a.m. UTC | #3

Quoting Stephen Smalley (2018-09-19 12:00:33)
> On 09/19/2018 12:52 PM, Taras Kondratiuk wrote:
> > When files on NFSv4 server are not properly labeled (label doesn't match
> > a policy on a client) they will end up with unlabeled_t type which is
> > too generic. We would like to be able to set a default context per
> > mount. 'defcontext' mount option looks like a nice solution, but it
> > doesn't seem to be fully implemented for native labeling. Default
> > context is stored, but is never used.
> > 
> > The patch adds a fallback to a default context if a received context is
> > invalid. If the inode context is already initialized, then it is left
> > untouched to preserve a context set locally on a client.
> 
> Can you explain the use case further?  Why are you exporting a 
> filesystem with security labeling enabled to a client that doesn't 
> understand all of the labels used within it?  Why wouldn't you just 
> disable NFSv4 security labeling and/or use a regular context= mount to 
> assign a single context to all files in the mount?

Client and server are two embedded devices. They are part of a bigger
modular system and normally run the same SW version, so they understand
each others NFSv4 SELinux labels. But during migration from one SW
version to another a client and a server may run different versions for
some time.

The immediate issue I'm trying to address is a migration between
releases with and without SELinux policy. A client (with policy) gets
initial SID labels from a server (without policy). Those labels are
considered invalid, so files remain unlabeled. This also causes lots of
errors from nfs_setsecurity() in dmesg.

Using 'context=' at mount point level is an option, but in a normal case
when both sides have a policy we need more granular labels. It is
possible to detect a case when a server doesn't have a policy and
remount with 'context=', but this special case handling looks a bit
ugly. Having something like 'defcontext' whould allow to avoid this
special case and unconditionally assign default context to the
mountpoint.

'defcontext' may also help during migration between SELinux-enabled
releases if a new release introduces labels that are not recognized by
older release. In this case they can also fallback to defcontext.

> 
> To be clear, defcontext= doesn't work that way for local/FS_USE_XATTR 
> filesystems. The context specified by it is only used for:
> 1) files that don't implement the xattr inode operations at all,
> 2) files that lack a security.selinux xattr,
> 3) the MLS portion of the context if it was missing (strictly as a 
> legacy compatibility mechanism for RHEL4 which predated the enabling of 
> the MLS field/logic).
> 
> A file with a security.selinux xattr that is invalid under policy for 
> any reason other than a missing MLS field will be handled as having the 
> unlabeled context.

inode_doinit_with_dentry() invokes security_context_to_sid_default()
that invokes security_context_to_sid_core() with 'force' flag. Won't
sidtab_context_to_sid() in this case allocate a new SID for the invalid
xattr context instead of leaving it unlabeled?

> 
> So this would be a divergence in semantics for defcontext= between 
> local/FS_USE_XATTR and NFS/FS_USE_NATIVE filesystems.

Yeah, I also didn't like this semantics mismatch. But from another point
defcontext allows to assign a context to files that would otherwise be
unlabeled. Something similar I'd like to achive for NFS.

> 
> > 
> > Signed-off-by: Taras Kondratiuk <[e-mail]>
> > ---
> >   security/selinux/hooks.c | 25 ++++++++++++++++++++++++-
> >   1 file changed, 24 insertions(+), 1 deletion(-)
> > 
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index ad9a9b8e9979..f7debe798bf5 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -6598,7 +6598,30 @@ static void selinux_inode_invalidate_secctx(struct inode *inode)
> >    */
> >   static int selinux_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen)
> >   {
> > -     return selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX, ctx, ctxlen, 0);
> > +     struct superblock_security_struct *sbsec;
> > +     struct inode_security_struct *isec;
> > +     int rc;
> > +
> > +     rc = selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX, ctx, ctxlen, 0);
> 
> In this case, we likely don't gain much by reusing 
> selinux_inode_setsecurity() here and could just inline the relevant 
> portion of it if we were to make this change.  Logically they mean 
> different things.
> 
> > +
> > +     /*
> > +      * In case of Native labeling with defcontext mount option fall back
> > +      * to a default SID if received context is invalid.
> > +      */
> > +     if (rc == -EINVAL) {
> > +             sbsec = inode->i_sb->s_security;
> > +             if (sbsec->behavior == SECURITY_FS_USE_NATIVE &&
> > +                 sbsec->flags & DEFCONTEXT_MNT) {
> > +                     isec = inode->i_security;
> > +                     if (!isec->initialized) {
> > +                             isec->sclass = inode_mode_to_security_class(inode->i_mode);
> > +                             isec->sid = sbsec->def_sid;
> > +                             isec->initialized = 1;
> > +                     }
> > +                     rc = 0;
> > +             }
> > +     }
> > +     return rc;
> >   }
> >   
> >   /*
> > 
>

Stephen Smalley Sept. 20, 2018, 2:49 p.m. UTC | #4

On 09/19/2018 10:41 PM, Taras Kondratiuk wrote:
> Quoting Stephen Smalley (2018-09-19 12:00:33)
>> On 09/19/2018 12:52 PM, Taras Kondratiuk wrote:
>>> When files on NFSv4 server are not properly labeled (label doesn't match
>>> a policy on a client) they will end up with unlabeled_t type which is
>>> too generic. We would like to be able to set a default context per
>>> mount. 'defcontext' mount option looks like a nice solution, but it
>>> doesn't seem to be fully implemented for native labeling. Default
>>> context is stored, but is never used.
>>>
>>> The patch adds a fallback to a default context if a received context is
>>> invalid. If the inode context is already initialized, then it is left
>>> untouched to preserve a context set locally on a client.
>>
>> Can you explain the use case further?  Why are you exporting a
>> filesystem with security labeling enabled to a client that doesn't
>> understand all of the labels used within it?  Why wouldn't you just
>> disable NFSv4 security labeling and/or use a regular context= mount to
>> assign a single context to all files in the mount?
> 
> Client and server are two embedded devices. They are part of a bigger
> modular system and normally run the same SW version, so they understand
> each others NFSv4 SELinux labels. But during migration from one SW
> version to another a client and a server may run different versions for
> some time.
> 
> The immediate issue I'm trying to address is a migration between
> releases with and without SELinux policy. A client (with policy) gets
> initial SID labels from a server (without policy). Those labels are
> considered invalid, so files remain unlabeled. This also causes lots of
> errors from nfs_setsecurity() in dmesg.

Why are you enabling SELinux on the server without loading a policy? 
Are you concerned about denials on the server? For that, you could 
always run it permissive until you are confident you have a working policy.

Why are you enabling security_label in exports before you have a policy 
loaded?  It doesn't appear that will ever work, since nfsd asks the 
security module for the labels, not the local filesystem directly.

I understand that this doesn't fully address your use case, but it seems 
like you could avoid this particular situation altogether just by 
loading a policy (at which point your server can return actual contexts) 
or by removing security_label from your exports (at which point your 
server won't try returning contexts and the client will just apply the 
default for nfs) until you get to the point of loading a policy.

> Using 'context=' at mount point level is an option, but in a normal case
> when both sides have a policy we need more granular labels. It is
> possible to detect a case when a server doesn't have a policy and
> remount with 'context=', but this special case handling looks a bit
> ugly. Having something like 'defcontext' whould allow to avoid this
> special case and unconditionally assign default context to the
> mountpoint.
> 
> 'defcontext' may also help during migration between SELinux-enabled
> releases if a new release introduces labels that are not recognized by
> older release. In this case they can also fallback to defcontext.

This is the more interesting case.  And what would these defcontext 
values be?  A context that is generally accessible to all domains on the 
client?  Or one that is restricted to only unconfined domains?  Would it 
be fundamentally different from unlabeled?  Will it really differ 
per-mount, and if so, why?

> 
>>
>> To be clear, defcontext= doesn't work that way for local/FS_USE_XATTR
>> filesystems. The context specified by it is only used for:
>> 1) files that don't implement the xattr inode operations at all,
>> 2) files that lack a security.selinux xattr,
>> 3) the MLS portion of the context if it was missing (strictly as a
>> legacy compatibility mechanism for RHEL4 which predated the enabling of
>> the MLS field/logic).
>>
>> A file with a security.selinux xattr that is invalid under policy for
>> any reason other than a missing MLS field will be handled as having the
>> unlabeled context.
> 
> inode_doinit_with_dentry() invokes security_context_to_sid_default()
> that invokes security_context_to_sid_core() with 'force' flag. Won't
> sidtab_context_to_sid() in this case allocate a new SID for the invalid
> xattr context instead of leaving it unlabeled?

It will be treated as having the unlabeled context for access control 
purposes and that is what getxattr will return to userspace processes 
unless they have CAP_MAC_ADMIN and SELinux mac_admin permission, but 
internally SELinux will keep track of the original xattr context value 
and will later retry to map it upon a policy reload, switching over to 
using it for access control and getxattr if it becomes valid under a new 
policy.  That support was added to support scenarios where a package 
manager sets a file context before it has loaded the policy module that 
defines it, or scenarios where one is generating a filesystem image for 
another OS/release with different policy. That is likely something you 
need/want for NFS as well if you want the client to start using the 
context as soon as it becomes valid upon a policy update/reload and not 
have to wait for the inode to be evicted.

> 
>>
>> So this would be a divergence in semantics for defcontext= between
>> local/FS_USE_XATTR and NFS/FS_USE_NATIVE filesystems.
> 
> Yeah, I also didn't like this semantics mismatch. But from another point
> defcontext allows to assign a context to files that would otherwise be
> unlabeled. Something similar I'd like to achive for NFS.

Yes, I can see the appeal of it.  I guess the question is whether we 
can/should do it via defcontext= or via some new option, and if we do it 
via defcontext=, can/should we change the semantics for local/xattr 
filesystems to match?  Wondering how widely defcontext= is actually used.

> 
>>
>>>
>>> Signed-off-by: Taras Kondratiuk <[e-mail]>
>>> ---
>>>    security/selinux/hooks.c | 25 ++++++++++++++++++++++++-
>>>    1 file changed, 24 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index ad9a9b8e9979..f7debe798bf5 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -6598,7 +6598,30 @@ static void selinux_inode_invalidate_secctx(struct inode *inode)
>>>     */
>>>    static int selinux_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen)
>>>    {
>>> -     return selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX, ctx, ctxlen, 0);
>>> +     struct superblock_security_struct *sbsec;
>>> +     struct inode_security_struct *isec;
>>> +     int rc;
>>> +
>>> +     rc = selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX, ctx, ctxlen, 0);
>>
>> In this case, we likely don't gain much by reusing
>> selinux_inode_setsecurity() here and could just inline the relevant
>> portion of it if we were to make this change.  Logically they mean
>> different things.
>>
>>> +
>>> +     /*
>>> +      * In case of Native labeling with defcontext mount option fall back
>>> +      * to a default SID if received context is invalid.
>>> +      */
>>> +     if (rc == -EINVAL) {
>>> +             sbsec = inode->i_sb->s_security;
>>> +             if (sbsec->behavior == SECURITY_FS_USE_NATIVE &&
>>> +                 sbsec->flags & DEFCONTEXT_MNT) {
>>> +                     isec = inode->i_security;
>>> +                     if (!isec->initialized) {
>>> +                             isec->sclass = inode_mode_to_security_class(inode->i_mode);
>>> +                             isec->sid = sbsec->def_sid;
>>> +                             isec->initialized = 1;
>>> +                     }
>>> +                     rc = 0;
>>> +             }
>>> +     }
>>> +     return rc;
>>>    }
>>>    
>>>    /*
>>>
>>

Jann Horn via Selinux Sept. 20, 2018, 10:59 p.m. UTC | #5

Quoting Stephen Smalley (2018-09-20 07:49:12)
> On 09/19/2018 10:41 PM, Taras Kondratiuk wrote:
> > Quoting Stephen Smalley (2018-09-19 12:00:33)
> >> On 09/19/2018 12:52 PM, Taras Kondratiuk wrote:
> >>> When files on NFSv4 server are not properly labeled (label doesn't match
> >>> a policy on a client) they will end up with unlabeled_t type which is
> >>> too generic. We would like to be able to set a default context per
> >>> mount. 'defcontext' mount option looks like a nice solution, but it
> >>> doesn't seem to be fully implemented for native labeling. Default
> >>> context is stored, but is never used.
> >>>
> >>> The patch adds a fallback to a default context if a received context is
> >>> invalid. If the inode context is already initialized, then it is left
> >>> untouched to preserve a context set locally on a client.
> >>
> >> Can you explain the use case further?  Why are you exporting a
> >> filesystem with security labeling enabled to a client that doesn't
> >> understand all of the labels used within it?  Why wouldn't you just
> >> disable NFSv4 security labeling and/or use a regular context= mount to
> >> assign a single context to all files in the mount?
> > 
> > Client and server are two embedded devices. They are part of a bigger
> > modular system and normally run the same SW version, so they understand
> > each others NFSv4 SELinux labels. But during migration from one SW
> > version to another a client and a server may run different versions for
> > some time.
> > 
> > The immediate issue I'm trying to address is a migration between
> > releases with and without SELinux policy. A client (with policy) gets
> > initial SID labels from a server (without policy). Those labels are
> > considered invalid, so files remain unlabeled. This also causes lots of
> > errors from nfs_setsecurity() in dmesg.
> 
> Why are you enabling SELinux on the server without loading a policy? 
> Are you concerned about denials on the server? For that, you could 
> always run it permissive until you are confident you have a working policy.
> 
> Why are you enabling security_label in exports before you have a policy 
> loaded?  It doesn't appear that will ever work, since nfsd asks the 
> security module for the labels, not the local filesystem directly.
> 
> I understand that this doesn't fully address your use case, but it seems 
> like you could avoid this particular situation altogether just by 
> loading a policy (at which point your server can return actual contexts) 
> or by removing security_label from your exports (at which point your 
> server won't try returning contexts and the client will just apply the 
> default for nfs) until you get to the point of loading a policy.

It wasn't intentional configuration. Server is running v4.4.x kernel
that had security labels enabled by default for NFS 4.2. This was a bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1406885
Commit that introduced security_label 32ddd944a056 ("nfsd: opt in to
labeled nfs per export") appeared in v4.11 only. 

We hit this bug during migration to newer releases with SELinux, but the
older release is already in the field and we need to handle it.

> 
> > Using 'context=' at mount point level is an option, but in a normal case
> > when both sides have a policy we need more granular labels. It is
> > possible to detect a case when a server doesn't have a policy and
> > remount with 'context=', but this special case handling looks a bit
> > ugly. Having something like 'defcontext' whould allow to avoid this
> > special case and unconditionally assign default context to the
> > mountpoint.
> > 
> > 'defcontext' may also help during migration between SELinux-enabled
> > releases if a new release introduces labels that are not recognized by
> > older release. In this case they can also fallback to defcontext.
> 
> This is the more interesting case.  And what would these defcontext 
> values be?  A context that is generally accessible to all domains on the 
> client?  Or one that is restricted to only unconfined domains?  Would it 
> be fundamentally different from unlabeled?  Will it really differ 
> per-mount, and if so, why?

Defcontext will be a restricted label. For example a part of confined
domains can access an exported flash disk, but they can't access
unlabeled_t. In one release the flash may have a coarse labels:
u:r:flash_t        .
u:r:flash_t        some.file
u:r:flash_t        licence.file
u:r:flash_secret_t secret_dir

In the next release licence.file may get a more granular label
(licence_t), but the older release still need to be able to read the
file via NFS. With defcontext=u:r:flash_t it will be able to do it.

In our case defcontext will usually match a label or export's root
directory, so it needs to be set per-mount.

> 
> > 
> >>
> >> To be clear, defcontext= doesn't work that way for local/FS_USE_XATTR
> >> filesystems. The context specified by it is only used for:
> >> 1) files that don't implement the xattr inode operations at all,
> >> 2) files that lack a security.selinux xattr,
> >> 3) the MLS portion of the context if it was missing (strictly as a
> >> legacy compatibility mechanism for RHEL4 which predated the enabling of
> >> the MLS field/logic).
> >>
> >> A file with a security.selinux xattr that is invalid under policy for
> >> any reason other than a missing MLS field will be handled as having the
> >> unlabeled context.
> > 
> > inode_doinit_with_dentry() invokes security_context_to_sid_default()
> > that invokes security_context_to_sid_core() with 'force' flag. Won't
> > sidtab_context_to_sid() in this case allocate a new SID for the invalid
> > xattr context instead of leaving it unlabeled?
> 
> It will be treated as having the unlabeled context for access control 
> purposes and that is what getxattr will return to userspace processes 
> unless they have CAP_MAC_ADMIN and SELinux mac_admin permission, but 
> internally SELinux will keep track of the original xattr context value 
> and will later retry to map it upon a policy reload, switching over to 
> using it for access control and getxattr if it becomes valid under a new 
> policy.  That support was added to support scenarios where a package 
> manager sets a file context before it has loaded the policy module that 
> defines it, or scenarios where one is generating a filesystem image for 
> another OS/release with different policy. That is likely something you 
> need/want for NFS as well if you want the client to start using the 
> context as soon as it becomes valid upon a policy update/reload and not 
> have to wait for the inode to be evicted.

So now handling of invalid labels is different for xattrs (labels are
preserved) and native (labels are discarded). Maybe it is worth to align
this behavior. It should make introduction of defcontext for native
easier.

> 
> > 
> >>
> >> So this would be a divergence in semantics for defcontext= between
> >> local/FS_USE_XATTR and NFS/FS_USE_NATIVE filesystems.
> > 
> > Yeah, I also didn't like this semantics mismatch. But from another point
> > defcontext allows to assign a context to files that would otherwise be
> > unlabeled. Something similar I'd like to achive for NFS.
> 
> Yes, I can see the appeal of it.  I guess the question is whether we 
> can/should do it via defcontext= or via some new option, and if we do it 
> via defcontext=, can/should we change the semantics for local/xattr 
> filesystems to match?  Wondering how widely defcontext= is actually used.

Is the current behavior of defcontext for xattrs intentional or it is a
side effect? Honestly it is a bit confusing. Without defcontext really
unlabeled files and files with invalid labels are treated as unlabeled.
Can a user without MAC_ADMIN even distinguish them? With defcontext only
really unlabeled files get default context, but invalid labels still
remain unlabeled.

IMO it would be more consistent if defcontext cover all "unlabeled"
groups. It seems unlikely to me that somebody who currently uses
defcontext can somehow rely on mapping invalid labels to unlabeled
instead of default context.

> 
> > 
> >>
> >>>
> >>> Signed-off-by: Taras Kondratiuk <[e-mail]>
> >>> ---
> >>>    security/selinux/hooks.c | 25 ++++++++++++++++++++++++-
> >>>    1 file changed, 24 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >>> index ad9a9b8e9979..f7debe798bf5 100644
> >>> --- a/security/selinux/hooks.c
> >>> +++ b/security/selinux/hooks.c
> >>> @@ -6598,7 +6598,30 @@ static void selinux_inode_invalidate_secctx(struct inode *inode)
> >>>     */
> >>>    static int selinux_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen)
> >>>    {
> >>> -     return selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX, ctx, ctxlen, 0);
> >>> +     struct superblock_security_struct *sbsec;
> >>> +     struct inode_security_struct *isec;
> >>> +     int rc;
> >>> +
> >>> +     rc = selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX, ctx, ctxlen, 0);
> >>
> >> In this case, we likely don't gain much by reusing
> >> selinux_inode_setsecurity() here and could just inline the relevant
> >> portion of it if we were to make this change.  Logically they mean
> >> different things.
> >>
> >>> +
> >>> +     /*
> >>> +      * In case of Native labeling with defcontext mount option fall back
> >>> +      * to a default SID if received context is invalid.
> >>> +      */
> >>> +     if (rc == -EINVAL) {
> >>> +             sbsec = inode->i_sb->s_security;
> >>> +             if (sbsec->behavior == SECURITY_FS_USE_NATIVE &&
> >>> +                 sbsec->flags & DEFCONTEXT_MNT) {
> >>> +                     isec = inode->i_security;
> >>> +                     if (!isec->initialized) {
> >>> +                             isec->sclass = inode_mode_to_security_class(inode->i_mode);
> >>> +                             isec->sid = sbsec->def_sid;
> >>> +                             isec->initialized = 1;
> >>> +                     }
> >>> +                     rc = 0;
> >>> +             }
> >>> +     }
> >>> +     return rc;
> >>>    }
> >>>    
> >>>    /*
> >>>
> >>
>

Stephen Smalley Sept. 21, 2018, 2:40 p.m. UTC | #6

On 09/20/2018 06:59 PM, Taras Kondratiuk wrote:
> Quoting Stephen Smalley (2018-09-20 07:49:12)
>> On 09/19/2018 10:41 PM, Taras Kondratiuk wrote:
>>> Quoting Stephen Smalley (2018-09-19 12:00:33)
>>>> On 09/19/2018 12:52 PM, Taras Kondratiuk wrote:
>>>>> When files on NFSv4 server are not properly labeled (label doesn't match
>>>>> a policy on a client) they will end up with unlabeled_t type which is
>>>>> too generic. We would like to be able to set a default context per
>>>>> mount. 'defcontext' mount option looks like a nice solution, but it
>>>>> doesn't seem to be fully implemented for native labeling. Default
>>>>> context is stored, but is never used.
>>>>>
>>>>> The patch adds a fallback to a default context if a received context is
>>>>> invalid. If the inode context is already initialized, then it is left
>>>>> untouched to preserve a context set locally on a client.
>>>>
>>>> Can you explain the use case further?  Why are you exporting a
>>>> filesystem with security labeling enabled to a client that doesn't
>>>> understand all of the labels used within it?  Why wouldn't you just
>>>> disable NFSv4 security labeling and/or use a regular context= mount to
>>>> assign a single context to all files in the mount?
>>>
>>> Client and server are two embedded devices. They are part of a bigger
>>> modular system and normally run the same SW version, so they understand
>>> each others NFSv4 SELinux labels. But during migration from one SW
>>> version to another a client and a server may run different versions for
>>> some time.
>>>
>>> The immediate issue I'm trying to address is a migration between
>>> releases with and without SELinux policy. A client (with policy) gets
>>> initial SID labels from a server (without policy). Those labels are
>>> considered invalid, so files remain unlabeled. This also causes lots of
>>> errors from nfs_setsecurity() in dmesg.
>>
>> Why are you enabling SELinux on the server without loading a policy?
>> Are you concerned about denials on the server? For that, you could
>> always run it permissive until you are confident you have a working policy.
>>
>> Why are you enabling security_label in exports before you have a policy
>> loaded?  It doesn't appear that will ever work, since nfsd asks the
>> security module for the labels, not the local filesystem directly.
>>
>> I understand that this doesn't fully address your use case, but it seems
>> like you could avoid this particular situation altogether just by
>> loading a policy (at which point your server can return actual contexts)
>> or by removing security_label from your exports (at which point your
>> server won't try returning contexts and the client will just apply the
>> default for nfs) until you get to the point of loading a policy.
> 
> It wasn't intentional configuration. Server is running v4.4.x kernel
> that had security labels enabled by default for NFS 4.2. This was a bug:
> https://bugzilla.redhat.com/show_bug.cgi?id=1406885
> Commit that introduced security_label 32ddd944a056 ("nfsd: opt in to
> labeled nfs per export") appeared in v4.11 only.
> 
> We hit this bug during migration to newer releases with SELinux, but the
> older release is already in the field and we need to handle it.

Ok, I understand the kernel bug, but not why your servers are in a state 
where SELinux is enabled but no policy is loaded.  That is not a 
well-tested code path aside from early system initialization prior to 
first policy load by systemd/init.  You would be better off either 
disabling SELinux on the servers entirely (thereby automatically 
disabling NFSv4.2 security labeling) or installing/loading a policy on 
the servers (thereby enabling them to produce valid security labels for 
the client at least to the extent that they agree on policy).  If you 
are concerned about denials on the server, then you could always leave 
the servers permissive initially and collect audit logs until you are 
confident your policy is adequate.

>>
>>>
>>>>
>>>> To be clear, defcontext= doesn't work that way for local/FS_USE_XATTR
>>>> filesystems. The context specified by it is only used for:
>>>> 1) files that don't implement the xattr inode operations at all,
>>>> 2) files that lack a security.selinux xattr,
>>>> 3) the MLS portion of the context if it was missing (strictly as a
>>>> legacy compatibility mechanism for RHEL4 which predated the enabling of
>>>> the MLS field/logic).
>>>>
>>>> A file with a security.selinux xattr that is invalid under policy for
>>>> any reason other than a missing MLS field will be handled as having the
>>>> unlabeled context.
>>>
>>> inode_doinit_with_dentry() invokes security_context_to_sid_default()
>>> that invokes security_context_to_sid_core() with 'force' flag. Won't
>>> sidtab_context_to_sid() in this case allocate a new SID for the invalid
>>> xattr context instead of leaving it unlabeled?
>>
>> It will be treated as having the unlabeled context for access control
>> purposes and that is what getxattr will return to userspace processes
>> unless they have CAP_MAC_ADMIN and SELinux mac_admin permission, but
>> internally SELinux will keep track of the original xattr context value
>> and will later retry to map it upon a policy reload, switching over to
>> using it for access control and getxattr if it becomes valid under a new
>> policy.  That support was added to support scenarios where a package
>> manager sets a file context before it has loaded the policy module that
>> defines it, or scenarios where one is generating a filesystem image for
>> another OS/release with different policy. That is likely something you
>> need/want for NFS as well if you want the client to start using the
>> context as soon as it becomes valid upon a policy update/reload and not
>> have to wait for the inode to be evicted.
> 
> So now handling of invalid labels is different for xattrs (labels are
> preserved) and native (labels are discarded). Maybe it is worth to align
> this behavior. It should make introduction of defcontext for native
> easier.

Perhaps.  However, this handling of invalid labels is in conflict with 
your suggested behavior for defcontext=.  If we set the inode sid to the 
superblock def_sid on an invalid context, then we lose the association 
to the original context value. The support for deferred mapping of 
contexts requires allocating a new SID for the invalid context and 
storing that SID in the inode, so that if the context later becomes 
valid upon a policy update/reload, the inode SID will refer to the now 
valid context.  To combine the two, we would need 
security_context_to_sid_core() to save the def_sid in the context 
structure for invalid contexts, and change sidtab_search_core() to use 
that value instead of SECINITSID_UNLABELED for invalid SIDs.  Then the 
inode would be treated as having the defcontext for access control and 
getxattr() w/o CAP_MAC_ADMIN purposes, but a subsequent policy 
update/reload that makes the context valid would automatically cause 
subsequent accesses to the inode to start using the original context 
value for access control and getxattr() purposes.  I think that's the 
behavior you want.

>>>> So this would be a divergence in semantics for defcontext= between
>>>> local/FS_USE_XATTR and NFS/FS_USE_NATIVE filesystems.
>>>
>>> Yeah, I also didn't like this semantics mismatch. But from another point
>>> defcontext allows to assign a context to files that would otherwise be
>>> unlabeled. Something similar I'd like to achive for NFS.
>>
>> Yes, I can see the appeal of it.  I guess the question is whether we
>> can/should do it via defcontext= or via some new option, and if we do it
>> via defcontext=, can/should we change the semantics for local/xattr
>> filesystems to match?  Wondering how widely defcontext= is actually used.
> 
> Is the current behavior of defcontext for xattrs intentional or it is a
> side effect? Honestly it is a bit confusing. Without defcontext really
> unlabeled files and files with invalid labels are treated as unlabeled.
> Can a user without MAC_ADMIN even distinguish them? With defcontext only
> really unlabeled files get default context, but invalid labels still
> remain unlabeled.

It was intentional at the time, but the history is somewhat convoluted. 
The two cases are actually distinguished by different initial SIDs 
(SECINITSID_FILE vs SECINITSID_UNLABELED) and used to have different 
types in policy (file_t vs unlabeled_t), but this distinction has been 
coalesced in modern policies (where file_t is an alias of unlabeled_t).
IIRC, the original distinction was to support migration from non-SELinux 
to SELinux since filesystems were initially unlabeled. defcontext= was 
originally just a way of specifying per-mount alternatives to the global 
policy definition of the context for SECINITSID_FILE.

> 
> IMO it would be more consistent if defcontext cover all "unlabeled"
> groups. It seems unlikely to me that somebody who currently uses
> defcontext can somehow rely on mapping invalid labels to unlabeled
> instead of default context.

Yes, and that seems more consistent with the current documentation in 
the mount man page for defcontext=.

I'd be inclined to change selinux_inode_notifysecctx() to call 
security_context_to_sid_default() directly instead of using 
selinux_inode_setsecurity() and change security_context_to_sid_core() 
and sidtab_search_core() as suggested above to save and use the def_sid 
instead of SECINITSID_UNLABELED always (initializing the context def_sid 
to SECINITSID_UNLABELED as the default).  selinux_inode_setsecurity() we 
should leave unchanged, or if we change it at all, it should be more 
like the handling in selinux_inode_setxattr().  The notifysecctx hook is 
invoked by the filesystem to notify the security module of the file's 
existing security context, so in that case we always want the _default 
behavior, whereas the setsecurity hook is invoked by the vfs or the 
filesystem to set the security context of a file to a new value, so in 
that case we would only use the _force interface if the caller had 
CAP_MAC_ADMIN.

Paul, what say you?  NB This would be a user-visible behavior change for 
mounts specifying defcontext= on xattr filesystems; files with invalid 
contexts will then show up with the defcontext value instead of the 
unlabeled context.  If that's too risky, then we'd need a flag or 
something to security_context_to_sid_default() to distinguish the 
behaviors and only set it when called from selinux_inode_notifysecctx().

Jann Horn via Selinux Sept. 24, 2018, 9:17 p.m. UTC | #7

Quoting Stephen Smalley (2018-09-21 07:40:58)
> On 09/20/2018 06:59 PM, Taras Kondratiuk wrote:
> > Quoting Stephen Smalley (2018-09-20 07:49:12)
> >> On 09/19/2018 10:41 PM, Taras Kondratiuk wrote:
> >>> Quoting Stephen Smalley (2018-09-19 12:00:33)
> >>>> On 09/19/2018 12:52 PM, Taras Kondratiuk wrote:
> >>>>> When files on NFSv4 server are not properly labeled (label doesn't match
> >>>>> a policy on a client) they will end up with unlabeled_t type which is
> >>>>> too generic. We would like to be able to set a default context per
> >>>>> mount. 'defcontext' mount option looks like a nice solution, but it
> >>>>> doesn't seem to be fully implemented for native labeling. Default
> >>>>> context is stored, but is never used.
> >>>>>
> >>>>> The patch adds a fallback to a default context if a received context is
> >>>>> invalid. If the inode context is already initialized, then it is left
> >>>>> untouched to preserve a context set locally on a client.
> >>>>
> >>>> Can you explain the use case further?  Why are you exporting a
> >>>> filesystem with security labeling enabled to a client that doesn't
> >>>> understand all of the labels used within it?  Why wouldn't you just
> >>>> disable NFSv4 security labeling and/or use a regular context= mount to
> >>>> assign a single context to all files in the mount?
> >>>
> >>> Client and server are two embedded devices. They are part of a bigger
> >>> modular system and normally run the same SW version, so they understand
> >>> each others NFSv4 SELinux labels. But during migration from one SW
> >>> version to another a client and a server may run different versions for
> >>> some time.
> >>>
> >>> The immediate issue I'm trying to address is a migration between
> >>> releases with and without SELinux policy. A client (with policy) gets
> >>> initial SID labels from a server (without policy). Those labels are
> >>> considered invalid, so files remain unlabeled. This also causes lots of
> >>> errors from nfs_setsecurity() in dmesg.
> >>
> >> Why are you enabling SELinux on the server without loading a policy?
> >> Are you concerned about denials on the server? For that, you could
> >> always run it permissive until you are confident you have a working policy.
> >>
> >> Why are you enabling security_label in exports before you have a policy
> >> loaded?  It doesn't appear that will ever work, since nfsd asks the
> >> security module for the labels, not the local filesystem directly.
> >>
> >> I understand that this doesn't fully address your use case, but it seems
> >> like you could avoid this particular situation altogether just by
> >> loading a policy (at which point your server can return actual contexts)
> >> or by removing security_label from your exports (at which point your
> >> server won't try returning contexts and the client will just apply the
> >> default for nfs) until you get to the point of loading a policy.
> > 
> > It wasn't intentional configuration. Server is running v4.4.x kernel
> > that had security labels enabled by default for NFS 4.2. This was a bug:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1406885
> > Commit that introduced security_label 32ddd944a056 ("nfsd: opt in to
> > labeled nfs per export") appeared in v4.11 only.
> > 
> > We hit this bug during migration to newer releases with SELinux, but the
> > older release is already in the field and we need to handle it.
> 
> Ok, I understand the kernel bug, but not why your servers are in a state 
> where SELinux is enabled but no policy is loaded.  That is not a 
> well-tested code path aside from early system initialization prior to 
> first policy load by systemd/init.  You would be better off either 
> disabling SELinux on the servers entirely (thereby automatically 
> disabling NFSv4.2 security labeling) or installing/loading a policy on 
> the servers (thereby enabling them to produce valid security labels for 
> the client at least to the extent that they agree on policy).  If you 
> are concerned about denials on the server, then you could always leave 
> the servers permissive initially and collect audit logs until you are 
> confident your policy is adequate.

It wasn't intentional either. The same kernel configuration is used
accross several products. We roll out SELinux for products one by one,
so some products don't have SELinux policy and /etc/selinux/config yet
while SELinux kernel config is enabled. In hindsight we should have
explicitly disabled SELinux in /etc/selinux/config for those products.

Since it is not a well-tested code I'm wondering shouldn't systemd/init
be disabling SELinux if config file or policy is missing?

> 
> >>
> >>>
> >>>>
> >>>> To be clear, defcontext= doesn't work that way for local/FS_USE_XATTR
> >>>> filesystems. The context specified by it is only used for:
> >>>> 1) files that don't implement the xattr inode operations at all,
> >>>> 2) files that lack a security.selinux xattr,
> >>>> 3) the MLS portion of the context if it was missing (strictly as a
> >>>> legacy compatibility mechanism for RHEL4 which predated the enabling of
> >>>> the MLS field/logic).
> >>>>
> >>>> A file with a security.selinux xattr that is invalid under policy for
> >>>> any reason other than a missing MLS field will be handled as having the
> >>>> unlabeled context.
> >>>
> >>> inode_doinit_with_dentry() invokes security_context_to_sid_default()
> >>> that invokes security_context_to_sid_core() with 'force' flag. Won't
> >>> sidtab_context_to_sid() in this case allocate a new SID for the invalid
> >>> xattr context instead of leaving it unlabeled?
> >>
> >> It will be treated as having the unlabeled context for access control
> >> purposes and that is what getxattr will return to userspace processes
> >> unless they have CAP_MAC_ADMIN and SELinux mac_admin permission, but
> >> internally SELinux will keep track of the original xattr context value
> >> and will later retry to map it upon a policy reload, switching over to
> >> using it for access control and getxattr if it becomes valid under a new
> >> policy.  That support was added to support scenarios where a package
> >> manager sets a file context before it has loaded the policy module that
> >> defines it, or scenarios where one is generating a filesystem image for
> >> another OS/release with different policy. That is likely something you
> >> need/want for NFS as well if you want the client to start using the
> >> context as soon as it becomes valid upon a policy update/reload and not
> >> have to wait for the inode to be evicted.
> > 
> > So now handling of invalid labels is different for xattrs (labels are
> > preserved) and native (labels are discarded). Maybe it is worth to align
> > this behavior. It should make introduction of defcontext for native
> > easier.
> 
> Perhaps.  However, this handling of invalid labels is in conflict with 
> your suggested behavior for defcontext=.  If we set the inode sid to the 
> superblock def_sid on an invalid context, then we lose the association 
> to the original context value. The support for deferred mapping of 
> contexts requires allocating a new SID for the invalid context and 
> storing that SID in the inode, so that if the context later becomes 
> valid upon a policy update/reload, the inode SID will refer to the now 
> valid context.  To combine the two, we would need 
> security_context_to_sid_core() to save the def_sid in the context 
> structure for invalid contexts, and change sidtab_search_core() to use 
> that value instead of SECINITSID_UNLABELED for invalid SIDs.  Then the 
> inode would be treated as having the defcontext for access control and 
> getxattr() w/o CAP_MAC_ADMIN purposes, but a subsequent policy 
> update/reload that makes the context valid would automatically cause 
> subsequent accesses to the inode to start using the original context 
> value for access control and getxattr() purposes.  I think that's the 
> behavior you want.

Right, that's exactly it.

> 
> >>>> So this would be a divergence in semantics for defcontext= between
> >>>> local/FS_USE_XATTR and NFS/FS_USE_NATIVE filesystems.
> >>>
> >>> Yeah, I also didn't like this semantics mismatch. But from another point
> >>> defcontext allows to assign a context to files that would otherwise be
> >>> unlabeled. Something similar I'd like to achive for NFS.
> >>
> >> Yes, I can see the appeal of it.  I guess the question is whether we
> >> can/should do it via defcontext= or via some new option, and if we do it
> >> via defcontext=, can/should we change the semantics for local/xattr
> >> filesystems to match?  Wondering how widely defcontext= is actually used.
> > 
> > Is the current behavior of defcontext for xattrs intentional or it is a
> > side effect? Honestly it is a bit confusing. Without defcontext really
> > unlabeled files and files with invalid labels are treated as unlabeled.
> > Can a user without MAC_ADMIN even distinguish them? With defcontext only
> > really unlabeled files get default context, but invalid labels still
> > remain unlabeled.
> 
> It was intentional at the time, but the history is somewhat convoluted. 
> The two cases are actually distinguished by different initial SIDs 
> (SECINITSID_FILE vs SECINITSID_UNLABELED) and used to have different 
> types in policy (file_t vs unlabeled_t), but this distinction has been 
> coalesced in modern policies (where file_t is an alias of unlabeled_t).
> IIRC, the original distinction was to support migration from non-SELinux 
> to SELinux since filesystems were initially unlabeled. defcontext= was 
> originally just a way of specifying per-mount alternatives to the global 
> policy definition of the context for SECINITSID_FILE.
> 
> > 
> > IMO it would be more consistent if defcontext cover all "unlabeled"
> > groups. It seems unlikely to me that somebody who currently uses
> > defcontext can somehow rely on mapping invalid labels to unlabeled
> > instead of default context.
> 
> Yes, and that seems more consistent with the current documentation in 
> the mount man page for defcontext=.
> 
> I'd be inclined to change selinux_inode_notifysecctx() to call 
> security_context_to_sid_default() directly instead of using 
> selinux_inode_setsecurity() and change security_context_to_sid_core() 
> and sidtab_search_core() as suggested above to save and use the def_sid 
> instead of SECINITSID_UNLABELED always (initializing the context def_sid 
> to SECINITSID_UNLABELED as the default).  selinux_inode_setsecurity() we 
> should leave unchanged, or if we change it at all, it should be more 
> like the handling in selinux_inode_setxattr().  The notifysecctx hook is 
> invoked by the filesystem to notify the security module of the file's 
> existing security context, so in that case we always want the _default 
> behavior, whereas the setsecurity hook is invoked by the vfs or the 
> filesystem to set the security context of a file to a new value, so in 
> that case we would only use the _force interface if the caller had 
> CAP_MAC_ADMIN.
> 
> Paul, what say you?  NB This would be a user-visible behavior change for 
> mounts specifying defcontext= on xattr filesystems; files with invalid 
> contexts will then show up with the defcontext value instead of the 
> unlabeled context.  If that's too risky, then we'd need a flag or 
> something to security_context_to_sid_default() to distinguish the 
> behaviors and only set it when called from selinux_inode_notifysecctx().

Paul, if you are OK with this approach, then I'll prepare a patch.

Paul Moore Sept. 25, 2018, 3:46 a.m. UTC | #8

On Fri, Sep 21, 2018 at 10:39 AM Stephen Smalley <[e-mail]> wrote:
> On 09/20/2018 06:59 PM, Taras Kondratiuk wrote:
> > Quoting Stephen Smalley (2018-09-20 07:49:12)
> >> On 09/19/2018 10:41 PM, Taras Kondratiuk wrote:
> >>> Quoting Stephen Smalley (2018-09-19 12:00:33)
> >>>> On 09/19/2018 12:52 PM, Taras Kondratiuk wrote:

...

> > IMO it would be more consistent if defcontext cover all "unlabeled"
> > groups. It seems unlikely to me that somebody who currently uses
> > defcontext can somehow rely on mapping invalid labels to unlabeled
> > instead of default context.
>
> Yes, and that seems more consistent with the current documentation in
> the mount man page for defcontext=.
>
> I'd be inclined to change selinux_inode_notifysecctx() to call
> security_context_to_sid_default() directly instead of using
> selinux_inode_setsecurity() and change security_context_to_sid_core()
> and sidtab_search_core() as suggested above to save and use the def_sid
> instead of SECINITSID_UNLABELED always (initializing the context def_sid
> to SECINITSID_UNLABELED as the default).  selinux_inode_setsecurity() we
> should leave unchanged, or if we change it at all, it should be more
> like the handling in selinux_inode_setxattr().  The notifysecctx hook is
> invoked by the filesystem to notify the security module of the file's
> existing security context, so in that case we always want the _default
> behavior, whereas the setsecurity hook is invoked by the vfs or the
> filesystem to set the security context of a file to a new value, so in
> that case we would only use the _force interface if the caller had
> CAP_MAC_ADMIN.
>
> Paul, what say you?  NB This would be a user-visible behavior change for
> mounts specifying defcontext= on xattr filesystems; files with invalid
> contexts will then show up with the defcontext value instead of the
> unlabeled context.  If that's too risky, then we'd need a flag or
> something to security_context_to_sid_default() to distinguish the
> behaviors and only set it when called from selinux_inode_notifysecctx().

Visible changes like this are always worrisome, even though I think it
is safe to assume that the defcontext option is not widely used.  I'd
feel much better if this change was opt-in.

Which brings about it's own problems.  We have the policy capability
functionality, but that is likely a poor fit for this as the policy
capabilities are usually controlled by the Linux distribution while
the mount options are set by the system's administrator when the
filesystem is mounted.  We could add a toggle somewhere in selinuxfs,
but I really dislike that idea, and would prefer to find a different
solution if possible.  I'm not sure how much flak we would get for
introducing a new mount option, but perhaps that is the best way to
handle this: defcontext would continue to behave as it does now, but
new option X would behave as mentioned in this thread.

Thoughts?

Jann Horn via Selinux Sept. 25, 2018, 5:45 a.m. UTC | #9

Quoting Paul Moore (2018-09-24 20:46:57)
> On Fri, Sep 21, 2018 at 10:39 AM Stephen Smalley <[e-mail]> wrote:
> > On 09/20/2018 06:59 PM, Taras Kondratiuk wrote:
> > > Quoting Stephen Smalley (2018-09-20 07:49:12)
> > >> On 09/19/2018 10:41 PM, Taras Kondratiuk wrote:
> > >>> Quoting Stephen Smalley (2018-09-19 12:00:33)
> > >>>> On 09/19/2018 12:52 PM, Taras Kondratiuk wrote:
> 
> ...
> 
> > > IMO it would be more consistent if defcontext cover all "unlabeled"
> > > groups. It seems unlikely to me that somebody who currently uses
> > > defcontext can somehow rely on mapping invalid labels to unlabeled
> > > instead of default context.
> >
> > Yes, and that seems more consistent with the current documentation in
> > the mount man page for defcontext=.
> >
> > I'd be inclined to change selinux_inode_notifysecctx() to call
> > security_context_to_sid_default() directly instead of using
> > selinux_inode_setsecurity() and change security_context_to_sid_core()
> > and sidtab_search_core() as suggested above to save and use the def_sid
> > instead of SECINITSID_UNLABELED always (initializing the context def_sid
> > to SECINITSID_UNLABELED as the default).  selinux_inode_setsecurity() we
> > should leave unchanged, or if we change it at all, it should be more
> > like the handling in selinux_inode_setxattr().  The notifysecctx hook is
> > invoked by the filesystem to notify the security module of the file's
> > existing security context, so in that case we always want the _default
> > behavior, whereas the setsecurity hook is invoked by the vfs or the
> > filesystem to set the security context of a file to a new value, so in
> > that case we would only use the _force interface if the caller had
> > CAP_MAC_ADMIN.
> >
> > Paul, what say you?  NB This would be a user-visible behavior change for
> > mounts specifying defcontext= on xattr filesystems; files with invalid
> > contexts will then show up with the defcontext value instead of the
> > unlabeled context.  If that's too risky, then we'd need a flag or
> > something to security_context_to_sid_default() to distinguish the
> > behaviors and only set it when called from selinux_inode_notifysecctx().
> 
> Visible changes like this are always worrisome, even though I think it
> is safe to assume that the defcontext option is not widely used.  I'd
> feel much better if this change was opt-in.
> 
> Which brings about it's own problems.  We have the policy capability
> functionality, but that is likely a poor fit for this as the policy
> capabilities are usually controlled by the Linux distribution while
> the mount options are set by the system's administrator when the
> filesystem is mounted.  We could add a toggle somewhere in selinuxfs,
> but I really dislike that idea, and would prefer to find a different
> solution if possible.  I'm not sure how much flak we would get for
> introducing a new mount option, but perhaps that is the best way to
> handle this: defcontext would continue to behave as it does now, but
> new option X would behave as mentioned in this thread.
> 
> Thoughts?

The new option X will also mean "default context", so it will be pretty
hard to come up with a different but a sensible name. I'm afraid that
having two options with hardly distinguishable semantics will be very
confusing.

What about a kernel config option that modifies the behavior of
defcontext?

Stephen Smalley Sept. 25, 2018, 2 p.m. UTC | #10

On 09/25/2018 01:45 AM, Taras Kondratiuk wrote:
> Quoting Paul Moore (2018-09-24 20:46:57)
>> On Fri, Sep 21, 2018 at 10:39 AM Stephen Smalley <[e-mail]> wrote:
>>> On 09/20/2018 06:59 PM, Taras Kondratiuk wrote:
>>>> Quoting Stephen Smalley (2018-09-20 07:49:12)
>>>>> On 09/19/2018 10:41 PM, Taras Kondratiuk wrote:
>>>>>> Quoting Stephen Smalley (2018-09-19 12:00:33)
>>>>>>> On 09/19/2018 12:52 PM, Taras Kondratiuk wrote:
>>
>> ...
>>
>>>> IMO it would be more consistent if defcontext cover all "unlabeled"
>>>> groups. It seems unlikely to me that somebody who currently uses
>>>> defcontext can somehow rely on mapping invalid labels to unlabeled
>>>> instead of default context.
>>>
>>> Yes, and that seems more consistent with the current documentation in
>>> the mount man page for defcontext=.
>>>
>>> I'd be inclined to change selinux_inode_notifysecctx() to call
>>> security_context_to_sid_default() directly instead of using
>>> selinux_inode_setsecurity() and change security_context_to_sid_core()
>>> and sidtab_search_core() as suggested above to save and use the def_sid
>>> instead of SECINITSID_UNLABELED always (initializing the context def_sid
>>> to SECINITSID_UNLABELED as the default).  selinux_inode_setsecurity() we
>>> should leave unchanged, or if we change it at all, it should be more
>>> like the handling in selinux_inode_setxattr().  The notifysecctx hook is
>>> invoked by the filesystem to notify the security module of the file's
>>> existing security context, so in that case we always want the _default
>>> behavior, whereas the setsecurity hook is invoked by the vfs or the
>>> filesystem to set the security context of a file to a new value, so in
>>> that case we would only use the _force interface if the caller had
>>> CAP_MAC_ADMIN.
>>>
>>> Paul, what say you?  NB This would be a user-visible behavior change for
>>> mounts specifying defcontext= on xattr filesystems; files with invalid
>>> contexts will then show up with the defcontext value instead of the
>>> unlabeled context.  If that's too risky, then we'd need a flag or
>>> something to security_context_to_sid_default() to distinguish the
>>> behaviors and only set it when called from selinux_inode_notifysecctx().
>>
>> Visible changes like this are always worrisome, even though I think it
>> is safe to assume that the defcontext option is not widely used.  I'd
>> feel much better if this change was opt-in.
>>
>> Which brings about it's own problems.  We have the policy capability
>> functionality, but that is likely a poor fit for this as the policy
>> capabilities are usually controlled by the Linux distribution while
>> the mount options are set by the system's administrator when the
>> filesystem is mounted.  We could add a toggle somewhere in selinuxfs,
>> but I really dislike that idea, and would prefer to find a different
>> solution if possible.  I'm not sure how much flak we would get for
>> introducing a new mount option, but perhaps that is the best way to
>> handle this: defcontext would continue to behave as it does now, but
>> new option X would behave as mentioned in this thread.
>>
>> Thoughts?
> 
> The new option X will also mean "default context", so it will be pretty
> hard to come up with a different but a sensible name. I'm afraid that
> having two options with hardly distinguishable semantics will be very
> confusing.
> 
> What about a kernel config option that modifies the behavior of
> defcontext?

First, the existing documentation for defcontext= leaves open the door 
to the proposed new behavior.  From mount(8):
"You can set the default security context for  unlabeled  files  using 
defcontext= option.  This overrides the value set for unlabeled files in 
the policy and requires a filesystem that supports xattr labeling."

"Unlabeled files" could just mean files without any xattr, or it could 
mean all files that either lack an xattr or have an invalid one under 
the policy, since both sets of files are currently mapped to the 
unlabeled context.

Second, under what conditions would this situation break existing 
userspace?  The admin would have had to mount an xattr filesystem with 
defcontext= specified, and that filesystem would have to both contain 
files without any xattrs and files with invalid ones (otherwise how they 
are treated by the kernel is irrelevant), and the policy would need to 
distinguish access to files without xattrs vs files with invalid ones. 
Seems unlikely.

Third, the fact that policy maintainers remapped both SECINITSID_FILE 
(the default value for defcontext) and SECINITSID_UNLABELED (used for 
invalid contexts) to the unlabeled context (getting rid of file_t as a 
separate type, aliased to unlabeled_t) long ago suggests that there is 
no meaningful difference there.

I'm inclined to just change the behavior for defcontext= unconditionally 
and have it apply to both native and xattr labeling.  If that's a no-go, 
then the simplest solution is to just leave defcontext= behavior 
unchanged for xattr labeling and only implement the new semantics for 
native labeling.  That's just a matter of adding a flag to 
security_context_to_sid_default() and only setting it when calling from 
selinux_inode_notifysecctx().

Paul Moore Sept. 25, 2018, 3:41 p.m. UTC | #11

On Tue, Sep 25, 2018 at 1:45 AM Taras Kondratiuk <[e-mail]> wrote:
> Quoting Paul Moore (2018-09-24 20:46:57)
> > On Fri, Sep 21, 2018 at 10:39 AM Stephen Smalley <[e-mail]> wrote:
> > > On 09/20/2018 06:59 PM, Taras Kondratiuk wrote:
> > > > Quoting Stephen Smalley (2018-09-20 07:49:12)
> > > >> On 09/19/2018 10:41 PM, Taras Kondratiuk wrote:
> > > >>> Quoting Stephen Smalley (2018-09-19 12:00:33)
> > > >>>> On 09/19/2018 12:52 PM, Taras Kondratiuk wrote:
> >
> > ...
> >
> > > > IMO it would be more consistent if defcontext cover all "unlabeled"
> > > > groups. It seems unlikely to me that somebody who currently uses
> > > > defcontext can somehow rely on mapping invalid labels to unlabeled
> > > > instead of default context.
> > >
> > > Yes, and that seems more consistent with the current documentation in
> > > the mount man page for defcontext=.
> > >
> > > I'd be inclined to change selinux_inode_notifysecctx() to call
> > > security_context_to_sid_default() directly instead of using
> > > selinux_inode_setsecurity() and change security_context_to_sid_core()
> > > and sidtab_search_core() as suggested above to save and use the def_sid
> > > instead of SECINITSID_UNLABELED always (initializing the context def_sid
> > > to SECINITSID_UNLABELED as the default).  selinux_inode_setsecurity() we
> > > should leave unchanged, or if we change it at all, it should be more
> > > like the handling in selinux_inode_setxattr().  The notifysecctx hook is
> > > invoked by the filesystem to notify the security module of the file's
> > > existing security context, so in that case we always want the _default
> > > behavior, whereas the setsecurity hook is invoked by the vfs or the
> > > filesystem to set the security context of a file to a new value, so in
> > > that case we would only use the _force interface if the caller had
> > > CAP_MAC_ADMIN.
> > >
> > > Paul, what say you?  NB This would be a user-visible behavior change for
> > > mounts specifying defcontext= on xattr filesystems; files with invalid
> > > contexts will then show up with the defcontext value instead of the
> > > unlabeled context.  If that's too risky, then we'd need a flag or
> > > something to security_context_to_sid_default() to distinguish the
> > > behaviors and only set it when called from selinux_inode_notifysecctx().
> >
> > Visible changes like this are always worrisome, even though I think it
> > is safe to assume that the defcontext option is not widely used.  I'd
> > feel much better if this change was opt-in.
> >
> > Which brings about it's own problems.  We have the policy capability
> > functionality, but that is likely a poor fit for this as the policy
> > capabilities are usually controlled by the Linux distribution while
> > the mount options are set by the system's administrator when the
> > filesystem is mounted.  We could add a toggle somewhere in selinuxfs,
> > but I really dislike that idea, and would prefer to find a different
> > solution if possible.  I'm not sure how much flak we would get for
> > introducing a new mount option, but perhaps that is the best way to
> > handle this: defcontext would continue to behave as it does now, but
> > new option X would behave as mentioned in this thread.
> >
> > Thoughts?
>
> The new option X will also mean "default context", so it will be pretty
> hard to come up with a different but a sensible name. I'm afraid that
> having two options with hardly distinguishable semantics will be very
> confusing.
>
> What about a kernel config option that modifies the behavior of
> defcontext?

A kernel config option would be going in the wrong direction; that
would put it almost completely under the control of the distribution.

Paul Moore Sept. 25, 2018, 4:03 p.m. UTC | #12

On Tue, Sep 25, 2018 at 9:58 AM Stephen Smalley <[e-mail]> wrote:
> On 09/25/2018 01:45 AM, Taras Kondratiuk wrote:
> > Quoting Paul Moore (2018-09-24 20:46:57)
> >> On Fri, Sep 21, 2018 at 10:39 AM Stephen Smalley <[e-mail]> wrote:
> >>> On 09/20/2018 06:59 PM, Taras Kondratiuk wrote:
> >>>> Quoting Stephen Smalley (2018-09-20 07:49:12)
> >>>>> On 09/19/2018 10:41 PM, Taras Kondratiuk wrote:
> >>>>>> Quoting Stephen Smalley (2018-09-19 12:00:33)
> >>>>>>> On 09/19/2018 12:52 PM, Taras Kondratiuk wrote:
> >>
> >> ...
> >>
> >>>> IMO it would be more consistent if defcontext cover all "unlabeled"
> >>>> groups. It seems unlikely to me that somebody who currently uses
> >>>> defcontext can somehow rely on mapping invalid labels to unlabeled
> >>>> instead of default context.
> >>>
> >>> Yes, and that seems more consistent with the current documentation in
> >>> the mount man page for defcontext=.
> >>>
> >>> I'd be inclined to change selinux_inode_notifysecctx() to call
> >>> security_context_to_sid_default() directly instead of using
> >>> selinux_inode_setsecurity() and change security_context_to_sid_core()
> >>> and sidtab_search_core() as suggested above to save and use the def_sid
> >>> instead of SECINITSID_UNLABELED always (initializing the context def_sid
> >>> to SECINITSID_UNLABELED as the default).  selinux_inode_setsecurity() we
> >>> should leave unchanged, or if we change it at all, it should be more
> >>> like the handling in selinux_inode_setxattr().  The notifysecctx hook is
> >>> invoked by the filesystem to notify the security module of the file's
> >>> existing security context, so in that case we always want the _default
> >>> behavior, whereas the setsecurity hook is invoked by the vfs or the
> >>> filesystem to set the security context of a file to a new value, so in
> >>> that case we would only use the _force interface if the caller had
> >>> CAP_MAC_ADMIN.
> >>>
> >>> Paul, what say you?  NB This would be a user-visible behavior change for
> >>> mounts specifying defcontext= on xattr filesystems; files with invalid
> >>> contexts will then show up with the defcontext value instead of the
> >>> unlabeled context.  If that's too risky, then we'd need a flag or
> >>> something to security_context_to_sid_default() to distinguish the
> >>> behaviors and only set it when called from selinux_inode_notifysecctx().
> >>
> >> Visible changes like this are always worrisome, even though I think it
> >> is safe to assume that the defcontext option is not widely used.  I'd
> >> feel much better if this change was opt-in.
> >>
> >> Which brings about it's own problems.  We have the policy capability
> >> functionality, but that is likely a poor fit for this as the policy
> >> capabilities are usually controlled by the Linux distribution while
> >> the mount options are set by the system's administrator when the
> >> filesystem is mounted.  We could add a toggle somewhere in selinuxfs,
> >> but I really dislike that idea, and would prefer to find a different
> >> solution if possible.  I'm not sure how much flak we would get for
> >> introducing a new mount option, but perhaps that is the best way to
> >> handle this: defcontext would continue to behave as it does now, but
> >> new option X would behave as mentioned in this thread.
> >>
> >> Thoughts?
> >
> > The new option X will also mean "default context", so it will be pretty
> > hard to come up with a different but a sensible name. I'm afraid that
> > having two options with hardly distinguishable semantics will be very
> > confusing.
> >
> > What about a kernel config option that modifies the behavior of
> > defcontext?
>
> First, the existing documentation for defcontext= leaves open the door
> to the proposed new behavior.  From mount(8):
> "You can set the default security context for  unlabeled  files  using
> defcontext= option.  This overrides the value set for unlabeled files in
> the policy and requires a filesystem that supports xattr labeling."
>
> "Unlabeled files" could just mean files without any xattr, or it could
> mean all files that either lack an xattr or have an invalid one under
> the policy, since both sets of files are currently mapped to the
> unlabeled context.

This may be true for the major SELinux policies being shipped by
distributions, but can we say this holds true for *all* SELinux
policies in use today?

> Second, under what conditions would this situation break existing
> userspace?  The admin would have had to mount an xattr filesystem with
> defcontext= specified, and that filesystem would have to both contain
> files without any xattrs and files with invalid ones (otherwise how they
> are treated by the kernel is irrelevant), and the policy would need to
> distinguish access to files without xattrs vs files with invalid ones.
> Seems unlikely.

I think you answered your own question.  Yes, it is unlikely, but I
*really* dislike changing visible behavior like this without some
explicit action on behalf of the user/admin.  We've done it in the
past and it has left me uneasy each time.

> Third, the fact that policy maintainers remapped both SECINITSID_FILE
> (the default value for defcontext) and SECINITSID_UNLABELED (used for
> invalid contexts) to the unlabeled context (getting rid of file_t as a
> separate type, aliased to unlabeled_t) long ago suggests that there is
> no meaningful difference there.

Related to the comments above.

> I'm inclined to just change the behavior for defcontext= unconditionally
> and have it apply to both native and xattr labeling.  If that's a no-go,
> then the simplest solution is to just leave defcontext= behavior
> unchanged for xattr labeling and only implement the new semantics for
> native labeling.  That's just a matter of adding a flag to
> security_context_to_sid_default() and only setting it when calling from
> selinux_inode_notifysecctx().

Neither option is very appealing to me, but that doesn't mean I'm saying "no".

From a sanity and consistency point of view I think option #1 (change
the defcontext behavior) is a better choice, and I tend to favor this
consistency even with the understanding that it could result in some
unexpected behavior for users.  However, if we get complaints, I'm
going to revert this without a second thought.

So to answer your question Taras, go ahead and prepare a patch so we
can take a look.  A bit of fair warning that it might get delayed
until after the upcoming merge window since we are already at -rc5; I
want this to have plenty of time in -next.

Thanks guys.

Stephen Smalley Sept. 25, 2018, 4:39 p.m. UTC | #13

On 09/25/2018 12:03 PM, Paul Moore wrote:
> On Tue, Sep 25, 2018 at 9:58 AM Stephen Smalley <[e-mail]> wrote:
>> On 09/25/2018 01:45 AM, Taras Kondratiuk wrote:
>>> Quoting Paul Moore (2018-09-24 20:46:57)
>>>> On Fri, Sep 21, 2018 at 10:39 AM Stephen Smalley <[e-mail]> wrote:
>>>>> On 09/20/2018 06:59 PM, Taras Kondratiuk wrote:
>>>>>> Quoting Stephen Smalley (2018-09-20 07:49:12)
>>>>>>> On 09/19/2018 10:41 PM, Taras Kondratiuk wrote:
>>>>>>>> Quoting Stephen Smalley (2018-09-19 12:00:33)
>>>>>>>>> On 09/19/2018 12:52 PM, Taras Kondratiuk wrote:
>>>>
>>>> ...
>>>>
>>>>>> IMO it would be more consistent if defcontext cover all "unlabeled"
>>>>>> groups. It seems unlikely to me that somebody who currently uses
>>>>>> defcontext can somehow rely on mapping invalid labels to unlabeled
>>>>>> instead of default context.
>>>>>
>>>>> Yes, and that seems more consistent with the current documentation in
>>>>> the mount man page for defcontext=.
>>>>>
>>>>> I'd be inclined to change selinux_inode_notifysecctx() to call
>>>>> security_context_to_sid_default() directly instead of using
>>>>> selinux_inode_setsecurity() and change security_context_to_sid_core()
>>>>> and sidtab_search_core() as suggested above to save and use the def_sid
>>>>> instead of SECINITSID_UNLABELED always (initializing the context def_sid
>>>>> to SECINITSID_UNLABELED as the default).  selinux_inode_setsecurity() we
>>>>> should leave unchanged, or if we change it at all, it should be more
>>>>> like the handling in selinux_inode_setxattr().  The notifysecctx hook is
>>>>> invoked by the filesystem to notify the security module of the file's
>>>>> existing security context, so in that case we always want the _default
>>>>> behavior, whereas the setsecurity hook is invoked by the vfs or the
>>>>> filesystem to set the security context of a file to a new value, so in
>>>>> that case we would only use the _force interface if the caller had
>>>>> CAP_MAC_ADMIN.
>>>>>
>>>>> Paul, what say you?  NB This would be a user-visible behavior change for
>>>>> mounts specifying defcontext= on xattr filesystems; files with invalid
>>>>> contexts will then show up with the defcontext value instead of the
>>>>> unlabeled context.  If that's too risky, then we'd need a flag or
>>>>> something to security_context_to_sid_default() to distinguish the
>>>>> behaviors and only set it when called from selinux_inode_notifysecctx().
>>>>
>>>> Visible changes like this are always worrisome, even though I think it
>>>> is safe to assume that the defcontext option is not widely used.  I'd
>>>> feel much better if this change was opt-in.
>>>>
>>>> Which brings about it's own problems.  We have the policy capability
>>>> functionality, but that is likely a poor fit for this as the policy
>>>> capabilities are usually controlled by the Linux distribution while
>>>> the mount options are set by the system's administrator when the
>>>> filesystem is mounted.  We could add a toggle somewhere in selinuxfs,
>>>> but I really dislike that idea, and would prefer to find a different
>>>> solution if possible.  I'm not sure how much flak we would get for
>>>> introducing a new mount option, but perhaps that is the best way to
>>>> handle this: defcontext would continue to behave as it does now, but
>>>> new option X would behave as mentioned in this thread.
>>>>
>>>> Thoughts?
>>>
>>> The new option X will also mean "default context", so it will be pretty
>>> hard to come up with a different but a sensible name. I'm afraid that
>>> having two options with hardly distinguishable semantics will be very
>>> confusing.
>>>
>>> What about a kernel config option that modifies the behavior of
>>> defcontext?
>>
>> First, the existing documentation for defcontext= leaves open the door
>> to the proposed new behavior.  From mount(8):
>> "You can set the default security context for  unlabeled  files  using
>> defcontext= option.  This overrides the value set for unlabeled files in
>> the policy and requires a filesystem that supports xattr labeling."
>>
>> "Unlabeled files" could just mean files without any xattr, or it could
>> mean all files that either lack an xattr or have an invalid one under
>> the policy, since both sets of files are currently mapped to the
>> unlabeled context.
> 
> This may be true for the major SELinux policies being shipped by
> distributions, but can we say this holds true for *all* SELinux
> policies in use today?
> 
>> Second, under what conditions would this situation break existing
>> userspace?  The admin would have had to mount an xattr filesystem with
>> defcontext= specified, and that filesystem would have to both contain
>> files without any xattrs and files with invalid ones (otherwise how they
>> are treated by the kernel is irrelevant), and the policy would need to
>> distinguish access to files without xattrs vs files with invalid ones.
>> Seems unlikely.
> 
> I think you answered your own question.  Yes, it is unlikely, but I
> *really* dislike changing visible behavior like this without some
> explicit action on behalf of the user/admin.  We've done it in the
> past and it has left me uneasy each time.
> 
>> Third, the fact that policy maintainers remapped both SECINITSID_FILE
>> (the default value for defcontext) and SECINITSID_UNLABELED (used for
>> invalid contexts) to the unlabeled context (getting rid of file_t as a
>> separate type, aliased to unlabeled_t) long ago suggests that there is
>> no meaningful difference there.
> 
> Related to the comments above.
> 
>> I'm inclined to just change the behavior for defcontext= unconditionally
>> and have it apply to both native and xattr labeling.  If that's a no-go,
>> then the simplest solution is to just leave defcontext= behavior
>> unchanged for xattr labeling and only implement the new semantics for
>> native labeling.  That's just a matter of adding a flag to
>> security_context_to_sid_default() and only setting it when calling from
>> selinux_inode_notifysecctx().
> 
> Neither option is very appealing to me, but that doesn't mean I'm saying "no".
> 
>  From a sanity and consistency point of view I think option #1 (change
> the defcontext behavior) is a better choice, and I tend to favor this
> consistency even with the understanding that it could result in some
> unexpected behavior for users.  However, if we get complaints, I'm
> going to revert this without a second thought.

In that case, I'd suggest splitting it into two patches; first one only 
enables the new behavior for native labeling filesystems (as per the 
above, via a flag to security_context_to_sid_default), and the second 
patch drops the flag and does it unconditionally.  Then you can always 
revert the latter without affecting the former.

> 
> So to answer your question Taras, go ahead and prepare a patch so we
> can take a look.  A bit of fair warning that it might get delayed
> until after the upcoming merge window since we are already at -rc5; I
> want this to have plenty of time in -next.
> 
> Thanks guys.
>

Jann Horn via Selinux Sept. 25, 2018, 7:10 p.m. UTC | #14

Quoting Stephen Smalley (2018-09-25 09:39:55)
> On 09/25/2018 12:03 PM, Paul Moore wrote:
> > On Tue, Sep 25, 2018 at 9:58 AM Stephen Smalley <[e-mail]> wrote:

<snip>

> >> I'm inclined to just change the behavior for defcontext= unconditionally
> >> and have it apply to both native and xattr labeling.  If that's a no-go,
> >> then the simplest solution is to just leave defcontext= behavior
> >> unchanged for xattr labeling and only implement the new semantics for
> >> native labeling.  That's just a matter of adding a flag to
> >> security_context_to_sid_default() and only setting it when calling from
> >> selinux_inode_notifysecctx().
> > 
> > Neither option is very appealing to me, but that doesn't mean I'm saying "no".
> > 
> >  From a sanity and consistency point of view I think option #1 (change
> > the defcontext behavior) is a better choice, and I tend to favor this
> > consistency even with the understanding that it could result in some
> > unexpected behavior for users.  However, if we get complaints, I'm
> > going to revert this without a second thought.
> 
> In that case, I'd suggest splitting it into two patches; first one only 
> enables the new behavior for native labeling filesystems (as per the 
> above, via a flag to security_context_to_sid_default), and the second 
> patch drops the flag and does it unconditionally.  Then you can always 
> revert the latter without affecting the former.
> 
> > 
> > So to answer your question Taras, go ahead and prepare a patch so we
> > can take a look.  A bit of fair warning that it might get delayed
> > until after the upcoming merge window since we are already at -rc5; I
> > want this to have plenty of time in -next.
> > 
> > Thanks guys.

Thanks. I'll prepare patches is a few days.

Jann Horn via Selinux Oct. 2, 2018, 6:48 p.m. UTC | #15

Quoting Stephen Smalley (2018-09-21 07:40:58)
> If we set the inode sid to the superblock def_sid on an invalid
> context, then we lose the association to the original context value.
> The support for deferred mapping of contexts requires allocating a new
> SID for the invalid context and storing that SID in the inode, so that
> if the context later becomes valid upon a policy update/reload, the
> inode SID will refer to the now valid context.
> To combine the two, we would need security_context_to_sid_core() to
> save the def_sid in the context structure for invalid contexts, and
> change sidtab_search_core() to use that value instead of
> SECINITSID_UNLABELED for invalid SIDs.  Then the inode would be
> treated as having the defcontext for access control and getxattr() w/o
> CAP_MAC_ADMIN purposes, but a subsequent policy update/reload that
> makes the context valid would automatically cause subsequent accesses
> to the inode to start using the original context value for access
> control and getxattr() purposes.  I think that's the behavior you
> want.
> 

While implementing the change I've realized that storing default context
for sidtab_search_core() in the context structure is not enough to
achieve the desired behavior. The same invalid context may exist in two
mounts with different 'defcontext', so default context can't be a
property of a context structure.

One way to address it is to propagate default context to sidtab_search() all
the way from inode hooks. But that will be a bit intrusive. Something like
avc_has_perm_default() will need to be added.

Other way is to check for context validity in inode hooks and provide a default
context to avc_has_perm() if the inode's sid is invalid. But this may have
performance implication since validity check will be done each time in fast
path.

Do you see any other options?

Stephen Smalley Oct. 2, 2018, 7:41 p.m. UTC | #16

On 10/02/2018 02:48 PM, Taras Kondratiuk wrote:
> Quoting Stephen Smalley (2018-09-21 07:40:58)
>> If we set the inode sid to the superblock def_sid on an invalid
>> context, then we lose the association to the original context value.
>> The support for deferred mapping of contexts requires allocating a new
>> SID for the invalid context and storing that SID in the inode, so that
>> if the context later becomes valid upon a policy update/reload, the
>> inode SID will refer to the now valid context.
>> To combine the two, we would need security_context_to_sid_core() to
>> save the def_sid in the context structure for invalid contexts, and
>> change sidtab_search_core() to use that value instead of
>> SECINITSID_UNLABELED for invalid SIDs.  Then the inode would be
>> treated as having the defcontext for access control and getxattr() w/o
>> CAP_MAC_ADMIN purposes, but a subsequent policy update/reload that
>> makes the context valid would automatically cause subsequent accesses
>> to the inode to start using the original context value for access
>> control and getxattr() purposes.  I think that's the behavior you
>> want.
>>
> 
> While implementing the change I've realized that storing default context
> for sidtab_search_core() in the context structure is not enough to
> achieve the desired behavior. The same invalid context may exist in two
> mounts with different 'defcontext', so default context can't be a
> property of a context structure.
> 
> One way to address it is to propagate default context to sidtab_search() all
> the way from inode hooks. But that will be a bit intrusive. Something like
> avc_has_perm_default() will need to be added.
> 
> Other way is to check for context validity in inode hooks and provide a default
> context to avc_has_perm() if the inode's sid is invalid. But this may have
> performance implication since validity check will be done each time in fast
> path.
> 
> Do you see any other options?

I think the original approach is still best.  The def_sid can be part of 
the context structure; you just need to update context_cmp() to compare 
it (as part of the first return statement) so that the same invalid 
context in two mounts with different defcontext= values will allocate 
different SIDs.  Likewise context_cpy() needs to copy the def_sid.

As a separate matter, you should also modify 
security_context_to_sid_force() to pass down the sbsec->def_sid so that 
when userspace with CAP_MAC_ADMIN sets an invalid context on an inode, 
the default context will be used in the same manner.

Jann Horn via Selinux Oct. 3, 2018, 12:58 a.m. UTC | #17

Quoting Stephen Smalley (2018-10-02 12:41:54)
> On 10/02/2018 02:48 PM, Taras Kondratiuk wrote:
> > Quoting Stephen Smalley (2018-09-21 07:40:58)
> >> If we set the inode sid to the superblock def_sid on an invalid
> >> context, then we lose the association to the original context value.
> >> The support for deferred mapping of contexts requires allocating a new
> >> SID for the invalid context and storing that SID in the inode, so that
> >> if the context later becomes valid upon a policy update/reload, the
> >> inode SID will refer to the now valid context.
> >> To combine the two, we would need security_context_to_sid_core() to
> >> save the def_sid in the context structure for invalid contexts, and
> >> change sidtab_search_core() to use that value instead of
> >> SECINITSID_UNLABELED for invalid SIDs.  Then the inode would be
> >> treated as having the defcontext for access control and getxattr() w/o
> >> CAP_MAC_ADMIN purposes, but a subsequent policy update/reload that
> >> makes the context valid would automatically cause subsequent accesses
> >> to the inode to start using the original context value for access
> >> control and getxattr() purposes.  I think that's the behavior you
> >> want.
> >>
> > 
> > While implementing the change I've realized that storing default context
> > for sidtab_search_core() in the context structure is not enough to
> > achieve the desired behavior. The same invalid context may exist in two
> > mounts with different 'defcontext', so default context can't be a
> > property of a context structure.
> > 
> > One way to address it is to propagate default context to sidtab_search() all
> > the way from inode hooks. But that will be a bit intrusive. Something like
> > avc_has_perm_default() will need to be added.
> > 
> > Other way is to check for context validity in inode hooks and provide a default
> > context to avc_has_perm() if the inode's sid is invalid. But this may have
> > performance implication since validity check will be done each time in fast
> > path.
> > 
> > Do you see any other options?
> 
> I think the original approach is still best.  The def_sid can be part of 
> the context structure; you just need to update context_cmp() to compare 
> it (as part of the first return statement) so that the same invalid 
> context in two mounts with different defcontext= values will allocate 
> different SIDs.  Likewise context_cpy() needs to copy the def_sid.
> 
> As a separate matter, you should also modify 
> security_context_to_sid_force() to pass down the sbsec->def_sid so that 
> when userspace with CAP_MAC_ADMIN sets an invalid context on an inode, 
> the default context will be used in the same manner.

Thanks. That's really a better approach.

Patch

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index ad9a9b8e9979..f7debe798bf5 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6598,7 +6598,30 @@  static void selinux_inode_invalidate_secctx(struct inode *inode)
  */
 static int selinux_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen)
 {
-	return selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX, ctx, ctxlen, 0);
+	struct superblock_security_struct *sbsec;
+	struct inode_security_struct *isec;
+	int rc;
+
+	rc = selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX, ctx, ctxlen, 0);
+
+	/*
+	 * In case of Native labeling with defcontext mount option fall back
+	 * to a default SID if received context is invalid.
+	 */
+	if (rc == -EINVAL) {
+		sbsec = inode->i_sb->s_security;
+		if (sbsec->behavior == SECURITY_FS_USE_NATIVE &&
+		    sbsec->flags & DEFCONTEXT_MNT) {
+			isec = inode->i_security;
+			if (!isec->initialized) {
+				isec->sclass = inode_mode_to_security_class(inode->i_mode);
+				isec->sid = sbsec->def_sid;
+				isec->initialized = 1;
+			}
+			rc = 0;
+		}
+	}
+	return rc;
 }
 
 /*