From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751470AbdGRNhb (ORCPT ); Tue, 18 Jul 2017 09:37:31 -0400 Received: from out02.mta.xmission.com ([166.70.13.232]:60062 "EHLO out02.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751399AbdGRNh1 (ORCPT ); Tue, 18 Jul 2017 09:37:27 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: Vivek Goyal Cc: Stefan Berger , Stefan Berger , 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 References: <1499785511-17192-1-git-send-email-stefanb@linux.vnet.ibm.com> <1499785511-17192-2-git-send-email-stefanb@linux.vnet.ibm.com> <20170717185811.GC15794@redhat.com> <7a39e8a6-a33b-f6a8-3fd5-6211c075ab91@linux.vnet.ibm.com> <20170718114849.GA8233@redhat.com> <55971eea-fde2-439a-2fe5-d0ae5e80bc22@linux.vnet.ibm.com> <20170718123009.GB8233@redhat.com> <20170718123603.GC8233@redhat.com> Date: Tue, 18 Jul 2017 08:29:28 -0500 In-Reply-To: <20170718123603.GC8233@redhat.com> (Vivek Goyal's message of "Tue, 18 Jul 2017 08:36:03 -0400") Message-ID: <87shhtho07.fsf@xmission.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1dXSgn-0000bI-FE;;;mid=<87shhtho07.fsf@xmission.com>;;;hst=in01.mta.xmission.com;;;ip=67.3.213.87;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX19Tr9QCX0BngJNd6T23QeD9kbem3q3JOzs= X-SA-Exim-Connect-IP: 67.3.213.87 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 0.0 TVD_RCVD_IP Message was received from an IP address * 0.7 XMSubLong Long Subject * 0.0 T_TM2_M_HEADER_IN_MSG BODY: No description available. * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.4999] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa04 1397; Body=1 Fuz1=1 Fuz2=1] * 0.1 XMSolicitRefs_0 Weightloss drug X-Spam-DCC: XMission; sa04 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: ;Vivek Goyal X-Spam-Relay-Country: X-Spam-Timing: total 406 ms - load_scoreonly_sql: 0.04 (0.0%), signal_user_changed: 2.2 (0.5%), b_tie_ro: 1.63 (0.4%), parse: 0.75 (0.2%), extract_message_metadata: 3.5 (0.9%), get_uri_detail_list: 2.1 (0.5%), tests_pri_-1000: 3.2 (0.8%), tests_pri_-950: 0.93 (0.2%), tests_pri_-900: 0.78 (0.2%), tests_pri_-400: 23 (5.7%), check_bayes: 22 (5.5%), b_tokenize: 8 (2.0%), b_tok_get_all: 8 (1.9%), b_comp_prob: 2.1 (0.5%), b_tok_touch_all: 3.0 (0.8%), b_finish: 0.52 (0.1%), tests_pri_0: 362 (89.2%), check_dkim_signature: 0.44 (0.1%), check_dkim_adsp: 2.8 (0.7%), tests_pri_500: 3.3 (0.8%), rewrite_mail: 0.00 (0.0%) Subject: Re: [PATCH v2] xattr: Enable security.capability in user namespaces X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in01.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Vivek Goyal writes: > On Tue, Jul 18, 2017 at 08:30:09AM -0400, Vivek Goyal wrote: >> On Tue, Jul 18, 2017 at 08:05:18AM -0400, Stefan Berger wrote: >> > On 07/18/2017 07:48 AM, Vivek Goyal wrote: >> > > On Mon, Jul 17, 2017 at 04:50:22PM -0400, Stefan Berger wrote: >> > > > On 07/17/2017 02:58 PM, Vivek Goyal wrote: >> > > > > On Tue, Jul 11, 2017 at 11:05:11AM -0400, Stefan Berger wrote: >> > > > > >> > > > > [..] >> > > > > > +/* >> > > > > > + * xattr_list_userns_rewrite - Rewrite list of xattr names for user namespaces >> > > > > > + * or determine needed size for attribute list >> > > > > > + * in case size == 0 >> > > > > > + * >> > > > > > + * In a user namespace we do not present all extended attributes to the >> > > > > > + * user. We filter out those that are in the list of userns supported xattr. >> > > > > > + * Besides that we filter out those with @uid= when there is no mapping >> > > > > > + * for that uid in the current user namespace. >> > > > > > + * >> > > > > > + * @list: list of 0-byte separated xattr names >> > > > > > + * @size: the size of the list; may be 0 to determine needed list size >> > > > > > + * @list_maxlen: allocated buffer size of list >> > > > > > + */ >> > > > > > +static ssize_t >> > > > > > +xattr_list_userns_rewrite(char *list, ssize_t size, size_t list_maxlen) >> > > > > > +{ >> > > > > > + char *nlist = NULL; >> > > > > > + size_t s_off, len, nlen; >> > > > > > + ssize_t d_off; >> > > > > > + char *name, *newname; >> > > > > > + >> > > > > > + if (!list || size < 0 || current_user_ns() == &init_user_ns) >> > > > > size will never be less than 0 here. Only caller calls this function only >> > > > > if size is >0. So can we remove this? >> > > > Correct. >> > > > >> > > > > What about case of "!list". So if user space called listxattr(foo, NULL, >> > > > > 0), we want to return the size of buffer as if all the xattrs will be >> > > > > returned to user space. But in practice we probably will filter out some >> > > > > xattrs so actually returned string will be smaller than size reported >> > > > > previously. >> > > > This case of size=0 is a problem in userns. Depending on the mapping of the >> > > > userid's the list can expand. A security.foo@uid=100 can become >> > > > security.foo@uid=100000, if the mapping is set up so that uid 100 on the >> > > > host becomes uid 100000 inside the container. So for now we only have >> > > > security.capability and the way I solved this is by allocating a 65k buffer >> > > > when calling from a userns. In this buffer where we gather the xattr names >> > > > and then walk them to determine the size that's needed for the buffer by >> > > > simulating the rewriting. It's not nice but I don't know of any other >> > > > solution. >> > > Hi Stefan, >> > > >> > > For the case of size==0, why don't we iterate through all the xattr, >> > > filter them, remap them and then return the size to process in user >> > > namespace. That should fix this? I thought that's what >> > >> > >> > For the size==0 we need a temp. buffer where the raw xattr names are written >> > to so that the xattr_list_userns_rewrite() can actually rewrite what the >> > filesystem drivers returned. >> >> I am probably missing something, but for the case of size==0, we don't >> have to copy all xattrs. We just need to determine size. So we can walk >> through each xattr, remap it and add to the size. I mean there should not >> be a need to allocate this 65K buffer. Just enough space needed to be >> able to store remapped xattr. >> >> You are already doing it in xattr_parse_uid_from_kuid(). It returns the >> buffer containing remapped xattr. So we should be able to just determine >> the size and free the buffer. And do it for all the xattrs returned by >> filesystem. >> >> What am I missing? > > Oh, I think I get it. If I don't pass a buffer to underlying driver, then > it will just return the size (and not actual list). So that's why you are > allocating that big buffer and getting the whole list internally, doing > mapping and returning size to user space. Hmm... A valid reason to be leary of storing attributs in the xattrs. Eric From mboxrd@z Thu Jan 1 00:00:00 1970 From: ebiederm@xmission.com (Eric W. Biederman) Date: Tue, 18 Jul 2017 08:29:28 -0500 Subject: [PATCH v2] xattr: Enable security.capability in user namespaces In-Reply-To: <20170718123603.GC8233@redhat.com> (Vivek Goyal's message of "Tue, 18 Jul 2017 08:36:03 -0400") References: <1499785511-17192-1-git-send-email-stefanb@linux.vnet.ibm.com> <1499785511-17192-2-git-send-email-stefanb@linux.vnet.ibm.com> <20170717185811.GC15794@redhat.com> <7a39e8a6-a33b-f6a8-3fd5-6211c075ab91@linux.vnet.ibm.com> <20170718114849.GA8233@redhat.com> <55971eea-fde2-439a-2fe5-d0ae5e80bc22@linux.vnet.ibm.com> <20170718123009.GB8233@redhat.com> <20170718123603.GC8233@redhat.com> Message-ID: <87shhtho07.fsf@xmission.com> To: linux-security-module@vger.kernel.org List-Id: linux-security-module.vger.kernel.org Vivek Goyal writes: > On Tue, Jul 18, 2017 at 08:30:09AM -0400, Vivek Goyal wrote: >> On Tue, Jul 18, 2017 at 08:05:18AM -0400, Stefan Berger wrote: >> > On 07/18/2017 07:48 AM, Vivek Goyal wrote: >> > > On Mon, Jul 17, 2017 at 04:50:22PM -0400, Stefan Berger wrote: >> > > > On 07/17/2017 02:58 PM, Vivek Goyal wrote: >> > > > > On Tue, Jul 11, 2017 at 11:05:11AM -0400, Stefan Berger wrote: >> > > > > >> > > > > [..] >> > > > > > +/* >> > > > > > + * xattr_list_userns_rewrite - Rewrite list of xattr names for user namespaces >> > > > > > + * or determine needed size for attribute list >> > > > > > + * in case size == 0 >> > > > > > + * >> > > > > > + * In a user namespace we do not present all extended attributes to the >> > > > > > + * user. We filter out those that are in the list of userns supported xattr. >> > > > > > + * Besides that we filter out those with @uid= when there is no mapping >> > > > > > + * for that uid in the current user namespace. >> > > > > > + * >> > > > > > + * @list: list of 0-byte separated xattr names >> > > > > > + * @size: the size of the list; may be 0 to determine needed list size >> > > > > > + * @list_maxlen: allocated buffer size of list >> > > > > > + */ >> > > > > > +static ssize_t >> > > > > > +xattr_list_userns_rewrite(char *list, ssize_t size, size_t list_maxlen) >> > > > > > +{ >> > > > > > + char *nlist = NULL; >> > > > > > + size_t s_off, len, nlen; >> > > > > > + ssize_t d_off; >> > > > > > + char *name, *newname; >> > > > > > + >> > > > > > + if (!list || size < 0 || current_user_ns() == &init_user_ns) >> > > > > size will never be less than 0 here. Only caller calls this function only >> > > > > if size is >0. So can we remove this? >> > > > Correct. >> > > > >> > > > > What about case of "!list". So if user space called listxattr(foo, NULL, >> > > > > 0), we want to return the size of buffer as if all the xattrs will be >> > > > > returned to user space. But in practice we probably will filter out some >> > > > > xattrs so actually returned string will be smaller than size reported >> > > > > previously. >> > > > This case of size=0 is a problem in userns. Depending on the mapping of the >> > > > userid's the list can expand. A security.foo at uid=100 can become >> > > > security.foo at uid=100000, if the mapping is set up so that uid 100 on the >> > > > host becomes uid 100000 inside the container. So for now we only have >> > > > security.capability and the way I solved this is by allocating a 65k buffer >> > > > when calling from a userns. In this buffer where we gather the xattr names >> > > > and then walk them to determine the size that's needed for the buffer by >> > > > simulating the rewriting. It's not nice but I don't know of any other >> > > > solution. >> > > Hi Stefan, >> > > >> > > For the case of size==0, why don't we iterate through all the xattr, >> > > filter them, remap them and then return the size to process in user >> > > namespace. That should fix this? I thought that's what >> > >> > >> > For the size==0 we need a temp. buffer where the raw xattr names are written >> > to so that the xattr_list_userns_rewrite() can actually rewrite what the >> > filesystem drivers returned. >> >> I am probably missing something, but for the case of size==0, we don't >> have to copy all xattrs. We just need to determine size. So we can walk >> through each xattr, remap it and add to the size. I mean there should not >> be a need to allocate this 65K buffer. Just enough space needed to be >> able to store remapped xattr. >> >> You are already doing it in xattr_parse_uid_from_kuid(). It returns the >> buffer containing remapped xattr. So we should be able to just determine >> the size and free the buffer. And do it for all the xattrs returned by >> filesystem. >> >> What am I missing? > > Oh, I think I get it. If I don't pass a buffer to underlying driver, then > it will just return the size (and not actual list). So that's why you are > allocating that big buffer and getting the whole list internally, doing > mapping and returning size to user space. Hmm... A valid reason to be leary of storing attributs in the xattrs. Eric -- 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="===============6742068158481241637==" MIME-Version: 1.0 From: Eric W. Biederman To: lkp@lists.01.org Subject: Re: [PATCH v2] xattr: Enable security.capability in user namespaces Date: Tue, 18 Jul 2017 08:29:28 -0500 Message-ID: <87shhtho07.fsf@xmission.com> In-Reply-To: <20170718123603.GC8233@redhat.com> List-Id: --===============6742068158481241637== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Vivek Goyal writes: > On Tue, Jul 18, 2017 at 08:30:09AM -0400, Vivek Goyal wrote: >> On Tue, Jul 18, 2017 at 08:05:18AM -0400, Stefan Berger wrote: >> > On 07/18/2017 07:48 AM, Vivek Goyal wrote: >> > > On Mon, Jul 17, 2017 at 04:50:22PM -0400, Stefan Berger wrote: >> > > > On 07/17/2017 02:58 PM, Vivek Goyal wrote: >> > > > > On Tue, Jul 11, 2017 at 11:05:11AM -0400, Stefan Berger wrote: >> > > > > = >> > > > > [..] >> > > > > > +/* >> > > > > > + * xattr_list_userns_rewrite - Rewrite list of xattr names fo= r user namespaces >> > > > > > + * or determine needed size for a= ttribute list >> > > > > > + * in case size =3D=3D 0 >> > > > > > + * >> > > > > > + * In a user namespace we do not present all extended attribu= tes to the >> > > > > > + * user. We filter out those that are in the list of userns s= upported xattr. >> > > > > > + * Besides that we filter out those with @uid=3D when th= ere is no mapping >> > > > > > + * for that uid in the current user namespace. >> > > > > > + * >> > > > > > + * @list: list of 0-byte separated xattr names >> > > > > > + * @size: the size of the list; may be 0 to determine = needed list size >> > > > > > + * @list_maxlen: allocated buffer size of list >> > > > > > + */ >> > > > > > +static ssize_t >> > > > > > +xattr_list_userns_rewrite(char *list, ssize_t size, size_t li= st_maxlen) >> > > > > > +{ >> > > > > > + char *nlist =3D NULL; >> > > > > > + size_t s_off, len, nlen; >> > > > > > + ssize_t d_off; >> > > > > > + char *name, *newname; >> > > > > > + >> > > > > > + if (!list || size < 0 || current_user_ns() =3D=3D &init_user= _ns) >> > > > > size will never be less than 0 here. Only caller calls this func= tion only >> > > > > if size is >0. So can we remove this? >> > > > Correct. >> > > > = >> > > > > What about case of "!list". So if user space called listxattr(fo= o, NULL, >> > > > > 0), we want to return the size of buffer as if all the xattrs wi= ll be >> > > > > returned to user space. But in practice we probably will filter = out some >> > > > > xattrs so actually returned string will be smaller than size rep= orted >> > > > > previously. >> > > > This case of size=3D0 is a problem in userns. Depending on the map= ping of the >> > > > userid's the list can expand. A security.foo(a)uid=3D100 can become >> > > > security.foo(a)uid=3D100000, if the mapping is set up so that uid = 100 on the >> > > > host becomes uid 100000 inside the container. So for now we only h= ave >> > > > security.capability and the way I solved this is by allocating a 6= 5k buffer >> > > > when calling from a userns. In this buffer where we gather the xat= tr names >> > > > and then walk them to determine the size that's needed for the buf= fer by >> > > > simulating the rewriting. It's not nice but I don't know of any ot= her >> > > > solution. >> > > Hi Stefan, >> > > = >> > > For the case of size=3D=3D0, why don't we iterate through all the xa= ttr, >> > > filter them, remap them and then return the size to process in user >> > > namespace. That should fix this? I thought that's what >> > = >> > = >> > For the size=3D=3D0 we need a temp. buffer where the raw xattr names a= re written >> > to so that the xattr_list_userns_rewrite() can actually rewrite what t= he >> > filesystem drivers returned. >> = >> I am probably missing something, but for the case of size=3D=3D0, we don= 't >> have to copy all xattrs. We just need to determine size. So we can walk >> through each xattr, remap it and add to the size. I mean there should not >> be a need to allocate this 65K buffer. Just enough space needed to be >> able to store remapped xattr. >> = >> You are already doing it in xattr_parse_uid_from_kuid(). It returns the >> buffer containing remapped xattr. So we should be able to just determine >> the size and free the buffer. And do it for all the xattrs returned by >> filesystem. >> = >> What am I missing? > > Oh, I think I get it. If I don't pass a buffer to underlying driver, then > it will just return the size (and not actual list). So that's why you are > allocating that big buffer and getting the whole list internally, doing > mapping and returning size to user space. Hmm... A valid reason to be leary of storing attributs in the xattrs. Eric --===============6742068158481241637==--