On Thu, Jul 7, 2016 at 11:44 PM, Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote: > On 7/7/2016 1:33 PM, Vivek Goyal wrote: >> On Tue, Jul 05, 2016 at 12:36:17PM -0700, Casey Schaufler wrote: >>> On 7/5/2016 8:50 AM, Vivek Goyal wrote: >>>> Provide a security hook to label new file correctly when a file is copied >>>> up from lower layer to upper layer of a overlay/union mount. >>>> >>>> This hook can prepare and switch to a new set of creds which are suitable >>>> for new file creation during copy up. Caller should revert to old creds >>>> after file creation. >>>> >>>> In SELinux, newly copied up file gets same label as lower file for >>>> non-context mounts. But it gets label specified in mount option context= >>>> for context mounts. >>>> >>>> Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx> >>>> --- >>>> fs/overlayfs/copy_up.c | 8 ++++++++ >>>> include/linux/lsm_hooks.h | 13 +++++++++++++ >>>> include/linux/security.h | 6 ++++++ >>>> security/security.c | 8 ++++++++ >>>> security/selinux/hooks.c | 27 +++++++++++++++++++++++++++ >>>> 5 files changed, 62 insertions(+) >>>> >>>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c >>>> index 80aa6f1..90dc362 100644 >>>> --- a/fs/overlayfs/copy_up.c >>>> +++ b/fs/overlayfs/copy_up.c >>>> @@ -246,6 +246,7 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir, >>>> struct dentry *upper = NULL; >>>> umode_t mode = stat->mode; >>>> int err; >>>> + const struct cred *old_creds = NULL; >>>> >>>> newdentry = ovl_lookup_temp(workdir, dentry); >>>> err = PTR_ERR(newdentry); >>>> @@ -258,10 +259,17 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir, >>>> if (IS_ERR(upper)) >>>> goto out1; >>>> >>>> + err = security_inode_copy_up(dentry, &old_creds); >>>> + if (err < 0) >>>> + goto out2; >>>> + >>>> /* Can't properly set mode on creation because of the umask */ >>>> stat->mode &= S_IFMT; >>>> err = ovl_create_real(wdir, newdentry, stat, link, NULL, true); >>>> stat->mode = mode; >>>> + if (old_creds) >>>> + revert_creds(old_creds); >>>> + >>>> if (err) >>>> goto out2; >>> I don't much care for the way part of the credential manipulation >>> is done in the caller and part is done the the security module. >>> If the caller is going to restore the old state, the caller should >>> save the old state. Conversely if the SM is setting the state it should restore it. This needs yet another hook, but that's fine, I think. >> One advantage of current patches is that we switch to new creds only if >> it is needed. For example, if there are no LSMs loaded, > > Point. > >> then there is >> no need to modify creds and make a switch to new creds. > > I'm not a fan of cred flipping. There are too many ways for it to go > wrong. Consider interrupts. I assume you've ruled that out as a possibility > in the caller, but I still think the practice is dangerous. > > I greatly prefer "create and set attributes" to "change cred, create and > reset cred". I know that has it's own set of problems, including races > and faking privilege. Yeah, we've talked about this. The races can be eliminated by always doing the create in a the temporary "workdir" area and atomically renaming to the final destination after everything has been set up. OTOH that has a performance impact that the cred flipping eliminates. >> But if I start allocating new creds and save old state in caller, then >> caller always has to do it (irrespective of the fact whether any LSM >> modified the creds or not). > > It starts getting messy when I have two modules that want to > change change the credential. Each module will have to check to > see if a module called before it has allocated a new cred. Doesn't seem to me too difficult: check if *credp == NULL and allocate if so. Can even invent a heper for this if needed. Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
