2013-01-17 15:48:01

by Jeff Layton

[permalink] [raw]
Subject: [PATCH RFC] selinux: make security_sb_clone_mnt_opts return an error on context mismatch

Sorry if this is tl;dr, but this is a complex problem and I'm not
sure what the right solution is...

I had the following problem reported a while back. If you mount the
same filesystem twice using NFSv4 with different contexts, then the
second context= option is ignored. For instance:

# mount server:/export /mnt/test1
# mount server:/export /mnt/test2 -o context=system_u:object_r:tmp_t:s0
# ls -dZ /mnt/test1
drwxrwxrwt. root root system_u:object_r:nfs_t:s0 /mnt/test1
# ls -dZ /mnt/test2
drwxrwxrwt. root root system_u:object_r:nfs_t:s0 /mnt/test2

When we call into SELinux to set the context of a "cloned" superblock,
it will currently just bail out when it notices that we're reusing an
existing superblock. Since the existing superblock is already set up and
presumably in use, we can't go overwriting its context with the one from
the "original" sb. Because of this, the second context= option in this
case cannot take effect.

This patch fixes this by turning security_sb_clone_mnt_opts into an int
return operation. When it finds that the "new" superblock that it has
been handed is already set up, it checks to see whether the contexts on
the old superblock match it. If it does, then it will just return
success, otherwise it'll return EINVAL and emit a printk to tell the
admin why the second mount failed.

(Side note: maybe EBUSY would be a better error there?)

Note that this patch may cause casualties (which is the reason for the
RFC). The NFSv4 code relies on being able to walk down to an export from
the pseudoroot. If you mount filesystems that are nested within one
another with different contexts, then this patch will make those mounts
fail in new and "exciting" ways.

For instance, suppose that /export is a separate filesystem on the
server:

# mount server:/ /mnt/test1
# mount salusa:/export /mnt/test2 -o context=system_u:object_r:tmp_t:s0
mount.nfs: an incorrect mount option was specified

...with the printk in the ring buffer. Because we *might* eventually
walk down to /mnt/test1/export, the mount is denied due to this patch.
The second mount needs the pseudoroot superblock, but that's already
present with the wrong context.

OTOH, if we mount these in the reverse order, then both mounts work,
because the pseudoroot superblock created when mounting /export is
discarded once that mount is done. If we then however try to walk into
that directory, the automount fails for the similar reasons:

# cd /mnt/test1/scratch/
-bash: cd: /mnt/test1/scratch/: Invalid argument

The story I've gotten from the SELinux folks that I've talked to is that
this is desirable behavior. In SELinux-land, mounting the same data
under different contexts is wrong -- there can be only one. I'm not sure
I like having these sorts of spurious errors though, even with a printk
that tries to explain them.

Another possibility might be to just emit the printk in this situation
but not turn this into an error. IOW, just warn the admin that their
context is being ignored. In the case of automounts though, it may be
difficult to connect the warnings to actual mounts.

Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfs/super.c | 3 +--
include/linux/security.h | 10 ++++++----
security/capability.c | 3 ++-
security/security.c | 4 ++--
security/selinux/hooks.c | 39 +++++++++++++++++++++++++++++++++++----
5 files changed, 46 insertions(+), 13 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 2e7e8c8..939b9f0 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -2426,10 +2426,9 @@ int nfs_clone_sb_security(struct super_block *s, struct dentry *mntroot,
struct nfs_mount_info *mount_info)
{
/* clone any lsm security options from the parent to the new sb */
- security_sb_clone_mnt_opts(mount_info->cloned->sb, s);
if (mntroot->d_inode->i_op != NFS_SB(s)->nfs_client->rpc_ops->dir_inode_ops)
return -ESTALE;
- return 0;
+ return security_sb_clone_mnt_opts(mount_info->cloned->sb, s);
}
EXPORT_SYMBOL_GPL(nfs_clone_sb_security);

