From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754211AbdGLTT4 (ORCPT ); Wed, 12 Jul 2017 15:19:56 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:47652 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753593AbdGLTTz (ORCPT ); Wed, 12 Jul 2017 15:19:55 -0400 Subject: Re: [PATCH v2] xattr: Enable security.capability in user namespaces To: Vivek Goyal , Stefan Berger References: <1499785511-17192-1-git-send-email-stefanb@linux.vnet.ibm.com> <1499785511-17192-2-git-send-email-stefanb@linux.vnet.ibm.com> <20170712175357.GA32609@redhat.com> Cc: ebiederm@xmission.com, containers@lists.linux-foundation.org, lkp@01.org, linux-kernel@vger.kernel.org, zohar@linux.vnet.ibm.com, tycho@docker.com, serge@hallyn.com, James.Bottomley@HansenPartnership.com, christian.brauner@mailbox.org, amir73il@gmail.com, linux-security-module@vger.kernel.org, casey@schaufler-ca.com From: Stefan Berger Date: Wed, 12 Jul 2017 15:19:46 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20170712175357.GA32609@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 x-cbid: 17071219-0016-0000-0000-000007297016 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00007356; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000214; SDB=6.00886626; UDB=6.00442593; IPR=6.00666775; BA=6.00005468; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00016201; XFM=3.00000015; UTC=2017-07-12 19:19:51 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17071219-0017-0000-0000-00003A8CCD29 Message-Id: <3368afe9-1e0c-6f6a-aba6-9ce26d2e45e4@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-07-12_06:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=1 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1706020000 definitions=main-1707120312 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/12/2017 01:53 PM, Vivek Goyal wrote: > On Tue, Jul 11, 2017 at 11:05:11AM -0400, Stefan Berger wrote: > > [..] >> @@ -301,14 +721,39 @@ ssize_t >> __vfs_getxattr(struct dentry *dentry, struct inode *inode, const char *name, >> void *value, size_t size) >> { >> - const struct xattr_handler *handler; >> + const struct xattr_handler *handler = NULL; >> + char *newname = NULL; >> + int ret, userns_supt_xattr; >> + struct user_namespace *userns = current_user_ns(); >> + >> + userns_supt_xattr = (xattr_is_userns_supported(name, false) >= 0); >> + > Hi Stephan, > >> + do { >> + kfree(newname); >> + >> + newname = xattr_userns_name(name, userns); > ^^^ > Will name be pointing to a freed string in second iteration of loop. Fixing for v3. > >> + if (IS_ERR(newname)) >> + return PTR_ERR(newname); >> + >> + if (!handler) { >> + name = newname; > Here we assign name and at the beginning of second iteration we free > newname. > > Also I am not sure why do we do this assignment only if handler is NULL. The handler shouldn't change but this optimization isn't helpful. Fixed through this patch: https://github.com/stefanberger/linux/commit/10828401b29a13f8c56f8fad0c0fb2690e4af878 > > BTW, I set cap_sys_admin on a file outside usernamespace and then launched > user namespace (mapping 1000 to 0). And then tried to do getcap on file > and I am not seeing security.capability set by host. Not sure what am I > doing wrong. getxattr() seems to return -ENODATA. Still debugging it. This was a regression due to the bug in the loop. I didn't have a test case (with runc) for it, now I do. > > Also, have we resovled the question of stacked filesystem like overlayfs. > There we are switching creds to mounter's creds when doing operations on > underlying filesystem. I am concenrned does that mean, we will get and > return security.capability to caller in usernamespace instead of > security.capability@uid=1000. I would have to test this, otherwise I don't know. I'll try it out with Docker. Stefan > > Vivek > >> + handler = xattr_resolve_name(inode, &name); >> + if (IS_ERR(handler)) { >> + ret = PTR_ERR(handler); >> + goto out; >> + } >> + if (!handler->get) { >> + ret = -EOPNOTSUPP; >> + goto out; >> + } >> + } >> + ret = handler->get(handler, dentry, inode, name, value, size); >> + userns = userns->parent; >> + } while ((ret == -ENODATA) && userns && userns_supt_xattr); >> >> - handler = xattr_resolve_name(inode, &name); >> - if (IS_ERR(handler)) >> - return PTR_ERR(handler); >> - if (!handler->get) >> - return -EOPNOTSUPP; >> - return handler->get(handler, dentry, inode, name, value, size); >> +out: >> + kfree(newname); >> + return ret; >> } >> EXPORT_SYMBOL(__vfs_getxattr); >> > Thanks > Vivek > From mboxrd@z Thu Jan 1 00:00:00 1970 From: stefanb@linux.vnet.ibm.com (Stefan Berger) Date: Wed, 12 Jul 2017 15:19:46 -0400 Subject: [PATCH v2] xattr: Enable security.capability in user namespaces In-Reply-To: <20170712175357.GA32609@redhat.com> References: <1499785511-17192-1-git-send-email-stefanb@linux.vnet.ibm.com> <1499785511-17192-2-git-send-email-stefanb@linux.vnet.ibm.com> <20170712175357.GA32609@redhat.com> Message-ID: <3368afe9-1e0c-6f6a-aba6-9ce26d2e45e4@linux.vnet.ibm.com> To: linux-security-module@vger.kernel.org List-Id: linux-security-module.vger.kernel.org On 07/12/2017 01:53 PM, Vivek Goyal wrote: > On Tue, Jul 11, 2017 at 11:05:11AM -0400, Stefan Berger wrote: > > [..] >> @@ -301,14 +721,39 @@ ssize_t >> __vfs_getxattr(struct dentry *dentry, struct inode *inode, const char *name, >> void *value, size_t size) >> { >> - const struct xattr_handler *handler; >> + const struct xattr_handler *handler = NULL; >> + char *newname = NULL; >> + int ret, userns_supt_xattr; >> + struct user_namespace *userns = current_user_ns(); >> + >> + userns_supt_xattr = (xattr_is_userns_supported(name, false) >= 0); >> + > Hi Stephan, > >> + do { >> + kfree(newname); >> + >> + newname = xattr_userns_name(name, userns); > ^^^ > Will name be pointing to a freed string in second iteration of loop. Fixing for v3. > >> + if (IS_ERR(newname)) >> + return PTR_ERR(newname); >> + >> + if (!handler) { >> + name = newname; > Here we assign name and at the beginning of second iteration we free > newname. > > Also I am not sure why do we do this assignment only if handler is NULL. The handler shouldn't change but this optimization isn't helpful. Fixed through this patch: https://github.com/stefanberger/linux/commit/10828401b29a13f8c56f8fad0c0fb2690e4af878 > > BTW, I set cap_sys_admin on a file outside usernamespace and then launched > user namespace (mapping 1000 to 0). And then tried to do getcap on file > and I am not seeing security.capability set by host. Not sure what am I > doing wrong. getxattr() seems to return -ENODATA. Still debugging it. This was a regression due to the bug in the loop. I didn't have a test case (with runc) for it, now I do. > > Also, have we resovled the question of stacked filesystem like overlayfs. > There we are switching creds to mounter's creds when doing operations on > underlying filesystem. I am concenrned does that mean, we will get and > return security.capability to caller in usernamespace instead of > security.capability at uid=1000. I would have to test this, otherwise I don't know. I'll try it out with Docker. Stefan > > Vivek > >> + handler = xattr_resolve_name(inode, &name); >> + if (IS_ERR(handler)) { >> + ret = PTR_ERR(handler); >> + goto out; >> + } >> + if (!handler->get) { >> + ret = -EOPNOTSUPP; >> + goto out; >> + } >> + } >> + ret = handler->get(handler, dentry, inode, name, value, size); >> + userns = userns->parent; >> + } while ((ret == -ENODATA) && userns && userns_supt_xattr); >> >> - handler = xattr_resolve_name(inode, &name); >> - if (IS_ERR(handler)) >> - return PTR_ERR(handler); >> - if (!handler->get) >> - return -EOPNOTSUPP; >> - return handler->get(handler, dentry, inode, name, value, size); >> +out: >> + kfree(newname); >> + return ret; >> } >> EXPORT_SYMBOL(__vfs_getxattr); >> > Thanks > Vivek > -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============2916041202046665418==" MIME-Version: 1.0 From: Stefan Berger To: lkp@lists.01.org Subject: Re: [PATCH v2] xattr: Enable security.capability in user namespaces Date: Wed, 12 Jul 2017 15:19:46 -0400 Message-ID: <3368afe9-1e0c-6f6a-aba6-9ce26d2e45e4@linux.vnet.ibm.com> In-Reply-To: <20170712175357.GA32609@redhat.com> List-Id: --===============2916041202046665418== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable On 07/12/2017 01:53 PM, Vivek Goyal wrote: > On Tue, Jul 11, 2017 at 11:05:11AM -0400, Stefan Berger wrote: > > [..] >> @@ -301,14 +721,39 @@ ssize_t >> __vfs_getxattr(struct dentry *dentry, struct inode *inode, const char = *name, >> void *value, size_t size) >> { >> - const struct xattr_handler *handler; >> + const struct xattr_handler *handler =3D NULL; >> + char *newname =3D NULL; >> + int ret, userns_supt_xattr; >> + struct user_namespace *userns =3D current_user_ns(); >> + >> + userns_supt_xattr =3D (xattr_is_userns_supported(name, false) >=3D 0); >> + > Hi Stephan, > >> + do { >> + kfree(newname); >> + >> + newname =3D xattr_userns_name(name, userns); > ^^^ > Will name be pointing to a freed string in second iteration of loop. Fixing for v3. > >> + if (IS_ERR(newname)) >> + return PTR_ERR(newname); >> + >> + if (!handler) { >> + name =3D newname; > Here we assign name and at the beginning of second iteration we free > newname. > > Also I am not sure why do we do this assignment only if handler is NULL. The handler shouldn't change but this optimization isn't helpful. Fixed = through this patch: https://github.com/stefanberger/linux/commit/10828401b29a13f8c56f8fad0c0fb2= 690e4af878 > > BTW, I set cap_sys_admin on a file outside usernamespace and then launched > user namespace (mapping 1000 to 0). And then tried to do getcap on file > and I am not seeing security.capability set by host. Not sure what am I > doing wrong. getxattr() seems to return -ENODATA. Still debugging it. This was a regression due to the bug in the loop. I didn't have a test = case (with runc) for it, now I do. > > Also, have we resovled the question of stacked filesystem like overlayfs. > There we are switching creds to mounter's creds when doing operations on > underlying filesystem. I am concenrned does that mean, we will get and > return security.capability to caller in usernamespace instead of > security.capability(a)uid=3D1000. I would have to test this, otherwise I don't know. I'll try it out with = Docker. Stefan > > Vivek > >> + handler =3D xattr_resolve_name(inode, &name); >> + if (IS_ERR(handler)) { >> + ret =3D PTR_ERR(handler); >> + goto out; >> + } >> + if (!handler->get) { >> + ret =3D -EOPNOTSUPP; >> + goto out; >> + } >> + } >> + ret =3D handler->get(handler, dentry, inode, name, value, size); >> + userns =3D userns->parent; >> + } while ((ret =3D=3D -ENODATA) && userns && userns_supt_xattr); >> = >> - handler =3D xattr_resolve_name(inode, &name); >> - if (IS_ERR(handler)) >> - return PTR_ERR(handler); >> - if (!handler->get) >> - return -EOPNOTSUPP; >> - return handler->get(handler, dentry, inode, name, value, size); >> +out: >> + kfree(newname); >> + return ret; >> } >> EXPORT_SYMBOL(__vfs_getxattr); >> = > Thanks > Vivek > --===============2916041202046665418==--