| Message ID | 20200826145247.10029-6-casey@schaufler-ca.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | LSM: Module stacking for AppArmor | expand |
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); > >
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.
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.
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.
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.
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.
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
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.
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.
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.
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.
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.
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 ...
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?
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?
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?
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.
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 --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);
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(-)