On 7/22/2015 8:56 AM, Seth Forshee wrote:
> On Tue, Jul 21, 2015 at 06:52:31PM -0700, Casey Schaufler wrote:
>> On 7/21/2015 1:35 PM, Seth Forshee wrote:
>>> On Thu, Jul 16, 2015 at 05:59:22PM -0700, Andy Lutomirski wrote:
>>>> On Thu, Jul 16, 2015 at 5:45 PM, Casey Schaufler <ca...@schaufler-ca.com>
>>>> wrote:
>>>>> On 7/16/2015 4:29 PM, Andy Lutomirski wrote:
>>>>>> I really don't see the benefit of making up extra rules that apply to
>>>>>> users outside a userns who try to access specifically a filesystem
>>>>>> with backing store. They wouldn't make sense for filesystems without
>>>>>> backing store.
>>>>> Sure it would. For Smack, it would be the label a file would be
>>>>> created with, which would be the label of the process creating
>>>>> the memory based filesystem. For SELinux the rules are more a
>>>>> touch more sophisticated, but I'm sure that Paul or Stephen could
>>>>> come up with how to determine it.
>>>>>
>>>>> The point, looping all the way back to the beginning, where we
>>>>> were talking about just ignoring the labels on the filesystem,
>>>>> is that if you use the same Smack label on the files in the
>>>>> filesystem as the backing store file has, we'll all be happy.
>>>>> If that label isn't something user can write to, he won't be
>>>>> able to write to the mounted objects, either. If there is no
>>>>> backing store then use the label of the process creating the
>>>>> filesystem, which will be the user, which will mean everything
>>>>> will work hunky dory.
>>>>>
>>>>> Yes, there's work involved, but I doubt there's a lot. Getting
>>>>> the label from the backing store or the creating process is
>>>>> simple enough.
>>>>>
>>> So something like the diff below (untested)?
>> I think that this is close, and quite good for someone
>> who isn't very familiar with Smack. It's definitely headed
>> in the right direction.
>>
>>> All I'm really doing is setting smk_default as you describe above and
>>> then using it instead of smk_of_current() in
>>> smack_inode_alloc_security() and instead of the label from the disk in
>>> smack_d_instantiate().
>> Let's say your backing store is a file labeled Rubble.
>>
>> mount -o smackfsroot=Rubble,smackfsdef=Rubble ...
>>
>> It is completely reasonable for a process labeled Flintstone to
>> have rwxa access to a file labeled Rubble.
>>
>> Smack rule: Flintstone Rubble rwxa
>>
>> In the case of writing to an existing Rubble file, what you
>> have looks fine. What's not so great is that if the Flintstone
>> process creates a file, it should be labeled Flintstone. Your
>> use of the smk_default, which is going to violate the principle
>> of least astonishment, and break the Smack policy as well.
>>
>> Let's make a minor change. Instead of using smackfsroot let's
>> use smackfstransmute and a slightly different access rule:
>>
>> mount -o smackfstransmute=Rubble,smackfsdef=Rubble ...
>>
>> Smack rule: Flintstone Rubble rwxat
>>
>> Now the only change we have to make to the Smack code is
>> that we don't want to create any files unless either the
>> process is labeled Rubble or the rule allowing the creation
>> has the "t" for transmute access. That should ensure that
>> everything is labeled Rubble. If it isn't, someone has mucked
>> with the metadata in a detectable way.
> All right, that kind of makes sense, but I'm still missing some pieces.
> Questions follow.
>
>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>>> index 32f598db0b0d..4597420ab933 100644
>>> --- a/include/linux/fs.h
>>> +++ b/include/linux/fs.h
>>> @@ -1486,6 +1486,10 @@ static inline void sb_start_intwrite(struct
>>> super_block *sb)
>>> __sb_start_write(sb, SB_FREEZE_FS, true);
>>> }
>>>
>>> +static inline bool sb_in_userns(struct super_block *sb)
>>> +{
>>> + return sb->s_user_ns != &init_user_ns;
>>> +}
>>>
>>> extern bool inode_owner_or_capable(const struct inode *inode);
>>>
>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>>> index a143328f75eb..591fd19294e7 100644
>>> --- a/security/smack/smack_lsm.c
>>> +++ b/security/smack/smack_lsm.c
>>> @@ -255,6 +255,10 @@ static struct smack_known *smk_fetch(const char *name,
>>> struct inode *ip,
>>> char *buffer;
>>> struct smack_known *skp = NULL;
>>>
>>> + /* Should never fetch xattrs from untrusted mounts */
>>> + if (WARN_ON(sb_in_userns(ip->i_sb)))
>>> + return ERR_PTR(-EPERM);
>>> +
>> Go ahead and fetch it, we'll check to make sure it's viable later.
>>
>>> if (ip->i_op->getxattr == NULL)
>>> return ERR_PTR(-EOPNOTSUPP);
>>>
>>> @@ -656,10 +660,14 @@ static int smack_sb_kern_mount(struct super_block
>>> *sb, int flags, void *data)
>>> */
>>> if (specified)
>>> return -EPERM;
>>> +
>>> /*
>>> - * Unprivileged mounts get root and default from the caller.
>>> + * User namespace mounts get root and default from the backing
>>> + * store, if there is one. Other unprivileged mounts get them
>>> + * from the caller.
>>> */
>>> - skp = smk_of_current();
>>> + skp = (sb_in_userns(sb) && sb->s_bdev) ?
>>> + smk_of_inode(sb->s_bdev->bd_inode) : smk_of_current();
>>> sp->smk_root = skp;
>>> sp->smk_default = skp;
>> sp->smk_flags |= SMK_INODE_TRANSMUTE;
> I assume that you meant skp and not sp here.
Actually, neither is correct. You want to set SMK_INODE_TRANSMUTE
in the smk_flags field of the root inode. That's easy:
transmute = 1;
and the code after "Initialize the root inode" will take care of it.
>>> }
>>> @@ -792,7 +800,12 @@ static int smack_bprm_secureexec(struct linux_binprm
>>> *bprm)
>>> */
>>> static int smack_inode_alloc_security(struct inode *inode)
>>> {
>>> - struct smack_known *skp = smk_of_current();
>>> + struct smack_known *skp;
>>> +
>>> + if (sb_in_userns(inode->i_sb))
>>> + skp = ((struct superblock_smack
>>> *)(inode->i_sb->s_security))->smk_default;
>>> + else
>>> + skp = smk_of_current();
>> This should be left alone.
>> smack_inode_init_security is where you could disallow access that doesn't
>> legitimately result in a Rubble label on the file. It's something like
>>
>> ... after the call may = smk_access_entry(...)
>> if (sb_in_userns(inode->i_sb))
>> if (skp != dsp && (may & MAY_TRANSMUTE) == 0)
>> return -EACCES;
> I'm not getting how this covers all cases.
>
> So we've set the transmute flag on the root inode. Files and directories
> created in the root directory get the same label, and directories also
> get the transmute attribute. That's all fine.
>
> What about an existing directory in the filesystem that already has a
> Slate label? I'm not getting what happens with this directory, or for
> new files created in this directory, which also relates to my other
> questions below.
>
> Also an aside - smk_access_entry looks weird. may is initialized to
> -ENOENT, and then rule_list is searched for a rule which matches the
> object and subject labels. Presumably it's possible that no rule could
> be found, otherwise the prior initialization of may is pointless. If
> this happens the following code treats it as though it always contains
> access flags even though it might contain -ENOENT. Nothing bad actually
> happens with a two's compliement representation of -ENOENT since it will
> just set a bit that's already set, but it still seems like it should
> have a may > 0 condition, for clarity if for no other reason.
My suggested code is just wrong. I wasn't looking at the whole code,
only the patch, and got myself confused. Apologies.
If we want to go straight for the jugular how about this? I'm assuming
that inode->i_sb->s_bdev->bd_inode is the inode of the backing store.
static int smack_inode_permission(struct inode *inode, int mask)
{
struct smk_audit_info ad;
int no_block = mask & MAY_NOT_BLOCK;
int rc;
mask &= (MAY_READ|MAY_WRITE|MAY_EXEC|MAY_APPEND);
/*
* No permission to check. Existence test. Yup, it's there.
*/
if (mask == 0)
return 0;
+ if (sb_in_userns(inode->i_sb)) &&
+ smk_of_inode(inode) != smk_of_inode(inode->i_sb->s_bdev->bd_inode))
+ return -EACCES;
+
/* May be droppable after audit */
if (no_block)
return -ECHILD;
smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_INODE);
smk_ad_setfield_u_fs_inode(&ad, inode);
rc = smk_curacc(smk_of_inode(inode), mask, &ad);
rc = smk_bu_inode(inode, mask, rc);
return rc;
}
>
>>> inode->i_security = new_inode_smack(skp);
>>> if (inode->i_security == NULL)
>>> @@ -3175,6 +3188,11 @@ static void smack_d_instantiate(struct dentry
>>> *opt_dentry, struct inode *inode)
>>> break;
>>> }
>>> /*
>>> + * Don't use labels from xattrs for unprivileged mounts.
>>> + */
>>> + if (sb_in_userns(inode->i_sb))
>>> + break;
>>> + /*
>> Again, use the label. Just check to make sure it's what you expect.
> What happens if it's not what I expect? smack_d_instantiate cannot fail
> ... so just use the default label? In that case why bother reading it at
> all? Or would we actually want to change the on-disk label if it didn't
> match?
>
>>> * No xattr support means, alas, no SMACK label.
>>> * Use the aforeapplied default.
>>> * It would be curious if the label of the task
>> Also untested.
>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>> the body of a message to majord...@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at http://www.tux.org/lkml/
>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
