diff mbox series

[v20,05/23] net: Prepare UDS for security module stacking

Message ID 20200826145247.10029-6-casey@schaufler-ca.com
State New
Headers show
Series LSM: Module stacking for AppArmor | expand

Commit Message

Casey Schaufler Aug. 26, 2020, 2:52 p.m. UTC
Change the data used in UDS SO_PEERSEC processing from a
secid to a more general struct lsmblob. Update the
security_socket_getpeersec_dgram() interface to use the
lsmblob. There is a small amount of scaffolding code
that will come out when the security_secid_to_secctx()
code is brought in line with the lsmblob.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
 include/linux/security.h |  7 +++++--
 include/net/af_unix.h    |  2 +-
 include/net/scm.h        |  8 +++++---
 net/ipv4/ip_sockglue.c   |  8 +++++---
 net/unix/af_unix.c       |  6 +++---
 security/security.c      | 18 +++++++++++++++---
 6 files changed, 34 insertions(+), 15 deletions(-)

Comments

James Morris Sept. 3, 2020, 4:28 p.m. UTC | #1
On Wed, 26 Aug 2020, Casey Schaufler wrote:

> Change the data used in UDS SO_PEERSEC processing from a
> secid to a more general struct lsmblob. Update the
> security_socket_getpeersec_dgram() interface to use the
> lsmblob. There is a small amount of scaffolding code
> that will come out when the security_secid_to_secctx()
> code is brought in line with the lsmblob.
> 
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>

This needs some review by networking folk, and/or LSM maintainers. You 
should probably cc netdev on anything touching the networking code.

> ---
>  include/linux/security.h |  7 +++++--
>  include/net/af_unix.h    |  2 +-
>  include/net/scm.h        |  8 +++++---
>  net/ipv4/ip_sockglue.c   |  8 +++++---
>  net/unix/af_unix.c       |  6 +++---
>  security/security.c      | 18 +++++++++++++++---
>  6 files changed, 34 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/security.h b/include/linux/security.h
> index e2ef982b3dd7..ae623b89cdf4 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1398,7 +1398,8 @@ int security_socket_shutdown(struct socket *sock, int how);
>  int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb);
>  int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
>  				      int __user *optlen, unsigned len);
> -int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid);
> +int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb,
> +				     struct lsmblob *blob);
>  int security_sk_alloc(struct sock *sk, int family, gfp_t priority);
>  void security_sk_free(struct sock *sk);
>  void security_sk_clone(const struct sock *sk, struct sock *newsk);
> @@ -1536,7 +1537,9 @@ static inline int security_socket_getpeersec_stream(struct socket *sock, char __
>  	return -ENOPROTOOPT;
>  }
>  
> -static inline int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid)
> +static inline int security_socket_getpeersec_dgram(struct socket *sock,
> +						   struct sk_buff *skb,
> +						   struct lsmblob *blob)
>  {
>  	return -ENOPROTOOPT;
>  }
> diff --git a/include/net/af_unix.h b/include/net/af_unix.h
> index f42fdddecd41..a86da0cb5ec1 100644
> --- a/include/net/af_unix.h
> +++ b/include/net/af_unix.h
> @@ -36,7 +36,7 @@ struct unix_skb_parms {
>  	kgid_t			gid;
>  	struct scm_fp_list	*fp;		/* Passed files		*/
>  #ifdef CONFIG_SECURITY_NETWORK
> -	u32			secid;		/* Security ID		*/
> +	struct lsmblob		lsmblob;	/* Security LSM data	*/
>  #endif
>  	u32			consumed;
>  } __randomize_layout;
> diff --git a/include/net/scm.h b/include/net/scm.h
> index 1ce365f4c256..e2e71c4bf9d0 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -33,7 +33,7 @@ struct scm_cookie {
>  	struct scm_fp_list	*fp;		/* Passed files		*/
>  	struct scm_creds	creds;		/* Skb credentials	*/
>  #ifdef CONFIG_SECURITY_NETWORK
> -	u32			secid;		/* Passed security ID 	*/
> +	struct lsmblob		lsmblob;	/* Passed LSM data	*/
>  #endif
>  };
>  
> @@ -46,7 +46,7 @@ struct scm_fp_list *scm_fp_dup(struct scm_fp_list *fpl);
>  #ifdef CONFIG_SECURITY_NETWORK
>  static __inline__ void unix_get_peersec_dgram(struct socket *sock, struct scm_cookie *scm)
>  {
> -	security_socket_getpeersec_dgram(sock, NULL, &scm->secid);
> +	security_socket_getpeersec_dgram(sock, NULL, &scm->lsmblob);
>  }
>  #else
>  static __inline__ void unix_get_peersec_dgram(struct socket *sock, struct scm_cookie *scm)
> @@ -97,7 +97,9 @@ static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct sc
>  	int err;
>  
>  	if (test_bit(SOCK_PASSSEC, &sock->flags)) {
> -		err = security_secid_to_secctx(scm->secid, &secdata, &seclen);
> +		/* Scaffolding - it has to be element 0 for now */
> +		err = security_secid_to_secctx(scm->lsmblob.secid[0],
> +					       &secdata, &seclen);
>  
>  		if (!err) {
>  			put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, seclen, secdata);
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index d2c223554ff7..551dfbc717e9 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -130,15 +130,17 @@ static void ip_cmsg_recv_checksum(struct msghdr *msg, struct sk_buff *skb,
>  
>  static void ip_cmsg_recv_security(struct msghdr *msg, struct sk_buff *skb)
>  {
> +	struct lsmblob lb;
>  	char *secdata;
> -	u32 seclen, secid;
> +	u32 seclen;
>  	int err;
>  
> -	err = security_socket_getpeersec_dgram(NULL, skb, &secid);
> +	err = security_socket_getpeersec_dgram(NULL, skb, &lb);
>  	if (err)
>  		return;
>  
> -	err = security_secid_to_secctx(secid, &secdata, &seclen);
> +	/* Scaffolding - it has to be element 0 */
> +	err = security_secid_to_secctx(lb.secid[0], &secdata, &seclen);
>  	if (err)
>  		return;
>  
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 181ea6fb56a6..c15668b80d1d 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -138,17 +138,17 @@ static struct hlist_head *unix_sockets_unbound(void *addr)
>  #ifdef CONFIG_SECURITY_NETWORK
>  static void unix_get_secdata(struct scm_cookie *scm, struct sk_buff *skb)
>  {
> -	UNIXCB(skb).secid = scm->secid;
> +	UNIXCB(skb).lsmblob = scm->lsmblob;
>  }
>  
>  static inline void unix_set_secdata(struct scm_cookie *scm, struct sk_buff *skb)
>  {
> -	scm->secid = UNIXCB(skb).secid;
> +	scm->lsmblob = UNIXCB(skb).lsmblob;
>  }
>  
>  static inline bool unix_secdata_eq(struct scm_cookie *scm, struct sk_buff *skb)
>  {
> -	return (scm->secid == UNIXCB(skb).secid);
> +	return lsmblob_equal(&scm->lsmblob, &(UNIXCB(skb).lsmblob));
>  }
>  #else
>  static inline void unix_get_secdata(struct scm_cookie *scm, struct sk_buff *skb)
> diff --git a/security/security.c b/security/security.c
> index d6d882b1f7d5..c42873876954 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2219,10 +2219,22 @@ int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
>  				optval, optlen, len);
>  }
>  
> -int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid)
> +int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb,
> +				     struct lsmblob *blob)
>  {
> -	return call_int_hook(socket_getpeersec_dgram, -ENOPROTOOPT, sock,
> -			     skb, secid);
> +	struct security_hook_list *hp;
> +	int rc = -ENOPROTOOPT;
> +
> +	hlist_for_each_entry(hp, &security_hook_heads.socket_getpeersec_dgram,
> +			     list) {
> +		if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> +			continue;
> +		rc = hp->hook.socket_getpeersec_dgram(sock, skb,
> +						&blob->secid[hp->lsmid->slot]);
> +		if (rc != 0)
> +			break;
> +	}
> +	return rc;
>  }
>  EXPORT_SYMBOL(security_socket_getpeersec_dgram);
>  
>
Paul Moore Sept. 4, 2020, 8:08 p.m. UTC | #2
On Wed, Aug 26, 2020 at 11:07 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> Change the data used in UDS SO_PEERSEC processing from a
> secid to a more general struct lsmblob. Update the
> security_socket_getpeersec_dgram() interface to use the
> lsmblob. There is a small amount of scaffolding code
> that will come out when the security_secid_to_secctx()
> code is brought in line with the lsmblob.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
>  include/linux/security.h |  7 +++++--
>  include/net/af_unix.h    |  2 +-
>  include/net/scm.h        |  8 +++++---
>  net/ipv4/ip_sockglue.c   |  8 +++++---
>  net/unix/af_unix.c       |  6 +++---
>  security/security.c      | 18 +++++++++++++++---
>  6 files changed, 34 insertions(+), 15 deletions(-)