diff --git a/include/linux/security.h b/include/linux/security.h
index 0f6afc6..908cf98 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1424,7 +1424,7 @@ struct security_operations {
struct path *new_path);
int (*sb_set_mnt_opts) (struct super_block *sb,
struct security_mnt_opts *opts);
- void (*sb_clone_mnt_opts) (const struct super_block *oldsb,
+ int (*sb_clone_mnt_opts) (const struct super_block *oldsb,
struct super_block *newsb);
int (*sb_parse_opts_str) (char *options, struct security_mnt_opts *opts);

@@ -1706,7 +1706,7 @@ int security_sb_mount(const char *dev_name, struct path *path,
int security_sb_umount(struct vfsmount *mnt, int flags);
int security_sb_pivotroot(struct path *old_path, struct path *new_path);
int security_sb_set_mnt_opts(struct super_block *sb, struct security_mnt_opts *opts);
-void security_sb_clone_mnt_opts(const struct super_block *oldsb,
+int security_sb_clone_mnt_opts(const struct super_block *oldsb,
struct super_block *newsb);
int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts);

@@ -1996,9 +1996,11 @@ static inline int security_sb_set_mnt_opts(struct super_block *sb,
return 0;
}

-static inline void security_sb_clone_mnt_opts(const struct super_block *oldsb,
+static inline int security_sb_clone_mnt_opts(const struct super_block *oldsb,
struct super_block *newsb)
-{ }
+{
+ return 0;
+}

static inline int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts)
{
diff --git a/security/capability.c b/security/capability.c
index 0fe5a02..60c9680 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -98,9 +98,10 @@ static int cap_sb_set_mnt_opts(struct super_block *sb,
return 0;
}

-static void cap_sb_clone_mnt_opts(const struct super_block *oldsb,
+static int cap_sb_clone_mnt_opts(const struct super_block *oldsb,
struct super_block *newsb)
{
+ return 0;
}

static int cap_sb_parse_opts_str(char *options, struct security_mnt_opts *opts)
diff --git a/security/security.c b/security/security.c
index daa97f4..f587683 100644
--- a/security/security.c
+++ b/security/security.c
@@ -299,10 +299,10 @@ int security_sb_set_mnt_opts(struct super_block *sb,
}
EXPORT_SYMBOL(security_sb_set_mnt_opts);

-void security_sb_clone_mnt_opts(const struct super_block *oldsb,
+int security_sb_clone_mnt_opts(const struct super_block *oldsb,
struct super_block *newsb)
{
- security_ops->sb_clone_mnt_opts(oldsb, newsb);
+ return security_ops->sb_clone_mnt_opts(oldsb, newsb);
}
EXPORT_SYMBOL(security_sb_clone_mnt_opts);

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 61a5336..79d06f2 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -750,7 +750,37 @@ out_double_mount:
goto out;
}

-static void selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
+static int selinux_cmp_sb_context(const struct super_block *oldsb,
+ const struct super_block *newsb)
+{
+ struct superblock_security_struct *old = oldsb->s_security;
+ struct superblock_security_struct *new = newsb->s_security;
+ char oldflags = old->flags & SE_MNTMASK;
+ char newflags = new->flags & SE_MNTMASK;
+
+ if (oldflags != newflags)
+ goto mismatch;
+ if ((oldflags & FSCONTEXT_MNT) && old->sid != new->sid)
+ goto mismatch;
+ if ((oldflags & CONTEXT_MNT) && old->mntpoint_sid != new->mntpoint_sid)
+ goto mismatch;
+ if ((oldflags & DEFCONTEXT_MNT) && old->def_sid != new->def_sid)
+ goto mismatch;
+ if (oldflags & ROOTCONTEXT_MNT) {
+ struct inode_security_struct *oldroot = oldsb->s_root->d_inode->i_security;
+ struct inode_security_struct *newroot = newsb->s_root->d_inode->i_security;
+ if (oldroot->sid != newroot->sid)
+ goto mismatch;
+ }
+ return 0;
+mismatch:
+ printk(KERN_WARNING "SELinux: mount invalid. Same superblock, "
+ "different security settings for (dev %s, "
+ "type %s)\n", newsb->s_id, newsb->s_type->name);
+ return -EINVAL;
+}
+
+static int selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
struct super_block *newsb)
{
const struct superblock_security_struct *oldsbsec = oldsb->s_security;
@@ -765,14 +795,14 @@ static void selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
* mount options. thus we can safely deal with this superblock later
*/
if (!ss_initialized)
- return;
+ return 0;

/* how can we clone if the old one wasn't set up?? */
BUG_ON(!(oldsbsec->flags & SE_SBINITIALIZED));

- /* if fs is reusing a sb, just let its options stand... */
+ /* if fs is reusing a sb, make sure that the contexts match */
if (newsbsec->flags & SE_SBINITIALIZED)
- return;
+ return selinux_cmp_sb_context(oldsb, newsb);

mutex_lock(&newsbsec->lock);

@@ -805,6 +835,7 @@ static void selinux_sb_clone_mnt_opts(const struct super_block *oldsb,

sb_finish_set_opts(newsb);
mutex_unlock(&newsbsec->lock);
+ return 0;
}

static int selinux_parse_opts_str(char *options,
--
1.7.11.7



2013-01-18 14:53:11

by Eric Paris

[permalink] [raw]
Subject: Re: [PATCH RFC] selinux: make security_sb_clone_mnt_opts return an error on context mismatch

Odd, but for once I agree with Casey :)

Acked-by: Eric Paris <[email protected]>

On Thu, Jan 17, 2013 at 6:04 PM, Casey Schaufler <[email protected]> wrote:
> On 1/17/2013 7:40 AM, Jeff Layton wrote:
>> Sorry if this is tl;dr, but this is a complex problem and I'm not
>> sure what the right solution is...
>>
>> I had the following problem reported a while back. If you mount the
>> same filesystem twice using NFSv4 with different contexts, then the
>> second context= option is ignored. For instance:
>>
>> # mount server:/export /mnt/test1
>> # mount server:/export /mnt/test2 -o context=system_u:object_r:tmp_t:s0
>> # ls -dZ /mnt/test1
>> drwxrwxrwt. root root system_u:object_r:nfs_t:s0 /mnt/test1
>> # ls -dZ /mnt/test2
>> drwxrwxrwt. root root system_u:object_r:nfs_t:s0 /mnt/test2
>>
>> When we call into SELinux to set the context of a "cloned" superblock,
>> it will currently just bail out when it notices that we're reusing an
>> existing superblock. Since the existing superblock is already set up and
>> presumably in use, we can't go overwriting its context with the one from
>> the "original" sb. Because of this, the second context= option in this
>> case cannot take effect.
>>
>> This patch fixes this by turning security_sb_clone_mnt_opts into an int
>> return operation. When it finds that the "new" superblock that it has
>> been handed is already set up, it checks to see whether the contexts on
>> the old superblock match it. If it does, then it will just return
>> success, otherwise it'll return EINVAL and emit a printk to tell the
>> admin why the second mount failed.
>>
>> (Side note: maybe EBUSY would be a better error there?)
>>
>> Note that this patch may cause casualties (which is the reason for the
>> RFC). The NFSv4 code relies on being able to walk down to an export from
>> the pseudoroot. If you mount filesystems that are nested within one
>> another with different contexts, then this patch will make those mounts
>> fail in new and "exciting" ways.
>>
>> For instance, suppose that /export is a separate filesystem on the
>> server:
>>
>> # mount server:/ /mnt/test1
>> # mount salusa:/export /mnt/test2 -o context=system_u:object_r:tmp_t:s0
>> mount.nfs: an incorrect mount option was specified
>>
>> ...with the printk in the ring buffer. Because we *might* eventually
>> walk down to /mnt/test1/export, the mount is denied due to this patch.
>> The second mount needs the pseudoroot superblock, but that's already
>> present with the wrong context.
>>
>> OTOH, if we mount these in the reverse order, then both mounts work,
>> because the pseudoroot superblock created when mounting /export is
>> discarded once that mount is done. If we then however try to walk into
>> that directory, the automount fails for the similar reasons:
>>
>> # cd /mnt/test1/scratch/
>> -bash: cd: /mnt/test1/scratch/: Invalid argument
>>
>> The story I've gotten from the SELinux folks that I've talked to is that
>> this is desirable behavior. In SELinux-land, mounting the same data
>> under different contexts is wrong -- there can be only one.
>
> It's hard to imaging a case where mounting the same
> data with different contexts would be "right", although
> I have seen cases where misguided individuals have
> suggested doing just that.
>
> I think your solution is sound, if potentially resulting
> in occasional moaning.
>
>> I'm not sure
>> I like having these sorts of spurious errors though, even with a printk
>> that tries to explain them.
>>
>> Another possibility might be to just emit the printk in this situation
>> but not turn this into an error. IOW, just warn the admin that their
>> context is being ignored. In the case of automounts though, it may be
>> difficult to connect the warnings to actual mounts.
>>
>> Signed-off-by: Jeff Layton <[email protected]>
>> ---
>> fs/nfs/super.c | 3 +--
>> include/linux/security.h | 10 ++++++----
>> security/capability.c | 3 ++-
>> security/security.c | 4 ++--
>> security/selinux/hooks.c | 39 +++++++++++++++++++++++++++++++++++----
>> 5 files changed, 46 insertions(+), 13 deletions(-)
>>
>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>> index 2e7e8c8..939b9f0 100644
>> --- a/fs/nfs/super.c
>> +++ b/fs/nfs/super.c
>> @@ -2426,10 +2426,9 @@ int nfs_clone_sb_security(struct super_block *s, struct dentry *mntroot,
>> struct nfs_mount_info *mount_info)
>> {
>> /* clone any lsm security options from the parent to the new sb */
>> - security_sb_clone_mnt_opts(mount_info->cloned->sb, s);
>> if (mntroot->d_inode->i_op != NFS_SB(s)->nfs_client->rpc_ops->dir_inode_ops)
>> return -ESTALE;
>> - return 0;
>> + return security_sb_clone_mnt_opts(mount_info->cloned->sb, s);
>> }
>> EXPORT_SYMBOL_GPL(nfs_clone_sb_security);
>>
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 0f6afc6..908cf98 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -1424,7 +1424,7 @@ struct security_operations {
>> struct path *new_path);
>> int (*sb_set_mnt_opts) (struct super_block *sb,
>> struct security_mnt_opts *opts);
>> - void (*sb_clone_mnt_opts) (const struct super_block *oldsb,
>> + int (*sb_clone_mnt_opts) (const struct super_block *oldsb,
>> struct super_block *newsb);
>> int (*sb_parse_opts_str) (char *options, struct security_mnt_opts *opts);
>>
>> @@ -1706,7 +1706,7 @@ int security_sb_mount(const char *dev_name, struct path *path,
>> int security_sb_umount(struct vfsmount *mnt, int flags);
>> int security_sb_pivotroot(struct path *old_path, struct path *new_path);
>> int security_sb_set_mnt_opts(struct super_block *sb, struct security_mnt_opts *opts);
>> -void security_sb_clone_mnt_opts(const struct super_block *oldsb,
>> +int security_sb_clone_mnt_opts(const struct super_block *oldsb,
>> struct super_block *newsb);
>> int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts);
>>
>> @@ -1996,9 +1996,11 @@ static inline int security_sb_set_mnt_opts(struct super_block *sb,
>> return 0;
>> }
>>
>> -static inline void security_sb_clone_mnt_opts(const struct super_block *oldsb,
>> +static inline int security_sb_clone_mnt_opts(const struct super_block *oldsb,
>> struct super_block *newsb)
>> -{ }
>> +{
>> + return 0;
>> +}
>>
>> static inline int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts)
>> {
>> diff --git a/security/capability.c b/security/capability.c
>> index 0fe5a02..60c9680 100644
>> --- a/security/capability.c
>> +++ b/security/capability.c
>> @@ -98,9 +98,10 @@ static int cap_sb_set_mnt_opts(struct super_block *sb,
>> return 0;
>> }
>>
>> -static void cap_sb_clone_mnt_opts(const struct super_block *oldsb,
>> +static int cap_sb_clone_mnt_opts(const struct super_block *oldsb,
>> struct super_block *newsb)
>> {
>> + return 0;
>> }
>>
>> static int cap_sb_parse_opts_str(char *options, struct security_mnt_opts *opts)
>> diff --git a/security/security.c b/security/security.c
>> index daa97f4..f587683 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -299,10 +299,10 @@ int security_sb_set_mnt_opts(struct super_block *sb,
>> }
>> EXPORT_SYMBOL(security_sb_set_mnt_opts);
>>
>> -void security_sb_clone_mnt_opts(const struct super_block *oldsb,
>> +int security_sb_clone_mnt_opts(const struct super_block *oldsb,
>> struct super_block *newsb)
>> {
>> - security_ops->sb_clone_mnt_opts(oldsb, newsb);
>> + return security_ops->sb_clone_mnt_opts(oldsb, newsb);
>> }
>> EXPORT_SYMBOL(security_sb_clone_mnt_opts);
>>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 61a5336..79d06f2 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -750,7 +750,37 @@ out_double_mount:
>> goto out;
>> }
>>
>> -static void selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
>> +static int selinux_cmp_sb_context(const struct super_block *oldsb,
>> + const struct super_block *newsb)
>> +{
>> + struct superblock_security_struct *old = oldsb->s_security;
>> + struct superblock_security_struct *new = newsb->s_security;
>> + char oldflags = old->flags & SE_MNTMASK;
>> + char newflags = new->flags & SE_MNTMASK;
>> +
>> + if (oldflags != newflags)
>> + goto mismatch;
>> + if ((oldflags & FSCONTEXT_MNT) && old->sid != new->sid)
>> + goto mismatch;
>> + if ((oldflags & CONTEXT_MNT) && old->mntpoint_sid != new->mntpoint_sid)
>> + goto mismatch;
>> + if ((oldflags & DEFCONTEXT_MNT) && old->def_sid != new->def_sid)
>> + goto mismatch;
>> + if (oldflags & ROOTCONTEXT_MNT) {
>> + struct inode_security_struct *oldroot = oldsb->s_root->d_inode->i_security;
>> + struct inode_security_struct *newroot = newsb->s_root->d_inode->i_security;
>> + if (oldroot->sid != newroot->sid)
>> + goto mismatch;
>> + }
>> + return 0;
>> +mismatch:
>> + printk(KERN_WARNING "SELinux: mount invalid. Same superblock, "
>> + "different security settings for (dev %s, "
>> + "type %s)\n", newsb->s_id, newsb->s_type->name);
>> + return -EINVAL;
>> +}
>> +
>> +static int selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
>> struct super_block *newsb)
>> {
>> const struct superblock_security_struct *oldsbsec = oldsb->s_security;
>> @@ -765,14 +795,14 @@ static void selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
>> * mount options. thus we can safely deal with this superblock later
>> */
>> if (!ss_initialized)
>> - return;
>> + return 0;
>>
>> /* how can we clone if the old one wasn't set up?? */
>> BUG_ON(!(oldsbsec->flags & SE_SBINITIALIZED));
>>
>> - /* if fs is reusing a sb, just let its options stand... */
>> + /* if fs is reusing a sb, make sure that the contexts match */
>> if (newsbsec->flags & SE_SBINITIALIZED)
>> - return;
>> + return selinux_cmp_sb_context(oldsb, newsb);
>>
>> mutex_lock(&newsbsec->lock);
>>
>> @@ -805,6 +835,7 @@ static void selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
>>
>> sb_finish_set_opts(newsb);
>> mutex_unlock(&newsbsec->lock);
>> + return 0;
>> }
>>
>> static int selinux_parse_opts_str(char *options,
>

2013-01-17 23:11:49

by Casey Schaufler

[permalink] [raw]
Subject: Re: [PATCH RFC] selinux: make security_sb_clone_mnt_opts return an error on context mismatch

On 1/17/2013 7:40 AM, Jeff Layton wrote:
> Sorry if this is tl;dr, but this is a complex problem and I'm not
> sure what the right solution is...
>
> I had the following problem reported a while back. If you mount the
> same filesystem twice using NFSv4 with different contexts, then the
> second context= option is ignored. For instance:
>
> # mount server:/export /mnt/test1
> # mount server:/export /mnt/test2 -o context=system_u:object_r:tmp_t:s0
> # ls -dZ /mnt/test1
> drwxrwxrwt. root root system_u:object_r:nfs_t:s0 /mnt/test1
> # ls -dZ /mnt/test2
> drwxrwxrwt. root root system_u:object_r:nfs_t:s0 /mnt/test2
>
> When we call into SELinux to set the context of a "cloned" superblock,
> it will currently just bail out when it notices that we're reusing an
> existing superblock. Since the existing superblock is already set up and
> presumably in use, we can't go overwriting its context with the one from
> the "original" sb. Because of this, the second context= option in this
> case cannot take effect.
>
> This patch fixes this by turning security_sb_clone_mnt_opts into an int
> return operation. When it finds that the "new" superblock that it has
> been handed is already set up, it checks to see whether the contexts on
> the old superblock match it. If it does, then it will just return
> success, otherwise it'll return EINVAL and emit a printk to tell the
> admin why the second mount failed.
>
> (Side note: maybe EBUSY would be a better error there?)
>
> Note that this patch may cause casualties (which is the reason for the
> RFC). The NFSv4 code relies on being able to walk down to an export from
> the pseudoroot. If you mount filesystems that are nested within one
> another with different contexts, then this patch will make those mounts
> fail in new and "exciting" ways.
>
> For instance, suppose that /export is a separate filesystem on the
> server:
>
> # mount server:/ /mnt/test1
> # mount salusa:/export /mnt/test2 -o context=system_u:object_r:tmp_t:s0
> mount.nfs: an incorrect mount option was specified
>
> ...with the printk in the ring buffer. Because we *might* eventually
> walk down to /mnt/test1/export, the mount is denied due to this patch.
> The second mount needs the pseudoroot superblock, but that's already
> present with the wrong context.
>
> OTOH, if we mount these in the reverse order, then both mounts work,
> because the pseudoroot superblock created when mounting /export is
> discarded once that mount is done. If we then however try to walk into
> that directory, the automount fails for the similar reasons:
>
> # cd /mnt/test1/scratch/
> -bash: cd: /mnt/test1/scratch/: Invalid argument
>
> The story I've gotten from the SELinux folks that I've talked to is that
> this is desirable behavior. In SELinux-land, mounting the same data
> under different contexts is wrong -- there can be only one.

It's hard to imaging a case where mounting the same
data with different contexts would be "right", although
I have seen cases where misguided individuals have
suggested doing just that.

I think your solution is sound, if potentially resulting
in occasional moaning.

> I'm not sure
> I like having these sorts of spurious errors though, even with a printk
> that tries to explain them.
>
> Another possibility might be to just emit the printk in this situation
> but not turn this into an error. IOW, just warn the admin that their
> context is being ignored. In the case of automounts though, it may be
> difficult to connect the warnings to actual mounts.
>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/nfs/super.c | 3 +--
> include/linux/security.h | 10 ++++++----
> security/capability.c | 3 ++-
> security/security.c | 4 ++--
> security/selinux/hooks.c | 39 +++++++++++++++++++++++++++++++++++----
> 5 files changed, 46 insertions(+), 13 deletions(-)
>
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index 2e7e8c8..939b9f0 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -2426,10 +2426,9 @@ int nfs_clone_sb_security(struct super_block *s, struct dentry *mntroot,
> struct nfs_mount_info *mount_info)
> {
> /* clone any lsm security options from the parent to the new sb */
> - security_sb_clone_mnt_opts(mount_info->cloned->sb, s);
> if (mntroot->d_inode->i_op != NFS_SB(s)->nfs_client->rpc_ops->dir_inode_ops)
> return -ESTALE;
> - return 0;
> + return security_sb_clone_mnt_opts(mount_info->cloned->sb, s);
> }
> EXPORT_SYMBOL_GPL(nfs_clone_sb_security);
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 0f6afc6..908cf98 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1424,7 +1424,7 @@ struct security_operations {
> struct path *new_path);
> int (*sb_set_mnt_opts) (struct super_block *sb,
> struct security_mnt_opts *opts);
> - void (*sb_clone_mnt_opts) (const struct super_block *oldsb,
> + int (*sb_clone_mnt_opts) (const struct super_block *oldsb,
> struct super_block *newsb);
> int (*sb_parse_opts_str) (char *options, struct security_mnt_opts *opts);
>
> @@ -1706,7 +1706,7 @@ int security_sb_mount(const char *dev_name, struct path *path,
> int security_sb_umount(struct vfsmount *mnt, int flags);
> int security_sb_pivotroot(struct path *old_path, struct path *new_path);
> int security_sb_set_mnt_opts(struct super_block *sb, struct security_mnt_opts *opts);
> -void security_sb_clone_mnt_opts(const struct super_block *oldsb,
> +int security_sb_clone_mnt_opts(const struct super_block *oldsb,
> struct super_block *newsb);
> int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts);
>
> @@ -1996,9 +1996,11 @@ static inline int security_sb_set_mnt_opts(struct super_block *sb,
> return 0;
> }
>
> -static inline void security_sb_clone_mnt_opts(const struct super_block *oldsb,
> +static inline int security_sb_clone_mnt_opts(const struct super_block *oldsb,
> struct super_block *newsb)
> -{ }
> +{
> + return 0;
> +}
>
> static inline int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts)
> {
> diff --git a/security/capability.c b/security/capability.c
> index 0fe5a02..60c9680 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -98,9 +98,10 @@ static int cap_sb_set_mnt_opts(struct super_block *sb,
> return 0;
> }
>
> -static void cap_sb_clone_mnt_opts(const struct super_block *oldsb,
> +static int cap_sb_clone_mnt_opts(const struct super_block *oldsb,
> struct super_block *newsb)
> {
> + return 0;
> }
>
> static int cap_sb_parse_opts_str(char *options, struct security_mnt_opts *opts)
> diff --git a/security/security.c b/security/security.c
> index daa97f4..f587683 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -299,10 +299,10 @@ int security_sb_set_mnt_opts(struct super_block *sb,
> }
> EXPORT_SYMBOL(security_sb_set_mnt_opts);
>
> -void security_sb_clone_mnt_opts(const struct super_block *oldsb,
> +int security_sb_clone_mnt_opts(const struct super_block *oldsb,
> struct super_block *newsb)
> {
> - security_ops->sb_clone_mnt_opts(oldsb, newsb);
> + return security_ops->sb_clone_mnt_opts(oldsb, newsb);
> }
> EXPORT_SYMBOL(security_sb_clone_mnt_opts);
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 61a5336..79d06f2 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -750,7 +750,37 @@ out_double_mount:
> goto out;
> }
>
> -static void selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
> +static int selinux_cmp_sb_context(const struct super_block *oldsb,
> + const struct super_block *newsb)
> +{
> + struct superblock_security_struct *old = oldsb->s_security;
> + struct superblock_security_struct *new = newsb->s_security;
> + char oldflags = old->flags & SE_MNTMASK;
> + char newflags = new->flags & SE_MNTMASK;
> +
> + if (oldflags != newflags)
> + goto mismatch;
> + if ((oldflags & FSCONTEXT_MNT) && old->sid != new->sid)
> + goto mismatch;
> + if ((oldflags & CONTEXT_MNT) && old->mntpoint_sid != new->mntpoint_sid)
> + goto mismatch;
> + if ((oldflags & DEFCONTEXT_MNT) && old->def_sid != new->def_sid)
> + goto mismatch;
> + if (oldflags & ROOTCONTEXT_MNT) {
> + struct inode_security_struct *oldroot = oldsb->s_root->d_inode->i_security;
> + struct inode_security_struct *newroot = newsb->s_root->d_inode->i_security;
> + if (oldroot->sid != newroot->sid)
> + goto mismatch;
> + }
> + return 0;
> +mismatch:
> + printk(KERN_WARNING "SELinux: mount invalid. Same superblock, "
> + "different security settings for (dev %s, "
> + "type %s)\n", newsb->s_id, newsb->s_type->name);
> + return -EINVAL;
> +}
> +
> +static int selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
> struct super_block *newsb)
> {
> const struct superblock_security_struct *oldsbsec = oldsb->s_security;
> @@ -765,14 +795,14 @@ static void selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
> * mount options. thus we can safely deal with this superblock later
> */
> if (!ss_initialized)
> - return;
> + return 0;
>
> /* how can we clone if the old one wasn't set up?? */
> BUG_ON(!(oldsbsec->flags & SE_SBINITIALIZED));
>
> - /* if fs is reusing a sb, just let its options stand... */
> + /* if fs is reusing a sb, make sure that the contexts match */
> if (newsbsec->flags & SE_SBINITIALIZED)
> - return;
> + return selinux_cmp_sb_context(oldsb, newsb);
>
> mutex_lock(&newsbsec->lock);
>
> @@ -805,6 +835,7 @@ static void selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
>
> sb_finish_set_opts(newsb);
> mutex_unlock(&newsbsec->lock);
> + return 0;
> }
>
> static int selinux_parse_opts_str(char *options,


2013-04-02 00:43:49

by James Morris

[permalink] [raw]
Subject: Re: [PATCH v2] selinux: make security_sb_clone_mnt_opts return an error on context mismatch

Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next


--
James Morris
<[email protected]>

2013-04-01 12:14:31

by Jeff Layton

[permalink] [raw]
Subject: [PATCH v2] selinux: make security_sb_clone_mnt_opts return an error on context mismatch

I had the following problem reported a while back. If you mount the
same filesystem twice using NFSv4 with different contexts, then the
second context= option is ignored. For instance:

# mount server:/export /mnt/test1
# mount server:/export /mnt/test2 -o context=system_u:object_r:tmp_t:s0
# ls -dZ /mnt/test1
drwxrwxrwt. root root system_u:object_r:nfs_t:s0 /mnt/test1
# ls -dZ /mnt/test2
drwxrwxrwt. root root system_u:object_r:nfs_t:s0 /mnt/test2

When we call into SELinux to set the context of a "cloned" superblock,
it will currently just bail out when it notices that we're reusing an
existing superblock. Since the existing superblock is already set up and
presumably in use, we can't go overwriting its context with the one from
the "original" sb. Because of this, the second context= option in this
case cannot take effect.

This patch fixes this by turning security_sb_clone_mnt_opts into an int
return operation. When it finds that the "new" superblock that it has
been handed is already set up, it checks to see whether the contexts on
the old superblock match it. If it does, then it will just return
success, otherwise it'll return -EBUSY and emit a printk to tell the
admin why the second mount failed.

Note that this patch may cause casualties. The NFSv4 code relies on
being able to walk down to an export from the pseudoroot. If you mount
filesystems that are nested within one another with different contexts,
then this patch will make those mounts fail in new and "exciting" ways.

For instance, suppose that /export is a separate filesystem on the
server:

# mount server:/ /mnt/test1
# mount salusa:/export /mnt/test2 -o context=system_u:object_r:tmp_t:s0
mount.nfs: an incorrect mount option was specified

...with the printk in the ring buffer. Because we *might* eventually
walk down to /mnt/test1/export, the mount is denied due to this patch.
The second mount needs the pseudoroot superblock, but that's already
present with the wrong context.

OTOH, if we mount these in the reverse order, then both mounts work,
because the pseudoroot superblock created when mounting /export is
discarded once that mount is done. If we then however try to walk into
that directory, the automount fails for the similar reasons:

# cd /mnt/test1/scratch/
-bash: cd: /mnt/test1/scratch: Device or resource busy

The story I've gotten from the SELinux folks that I've talked to is that
this is desirable behavior. In SELinux-land, mounting the same data
under different contexts is wrong -- there can be only one.

Cc: Steve Dickson <[email protected]>
Cc: Stephen Smalley <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
Acked-by: Eric Paris <[email protected]>
---
fs/nfs/super.c | 3 +--
include/linux/security.h | 10 ++++++----
security/capability.c | 3 ++-
security/security.c | 4 ++--
security/selinux/hooks.c | 39 +++++++++++++++++++++++++++++++++++----
5 files changed, 46 insertions(+), 13 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 2f8a29d..76924ad 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -2381,10 +2381,9 @@ int nfs_clone_sb_security(struct super_block *s, struct dentry *mntroot,
struct nfs_mount_info *mount_info)
{
/* clone any lsm security options from the parent to the new sb */
- security_sb_clone_mnt_opts(mount_info->cloned->sb, s);
if (mntroot->d_inode->i_op != NFS_SB(s)->nfs_client->rpc_ops->dir_inode_ops)
return -ESTALE;
- return 0;
+ return security_sb_clone_mnt_opts(mount_info->cloned->sb, s);
}
EXPORT_SYMBOL_GPL(nfs_clone_sb_security);

diff --git a/include/linux/security.h b/include/linux/security.h
index eee7478..4c7058d 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1436,7 +1436,7 @@ struct security_operations {
struct path *new_path);
int (*sb_set_mnt_opts) (struct super_block *sb,
struct security_mnt_opts *opts);
- void (*sb_clone_mnt_opts) (const struct super_block *oldsb,
+ int (*sb_clone_mnt_opts) (const struct super_block *oldsb,
struct super_block *newsb);
int (*sb_parse_opts_str) (char *options, struct security_mnt_opts *opts);

@@ -1721,7 +1721,7 @@ int security_sb_mount(const char *dev_name, struct path *path,
int security_sb_umount(struct vfsmount *mnt, int flags);
int security_sb_pivotroot(struct path *old_path, struct path *new_path);
int security_sb_set_mnt_opts(struct super_block *sb, struct security_mnt_opts *opts);
-void security_sb_clone_mnt_opts(const struct super_block *oldsb,
+int security_sb_clone_mnt_opts(const struct super_block *oldsb,
struct super_block *newsb);
int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts);

@@ -2011,9 +2011,11 @@ static inline int security_sb_set_mnt_opts(struct super_block *sb,
return 0;
}

-static inline void security_sb_clone_mnt_opts(const struct super_block *oldsb,
+static inline int security_sb_clone_mnt_opts(const struct super_block *oldsb,
struct super_block *newsb)
-{ }
+{
+ return 0;
+}

static inline int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts)
{
diff --git a/security/capability.c b/security/capability.c
index 5797750..a6290b6 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -98,9 +98,10 @@ static int cap_sb_set_mnt_opts(struct super_block *sb,
return 0;
}

-static void cap_sb_clone_mnt_opts(const struct super_block *oldsb,
+static int cap_sb_clone_mnt_opts(const struct super_block *oldsb,
struct super_block *newsb)
{
+ return 0;
}

static int cap_sb_parse_opts_str(char *options, struct security_mnt_opts *opts)
diff --git a/security/security.c b/security/security.c
index 7b88c6a..108281d 100644
--- a/security/security.c
+++ b/security/security.c
@@ -299,10 +299,10 @@ int security_sb_set_mnt_opts(struct super_block *sb,
}
EXPORT_SYMBOL(security_sb_set_mnt_opts);

-void security_sb_clone_mnt_opts(const struct super_block *oldsb,
+int security_sb_clone_mnt_opts(const struct super_block *oldsb,
struct super_block *newsb)
{
- security_ops->sb_clone_mnt_opts(oldsb, newsb);
+ return security_ops->sb_clone_mnt_opts(oldsb, newsb);
}
EXPORT_SYMBOL(security_sb_clone_mnt_opts);

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 2fa28c8..3c02be3 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -750,7 +750,37 @@ out_double_mount:
goto out;
}

-static void selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
+static int selinux_cmp_sb_context(const struct super_block *oldsb,
+ const struct super_block *newsb)
+{
+ struct superblock_security_struct *old = oldsb->s_security;
+ struct superblock_security_struct *new = newsb->s_security;
+ char oldflags = old->flags & SE_MNTMASK;
+ char newflags = new->flags & SE_MNTMASK;
+
+ if (oldflags != newflags)
+ goto mismatch;
+ if ((oldflags & FSCONTEXT_MNT) && old->sid != new->sid)
+ goto mismatch;
+ if ((oldflags & CONTEXT_MNT) && old->mntpoint_sid != new->mntpoint_sid)
+ goto mismatch;
+ if ((oldflags & DEFCONTEXT_MNT) && old->def_sid != new->def_sid)
+ goto mismatch;
+ if (oldflags & ROOTCONTEXT_MNT) {
+ struct inode_security_struct *oldroot = oldsb->s_root->d_inode->i_security;
+ struct inode_security_struct *newroot = newsb->s_root->d_inode->i_security;
+ if (oldroot->sid != newroot->sid)
+ goto mismatch;
+ }
+ return 0;
+mismatch:
+ printk(KERN_WARNING "SELinux: mount invalid. Same superblock, "
+ "different security settings for (dev %s, "
+ "type %s)\n", newsb->s_id, newsb->s_type->name);
+ return -EBUSY;
+}
+
+static int selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
struct super_block *newsb)
{
const struct superblock_security_struct *oldsbsec = oldsb->s_security;
@@ -765,14 +795,14 @@ static void selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
* mount options. thus we can safely deal with this superblock later
*/
if (!ss_initialized)
- return;
+ return 0;

/* how can we clone if the old one wasn't set up?? */
BUG_ON(!(oldsbsec->flags & SE_SBINITIALIZED));

- /* if fs is reusing a sb, just let its options stand... */
+ /* if fs is reusing a sb, make sure that the contexts match */
if (newsbsec->flags & SE_SBINITIALIZED)
- return;
+ return selinux_cmp_sb_context(oldsb, newsb);

mutex_lock(&newsbsec->lock);

@@ -805,6 +835,7 @@ static void selinux_sb_clone_mnt_opts(const struct super_block *oldsb,

sb_finish_set_opts(newsb);
mutex_unlock(&newsbsec->lock);
+ return 0;
}

static int selinux_parse_opts_str(char *options,
--
1.7.11.7