...

> diff --git a/include/net/af_unix.h b/include/net/af_unix.h
> index f42fdddecd41..a86da0cb5ec1 100644
> --- a/include/net/af_unix.h
> +++ b/include/net/af_unix.h
> @@ -36,7 +36,7 @@ struct unix_skb_parms {
>         kgid_t                  gid;
>         struct scm_fp_list      *fp;            /* Passed files         */
>  #ifdef CONFIG_SECURITY_NETWORK
> -       u32                     secid;          /* Security ID          */
> +       struct lsmblob          lsmblob;        /* Security LSM data    */

As mentioned in a previous revision, I remain concerned that this is
going to become a problem due to the size limit on unix_skb_parms.  I
would need to redo the math to be certain, but if I recall correctly
this would limit us to five LSMs assuming both that we don't need to
grow the per-LSM size of lsmblob *and* the netdev folks don't decide
to add more fields to the unix_skb_parms.

I lost track of that earlier discussion so I'm not sure where it ended
up, but if there is a viable alternative it might be a good idea to
pursue it.
Casey Schaufler Sept. 4, 2020, 9:35 p.m. UTC | #3
On 9/4/2020 1:08 PM, Paul Moore wrote:
> On Wed, Aug 26, 2020 at 11:07 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> Change the data used in UDS SO_PEERSEC processing from a
>> secid to a more general struct lsmblob. Update the
>> security_socket_getpeersec_dgram() interface to use the
>> lsmblob. There is a small amount of scaffolding code
>> that will come out when the security_secid_to_secctx()
>> code is brought in line with the lsmblob.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> ---
>>  include/linux/security.h |  7 +++++--
>>  include/net/af_unix.h    |  2 +-
>>  include/net/scm.h        |  8 +++++---
>>  net/ipv4/ip_sockglue.c   |  8 +++++---
>>  net/unix/af_unix.c       |  6 +++---
>>  security/security.c      | 18 +++++++++++++++---
>>  6 files changed, 34 insertions(+), 15 deletions(-)
> ...
>
>> diff --git a/include/net/af_unix.h b/include/net/af_unix.h
>> index f42fdddecd41..a86da0cb5ec1 100644
>> --- a/include/net/af_unix.h
>> +++ b/include/net/af_unix.h
>> @@ -36,7 +36,7 @@ struct unix_skb_parms {
>>         kgid_t                  gid;
>>         struct scm_fp_list      *fp;            /* Passed files         */
>>  #ifdef CONFIG_SECURITY_NETWORK
>> -       u32                     secid;          /* Security ID          */
>> +       struct lsmblob          lsmblob;        /* Security LSM data    */
> As mentioned in a previous revision, I remain concerned that this is
> going to become a problem due to the size limit on unix_skb_parms.  I
> would need to redo the math to be certain, but if I recall correctly
> this would limit us to five LSMs assuming both that we don't need to
> grow the per-LSM size of lsmblob *and* the netdev folks don't decide
> to add more fields to the unix_skb_parms.
>
> I lost track of that earlier discussion so I'm not sure where it ended
> up, but if there is a viable alternative it might be a good idea to
> pursue it.

Stephen had concerns about the lifecycle management involved. He also
pointed out that I had taken a cowards way out when allocations failed.
That could result in unexpected behavior when an allocation failed.
Fixing that would have required a major re-write of the currently simple
UDS attribute code, which I suspect would be as hard a sell to netdev as
expanding the secid to a lsmblob. I also thought I'd gotten netdev on the
CC: for this patch, but it looks like I missed it.

I did start on the UDS attribute re-do, and discovered that I was going
to have to introduce new failure paths, and that it might not be possible
to maintain compatibility for all cases because of the new possibilities
of failure.
Paul Moore Sept. 4, 2020, 9:53 p.m. UTC | #4
On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 9/4/2020 1:08 PM, Paul Moore wrote:
> > On Wed, Aug 26, 2020 at 11:07 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> Change the data used in UDS SO_PEERSEC processing from a
> >> secid to a more general struct lsmblob. Update the
> >> security_socket_getpeersec_dgram() interface to use the
> >> lsmblob. There is a small amount of scaffolding code
> >> that will come out when the security_secid_to_secctx()
> >> code is brought in line with the lsmblob.
> >>
> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> >> ---
> >>  include/linux/security.h |  7 +++++--
> >>  include/net/af_unix.h    |  2 +-
> >>  include/net/scm.h        |  8 +++++---
> >>  net/ipv4/ip_sockglue.c   |  8 +++++---
> >>  net/unix/af_unix.c       |  6 +++---
> >>  security/security.c      | 18 +++++++++++++++---
> >>  6 files changed, 34 insertions(+), 15 deletions(-)
> > ...
> >
> >> diff --git a/include/net/af_unix.h b/include/net/af_unix.h
> >> index f42fdddecd41..a86da0cb5ec1 100644
> >> --- a/include/net/af_unix.h
> >> +++ b/include/net/af_unix.h
> >> @@ -36,7 +36,7 @@ struct unix_skb_parms {
> >>         kgid_t                  gid;
> >>         struct scm_fp_list      *fp;            /* Passed files         */
> >>  #ifdef CONFIG_SECURITY_NETWORK
> >> -       u32                     secid;          /* Security ID          */
> >> +       struct lsmblob          lsmblob;        /* Security LSM data    */
> > As mentioned in a previous revision, I remain concerned that this is
> > going to become a problem due to the size limit on unix_skb_parms.  I
> > would need to redo the math to be certain, but if I recall correctly
> > this would limit us to five LSMs assuming both that we don't need to
> > grow the per-LSM size of lsmblob *and* the netdev folks don't decide
> > to add more fields to the unix_skb_parms.
> >
> > I lost track of that earlier discussion so I'm not sure where it ended
> > up, but if there is a viable alternative it might be a good idea to
> > pursue it.
>
> Stephen had concerns about the lifecycle management involved. He also
> pointed out that I had taken a cowards way out when allocations failed.
> That could result in unexpected behavior when an allocation failed.
> Fixing that would have required a major re-write of the currently simple
> UDS attribute code, which I suspect would be as hard a sell to netdev as
> expanding the secid to a lsmblob. I also thought I'd gotten netdev on the
> CC: for this patch, but it looks like I missed it.
>
> I did start on the UDS attribute re-do, and discovered that I was going
> to have to introduce new failure paths, and that it might not be possible
> to maintain compatibility for all cases because of the new possibilities
> of failure.

... and you're hoping to not be responsible for this code by the time
this becomes a limiting issue? ;)

I understand the concerns you mention, they are all valid as far as
I'm concerned, but I think we are going to get burned by this code as
it currently stands.
Casey Schaufler Sept. 4, 2020, 11:58 p.m. UTC | #5
On 9/4/2020 2:53 PM, Paul Moore wrote:
> On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 9/4/2020 1:08 PM, Paul Moore wrote:
>>> On Wed, Aug 26, 2020 at 11:07 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> Change the data used in UDS SO_PEERSEC processing from a
>>>> secid to a more general struct lsmblob. Update the
>>>> security_socket_getpeersec_dgram() interface to use the
>>>> lsmblob. There is a small amount of scaffolding code
>>>> that will come out when the security_secid_to_secctx()
>>>> code is brought in line with the lsmblob.
>>>>
>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>>> ---
>>>>  include/linux/security.h |  7 +++++--
>>>>  include/net/af_unix.h    |  2 +-
>>>>  include/net/scm.h        |  8 +++++---
>>>>  net/ipv4/ip_sockglue.c   |  8 +++++---
>>>>  net/unix/af_unix.c       |  6 +++---
>>>>  security/security.c      | 18 +++++++++++++++---
>>>>  6 files changed, 34 insertions(+), 15 deletions(-)
>>> ...
>>>
>>>> diff --git a/include/net/af_unix.h b/include/net/af_unix.h
>>>> index f42fdddecd41..a86da0cb5ec1 100644
>>>> --- a/include/net/af_unix.h
>>>> +++ b/include/net/af_unix.h
>>>> @@ -36,7 +36,7 @@ struct unix_skb_parms {
>>>>         kgid_t                  gid;
>>>>         struct scm_fp_list      *fp;            /* Passed files         */
>>>>  #ifdef CONFIG_SECURITY_NETWORK
>>>> -       u32                     secid;          /* Security ID          */
>>>> +       struct lsmblob          lsmblob;        /* Security LSM data    */
>>> As mentioned in a previous revision, I remain concerned that this is
>>> going to become a problem due to the size limit on unix_skb_parms.  I
>>> would need to redo the math to be certain, but if I recall correctly
>>> this would limit us to five LSMs assuming both that we don't need to
>>> grow the per-LSM size of lsmblob *and* the netdev folks don't decide
>>> to add more fields to the unix_skb_parms.
>>>
>>> I lost track of that earlier discussion so I'm not sure where it ended
>>> up, but if there is a viable alternative it might be a good idea to
>>> pursue it.
>> Stephen had concerns about the lifecycle management involved. He also
>> pointed out that I had taken a cowards way out when allocations failed.
>> That could result in unexpected behavior when an allocation failed.
>> Fixing that would have required a major re-write of the currently simple
>> UDS attribute code, which I suspect would be as hard a sell to netdev as
>> expanding the secid to a lsmblob. I also thought I'd gotten netdev on the
>> CC: for this patch, but it looks like I missed it.
>>
>> I did start on the UDS attribute re-do, and discovered that I was going
>> to have to introduce new failure paths, and that it might not be possible
>> to maintain compatibility for all cases because of the new possibilities
>> of failure.
> ... and you're hoping to not be responsible for this code by the time
> this becomes a limiting issue? ;)

Well, maybe. More likely that full dementia will have set in by the
time I get the alternative done correctly. It's a _lot_ more complicated.
I'm carefully watching what the BPF people are doing with their
memory management schemes in the hope they will come up with something
useful. 

> I understand the concerns you mention, they are all valid as far as
> I'm concerned, but I think we are going to get burned by this code as
> it currently stands.

Yes, I can see that. We're getting burned by the non-extensibility
of secids. It will take someone smarter than me to figure out how to
fit N secids into 32bits without danger of either failure or memory
allocation.
Paul Moore Sept. 5, 2020, 1:25 p.m. UTC | #6
On Fri, Sep 4, 2020 at 7:58 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 9/4/2020 2:53 PM, Paul Moore wrote:
> > On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 9/4/2020 1:08 PM, Paul Moore wrote:

...

> > I understand the concerns you mention, they are all valid as far as
> > I'm concerned, but I think we are going to get burned by this code as
> > it currently stands.
>
> Yes, I can see that. We're getting burned by the non-extensibility
> of secids. It will take someone smarter than me to figure out how to
> fit N secids into 32bits without danger of either failure or memory
> allocation.

Sooo what are the next steps here?  It sounds like there is some
agreement that the currently proposed unix_skb_params approach is a
problem, but it also sounds like you just want to merge it anyway?

I was sorta hoping for something a bit better.
Casey Schaufler Sept. 5, 2020, 6:13 p.m. UTC | #7
On 9/5/2020 6:25 AM, Paul Moore wrote:
> On Fri, Sep 4, 2020 at 7:58 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 9/4/2020 2:53 PM, Paul Moore wrote:
>>> On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> On 9/4/2020 1:08 PM, Paul Moore wrote:
> ...
>
>>> I understand the concerns you mention, they are all valid as far as
>>> I'm concerned, but I think we are going to get burned by this code as
>>> it currently stands.
>> Yes, I can see that. We're getting burned by the non-extensibility
>> of secids. It will take someone smarter than me to figure out how to
>> fit N secids into 32bits without danger of either failure or memory
>> allocation.
> Sooo what are the next steps here?  It sounds like there is some
> agreement that the currently proposed unix_skb_params approach is a
> problem, but it also sounds like you just want to merge it anyway?

There are real problems with all the approaches. This is by far the
least invasive of the lot. If this is acceptable for now I will commit
to including the dynamic allocation version in the full stacking
(e.g. Smack + SELinux) stage. If it isn't, well, this stage is going
to take even longer than it already has. Sigh.


> I was sorta hoping for something a bit better.

I will be looking at alternatives. I am very much open to suggestions.
I'm not even 100% convinced that Stephen's objections to my separate
allocation strategy outweigh its advantages. If you have an opinion on
that, I'd love to hear it.

Thank you
John Johansen Sept. 5, 2020, 7:05 p.m. UTC | #8
On 9/5/20 11:13 AM, Casey Schaufler wrote:
> On 9/5/2020 6:25 AM, Paul Moore wrote:
>> On Fri, Sep 4, 2020 at 7:58 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> On 9/4/2020 2:53 PM, Paul Moore wrote:
>>>> On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>> On 9/4/2020 1:08 PM, Paul Moore wrote:
>> ...
>>
>>>> I understand the concerns you mention, they are all valid as far as
>>>> I'm concerned, but I think we are going to get burned by this code as
>>>> it currently stands.
>>> Yes, I can see that. We're getting burned by the non-extensibility
>>> of secids. It will take someone smarter than me to figure out how to
>>> fit N secids into 32bits without danger of either failure or memory
>>> allocation.
>> Sooo what are the next steps here?  It sounds like there is some
>> agreement that the currently proposed unix_skb_params approach is a
>> problem, but it also sounds like you just want to merge it anyway?
> 
> There are real problems with all the approaches. This is by far the
> least invasive of the lot. If this is acceptable for now I will commit
> to including the dynamic allocation version in the full stacking
> (e.g. Smack + SELinux) stage. If it isn't, well, this stage is going
> to take even longer than it already has. Sigh.
> 
> 
>> I was sorta hoping for something a bit better.
> 
> I will be looking at alternatives. I am very much open to suggestions.
> I'm not even 100% convinced that Stephen's objections to my separate
> allocation strategy outweigh its advantages. If you have an opinion on
> that, I'd love to hear it.
> 

fwiw I prefer the separate allocation strategy, but as you have already
said it trading off one set of problems for another. I would rather see
this move forward and one set of trade offs isn't significantly worse
than the other to me so, either wfm.
Stephen Smalley Sept. 8, 2020, 1:28 a.m. UTC | #9
On Sat, Sep 5, 2020 at 3:07 PM John Johansen
<john.johansen@canonical.com> wrote:
>
> On 9/5/20 11:13 AM, Casey Schaufler wrote:
> > On 9/5/2020 6:25 AM, Paul Moore wrote:
> >> On Fri, Sep 4, 2020 at 7:58 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>> On 9/4/2020 2:53 PM, Paul Moore wrote:
> >>>> On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>>>> On 9/4/2020 1:08 PM, Paul Moore wrote:
> >> ...
> >>
> >>>> I understand the concerns you mention, they are all valid as far as
> >>>> I'm concerned, but I think we are going to get burned by this code as
> >>>> it currently stands.
> >>> Yes, I can see that. We're getting burned by the non-extensibility
> >>> of secids. It will take someone smarter than me to figure out how to
> >>> fit N secids into 32bits without danger of either failure or memory
> >>> allocation.
> >> Sooo what are the next steps here?  It sounds like there is some
> >> agreement that the currently proposed unix_skb_params approach is a
> >> problem, but it also sounds like you just want to merge it anyway?
> >
> > There are real problems with all the approaches. This is by far the
> > least invasive of the lot. If this is acceptable for now I will commit
> > to including the dynamic allocation version in the full stacking
> > (e.g. Smack + SELinux) stage. If it isn't, well, this stage is going
> > to take even longer than it already has. Sigh.
> >
> >
> >> I was sorta hoping for something a bit better.
> >
> > I will be looking at alternatives. I am very much open to suggestions.
> > I'm not even 100% convinced that Stephen's objections to my separate
> > allocation strategy outweigh its advantages. If you have an opinion on
> > that, I'd love to hear it.
> >
>
> fwiw I prefer the separate allocation strategy, but as you have already
> said it trading off one set of problems for another. I would rather see
> this move forward and one set of trade offs isn't significantly worse
> than the other to me so, either wfm.

I remain unclear that AppArmor needs this patch at all even when
support for SO_PEERSEC lands.
Contrary to the patch description, it is about supporting SCM_SECURITY
for datagram not SO_PEERSEC.  And I don't know of any actual users of
SCM_SECURITY even for SELinux, just SO_PEERSEC.
Stephen Smalley Sept. 8, 2020, 1:35 p.m. UTC | #10
On Mon, Sep 7, 2020 at 9:28 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Sat, Sep 5, 2020 at 3:07 PM John Johansen
> <john.johansen@canonical.com> wrote:
> >
> > On 9/5/20 11:13 AM, Casey Schaufler wrote:
> > > On 9/5/2020 6:25 AM, Paul Moore wrote:
> > >> On Fri, Sep 4, 2020 at 7:58 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > >>> On 9/4/2020 2:53 PM, Paul Moore wrote:
> > >>>> On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > >>>>> On 9/4/2020 1:08 PM, Paul Moore wrote:
> > >> ...
> > >>
> > >>>> I understand the concerns you mention, they are all valid as far as
> > >>>> I'm concerned, but I think we are going to get burned by this code as
> > >>>> it currently stands.
> > >>> Yes, I can see that. We're getting burned by the non-extensibility
> > >>> of secids. It will take someone smarter than me to figure out how to
> > >>> fit N secids into 32bits without danger of either failure or memory
> > >>> allocation.
> > >> Sooo what are the next steps here?  It sounds like there is some
> > >> agreement that the currently proposed unix_skb_params approach is a
> > >> problem, but it also sounds like you just want to merge it anyway?
> > >
> > > There are real problems with all the approaches. This is by far the
> > > least invasive of the lot. If this is acceptable for now I will commit
> > > to including the dynamic allocation version in the full stacking
> > > (e.g. Smack + SELinux) stage. If it isn't, well, this stage is going
> > > to take even longer than it already has. Sigh.
> > >
> > >
> > >> I was sorta hoping for something a bit better.
> > >
> > > I will be looking at alternatives. I am very much open to suggestions.
> > > I'm not even 100% convinced that Stephen's objections to my separate
> > > allocation strategy outweigh its advantages. If you have an opinion on
> > > that, I'd love to hear it.
> > >
> >
> > fwiw I prefer the separate allocation strategy, but as you have already
> > said it trading off one set of problems for another. I would rather see
> > this move forward and one set of trade offs isn't significantly worse
> > than the other to me so, either wfm.
>
> I remain unclear that AppArmor needs this patch at all even when
> support for SO_PEERSEC lands.
> Contrary to the patch description, it is about supporting SCM_SECURITY
> for datagram not SO_PEERSEC.  And I don't know of any actual users of
> SCM_SECURITY even for SELinux, just SO_PEERSEC.

I remembered that systemd once tried using SCM_SECURITY but that was a
bug since systemd was using it with stream sockets and that wasn't
supported by the kernel at the time,
https://bugzilla.redhat.com/show_bug.cgi?id=1224211, so systemd
switched over to using SO_PEERSEC.  Subsequently I did fix
SCM_SECURITY to work with stream sockets via kernel commit
37a9a8df8ce9de6ea73349c9ac8bdf6ba4ec4f70 but SO_PEERSEC is still
preferred.  Looking around, I see that there is still one usage of
SCM_SECURITY in systemd-journald but it doesn't seem to be required
(if provided, journald will pass the label along but nothing seems to
depend on it AFAICT).  In any event, I don't believe this patch is
needed to support stacking AppArmor.
Casey Schaufler Sept. 8, 2020, 11:37 p.m. UTC | #11
On 9/8/2020 6:35 AM, Stephen Smalley wrote:
> On Mon, Sep 7, 2020 at 9:28 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
>> On Sat, Sep 5, 2020 at 3:07 PM John Johansen
>> <john.johansen@canonical.com> wrote:
>>> On 9/5/20 11:13 AM, Casey Schaufler wrote:
>>>> On 9/5/2020 6:25 AM, Paul Moore wrote:
>>>>> On Fri, Sep 4, 2020 at 7:58 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>> On 9/4/2020 2:53 PM, Paul Moore wrote:
>>>>>>> On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>>>> On 9/4/2020 1:08 PM, Paul Moore wrote:
>>>>> ...
>>>>>
>>>>>>> I understand the concerns you mention, they are all valid as far as
>>>>>>> I'm concerned, but I think we are going to get burned by this code as
>>>>>>> it currently stands.
>>>>>> Yes, I can see that. We're getting burned by the non-extensibility
>>>>>> of secids. It will take someone smarter than me to figure out how to
>>>>>> fit N secids into 32bits without danger of either failure or memory
>>>>>> allocation.
>>>>> Sooo what are the next steps here?  It sounds like there is some
>>>>> agreement that the currently proposed unix_skb_params approach is a
>>>>> problem, but it also sounds like you just want to merge it anyway?
>>>> There are real problems with all the approaches. This is by far the
>>>> least invasive of the lot. If this is acceptable for now I will commit
>>>> to including the dynamic allocation version in the full stacking
>>>> (e.g. Smack + SELinux) stage. If it isn't, well, this stage is going
>>>> to take even longer than it already has. Sigh.
>>>>
>>>>
>>>>> I was sorta hoping for something a bit better.
>>>> I will be looking at alternatives. I am very much open to suggestions.
>>>> I'm not even 100% convinced that Stephen's objections to my separate
>>>> allocation strategy outweigh its advantages. If you have an opinion on
>>>> that, I'd love to hear it.
>>>>
>>> fwiw I prefer the separate allocation strategy, but as you have already
>>> said it trading off one set of problems for another. I would rather see
>>> this move forward and one set of trade offs isn't significantly worse
>>> than the other to me so, either wfm.
>> I remain unclear that AppArmor needs this patch at all even when
>> support for SO_PEERSEC lands.
>> Contrary to the patch description, it is about supporting SCM_SECURITY
>> for datagram not SO_PEERSEC.  And I don't know of any actual users of
>> SCM_SECURITY even for SELinux, just SO_PEERSEC.
> I remembered that systemd once tried using SCM_SECURITY but that was a
> bug since systemd was using it with stream sockets and that wasn't
> supported by the kernel at the time,
> https://bugzilla.redhat.com/show_bug.cgi?id=1224211, so systemd
> switched over to using SO_PEERSEC.  Subsequently I did fix
> SCM_SECURITY to work with stream sockets via kernel commit
> 37a9a8df8ce9de6ea73349c9ac8bdf6ba4ec4f70 but SO_PEERSEC is still
> preferred.  Looking around, I see that there is still one usage of
> SCM_SECURITY in systemd-journald but it doesn't seem to be required
> (if provided, journald will pass the label along but nothing seems to
> depend on it AFAICT).  In any event, I don't believe this patch is
> needed to support stacking AppArmor.

Stephen is, as is so often the case, correct. AppArmor has a stub
socket_getpeersec_dgram() that gets removed in patch 23. If I remove
it earlier and throw in a touch of scaffolding for secid_to_secctx()
we can leave the secid as is for now. This can't be the final solution
as AppArmor will be using the hook someday and we still have the all
modules case to worry about for the next phase. It also assumes that
The BPF module isn't going to suddenly sprout a security context.
John Johansen Sept. 9, 2020, 12:17 a.m. UTC | #12
On 9/8/20 6:35 AM, Stephen Smalley wrote:
> On Mon, Sep 7, 2020 at 9:28 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
>>
>> On Sat, Sep 5, 2020 at 3:07 PM John Johansen
>> <john.johansen@canonical.com> wrote:
>>>
>>> On 9/5/20 11:13 AM, Casey Schaufler wrote:
>>>> On 9/5/2020 6:25 AM, Paul Moore wrote:
>>>>> On Fri, Sep 4, 2020 at 7:58 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>> On 9/4/2020 2:53 PM, Paul Moore wrote:
>>>>>>> On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>>>> On 9/4/2020 1:08 PM, Paul Moore wrote:
>>>>> ...
>>>>>
>>>>>>> I understand the concerns you mention, they are all valid as far as
>>>>>>> I'm concerned, but I think we are going to get burned by this code as
>>>>>>> it currently stands.
>>>>>> Yes, I can see that. We're getting burned by the non-extensibility
>>>>>> of secids. It will take someone smarter than me to figure out how to
>>>>>> fit N secids into 32bits without danger of either failure or memory
>>>>>> allocation.
>>>>> Sooo what are the next steps here?  It sounds like there is some
>>>>> agreement that the currently proposed unix_skb_params approach is a
>>>>> problem, but it also sounds like you just want to merge it anyway?
>>>>
>>>> There are real problems with all the approaches. This is by far the
>>>> least invasive of the lot. If this is acceptable for now I will commit
>>>> to including the dynamic allocation version in the full stacking
>>>> (e.g. Smack + SELinux) stage. If it isn't, well, this stage is going
>>>> to take even longer than it already has. Sigh.
>>>>
>>>>
>>>>> I was sorta hoping for something a bit better.
>>>>
>>>> I will be looking at alternatives. I am very much open to suggestions.
>>>> I'm not even 100% convinced that Stephen's objections to my separate
>>>> allocation strategy outweigh its advantages. If you have an opinion on
>>>> that, I'd love to hear it.
>>>>
>>>
>>> fwiw I prefer the separate allocation strategy, but as you have already
>>> said it trading off one set of problems for another. I would rather see
>>> this move forward and one set of trade offs isn't significantly worse
>>> than the other to me so, either wfm.
>>
>> I remain unclear that AppArmor needs this patch at all even when
>> support for SO_PEERSEC lands.
>> Contrary to the patch description, it is about supporting SCM_SECURITY
>> for datagram not SO_PEERSEC.  And I don't know of any actual users of
>> SCM_SECURITY even for SELinux, just SO_PEERSEC.
> 
> I remembered that systemd once tried using SCM_SECURITY but that was a
> bug since systemd was using it with stream sockets and that wasn't
> supported by the kernel at the time,
> https://bugzilla.redhat.com/show_bug.cgi?id=1224211, so systemd
> switched over to using SO_PEERSEC.  Subsequently I did fix
> SCM_SECURITY to work with stream sockets via kernel commit
> 37a9a8df8ce9de6ea73349c9ac8bdf6ba4ec4f70 but SO_PEERSEC is still
> preferred.  Looking around, I see that there is still one usage of
> SCM_SECURITY in systemd-journald but it doesn't seem to be required
> (if provided, journald will pass the label along but nothing seems to
> depend on it AFAICT).  In any event, I don't believe this patch is
> needed to support stacking AppArmor.
> 

correct it is not currently needed. I have been playing with code to
handle it but it is not upstream yet.

Regardless this is something that will need to be solved at some
point.
John Johansen Sept. 9, 2020, 12:21 a.m. UTC | #13
On 9/8/20 4:37 PM, Casey Schaufler wrote:
> On 9/8/2020 6:35 AM, Stephen Smalley wrote:
>> On Mon, Sep 7, 2020 at 9:28 PM Stephen Smalley
>> <stephen.smalley.work@gmail.com> wrote:
>>> On Sat, Sep 5, 2020 at 3:07 PM John Johansen
>>> <john.johansen@canonical.com> wrote:
>>>> On 9/5/20 11:13 AM, Casey Schaufler wrote:
>>>>> On 9/5/2020 6:25 AM, Paul Moore wrote:
>>>>>> On Fri, Sep 4, 2020 at 7:58 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>>> On 9/4/2020 2:53 PM, Paul Moore wrote:
>>>>>>>> On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>>>>> On 9/4/2020 1:08 PM, Paul Moore wrote:
>>>>>> ...
>>>>>>
>>>>>>>> I understand the concerns you mention, they are all valid as far as
>>>>>>>> I'm concerned, but I think we are going to get burned by this code as
>>>>>>>> it currently stands.
>>>>>>> Yes, I can see that. We're getting burned by the non-extensibility
>>>>>>> of secids. It will take someone smarter than me to figure out how to
>>>>>>> fit N secids into 32bits without danger of either failure or memory
>>>>>>> allocation.
>>>>>> Sooo what are the next steps here?  It sounds like there is some
>>>>>> agreement that the currently proposed unix_skb_params approach is a
>>>>>> problem, but it also sounds like you just want to merge it anyway?
>>>>> There are real problems with all the approaches. This is by far the
>>>>> least invasive of the lot. If this is acceptable for now I will commit
>>>>> to including the dynamic allocation version in the full stacking
>>>>> (e.g. Smack + SELinux) stage. If it isn't, well, this stage is going
>>>>> to take even longer than it already has. Sigh.
>>>>>
>>>>>
>>>>>> I was sorta hoping for something a bit better.
>>>>> I will be looking at alternatives. I am very much open to suggestions.
>>>>> I'm not even 100% convinced that Stephen's objections to my separate
>>>>> allocation strategy outweigh its advantages. If you have an opinion on
>>>>> that, I'd love to hear it.
>>>>>
>>>> fwiw I prefer the separate allocation strategy, but as you have already
>>>> said it trading off one set of problems for another. I would rather see
>>>> this move forward and one set of trade offs isn't significantly worse
>>>> than the other to me so, either wfm.
>>> I remain unclear that AppArmor needs this patch at all even when
>>> support for SO_PEERSEC lands.
>>> Contrary to the patch description, it is about supporting SCM_SECURITY
>>> for datagram not SO_PEERSEC.  And I don't know of any actual users of
>>> SCM_SECURITY even for SELinux, just SO_PEERSEC.
>> I remembered that systemd once tried using SCM_SECURITY but that was a
>> bug since systemd was using it with stream sockets and that wasn't
>> supported by the kernel at the time,
>> https://bugzilla.redhat.com/show_bug.cgi?id=1224211, so systemd
>> switched over to using SO_PEERSEC.  Subsequently I did fix
>> SCM_SECURITY to work with stream sockets via kernel commit
>> 37a9a8df8ce9de6ea73349c9ac8bdf6ba4ec4f70 but SO_PEERSEC is still
>> preferred.  Looking around, I see that there is still one usage of
>> SCM_SECURITY in systemd-journald but it doesn't seem to be required
>> (if provided, journald will pass the label along but nothing seems to
>> depend on it AFAICT).  In any event, I don't believe this patch is
>> needed to support stacking AppArmor.
> 
> Stephen is, as is so often the case, correct. AppArmor has a stub
> socket_getpeersec_dgram() that gets removed in patch 23. If I remove

right but as I said before this is coming, I have been playing with
it and have code. So the series doesn't need it today but sooner than
later it will be needed

> it earlier and throw in a touch of scaffolding for secid_to_secctx()
> we can leave the secid as is for now. This can't be the final solution
> as AppArmor will be using the hook someday and we still have the all
> modules case to worry about for the next phase. It also assumes that
> The BPF module isn't going to suddenly sprout a security context.
> 

Yep this is something that needs to be dealt with, whether it is now
or kicked down the road a little further ...
Stephen Smalley Sept. 9, 2020, 1:19 p.m. UTC | #14
On Tue, Sep 8, 2020 at 8:21 PM John Johansen
<john.johansen@canonical.com> wrote:
>
> On 9/8/20 4:37 PM, Casey Schaufler wrote:
> > On 9/8/2020 6:35 AM, Stephen Smalley wrote:
> >> On Mon, Sep 7, 2020 at 9:28 PM Stephen Smalley
> >> <stephen.smalley.work@gmail.com> wrote:
> >>> On Sat, Sep 5, 2020 at 3:07 PM John Johansen
> >>> <john.johansen@canonical.com> wrote:
> >>>> On 9/5/20 11:13 AM, Casey Schaufler wrote:
> >>>>> On 9/5/2020 6:25 AM, Paul Moore wrote:
> >>>>>> On Fri, Sep 4, 2020 at 7:58 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>>>>>> On 9/4/2020 2:53 PM, Paul Moore wrote:
> >>>>>>>> On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>>>>>>>> On 9/4/2020 1:08 PM, Paul Moore wrote:
> >>>>>> ...
> >>>>>>
> >>>>>>>> I understand the concerns you mention, they are all valid as far as
> >>>>>>>> I'm concerned, but I think we are going to get burned by this code as
> >>>>>>>> it currently stands.
> >>>>>>> Yes, I can see that. We're getting burned by the non-extensibility
> >>>>>>> of secids. It will take someone smarter than me to figure out how to
> >>>>>>> fit N secids into 32bits without danger of either failure or memory
> >>>>>>> allocation.
> >>>>>> Sooo what are the next steps here?  It sounds like there is some
> >>>>>> agreement that the currently proposed unix_skb_params approach is a
> >>>>>> problem, but it also sounds like you just want to merge it anyway?
> >>>>> There are real problems with all the approaches. This is by far the
> >>>>> least invasive of the lot. If this is acceptable for now I will commit
> >>>>> to including the dynamic allocation version in the full stacking
> >>>>> (e.g. Smack + SELinux) stage. If it isn't, well, this stage is going
> >>>>> to take even longer than it already has. Sigh.
> >>>>>
> >>>>>
> >>>>>> I was sorta hoping for something a bit better.
> >>>>> I will be looking at alternatives. I am very much open to suggestions.
> >>>>> I'm not even 100% convinced that Stephen's objections to my separate
> >>>>> allocation strategy outweigh its advantages. If you have an opinion on
> >>>>> that, I'd love to hear it.
> >>>>>
> >>>> fwiw I prefer the separate allocation strategy, but as you have already
> >>>> said it trading off one set of problems for another. I would rather see
> >>>> this move forward and one set of trade offs isn't significantly worse
> >>>> than the other to me so, either wfm.
> >>> I remain unclear that AppArmor needs this patch at all even when
> >>> support for SO_PEERSEC lands.
> >>> Contrary to the patch description, it is about supporting SCM_SECURITY
> >>> for datagram not SO_PEERSEC.  And I don't know of any actual users of
> >>> SCM_SECURITY even for SELinux, just SO_PEERSEC.
> >> I remembered that systemd once tried using SCM_SECURITY but that was a
> >> bug since systemd was using it with stream sockets and that wasn't
> >> supported by the kernel at the time,
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1224211, so systemd
> >> switched over to using SO_PEERSEC.  Subsequently I did fix
> >> SCM_SECURITY to work with stream sockets via kernel commit
> >> 37a9a8df8ce9de6ea73349c9ac8bdf6ba4ec4f70 but SO_PEERSEC is still
> >> preferred.  Looking around, I see that there is still one usage of
> >> SCM_SECURITY in systemd-journald but it doesn't seem to be required
> >> (if provided, journald will pass the label along but nothing seems to
> >> depend on it AFAICT).  In any event, I don't believe this patch is
> >> needed to support stacking AppArmor.
> >
> > Stephen is, as is so often the case, correct. AppArmor has a stub
> > socket_getpeersec_dgram() that gets removed in patch 23. If I remove
>
> right but as I said before this is coming, I have been playing with
> it and have code. So the series doesn't need it today but sooner than
> later it will be needed

I don't understand why.  Is there a userspace component that relies on
SCM_SECURITY today for anything real (more than just blindly passing
it along and maybe writing to a log somewhere)?  And this doesn't
provide support for a composite SCM_SECURITY or SCM_CONTEXT, so it
doesn't really solve the stacking problem for it anyway.  What am I
missing?  Why do you care about this patch?
Casey Schaufler Sept. 9, 2020, 6:19 p.m. UTC | #15
On 9/9/2020 6:19 AM, Stephen Smalley wrote:
> On Tue, Sep 8, 2020 at 8:21 PM John Johansen
> <john.johansen@canonical.com> wrote:
>> On 9/8/20 4:37 PM, Casey Schaufler wrote:
>>> On 9/8/2020 6:35 AM, Stephen Smalley wrote:
>>>> On Mon, Sep 7, 2020 at 9:28 PM Stephen Smalley
>>>> <stephen.smalley.work@gmail.com> wrote:
>>>>> On Sat, Sep 5, 2020 at 3:07 PM John Johansen
>>>>> <john.johansen@canonical.com> wrote:
>>>>>> On 9/5/20 11:13 AM, Casey Schaufler wrote:
>>>>>>> On 9/5/2020 6:25 AM, Paul Moore wrote:
>>>>>>>> On Fri, Sep 4, 2020 at 7:58 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>>>>> On 9/4/2020 2:53 PM, Paul Moore wrote:
>>>>>>>>>> On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>>>>>>> On 9/4/2020 1:08 PM, Paul Moore wrote:
>>>>>>>> ...
>>>>>>>>
>>>>>>>>>> I understand the concerns you mention, they are all valid as far as
>>>>>>>>>> I'm concerned, but I think we are going to get burned by this code as
>>>>>>>>>> it currently stands.
>>>>>>>>> Yes, I can see that. We're getting burned by the non-extensibility
>>>>>>>>> of secids. It will take someone smarter than me to figure out how to
>>>>>>>>> fit N secids into 32bits without danger of either failure or memory
>>>>>>>>> allocation.
>>>>>>>> Sooo what are the next steps here?  It sounds like there is some
>>>>>>>> agreement that the currently proposed unix_skb_params approach is a
>>>>>>>> problem, but it also sounds like you just want to merge it anyway?
>>>>>>> There are real problems with all the approaches. This is by far the
>>>>>>> least invasive of the lot. If this is acceptable for now I will commit
>>>>>>> to including the dynamic allocation version in the full stacking
>>>>>>> (e.g. Smack + SELinux) stage. If it isn't, well, this stage is going
>>>>>>> to take even longer than it already has. Sigh.
>>>>>>>
>>>>>>>
>>>>>>>> I was sorta hoping for something a bit better.
>>>>>>> I will be looking at alternatives. I am very much open to suggestions.
>>>>>>> I'm not even 100% convinced that Stephen's objections to my separate
>>>>>>> allocation strategy outweigh its advantages. If you have an opinion on
>>>>>>> that, I'd love to hear it.
>>>>>>>
>>>>>> fwiw I prefer the separate allocation strategy, but as you have already
>>>>>> said it trading off one set of problems for another. I would rather see
>>>>>> this move forward and one set of trade offs isn't significantly worse
>>>>>> than the other to me so, either wfm.
>>>>> I remain unclear that AppArmor needs this patch at all even when
>>>>> support for SO_PEERSEC lands.
>>>>> Contrary to the patch description, it is about supporting SCM_SECURITY
>>>>> for datagram not SO_PEERSEC.  And I don't know of any actual users of
>>>>> SCM_SECURITY even for SELinux, just SO_PEERSEC.
>>>> I remembered that systemd once tried using SCM_SECURITY but that was a
>>>> bug since systemd was using it with stream sockets and that wasn't
>>>> supported by the kernel at the time,
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1224211, so systemd
>>>> switched over to using SO_PEERSEC.  Subsequently I did fix
>>>> SCM_SECURITY to work with stream sockets via kernel commit
>>>> 37a9a8df8ce9de6ea73349c9ac8bdf6ba4ec4f70 but SO_PEERSEC is still
>>>> preferred.  Looking around, I see that there is still one usage of
>>>> SCM_SECURITY in systemd-journald but it doesn't seem to be required
>>>> (if provided, journald will pass the label along but nothing seems to
>>>> depend on it AFAICT).  In any event, I don't believe this patch is
>>>> needed to support stacking AppArmor.
>>> Stephen is, as is so often the case, correct. AppArmor has a stub
>>> socket_getpeersec_dgram() that gets removed in patch 23. If I remove
>> right but as I said before this is coming, I have been playing with
>> it and have code. So the series doesn't need it today but sooner than
>> later it will be needed

Is sooner like 5.10, or 5.15? It matters.

> I don't understand why.  Is there a userspace component that relies on
> SCM_SECURITY today for anything real (more than just blindly passing
> it along and maybe writing to a log somewhere)?  And this doesn't
> provide support for a composite SCM_SECURITY or SCM_CONTEXT, so it
> doesn't really solve the stacking problem for it anyway.  What am I
> missing?  Why do you care about this patch?
John Johansen Sept. 9, 2020, 6:33 p.m. UTC | #16
On 9/9/20 11:19 AM, Casey Schaufler wrote:
> On 9/9/2020 6:19 AM, Stephen Smalley wrote:
>> On Tue, Sep 8, 2020 at 8:21 PM John Johansen
>> <john.johansen@canonical.com> wrote:
>>> On 9/8/20 4:37 PM, Casey Schaufler wrote:
>>>> On 9/8/2020 6:35 AM, Stephen Smalley wrote:
>>>>> On Mon, Sep 7, 2020 at 9:28 PM Stephen Smalley
>>>>> <stephen.smalley.work@gmail.com> wrote:
>>>>>> On Sat, Sep 5, 2020 at 3:07 PM John Johansen
>>>>>> <john.johansen@canonical.com> wrote:
>>>>>>> On 9/5/20 11:13 AM, Casey Schaufler wrote:
>>>>>>>> On 9/5/2020 6:25 AM, Paul Moore wrote:
>>>>>>>>> On Fri, Sep 4, 2020 at 7:58 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>>>>>> On 9/4/2020 2:53 PM, Paul Moore wrote:
>>>>>>>>>>> On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>>>>>>>> On 9/4/2020 1:08 PM, Paul Moore wrote:
>>>>>>>>> ...
>>>>>>>>>
>>>>>>>>>>> I understand the concerns you mention, they are all valid as far as
>>>>>>>>>>> I'm concerned, but I think we are going to get burned by this code as
>>>>>>>>>>> it currently stands.
>>>>>>>>>> Yes, I can see that. We're getting burned by the non-extensibility
>>>>>>>>>> of secids. It will take someone smarter than me to figure out how to
>>>>>>>>>> fit N secids into 32bits without danger of either failure or memory
>>>>>>>>>> allocation.
>>>>>>>>> Sooo what are the next steps here?  It sounds like there is some
>>>>>>>>> agreement that the currently proposed unix_skb_params approach is a
>>>>>>>>> problem, but it also sounds like you just want to merge it anyway?
>>>>>>>> There are real problems with all the approaches. This is by far the
>>>>>>>> least invasive of the lot. If this is acceptable for now I will commit
>>>>>>>> to including the dynamic allocation version in the full stacking
>>>>>>>> (e.g. Smack + SELinux) stage. If it isn't, well, this stage is going
>>>>>>>> to take even longer than it already has. Sigh.
>>>>>>>>
>>>>>>>>
>>>>>>>>> I was sorta hoping for something a bit better.
>>>>>>>> I will be looking at alternatives. I am very much open to suggestions.
>>>>>>>> I'm not even 100% convinced that Stephen's objections to my separate
>>>>>>>> allocation strategy outweigh its advantages. If you have an opinion on
>>>>>>>> that, I'd love to hear it.
>>>>>>>>
>>>>>>> fwiw I prefer the separate allocation strategy, but as you have already
>>>>>>> said it trading off one set of problems for another. I would rather see
>>>>>>> this move forward and one set of trade offs isn't significantly worse
>>>>>>> than the other to me so, either wfm.
>>>>>> I remain unclear that AppArmor needs this patch at all even when
>>>>>> support for SO_PEERSEC lands.
>>>>>> Contrary to the patch description, it is about supporting SCM_SECURITY
>>>>>> for datagram not SO_PEERSEC.  And I don't know of any actual users of
>>>>>> SCM_SECURITY even for SELinux, just SO_PEERSEC.
>>>>> I remembered that systemd once tried using SCM_SECURITY but that was a
>>>>> bug since systemd was using it with stream sockets and that wasn't
>>>>> supported by the kernel at the time,
>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1224211, so systemd
>>>>> switched over to using SO_PEERSEC.  Subsequently I did fix
>>>>> SCM_SECURITY to work with stream sockets via kernel commit
>>>>> 37a9a8df8ce9de6ea73349c9ac8bdf6ba4ec4f70 but SO_PEERSEC is still
>>>>> preferred.  Looking around, I see that there is still one usage of
>>>>> SCM_SECURITY in systemd-journald but it doesn't seem to be required
>>>>> (if provided, journald will pass the label along but nothing seems to
>>>>> depend on it AFAICT).  In any event, I don't believe this patch is
>>>>> needed to support stacking AppArmor.
>>>> Stephen is, as is so often the case, correct. AppArmor has a stub
>>>> socket_getpeersec_dgram() that gets removed in patch 23. If I remove
>>> right but as I said before this is coming, I have been playing with
>>> it and have code. So the series doesn't need it today but sooner than
>>> later it will be needed
> 
> Is sooner like 5.10, or 5.15? It matters.
> 

I can split SCM_SECURITY off from the rest of the unix mediation and
push it off for a while. So lets call it 5.15 or later.

>> I don't understand why.  Is there a userspace component that relies on
>> SCM_SECURITY today for anything real (more than just blindly passing
>> it along and maybe writing to a log somewhere)?  And this doesn't
>> provide support for a composite SCM_SECURITY or SCM_CONTEXT, so it
>> doesn't really solve the stacking problem for it anyway.  What am I
>> missing?  Why do you care about this patch?
John Johansen Sept. 9, 2020, 6:47 p.m. UTC | #17
On 9/9/20 6:19 AM, Stephen Smalley wrote:
> On Tue, Sep 8, 2020 at 8:21 PM John Johansen
> <john.johansen@canonical.com> wrote:
>>
>> On 9/8/20 4:37 PM, Casey Schaufler wrote:
>>> On 9/8/2020 6:35 AM, Stephen Smalley wrote:
>>>> On Mon, Sep 7, 2020 at 9:28 PM Stephen Smalley
>>>> <stephen.smalley.work@gmail.com> wrote:
>>>>> On Sat, Sep 5, 2020 at 3:07 PM John Johansen
>>>>> <john.johansen@canonical.com> wrote:
>>>>>> On 9/5/20 11:13 AM, Casey Schaufler wrote:
>>>>>>> On 9/5/2020 6:25 AM, Paul Moore wrote:
>>>>>>>> On Fri, Sep 4, 2020 at 7:58 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>>>>> On 9/4/2020 2:53 PM, Paul Moore wrote:
>>>>>>>>>> On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>>>>>>> On 9/4/2020 1:08 PM, Paul Moore wrote:
>>>>>>>> ...
>>>>>>>>
>>>>>>>>>> I understand the concerns you mention, they are all valid as far as
>>>>>>>>>> I'm concerned, but I think we are going to get burned by this code as
>>>>>>>>>> it currently stands.
>>>>>>>>> Yes, I can see that. We're getting burned by the non-extensibility
>>>>>>>>> of secids. It will take someone smarter than me to figure out how to
>>>>>>>>> fit N secids into 32bits without danger of either failure or memory
>>>>>>>>> allocation.
>>>>>>>> Sooo what are the next steps here?  It sounds like there is some
>>>>>>>> agreement that the currently proposed unix_skb_params approach is a
>>>>>>>> problem, but it also sounds like you just want to merge it anyway?
>>>>>>> There are real problems with all the approaches. This is by far the
>>>>>>> least invasive of the lot. If this is acceptable for now I will commit
>>>>>>> to including the dynamic allocation version in the full stacking
>>>>>>> (e.g. Smack + SELinux) stage. If it isn't, well, this stage is going
>>>>>>> to take even longer than it already has. Sigh.
>>>>>>>
>>>>>>>
>>>>>>>> I was sorta hoping for something a bit better.
>>>>>>> I will be looking at alternatives. I am very much open to suggestions.
>>>>>>> I'm not even 100% convinced that Stephen's objections to my separate
>>>>>>> allocation strategy outweigh its advantages. If you have an opinion on
>>>>>>> that, I'd love to hear it.
>>>>>>>
>>>>>> fwiw I prefer the separate allocation strategy, but as you have already
>>>>>> said it trading off one set of problems for another. I would rather see
>>>>>> this move forward and one set of trade offs isn't significantly worse
>>>>>> than the other to me so, either wfm.
>>>>> I remain unclear that AppArmor needs this patch at all even when
>>>>> support for SO_PEERSEC lands.
>>>>> Contrary to the patch description, it is about supporting SCM_SECURITY
>>>>> for datagram not SO_PEERSEC.  And I don't know of any actual users of
>>>>> SCM_SECURITY even for SELinux, just SO_PEERSEC.
>>>> I remembered that systemd once tried using SCM_SECURITY but that was a
>>>> bug since systemd was using it with stream sockets and that wasn't
>>>> supported by the kernel at the time,
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1224211, so systemd
>>>> switched over to using SO_PEERSEC.  Subsequently I did fix
>>>> SCM_SECURITY to work with stream sockets via kernel commit
>>>> 37a9a8df8ce9de6ea73349c9ac8bdf6ba4ec4f70 but SO_PEERSEC is still
>>>> preferred.  Looking around, I see that there is still one usage of
>>>> SCM_SECURITY in systemd-journald but it doesn't seem to be required
>>>> (if provided, journald will pass the label along but nothing seems to
>>>> depend on it AFAICT).  In any event, I don't believe this patch is
>>>> needed to support stacking AppArmor.
>>>
>>> Stephen is, as is so often the case, correct. AppArmor has a stub
>>> socket_getpeersec_dgram() that gets removed in patch 23. If I remove
>>
>> right but as I said before this is coming, I have been playing with
>> it and have code. So the series doesn't need it today but sooner than
>> later it will be needed
> 
> I don't understand why.  Is there a userspace component that relies on
> SCM_SECURITY today for anything real (more than just blindly passing
> it along and maybe writing to a log somewhere)?  And this doesn't
> provide support for a composite SCM_SECURITY or SCM_CONTEXT, so it
> doesn't really solve the stacking problem for it anyway.  What am I
> missing?  Why do you care about this patch?
> 


personally I don't atm, but there are people who do care about this in
there logs, whether they should or shouldn't is an entirely different
question.

Long term there may be some uses for it that I care about or "have to
care about." For now Casey can drop it from this series.
Paul Moore Sept. 10, 2020, 2:11 p.m. UTC | #18
On Wed, Sep 9, 2020 at 2:47 PM John Johansen
<john.johansen@canonical.com> wrote:
> ... For now Casey can drop it from this series.

As long as that whenever it reappears there is at the very least some
note of the limits in the commit description and the code (via
comments in the struct).  Of course that assumes we can't find an
alternate solution that we can all agree on which doesn't have these
stacking limits.
diff mbox series

Patch

diff --git a/include/linux/security.h b/include/linux/security.h
index e2ef982b3dd7..ae623b89cdf4 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1398,7 +1398,8 @@  int security_socket_shutdown(struct socket *sock, int how);
 int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb);
 int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
 				      int __user *optlen, unsigned len);
-int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid);
+int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb,
+				     struct lsmblob *blob);
 int security_sk_alloc(struct sock *sk, int family, gfp_t priority);
 void security_sk_free(struct sock *sk);
 void security_sk_clone(const struct sock *sk, struct sock *newsk);
@@ -1536,7 +1537,9 @@  static inline int security_socket_getpeersec_stream(struct socket *sock, char __
 	return -ENOPROTOOPT;
 }
 
-static inline int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid)
+static inline int security_socket_getpeersec_dgram(struct socket *sock,
+						   struct sk_buff *skb,
+						   struct lsmblob *blob)
 {
 	return -ENOPROTOOPT;
 }
diff --git a/include/net/af_unix.h b/include/net/af_unix.h
index f42fdddecd41..a86da0cb5ec1 100644
--- a/include/net/af_unix.h
+++ b/include/net/af_unix.h
@@ -36,7 +36,7 @@  struct unix_skb_parms {
 	kgid_t			gid;
 	struct scm_fp_list	*fp;		/* Passed files		*/
 #ifdef CONFIG_SECURITY_NETWORK
-	u32			secid;		/* Security ID		*/
+	struct lsmblob		lsmblob;	/* Security LSM data	*/
 #endif
 	u32			consumed;
 } __randomize_layout;
diff --git a/include/net/scm.h b/include/net/scm.h
index 1ce365f4c256..e2e71c4bf9d0 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h
@@ -33,7 +33,7 @@  struct scm_cookie {
 	struct scm_fp_list	*fp;		/* Passed files		*/
 	struct scm_creds	creds;		/* Skb credentials	*/
 #ifdef CONFIG_SECURITY_NETWORK
-	u32			secid;		/* Passed security ID 	*/
+	struct lsmblob		lsmblob;	/* Passed LSM data	*/
 #endif
 };
 
@@ -46,7 +46,7 @@  struct scm_fp_list *scm_fp_dup(struct scm_fp_list *fpl);
 #ifdef CONFIG_SECURITY_NETWORK
 static __inline__ void unix_get_peersec_dgram(struct socket *sock, struct scm_cookie *scm)
 {
-	security_socket_getpeersec_dgram(sock, NULL, &scm->secid);
+	security_socket_getpeersec_dgram(sock, NULL, &scm->lsmblob);
 }
 #else
 static __inline__ void unix_get_peersec_dgram(struct socket *sock, struct scm_cookie *scm)
@@ -97,7 +97,9 @@  static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct sc
 	int err;
 
 	if (test_bit(SOCK_PASSSEC, &sock->flags)) {
-		err = security_secid_to_secctx(scm->secid, &secdata, &seclen);
+		/* Scaffolding - it has to be element 0 for now */
+		err = security_secid_to_secctx(scm->lsmblob.secid[0],
+					       &secdata, &seclen);
 
 		if (!err) {
 			put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, seclen, secdata);
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index d2c223554ff7..551dfbc717e9 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -130,15 +130,17 @@  static void ip_cmsg_recv_checksum(struct msghdr *msg, struct sk_buff *skb,
 
 static void ip_cmsg_recv_security(struct msghdr *msg, struct sk_buff *skb)
 {
+	struct lsmblob lb;
 	char *secdata;
-	u32 seclen, secid;
+	u32 seclen;
 	int err;
 
-	err = security_socket_getpeersec_dgram(NULL, skb, &secid);
+	err = security_socket_getpeersec_dgram(NULL, skb, &lb);
 	if (err)
 		return;
 
-	err = security_secid_to_secctx(secid, &secdata, &seclen);
+	/* Scaffolding - it has to be element 0 */
+	err = security_secid_to_secctx(lb.secid[0], &secdata, &seclen);
 	if (err)
 		return;
 
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 181ea6fb56a6..c15668b80d1d 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -138,17 +138,17 @@  static struct hlist_head *unix_sockets_unbound(void *addr)
 #ifdef CONFIG_SECURITY_NETWORK
 static void unix_get_secdata(struct scm_cookie *scm, struct sk_buff *skb)
 {
-	UNIXCB(skb).secid = scm->secid;
+	UNIXCB(skb).lsmblob = scm->lsmblob;
 }
 
 static inline void unix_set_secdata(struct scm_cookie *scm, struct sk_buff *skb)
 {
-	scm->secid = UNIXCB(skb).secid;
+	scm->lsmblob = UNIXCB(skb).lsmblob;
 }
 
 static inline bool unix_secdata_eq(struct scm_cookie *scm, struct sk_buff *skb)
 {
-	return (scm->secid == UNIXCB(skb).secid);
+	return lsmblob_equal(&scm->lsmblob, &(UNIXCB(skb).lsmblob));
 }
 #else
 static inline void unix_get_secdata(struct scm_cookie *scm, struct sk_buff *skb)
diff --git a/security/security.c b/security/security.c
index d6d882b1f7d5..c42873876954 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2219,10 +2219,22 @@  int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
 				optval, optlen, len);
 }
 
-int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid)
+int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb,
+				     struct lsmblob *blob)
 {
-	return call_int_hook(socket_getpeersec_dgram, -ENOPROTOOPT, sock,
-			     skb, secid);
+	struct security_hook_list *hp;
+	int rc = -ENOPROTOOPT;
+
+	hlist_for_each_entry(hp, &security_hook_heads.socket_getpeersec_dgram,
+			     list) {
+		if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
+			continue;
+		rc = hp->hook.socket_getpeersec_dgram(sock, skb,
+						&blob->secid[hp->lsmid->slot]);
+		if (rc != 0)
+			break;
+	}
+	return rc;
 }
 EXPORT_SYMBOL(security_socket_getpeersec_dgram);