| Message ID | a013db22-6179-f74f-b96d-d2c7384dfd12@schaufler-ca.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 16, 2017 at 6:35 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: > Subject: [PATCH RFC 10/9] LSM: netlabel control on sendmsg > > This is incomplete as it only addresses the sendmsg hook, > but is presented to assess the viability of the approach. > > Create a netlabel KAPI to compare two netlabel security > attribute structures. Use this to determine if all of the > security modules agree on how a packet ought to be labeled > in the send operation. Refuse the send if they don't agree. > A module that does not report a netlabel security attribute > is assumed to not care what, if any, attribute is attached > to the packet. > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> ... > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 8bdd418..1e7b76d 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -28,6 +28,8 @@ > #include <linux/init.h> > #include <linux/rculist.h> > > +struct netlbl_lsm_secattr; > + > /** > * Security hooks for program execution operations. > * > @@ -781,6 +783,7 @@ > * @sock contains the socket structure. > * @msg contains the message to be transmitted. > * @size contains the size of message. > + * @attrs points to the network attributes on return. > * Return 0 if permission is granted. > * @socket_recvmsg: > * Check permission before receiving a message from a socket. > @@ -1575,7 +1578,7 @@ union security_list_options { > int (*socket_listen)(struct socket *sock, int backlog); > int (*socket_accept)(struct socket *sock, struct socket *newsock); > int (*socket_sendmsg)(struct socket *sock, struct msghdr *msg, > - int size); > + int size, struct netlbl_lsm_secattr **attrs); I might mark 'attrs' as const to be safe. > diff --git a/net/netlabel/netlabel_kapi.c b/net/netlabel/netlabel_kapi.c > index 0dd1a60..926a46b 100644 > --- a/net/netlabel/netlabel_kapi.c > +++ b/net/netlabel/netlabel_kapi.c > @@ -1461,6 +1461,60 @@ int netlbl_cache_add(const struct sk_buff *skb, u16 family, > return -ENOMSG; > } > > +/** > + * netlbl_secattr_equal - Compare two lsm secattrs > + * @secattr_a: one security attribute > + * @secattr_b: the other security attribute > + * > + * Description: > + * Compare two lsm security attribute structures. Returns true > + * if they are the same, false otherwise. > + * > + */ > +bool netlbl_secattr_equal(const struct netlbl_lsm_secattr *secattr_a, > + const struct netlbl_lsm_secattr *secattr_b) > +{ > + struct netlbl_lsm_catmap *iter_a; > + struct netlbl_lsm_catmap *iter_b; > + > + if (secattr_a == secattr_b) > + return true; > + if (!secattr_a || !secattr_b) > + return false; > + > + if ((secattr_a->flags & NETLBL_SECATTR_SECID) && > + (secattr_b->flags & NETLBL_SECATTR_SECID)) > + return secattr_a->attr.secid.common == > + secattr_b->attr.secid.common; Comparing secids across different LSMs doesn't really make sense, and might even be dangerous; even if the scalar value was the same, the semantic value could be very different. I wonder if we just need to document that this function is only to be used when comparing secattrs across LSMs and if there is only a secid and nothing else to compare we should fail the comparison. Of course this would mean that the cooperating LSMs would need to make sure the other portions of the secattr were populated even when the secid was used, but that should be okay ... if memory serves the secid is only really used as part of the NetLabel cache for inbound traffic. > + if ((secattr_a->flags & NETLBL_SECATTR_MLS_LVL) != > + (secattr_b->flags & NETLBL_SECATTR_MLS_LVL)) > + return false; > + > + if ((secattr_a->flags & NETLBL_SECATTR_MLS_LVL) && > + secattr_a->attr.mls.lvl != secattr_b->attr.mls.lvl) > + return false; > + > + if ((secattr_a->flags & NETLBL_SECATTR_MLS_CAT) != > + (secattr_b->flags & NETLBL_SECATTR_MLS_CAT)) > + return false; > + > + iter_a = secattr_a->attr.mls.cat; > + iter_b = secattr_b->attr.mls.cat; > + > + while (iter_a && iter_b) { > + if (iter_a->startbit != iter_b->startbit) > + return false; > + if (memcmp(iter_a->bitmap, iter_b->bitmap, > + sizeof(iter_a->bitmap))) > + return false; > + iter_a = iter_a->next; > + iter_b = iter_b->next; > + } > + > + return !iter_a && !iter_b; > +} It might be nice to abstract the bitmap comparison out to another function, e.g. netlbl_catmap_equal(...), but I understand that this is just a high-level patch designed to shop around the idea and this is very much an implementation nit. As previously discussed, I don't have a problem with the idea of the netlbl_secattr_equal() function. > + > /* > * Protocol Engine Functions > */ > diff --git a/security/security.c b/security/security.c > index 8118377..e2e8c5c 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -28,6 +28,7 @@ > #include <linux/msg.h> > #include <net/flow.h> > #include <net/sock.h> > +#include <net/netlabel.h> > > #define MAX_LSM_EVM_XATTR 2 > > @@ -2084,7 +2085,27 @@ int security_socket_accept(struct socket *sock, struct socket *newsock) > > int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size) > { > - return call_int_hook(socket_sendmsg, 0, sock, msg, size); > + struct security_hook_list *hp; > + int rc; > + struct netlbl_lsm_secattr *pattrs = NULL; > + struct netlbl_lsm_secattr *attrs = NULL; > + > + list_for_each_entry(hp, &security_hook_heads.socket_sendmsg, list) { > + rc = hp->hook.socket_sendmsg(sock, msg, size, &attrs); > + if (rc) > + return rc; > + /* > + * Only do the check if the current module reports > + * an attribute, and there is something to compare it to. > + */ > + if (attrs) { > + if (!pattrs) > + pattrs = attrs; > + else if (!netlbl_secattr_equal(pattrs, attrs)) > + return -EACCES; > + } > + } > + return 0; > } Seems reasonable at this stage, although I wonder if we could do something so that we only check equality when the socket's label changes. Under SELinux we never change the label once it is set, and with Smack the label can only changes via setsockopt(), yes/no?
On 2/17/2017 2:26 PM, Paul Moore wrote: > On Thu, Feb 16, 2017 at 6:35 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >> Subject: [PATCH RFC 10/9] LSM: netlabel control on sendmsg >> >> This is incomplete as it only addresses the sendmsg hook, >> but is presented to assess the viability of the approach. >> >> Create a netlabel KAPI to compare two netlabel security >> attribute structures. Use this to determine if all of the >> security modules agree on how a packet ought to be labeled >> in the send operation. Refuse the send if they don't agree. >> A module that does not report a netlabel security attribute >> is assumed to not care what, if any, attribute is attached >> to the packet. >> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > .. > >> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >> index 8bdd418..1e7b76d 100644 >> --- a/include/linux/lsm_hooks.h >> +++ b/include/linux/lsm_hooks.h >> @@ -28,6 +28,8 @@ >> #include <linux/init.h> >> #include <linux/rculist.h> >> >> +struct netlbl_lsm_secattr; >> + >> /** >> * Security hooks for program execution operations. >> * >> @@ -781,6 +783,7 @@ >> * @sock contains the socket structure. >> * @msg contains the message to be transmitted. >> * @size contains the size of message. >> + * @attrs points to the network attributes on return. >> * Return 0 if permission is granted. >> * @socket_recvmsg: >> * Check permission before receiving a message from a socket. >> @@ -1575,7 +1578,7 @@ union security_list_options { >> int (*socket_listen)(struct socket *sock, int backlog); >> int (*socket_accept)(struct socket *sock, struct socket *newsock); >> int (*socket_sendmsg)(struct socket *sock, struct msghdr *msg, >> - int size); >> + int size, struct netlbl_lsm_secattr **attrs); > I might mark 'attrs' as const to be safe. But ... socket_sendmsg is goin to change it's content. >> diff --git a/net/netlabel/netlabel_kapi.c b/net/netlabel/netlabel_kapi.c >> index 0dd1a60..926a46b 100644 >> --- a/net/netlabel/netlabel_kapi.c >> +++ b/net/netlabel/netlabel_kapi.c >> @@ -1461,6 +1461,60 @@ int netlbl_cache_add(const struct sk_buff *skb, u16 family, >> return -ENOMSG; >> } >> >> +/** >> + * netlbl_secattr_equal - Compare two lsm secattrs >> + * @secattr_a: one security attribute >> + * @secattr_b: the other security attribute >> + * >> + * Description: >> + * Compare two lsm security attribute structures. Returns true >> + * if they are the same, false otherwise. >> + * >> + */ >> +bool netlbl_secattr_equal(const struct netlbl_lsm_secattr *secattr_a, >> + const struct netlbl_lsm_secattr *secattr_b) >> +{ >> + struct netlbl_lsm_catmap *iter_a; >> + struct netlbl_lsm_catmap *iter_b; >> + >> + if (secattr_a == secattr_b) >> + return true; >> + if (!secattr_a || !secattr_b) >> + return false; >> + >> + if ((secattr_a->flags & NETLBL_SECATTR_SECID) && >> + (secattr_b->flags & NETLBL_SECATTR_SECID)) >> + return secattr_a->attr.secid.common == >> + secattr_b->attr.secid.common; > Comparing secids across different LSMs doesn't really make sense, and > might even be dangerous; even if the scalar value was the same, the > semantic value could be very different. I wonder if we just need to > document that this function is only to be used when comparing secattrs > across LSMs and if there is only a secid and nothing else to compare > we should fail the comparison. The common field of a struct secids provides the information necessary to identify all of the module specific secids. If two secid.common values are the same you know that the secid.selinux and the secid.smack values in the two structs match. So, secid1.common == secid2.common implies that secid1.selinux == secid2.selinux && secid1.smack == secid2.smack The current patchset supports a limited, but efficient function for setting the .common given a .selinux and a .smack. A completely general function is impossible, as you can't fit two 32 bit numbers into a single 32 bit value, but it is possible to provide a list and hash based scheme that will be no worse than horrible. > Of course this would mean that the cooperating LSMs would need to make > sure the other portions of the secattr were populated even when the > secid was used, but that should be okay ... if memory serves the secid > is only really used as part of the NetLabel cache for inbound traffic. That's my recollection as well, but if by some chance all the modules have it set, the check is efficient. >> + if ((secattr_a->flags & NETLBL_SECATTR_MLS_LVL) != >> + (secattr_b->flags & NETLBL_SECATTR_MLS_LVL)) >> + return false; >> + >> + if ((secattr_a->flags & NETLBL_SECATTR_MLS_LVL) && >> + secattr_a->attr.mls.lvl != secattr_b->attr.mls.lvl) >> + return false; >> + >> + if ((secattr_a->flags & NETLBL_SECATTR_MLS_CAT) != >> + (secattr_b->flags & NETLBL_SECATTR_MLS_CAT)) >> + return false; >> + >> + iter_a = secattr_a->attr.mls.cat; >> + iter_b = secattr_b->attr.mls.cat; >> + >> + while (iter_a && iter_b) { >> + if (iter_a->startbit != iter_b->startbit) >> + return false; >> + if (memcmp(iter_a->bitmap, iter_b->bitmap, >> + sizeof(iter_a->bitmap))) >> + return false; >> + iter_a = iter_a->next; >> + iter_b = iter_b->next; >> + } >> + >> + return !iter_a && !iter_b; >> +} > It might be nice to abstract the bitmap comparison out to another > function, e.g. netlbl_catmap_equal(...), but I understand that this is > just a high-level patch designed to shop around the idea and this is > very much an implementation nit. I've never been a fan of single-use functions, but as I'm playing in your yard we'll stick with your preferences. > As previously discussed, I don't have a problem with the idea of the > netlbl_secattr_equal() function. > >> + >> /* >> * Protocol Engine Functions >> */ >> diff --git a/security/security.c b/security/security.c >> index 8118377..e2e8c5c 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -28,6 +28,7 @@ >> #include <linux/msg.h> >> #include <net/flow.h> >> #include <net/sock.h> >> +#include <net/netlabel.h> >> >> #define MAX_LSM_EVM_XATTR 2 >> >> @@ -2084,7 +2085,27 @@ int security_socket_accept(struct socket *sock, struct socket *newsock) >> >> int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size) >> { >> - return call_int_hook(socket_sendmsg, 0, sock, msg, size); >> + struct security_hook_list *hp; >> + int rc; >> + struct netlbl_lsm_secattr *pattrs = NULL; >> + struct netlbl_lsm_secattr *attrs = NULL; >> + >> + list_for_each_entry(hp, &security_hook_heads.socket_sendmsg, list) { >> + rc = hp->hook.socket_sendmsg(sock, msg, size, &attrs); >> + if (rc) >> + return rc; >> + /* >> + * Only do the check if the current module reports >> + * an attribute, and there is something to compare it to. >> + */ >> + if (attrs) { >> + if (!pattrs) >> + pattrs = attrs; >> + else if (!netlbl_secattr_equal(pattrs, attrs)) >> + return -EACCES; >> + } >> + } >> + return 0; >> } > Seems reasonable at this stage, although I wonder if we could do > something so that we only check equality when the socket's label > changes. Under SELinux we never change the label once it is set, and > with Smack the label can only changes via setsockopt(), yes/no? That was my original thought, but the socket creation case gets quite ugly. If the labels would result in different network attributes the socket creation would fail, and everything breaks. Now we could cache the result when the value is set, but then the modules have to know to do that. I also don't think it would work in the presence of Smack single-label hosts, where Smack wouldn't be putting a label on the packet, and SELinux might want one. -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 17, 2017 at 6:20 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: > On 2/17/2017 2:26 PM, Paul Moore wrote: >> On Thu, Feb 16, 2017 at 6:35 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >>> Subject: [PATCH RFC 10/9] LSM: netlabel control on sendmsg >>> >>> This is incomplete as it only addresses the sendmsg hook, >>> but is presented to assess the viability of the approach. >>> >>> Create a netlabel KAPI to compare two netlabel security >>> attribute structures. Use this to determine if all of the >>> security modules agree on how a packet ought to be labeled >>> in the send operation. Refuse the send if they don't agree. >>> A module that does not report a netlabel security attribute >>> is assumed to not care what, if any, attribute is attached >>> to the packet. >>> >>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >> .. >> >>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >>> index 8bdd418..1e7b76d 100644 >>> --- a/include/linux/lsm_hooks.h >>> +++ b/include/linux/lsm_hooks.h >>> @@ -28,6 +28,8 @@ >>> #include <linux/init.h> >>> #include <linux/rculist.h> >>> >>> +struct netlbl_lsm_secattr; >>> + >>> /** >>> * Security hooks for program execution operations. >>> * >>> @@ -781,6 +783,7 @@ >>> * @sock contains the socket structure. >>> * @msg contains the message to be transmitted. >>> * @size contains the size of message. >>> + * @attrs points to the network attributes on return. >>> * Return 0 if permission is granted. >>> * @socket_recvmsg: >>> * Check permission before receiving a message from a socket. >>> @@ -1575,7 +1578,7 @@ union security_list_options { >>> int (*socket_listen)(struct socket *sock, int backlog); >>> int (*socket_accept)(struct socket *sock, struct socket *newsock); >>> int (*socket_sendmsg)(struct socket *sock, struct msghdr *msg, >>> - int size); >>> + int size, struct netlbl_lsm_secattr **attrs); >> I might mark 'attrs' as const to be safe. > > But ... socket_sendmsg is goin to change it's content. Sorry, yes, you're right ... I had this backwards in my mind. >>> diff --git a/net/netlabel/netlabel_kapi.c b/net/netlabel/netlabel_kapi.c >>> index 0dd1a60..926a46b 100644 >>> --- a/net/netlabel/netlabel_kapi.c >>> +++ b/net/netlabel/netlabel_kapi.c >>> @@ -1461,6 +1461,60 @@ int netlbl_cache_add(const struct sk_buff *skb, u16 family, >>> return -ENOMSG; >>> } >>> >>> +/** >>> + * netlbl_secattr_equal - Compare two lsm secattrs >>> + * @secattr_a: one security attribute >>> + * @secattr_b: the other security attribute >>> + * >>> + * Description: >>> + * Compare two lsm security attribute structures. Returns true >>> + * if they are the same, false otherwise. >>> + * >>> + */ >>> +bool netlbl_secattr_equal(const struct netlbl_lsm_secattr *secattr_a, >>> + const struct netlbl_lsm_secattr *secattr_b) >>> +{ >>> + struct netlbl_lsm_catmap *iter_a; >>> + struct netlbl_lsm_catmap *iter_b; >>> + >>> + if (secattr_a == secattr_b) >>> + return true; >>> + if (!secattr_a || !secattr_b) >>> + return false; >>> + >>> + if ((secattr_a->flags & NETLBL_SECATTR_SECID) && >>> + (secattr_b->flags & NETLBL_SECATTR_SECID)) >>> + return secattr_a->attr.secid.common == >>> + secattr_b->attr.secid.common; >> >> Comparing secids across different LSMs doesn't really make sense, and >> might even be dangerous; even if the scalar value was the same, the >> semantic value could be very different. I wonder if we just need to >> document that this function is only to be used when comparing secattrs >> across LSMs and if there is only a secid and nothing else to compare >> we should fail the comparison. > > The common field of a struct secids provides the information > necessary to identify all of the module specific secids. If two > secid.common values are the same you know that the secid.selinux > and the secid.smack values in the two structs match. > > So, secid1.common == secid2.common implies that > secid1.selinux == secid2.selinux && > secid1.smack == secid2.smack > > The current patchset supports a limited, but efficient > function for setting the .common given a .selinux and a .smack. > A completely general function is impossible, as you can't fit > two 32 bit numbers into a single 32 bit value, but it is possible > to provide a list and hash based scheme that will be no worse > than horrible. I wasn't paying close enough attention to the patch and I missed that somewhere the secid changed from a integer to a struct ... which is definitely eyebrow raising in itself. I'm haven't had a chance to look at your last patchset from late January, but I'm guessing the secid conversion happened there, yes? I should go look at that before I comment further, but my initial reaction is a bit ... skeptical. Which patch from the original patchset did the secid conversion? >>> + if ((secattr_a->flags & NETLBL_SECATTR_MLS_LVL) != >>> + (secattr_b->flags & NETLBL_SECATTR_MLS_LVL)) >>> + return false; >>> + >>> + if ((secattr_a->flags & NETLBL_SECATTR_MLS_LVL) && >>> + secattr_a->attr.mls.lvl != secattr_b->attr.mls.lvl) >>> + return false; >>> + >>> + if ((secattr_a->flags & NETLBL_SECATTR_MLS_CAT) != >>> + (secattr_b->flags & NETLBL_SECATTR_MLS_CAT)) >>> + return false; >>> + >>> + iter_a = secattr_a->attr.mls.cat; >>> + iter_b = secattr_b->attr.mls.cat; >>> + >>> + while (iter_a && iter_b) { >>> + if (iter_a->startbit != iter_b->startbit) >>> + return false; >>> + if (memcmp(iter_a->bitmap, iter_b->bitmap, >>> + sizeof(iter_a->bitmap))) >>> + return false; >>> + iter_a = iter_a->next; >>> + iter_b = iter_b->next; >>> + } >>> + >>> + return !iter_a && !iter_b; >>> +} >> >> It might be nice to abstract the bitmap comparison out to another >> function, e.g. netlbl_catmap_equal(...), but I understand that this is >> just a high-level patch designed to shop around the idea and this is >> very much an implementation nit. > > I've never been a fan of single-use functions, but as > I'm playing in your yard we'll stick with your preferences. Yes, I agree with you, I guess I was trying to hide some of the bitmap implementation guts so the logic in this function was a bit more clear, but in hindsight that seems a bit foolish, especially since it is unlikely that anyone else would ever have a need for a catmap_equal() routine. Keep it as you've written it, sorry for the noise. >>> diff --git a/security/security.c b/security/security.c >>> index 8118377..e2e8c5c 100644 >>> --- a/security/security.c >>> +++ b/security/security.c >>> @@ -28,6 +28,7 @@ >>> #include <linux/msg.h> >>> #include <net/flow.h> >>> #include <net/sock.h> >>> +#include <net/netlabel.h> >>> >>> #define MAX_LSM_EVM_XATTR 2 >>> >>> @@ -2084,7 +2085,27 @@ int security_socket_accept(struct socket *sock, struct socket *newsock) >>> >>> int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size) >>> { >>> - return call_int_hook(socket_sendmsg, 0, sock, msg, size); >>> + struct security_hook_list *hp; >>> + int rc; >>> + struct netlbl_lsm_secattr *pattrs = NULL; >>> + struct netlbl_lsm_secattr *attrs = NULL; >>> + >>> + list_for_each_entry(hp, &security_hook_heads.socket_sendmsg, list) { >>> + rc = hp->hook.socket_sendmsg(sock, msg, size, &attrs); >>> + if (rc) >>> + return rc; >>> + /* >>> + * Only do the check if the current module reports >>> + * an attribute, and there is something to compare it to. >>> + */ >>> + if (attrs) { >>> + if (!pattrs) >>> + pattrs = attrs; >>> + else if (!netlbl_secattr_equal(pattrs, attrs)) >>> + return -EACCES; >>> + } >>> + } >>> + return 0; >>> } >> >> Seems reasonable at this stage, although I wonder if we could do >> something so that we only check equality when the socket's label >> changes. Under SELinux we never change the label once it is set, and >> with Smack the label can only changes via setsockopt(), yes/no? > > That was my original thought, but the socket creation > case gets quite ugly. If the labels would result in different > network attributes the socket creation would fail, and > everything breaks. We can fail socket creation now, depending on the desires of the LSM. Failing socket creation when the stacked LSMs don't agree on the socket's label seems like a better option to me, why it is uglier than waiting until the write/send? > Now we could cache the result when the > value is set, but then the modules have to know to do that. > I also don't think it would work in the presence of Smack > single-label hosts, where Smack wouldn't be putting a label > on the packet, and SELinux might want one.
On 2/18/2017 6:18 AM, Paul Moore wrote: > On Fri, Feb 17, 2017 at 6:20 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 2/17/2017 2:26 PM, Paul Moore wrote: >>> On Thu, Feb 16, 2017 at 6:35 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >>>> Subject: [PATCH RFC 10/9] LSM: netlabel control on sendmsg >>>> >>>> This is incomplete as it only addresses the sendmsg hook, >>>> but is presented to assess the viability of the approach. >>>> >>>> Create a netlabel KAPI to compare two netlabel security >>>> attribute structures. Use this to determine if all of the >>>> security modules agree on how a packet ought to be labeled >>>> in the send operation. Refuse the send if they don't agree. >>>> A module that does not report a netlabel security attribute >>>> is assumed to not care what, if any, attribute is attached >>>> to the packet. >>>> >>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>> .. >>> >>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >>>> index 8bdd418..1e7b76d 100644 >>>> --- a/include/linux/lsm_hooks.h >>>> +++ b/include/linux/lsm_hooks.h >>>> @@ -28,6 +28,8 @@ >>>> #include <linux/init.h> >>>> #include <linux/rculist.h> >>>> >>>> +struct netlbl_lsm_secattr; >>>> + >>>> /** >>>> * Security hooks for program execution operations. >>>> * >>>> @@ -781,6 +783,7 @@ >>>> * @sock contains the socket structure. >>>> * @msg contains the message to be transmitted. >>>> * @size contains the size of message. >>>> + * @attrs points to the network attributes on return. >>>> * Return 0 if permission is granted. >>>> * @socket_recvmsg: >>>> * Check permission before receiving a message from a socket. >>>> @@ -1575,7 +1578,7 @@ union security_list_options { >>>> int (*socket_listen)(struct socket *sock, int backlog); >>>> int (*socket_accept)(struct socket *sock, struct socket *newsock); >>>> int (*socket_sendmsg)(struct socket *sock, struct msghdr *msg, >>>> - int size); >>>> + int size, struct netlbl_lsm_secattr **attrs); >>> I might mark 'attrs' as const to be safe. >> But ... socket_sendmsg is goin to change it's content. > Sorry, yes, you're right ... I had this backwards in my mind. > >>>> diff --git a/net/netlabel/netlabel_kapi.c b/net/netlabel/netlabel_kapi.c >>>> index 0dd1a60..926a46b 100644 >>>> --- a/net/netlabel/netlabel_kapi.c >>>> +++ b/net/netlabel/netlabel_kapi.c >>>> @@ -1461,6 +1461,60 @@ int netlbl_cache_add(const struct sk_buff *skb, u16 family, >>>> return -ENOMSG; >>>> } >>>> >>>> +/** >>>> + * netlbl_secattr_equal - Compare two lsm secattrs >>>> + * @secattr_a: one security attribute >>>> + * @secattr_b: the other security attribute >>>> + * >>>> + * Description: >>>> + * Compare two lsm security attribute structures. Returns true >>>> + * if they are the same, false otherwise. >>>> + * >>>> + */ >>>> +bool netlbl_secattr_equal(const struct netlbl_lsm_secattr *secattr_a, >>>> + const struct netlbl_lsm_secattr *secattr_b) >>>> +{ >>>> + struct netlbl_lsm_catmap *iter_a; >>>> + struct netlbl_lsm_catmap *iter_b; >>>> + >>>> + if (secattr_a == secattr_b) >>>> + return true; >>>> + if (!secattr_a || !secattr_b) >>>> + return false; >>>> + >>>> + if ((secattr_a->flags & NETLBL_SECATTR_SECID) && >>>> + (secattr_b->flags & NETLBL_SECATTR_SECID)) >>>> + return secattr_a->attr.secid.common == >>>> + secattr_b->attr.secid.common; >>> Comparing secids across different LSMs doesn't really make sense, and >>> might even be dangerous; even if the scalar value was the same, the >>> semantic value could be very different. I wonder if we just need to >>> document that this function is only to be used when comparing secattrs >>> across LSMs and if there is only a secid and nothing else to compare >>> we should fail the comparison. >> The common field of a struct secids provides the information >> necessary to identify all of the module specific secids. If two >> secid.common values are the same you know that the secid.selinux >> and the secid.smack values in the two structs match. >> >> So, secid1.common == secid2.common implies that >> secid1.selinux == secid2.selinux && >> secid1.smack == secid2.smack >> >> The current patchset supports a limited, but efficient >> function for setting the .common given a .selinux and a .smack. >> A completely general function is impossible, as you can't fit >> two 32 bit numbers into a single 32 bit value, but it is possible >> to provide a list and hash based scheme that will be no worse >> than horrible. > I wasn't paying close enough attention to the patch and I missed that > somewhere the secid changed from a integer to a struct ... which is > definitely eyebrow raising in itself. I'm haven't had a chance to > look at your last patchset from late January, but I'm guessing the > secid conversion happened there, yes? I should go look at that before > I comment further, but my initial reaction is a bit ... skeptical. > Which patch from the original patchset did the secid conversion? [PATCH 8/9] Subject: [PATCH] LSM: Provide for multiple secids when stacking modules In the case where you don't have SELinux and Smack (or any combination of modules that use secids) you get a single u32. If you do have SELinux and Smack you get a struct with 3 u32's, one for each of the modules and a third that's "common". There's a function that takes the "common" value and fills in the SELinux and Smack values, and another that takes the two modules specific values and gives you the "common". There is a one-to-one mapping of common to the SELinux and Smack pair. The current implementation of the mapping function is limited. I made the data bigger to save doing mapping calculations all over the place. I've implemented it both ways and given the use pattern of secids I think this is the better choice. It's certainly much less code. >>>> + if ((secattr_a->flags & NETLBL_SECATTR_MLS_LVL) != >>>> + (secattr_b->flags & NETLBL_SECATTR_MLS_LVL)) >>>> + return false; >>>> + >>>> + if ((secattr_a->flags & NETLBL_SECATTR_MLS_LVL) && >>>> + secattr_a->attr.mls.lvl != secattr_b->attr.mls.lvl) >>>> + return false; >>>> + >>>> + if ((secattr_a->flags & NETLBL_SECATTR_MLS_CAT) != >>>> + (secattr_b->flags & NETLBL_SECATTR_MLS_CAT)) >>>> + return false; >>>> + >>>> + iter_a = secattr_a->attr.mls.cat; >>>> + iter_b = secattr_b->attr.mls.cat; >>>> + >>>> + while (iter_a && iter_b) { >>>> + if (iter_a->startbit != iter_b->startbit) >>>> + return false; >>>> + if (memcmp(iter_a->bitmap, iter_b->bitmap, >>>> + sizeof(iter_a->bitmap))) >>>> + return false; >>>> + iter_a = iter_a->next; >>>> + iter_b = iter_b->next; >>>> + } >>>> + >>>> + return !iter_a && !iter_b; >>>> +} >>> It might be nice to abstract the bitmap comparison out to another >>> function, e.g. netlbl_catmap_equal(...), but I understand that this is >>> just a high-level patch designed to shop around the idea and this is >>> very much an implementation nit. >> I've never been a fan of single-use functions, but as >> I'm playing in your yard we'll stick with your preferences. > Yes, I agree with you, I guess I was trying to hide some of the bitmap > implementation guts so the logic in this function was a bit more > clear, but in hindsight that seems a bit foolish, especially since it > is unlikely that anyone else would ever have a need for a > catmap_equal() routine. Keep it as you've written it, sorry for the > noise. > >>>> diff --git a/security/security.c b/security/security.c >>>> index 8118377..e2e8c5c 100644 >>>> --- a/security/security.c >>>> +++ b/security/security.c >>>> @@ -28,6 +28,7 @@ >>>> #include <linux/msg.h> >>>> #include <net/flow.h> >>>> #include <net/sock.h> >>>> +#include <net/netlabel.h> >>>> >>>> #define MAX_LSM_EVM_XATTR 2 >>>> >>>> @@ -2084,7 +2085,27 @@ int security_socket_accept(struct socket *sock, struct socket *newsock) >>>> >>>> int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size) >>>> { >>>> - return call_int_hook(socket_sendmsg, 0, sock, msg, size); >>>> + struct security_hook_list *hp; >>>> + int rc; >>>> + struct netlbl_lsm_secattr *pattrs = NULL; >>>> + struct netlbl_lsm_secattr *attrs = NULL; >>>> + >>>> + list_for_each_entry(hp, &security_hook_heads.socket_sendmsg, list) { >>>> + rc = hp->hook.socket_sendmsg(sock, msg, size, &attrs); >>>> + if (rc) >>>> + return rc; >>>> + /* >>>> + * Only do the check if the current module reports >>>> + * an attribute, and there is something to compare it to. >>>> + */ >>>> + if (attrs) { >>>> + if (!pattrs) >>>> + pattrs = attrs; >>>> + else if (!netlbl_secattr_equal(pattrs, attrs)) >>>> + return -EACCES; >>>> + } >>>> + } >>>> + return 0; >>>> } >>> Seems reasonable at this stage, although I wonder if we could do >>> something so that we only check equality when the socket's label >>> changes. Under SELinux we never change the label once it is set, and >>> with Smack the label can only changes via setsockopt(), yes/no? >> That was my original thought, but the socket creation >> case gets quite ugly. If the labels would result in different >> network attributes the socket creation would fail, and >> everything breaks. > We can fail socket creation now, depending on the desires of the LSM. > Failing socket creation when the stacked LSMs don't agree on the > socket's label seems like a better option to me, why it is uglier than > waiting until the write/send? There are a number of reasons and ways that Smack may choose to label a packet differently from the value set at socket creation. If the socket send value is the ambient label, or the destination is a single label host Smack will want to send the packet unlabeled. If the send value is changed (fsetxattr is the mechanism, BTW) the label could match up with what SELinux wants. The Smack label to CIPSO mapping can be changed after the socket is created. That's unlikely, but possible. And then there's the experiment where I tried doing the fail at creation. Systemd was not at all happy. It may be possible to come with a configuration of the two modules (SELinux doing full labeling, Smack CIPSO mapping set to match SELinux domains) that could work, but it would require significant changes in the initialization for both. Configuration that is currently done by user space code would have to move into the pre-init kernel. I can also see cases where an as yet undefined module might implement some sort of handling caveats on packets. It might agree with SELinux/Smack most of the time, but add a category if the packet contains specific data. >> Now we could cache the result when the >> value is set, but then the modules have to know to do that. >> I also don't think it would work in the presence of Smack >> single-label hosts, where Smack wouldn't be putting a label >> on the packet, and SELinux might want one. -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Feb 18, 2017 at 12:58 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: > On 2/18/2017 6:18 AM, Paul Moore wrote: >> On Fri, Feb 17, 2017 at 6:20 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >>> On 2/17/2017 2:26 PM, Paul Moore wrote: >>>> On Thu, Feb 16, 2017 at 6:35 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: ... >> I wasn't paying close enough attention to the patch and I missed that >> somewhere the secid changed from a integer to a struct ... which is >> definitely eyebrow raising in itself. I'm haven't had a chance to >> look at your last patchset from late January, but I'm guessing the >> secid conversion happened there, yes? I should go look at that before >> I comment further, but my initial reaction is a bit ... skeptical. >> Which patch from the original patchset did the secid conversion? > > [PATCH 8/9] Subject: [PATCH] LSM: Provide for multiple secids when > stacking modules > > In the case where you don't have SELinux and Smack > (or any combination of modules that use secids) you > get a single u32. If you do have SELinux and Smack > you get a struct with 3 u32's, one for each of the > modules and a third that's "common". There's a function > that takes the "common" value and fills in the SELinux > and Smack values, and another that takes the two > modules specific values and gives you the "common". > There is a one-to-one mapping of common to the SELinux > and Smack pair. The current implementation of the > mapping function is limited. So the common value is only ever used to reference the secid struct containing the secids from the stacked LSMs? Okay. >> We can fail socket creation now, depending on the desires of the LSM. >> Failing socket creation when the stacked LSMs don't agree on the >> socket's label seems like a better option to me, why it is uglier than >> waiting until the write/send? > > There are a number of reasons and ways that Smack may > choose to label a packet differently from the value set > at socket creation. If the socket send value is the ambient > label, or the destination is a single label host Smack > will want to send the packet unlabeled. If the send value > is changed (fsetxattr is the mechanism, BTW) the label > could match up with what SELinux wants. Is it only a matter of labeled vs unlabeled? If so, one could (and arguably should) just handle that via the NetLabel configuration and then it wouldn't matter, you wouldn't have to check this on write. Excluding the labeled/unlabeled concern for a moment, if the only way to change the Smack label on a socket is via fsetxattr it seems like it would be much more desirable to simply check the socket label on the fsetxattr operation instead of write/send. However, if there is much more to it than what you've mentioned above, feel free to disregard. > The Smack label > to CIPSO mapping can be changed after the socket is created. > That's unlikely, but possible. FWIW the NetLabel mappings can change at any time for any LSM, that isn't specific to Smack, but the mapping doesn't change the semantic meaning of the label, it merely translates it to a different DOI (to borrow terms a bit). This is why we are comparing the netlbl_lsm_secattr values and not the CIPSO/CALIPSO tags. > And then there's the experiment where I tried doing > the fail at creation. Systemd was not at all happy. Out of curiosity, do you still have the logs from the failures? I think it might be interesting to see the problems people are going to face with this, because I still have serious doubts that this is going to solve the problems that people think it is going to solve. > It may be possible to come with a configuration of > the two modules (SELinux doing full labeling, Smack > CIPSO mapping set to match SELinux domains) that could > work, but it would require significant changes in > the initialization for both. Configuration that is > currently done by user space code would have to move > into the pre-init kernel. Right now I'm just curious, but it would be interesting to see how far away we are from getting this to work. > I can also see cases where an as yet undefined module > might implement some sort of handling caveats on packets. > It might agree with SELinux/Smack most of the time, but > add a category if the packet contains specific data. I agree that we don't want to code ourselves into a corner, but we don't want to make life unnecessarily complex for some undefined future LSM.
On 02/20/2017 02:33 PM, Paul Moore wrote: > On Sat, Feb 18, 2017 at 12:58 PM, Casey Schaufler > <casey@schaufler-ca.com> wrote: >> On 2/18/2017 6:18 AM, Paul Moore wrote: >>> On Fri, Feb 17, 2017 at 6:20 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >>>> On 2/17/2017 2:26 PM, Paul Moore wrote: >>>>> On Thu, Feb 16, 2017 at 6:35 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: > > ... > >>> I wasn't paying close enough attention to the patch and I missed that >>> somewhere the secid changed from a integer to a struct ... which is >>> definitely eyebrow raising in itself. I'm haven't had a chance to >>> look at your last patchset from late January, but I'm guessing the >>> secid conversion happened there, yes? I should go look at that before >>> I comment further, but my initial reaction is a bit ... skeptical. >>> Which patch from the original patchset did the secid conversion? >> >> [PATCH 8/9] Subject: [PATCH] LSM: Provide for multiple secids when >> stacking modules >> >> In the case where you don't have SELinux and Smack >> (or any combination of modules that use secids) you >> get a single u32. If you do have SELinux and Smack >> you get a struct with 3 u32's, one for each of the >> modules and a third that's "common". There's a function >> that takes the "common" value and fills in the SELinux >> and Smack values, and another that takes the two >> modules specific values and gives you the "common". >> There is a one-to-one mapping of common to the SELinux >> and Smack pair. The current implementation of the >> mapping function is limited. > > So the common value is only ever used to reference the secid struct > containing the secids from the stacked LSMs? Okay. > >>> We can fail socket creation now, depending on the desires of the LSM. >>> Failing socket creation when the stacked LSMs don't agree on the >>> socket's label seems like a better option to me, why it is uglier than >>> waiting until the write/send? >> >> There are a number of reasons and ways that Smack may >> choose to label a packet differently from the value set >> at socket creation. If the socket send value is the ambient >> label, or the destination is a single label host Smack >> will want to send the packet unlabeled. If the send value >> is changed (fsetxattr is the mechanism, BTW) the label >> could match up with what SELinux wants. > > Is it only a matter of labeled vs unlabeled? If so, one could (and > arguably should) just handle that via the NetLabel configuration and > then it wouldn't matter, you wouldn't have to check this on write. > > Excluding the labeled/unlabeled concern for a moment, if the only way > to change the Smack label on a socket is via fsetxattr it seems like > it would be much more desirable to simply check the socket label on > the fsetxattr operation instead of write/send. However, if there is > much more to it than what you've mentioned above, feel free to > disregard. > I know for apparmor we will want to be able to do more than just checking at fsetxattr, due to the lazy stacked label construction and delegation. But that code hasn't landed yet so it hardly counts for much. -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 20, 2017 at 9:18 PM, John Johansen <john.johansen@canonical.com> wrote: > On 02/20/2017 02:33 PM, Paul Moore wrote: >> On Sat, Feb 18, 2017 at 12:58 PM, Casey Schaufler >> <casey@schaufler-ca.com> wrote: >>> On 2/18/2017 6:18 AM, Paul Moore wrote: >>>> On Fri, Feb 17, 2017 at 6:20 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >>>>> On 2/17/2017 2:26 PM, Paul Moore wrote: >>>>>> On Thu, Feb 16, 2017 at 6:35 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >> >> ... >> >>>> I wasn't paying close enough attention to the patch and I missed that >>>> somewhere the secid changed from a integer to a struct ... which is >>>> definitely eyebrow raising in itself. I'm haven't had a chance to >>>> look at your last patchset from late January, but I'm guessing the >>>> secid conversion happened there, yes? I should go look at that before >>>> I comment further, but my initial reaction is a bit ... skeptical. >>>> Which patch from the original patchset did the secid conversion? >>> >>> [PATCH 8/9] Subject: [PATCH] LSM: Provide for multiple secids when >>> stacking modules >>> >>> In the case where you don't have SELinux and Smack >>> (or any combination of modules that use secids) you >>> get a single u32. If you do have SELinux and Smack >>> you get a struct with 3 u32's, one for each of the >>> modules and a third that's "common". There's a function >>> that takes the "common" value and fills in the SELinux >>> and Smack values, and another that takes the two >>> modules specific values and gives you the "common". >>> There is a one-to-one mapping of common to the SELinux >>> and Smack pair. The current implementation of the >>> mapping function is limited. >> >> So the common value is only ever used to reference the secid struct >> containing the secids from the stacked LSMs? Okay. >> >>>> We can fail socket creation now, depending on the desires of the LSM. >>>> Failing socket creation when the stacked LSMs don't agree on the >>>> socket's label seems like a better option to me, why it is uglier than >>>> waiting until the write/send? >>> >>> There are a number of reasons and ways that Smack may >>> choose to label a packet differently from the value set >>> at socket creation. If the socket send value is the ambient >>> label, or the destination is a single label host Smack >>> will want to send the packet unlabeled. If the send value >>> is changed (fsetxattr is the mechanism, BTW) the label >>> could match up with what SELinux wants. >> >> Is it only a matter of labeled vs unlabeled? If so, one could (and >> arguably should) just handle that via the NetLabel configuration and >> then it wouldn't matter, you wouldn't have to check this on write. >> >> Excluding the labeled/unlabeled concern for a moment, if the only way >> to change the Smack label on a socket is via fsetxattr it seems like >> it would be much more desirable to simply check the socket label on >> the fsetxattr operation instead of write/send. However, if there is >> much more to it than what you've mentioned above, feel free to >> disregard. > > I know for apparmor we will want to be able to do more than just > checking at fsetxattr, due to the lazy stacked label construction > and delegation. But that code hasn't landed yet so it hardly > counts for much. Can you give us a quick preview on where you'll need to do checking? Possibly even when that code might land upstream?
On 2/20/2017 6:18 PM, John Johansen wrote: > On 02/20/2017 02:33 PM, Paul Moore wrote: >> On Sat, Feb 18, 2017 at 12:58 PM, Casey Schaufler >> <casey@schaufler-ca.com> wrote: >>> On 2/18/2017 6:18 AM, Paul Moore wrote: >>>> On Fri, Feb 17, 2017 at 6:20 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >>>>> On 2/17/2017 2:26 PM, Paul Moore wrote: >>>>>> On Thu, Feb 16, 2017 at 6:35 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >> ... >> >>>> I wasn't paying close enough attention to the patch and I missed that >>>> somewhere the secid changed from a integer to a struct ... which is >>>> definitely eyebrow raising in itself. I'm haven't had a chance to >>>> look at your last patchset from late January, but I'm guessing the >>>> secid conversion happened there, yes? I should go look at that before >>>> I comment further, but my initial reaction is a bit ... skeptical. >>>> Which patch from the original patchset did the secid conversion? >>> [PATCH 8/9] Subject: [PATCH] LSM: Provide for multiple secids when >>> stacking modules >>> >>> In the case where you don't have SELinux and Smack >>> (or any combination of modules that use secids) you >>> get a single u32. If you do have SELinux and Smack >>> you get a struct with 3 u32's, one for each of the >>> modules and a third that's "common". There's a function >>> that takes the "common" value and fills in the SELinux >>> and Smack values, and another that takes the two >>> modules specific values and gives you the "common". >>> There is a one-to-one mapping of common to the SELinux >>> and Smack pair. The current implementation of the >>> mapping function is limited. >> So the common value is only ever used to reference the secid struct >> containing the secids from the stacked LSMs? Okay. >> >>>> We can fail socket creation now, depending on the desires of the LSM. >>>> Failing socket creation when the stacked LSMs don't agree on the >>>> socket's label seems like a better option to me, why it is uglier than >>>> waiting until the write/send? >>> There are a number of reasons and ways that Smack may >>> choose to label a packet differently from the value set >>> at socket creation. If the socket send value is the ambient >>> label, or the destination is a single label host Smack >>> will want to send the packet unlabeled. If the send value >>> is changed (fsetxattr is the mechanism, BTW) the label >>> could match up with what SELinux wants. >> Is it only a matter of labeled vs unlabeled? If so, one could (and >> arguably should) just handle that via the NetLabel configuration and >> then it wouldn't matter, you wouldn't have to check this on write. >> >> Excluding the labeled/unlabeled concern for a moment, if the only way >> to change the Smack label on a socket is via fsetxattr it seems like >> it would be much more desirable to simply check the socket label on >> the fsetxattr operation instead of write/send. However, if there is >> much more to it than what you've mentioned above, feel free to >> disregard. >> > I know for apparmor we will want to be able to do more than just > checking at fsetxattr, due to the lazy stacked label construction > and delegation. But that code hasn't landed yet so it hardly > counts for much. You've been teasing us with the AppArmor update for quite some time now. It would be very helpful if we could start seeing code. -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/21/2017 04:57 AM, Paul Moore wrote: > On Mon, Feb 20, 2017 at 9:18 PM, John Johansen > <john.johansen@canonical.com> wrote: >> On 02/20/2017 02:33 PM, Paul Moore wrote: >>> On Sat, Feb 18, 2017 at 12:58 PM, Casey Schaufler >>> <casey@schaufler-ca.com> wrote: >>>> On 2/18/2017 6:18 AM, Paul Moore wrote: >>>>> On Fri, Feb 17, 2017 at 6:20 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >>>>>> On 2/17/2017 2:26 PM, Paul Moore wrote: >>>>>>> On Thu, Feb 16, 2017 at 6:35 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >>> >>> ... >>> >>>>> I wasn't paying close enough attention to the patch and I missed that >>>>> somewhere the secid changed from a integer to a struct ... which is >>>>> definitely eyebrow raising in itself. I'm haven't had a chance to >>>>> look at your last patchset from late January, but I'm guessing the >>>>> secid conversion happened there, yes? I should go look at that before >>>>> I comment further, but my initial reaction is a bit ... skeptical. >>>>> Which patch from the original patchset did the secid conversion? >>>> >>>> [PATCH 8/9] Subject: [PATCH] LSM: Provide for multiple secids when >>>> stacking modules >>>> >>>> In the case where you don't have SELinux and Smack >>>> (or any combination of modules that use secids) you >>>> get a single u32. If you do have SELinux and Smack >>>> you get a struct with 3 u32's, one for each of the >>>> modules and a third that's "common". There's a function >>>> that takes the "common" value and fills in the SELinux >>>> and Smack values, and another that takes the two >>>> modules specific values and gives you the "common". >>>> There is a one-to-one mapping of common to the SELinux >>>> and Smack pair. The current implementation of the >>>> mapping function is limited. >>> >>> So the common value is only ever used to reference the secid struct >>> containing the secids from the stacked LSMs? Okay. >>> >>>>> We can fail socket creation now, depending on the desires of the LSM. >>>>> Failing socket creation when the stacked LSMs don't agree on the >>>>> socket's label seems like a better option to me, why it is uglier than >>>>> waiting until the write/send? >>>> >>>> There are a number of reasons and ways that Smack may >>>> choose to label a packet differently from the value set >>>> at socket creation. If the socket send value is the ambient >>>> label, or the destination is a single label host Smack >>>> will want to send the packet unlabeled. If the send value >>>> is changed (fsetxattr is the mechanism, BTW) the label >>>> could match up with what SELinux wants. >>> >>> Is it only a matter of labeled vs unlabeled? If so, one could (and >>> arguably should) just handle that via the NetLabel configuration and >>> then it wouldn't matter, you wouldn't have to check this on write. >>> >>> Excluding the labeled/unlabeled concern for a moment, if the only way >>> to change the Smack label on a socket is via fsetxattr it seems like >>> it would be much more desirable to simply check the socket label on >>> the fsetxattr operation instead of write/send. However, if there is >>> much more to it than what you've mentioned above, feel free to >>> disregard. >> >> I know for apparmor we will want to be able to do more than just >> checking at fsetxattr, due to the lazy stacked label construction >> and delegation. But that code hasn't landed yet so it hardly >> counts for much. > > Can you give us a quick preview on where you'll need to do checking? > Possibly even when that code might land upstream? > The network code is still very much at an experimental stage, and I haven't had a chance to play with it for a few months. So it can certainly be changed Generally speaking apparmor would prefer to be capable of labeling sockets after creation. At creation it will get a basic/label or type but ideally this could be changed later. We would love to be able to update the socket label at bind, and connect, and at times when the socket is passed or inherited between tasks, along with doing something via fsetxattr or a similar mechanism. So doing things at write/send shouldn't be necessary except perhaps for unconnected sockets, I would have to go look at those again. Some of it depends on how lazy/eager we are at constructing the label for the socket. There is room to adjust and play, the current set of hooks aren't ideal anyways. A large part of the rewrite work was to make apparmor fit better within the current LSM framework, so I am not going to get too worked up about whatever requiremnts/decisions are made here. The apparmor code will adjust, I would just prefer being able to update the label after socket creation. As for when something will land, that is tough to say. I have the current distraction to get out of my way and then its back to upstreaming more of the code. I expect I should be able to get the RFC for the implied labeling and apparmor internal stacking out by the end of next week. The networking will need the full type splitting work (its beyond the implied label work I working on landing). So I expect it will be a while yet. I have been very resistant to doing new feature work until I can get the current diff landed. With that being said, once I get the next RFC up, I could look at getting a base networking patch based on the RFC together for discussion, it wouldn't be anywhere close to a finished patch but would give you plenty of opportunity to tell me all that I am doing wrong. course
On 02/21/2017 08:21 AM, Casey Schaufler wrote: > On 2/20/2017 6:18 PM, John Johansen wrote: >> On 02/20/2017 02:33 PM, Paul Moore wrote: >>> On Sat, Feb 18, 2017 at 12:58 PM, Casey Schaufler >>> <casey@schaufler-ca.com> wrote: >>>> On 2/18/2017 6:18 AM, Paul Moore wrote: >>>>> On Fri, Feb 17, 2017 at 6:20 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >>>>>> On 2/17/2017 2:26 PM, Paul Moore wrote: >>>>>>> On Thu, Feb 16, 2017 at 6:35 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >>> ... >>> >>>>> I wasn't paying close enough attention to the patch and I missed that >>>>> somewhere the secid changed from a integer to a struct ... which is >>>>> definitely eyebrow raising in itself. I'm haven't had a chance to >>>>> look at your last patchset from late January, but I'm guessing the >>>>> secid conversion happened there, yes? I should go look at that before >>>>> I comment further, but my initial reaction is a bit ... skeptical. >>>>> Which patch from the original patchset did the secid conversion? >>>> [PATCH 8/9] Subject: [PATCH] LSM: Provide for multiple secids when >>>> stacking modules >>>> >>>> In the case where you don't have SELinux and Smack >>>> (or any combination of modules that use secids) you >>>> get a single u32. If you do have SELinux and Smack >>>> you get a struct with 3 u32's, one for each of the >>>> modules and a third that's "common". There's a function >>>> that takes the "common" value and fills in the SELinux >>>> and Smack values, and another that takes the two >>>> modules specific values and gives you the "common". >>>> There is a one-to-one mapping of common to the SELinux >>>> and Smack pair. The current implementation of the >>>> mapping function is limited. >>> So the common value is only ever used to reference the secid struct >>> containing the secids from the stacked LSMs? Okay. >>> >>>>> We can fail socket creation now, depending on the desires of the LSM. >>>>> Failing socket creation when the stacked LSMs don't agree on the >>>>> socket's label seems like a better option to me, why it is uglier than >>>>> waiting until the write/send? >>>> There are a number of reasons and ways that Smack may >>>> choose to label a packet differently from the value set >>>> at socket creation. If the socket send value is the ambient >>>> label, or the destination is a single label host Smack >>>> will want to send the packet unlabeled. If the send value >>>> is changed (fsetxattr is the mechanism, BTW) the label >>>> could match up with what SELinux wants. >>> Is it only a matter of labeled vs unlabeled? If so, one could (and >>> arguably should) just handle that via the NetLabel configuration and >>> then it wouldn't matter, you wouldn't have to check this on write. >>> >>> Excluding the labeled/unlabeled concern for a moment, if the only way >>> to change the Smack label on a socket is via fsetxattr it seems like >>> it would be much more desirable to simply check the socket label on >>> the fsetxattr operation instead of write/send. However, if there is >>> much more to it than what you've mentioned above, feel free to >>> disregard. >>> >> I know for apparmor we will want to be able to do more than just >> checking at fsetxattr, due to the lazy stacked label construction >> and delegation. But that code hasn't landed yet so it hardly >> counts for much. > > You've been teasing us with the AppArmor update for quite some > time now. It would be very helpful if we could start seeing code. > > Indeed, I am hoping to be able to get an RFC for the implied labeling code up next week. Its not what is needed to do networking right (I need to rework it to full type splitting) but a step in the direction of finally getting the diff upstreamed. -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 8bdd418..1e7b76d 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -28,6 +28,8 @@ #include <linux/init.h> #include <linux/rculist.h> +struct netlbl_lsm_secattr; + /** * Security hooks for program execution operations. * @@ -781,6 +783,7 @@ * @sock contains the socket structure. * @msg contains the message to be transmitted. * @size contains the size of message. + * @attrs points to the network attributes on return. * Return 0 if permission is granted. * @socket_recvmsg: * Check permission before receiving a message from a socket. @@ -1575,7 +1578,7 @@ union security_list_options { int (*socket_listen)(struct socket *sock, int backlog); int (*socket_accept)(struct socket *sock, struct socket *newsock); int (*socket_sendmsg)(struct socket *sock, struct msghdr *msg, - int size); + int size, struct netlbl_lsm_secattr **attrs); int (*socket_recvmsg)(struct socket *sock, struct msghdr *msg, int size, int flags); int (*socket_getsockname)(struct socket *sock); diff --git a/include/net/netlabel.h b/include/net/netlabel.h index 8cdd2d6..3cda2f3 100644 --- a/include/net/netlabel.h +++ b/include/net/netlabel.h @@ -472,6 +472,8 @@ int netlbl_catmap_setlong(struct netlbl_lsm_catmap **catmap, u32 offset, unsigned long bitmap, gfp_t flags); +bool netlbl_secattr_equal(const struct netlbl_lsm_secattr *secattr_a, + const struct netlbl_lsm_secattr *secattr_b); /* Bitmap functions */ @@ -623,6 +625,12 @@ static inline int netlbl_catmap_setlong(struct netlbl_lsm_catmap **catmap, { return 0; } +static inline bool netlbl_secattr_equal( + const struct netlbl_lsm_secattr *secattr_a, + const struct netlbl_lsm_secattr *secattr_b) +{ + return true; +} static inline int netlbl_enabled(void) { return 0; diff --git a/net/netlabel/netlabel_kapi.c b/net/netlabel/netlabel_kapi.c index 0dd1a60..926a46b 100644 --- a/net/netlabel/netlabel_kapi.c +++ b/net/netlabel/netlabel_kapi.c @@ -1461,6 +1461,60 @@ int netlbl_cache_add(const struct sk_buff *skb, u16 family, return -ENOMSG; } +/** + * netlbl_secattr_equal - Compare two lsm secattrs + * @secattr_a: one security attribute + * @secattr_b: the other security attribute + * + * Description: + * Compare two lsm security attribute structures. Returns true + * if they are the same, false otherwise. + * + */ +bool netlbl_secattr_equal(const struct netlbl_lsm_secattr *secattr_a, + const struct netlbl_lsm_secattr *secattr_b) +{ + struct netlbl_lsm_catmap *iter_a; + struct netlbl_lsm_catmap *iter_b; + + if (secattr_a == secattr_b) + return true; + if (!secattr_a || !secattr_b) + return false; + + if ((secattr_a->flags & NETLBL_SECATTR_SECID) && + (secattr_b->flags & NETLBL_SECATTR_SECID)) + return secattr_a->attr.secid.common == + secattr_b->attr.secid.common; + + if ((secattr_a->flags & NETLBL_SECATTR_MLS_LVL) != + (secattr_b->flags & NETLBL_SECATTR_MLS_LVL)) + return false; + + if ((secattr_a->flags & NETLBL_SECATTR_MLS_LVL) && + secattr_a->attr.mls.lvl != secattr_b->attr.mls.lvl) + return false; + + if ((secattr_a->flags & NETLBL_SECATTR_MLS_CAT) != + (secattr_b->flags & NETLBL_SECATTR_MLS_CAT)) + return false; + + iter_a = secattr_a->attr.mls.cat; + iter_b = secattr_b->attr.mls.cat; + + while (iter_a && iter_b) { + if (iter_a->startbit != iter_b->startbit) + return false; + if (memcmp(iter_a->bitmap, iter_b->bitmap, + sizeof(iter_a->bitmap))) + return false; + iter_a = iter_a->next; + iter_b = iter_b->next; + } + + return !iter_a && !iter_b; +} + /* * Protocol Engine Functions */ diff --git a/security/security.c b/security/security.c index 8118377..e2e8c5c 100644 --- a/security/security.c +++ b/security/security.c @@ -28,6 +28,7 @@ #include <linux/msg.h> #include <net/flow.h> #include <net/sock.h> +#include <net/netlabel.h> #define MAX_LSM_EVM_XATTR 2 @@ -2084,7 +2085,27 @@ int security_socket_accept(struct socket *sock, struct socket *newsock) int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size) { - return call_int_hook(socket_sendmsg, 0, sock, msg, size); + struct security_hook_list *hp; + int rc; + struct netlbl_lsm_secattr *pattrs = NULL; + struct netlbl_lsm_secattr *attrs = NULL; + + list_for_each_entry(hp, &security_hook_heads.socket_sendmsg, list) { + rc = hp->hook.socket_sendmsg(sock, msg, size, &attrs); + if (rc) + return rc; + /* + * Only do the check if the current module reports + * an attribute, and there is something to compare it to. + */ + if (attrs) { + if (!pattrs) + pattrs = attrs; + else if (!netlbl_secattr_equal(pattrs, attrs)) + return -EACCES; + } + } + return 0; } int security_socket_recvmsg(struct socket *sock, struct msghdr *msg, diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index ab099fc..5523421 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -4419,8 +4419,13 @@ static int selinux_socket_accept(struct socket *sock, struct socket *newsock) } static int selinux_socket_sendmsg(struct socket *sock, struct msghdr *msg, - int size) + int size, struct netlbl_lsm_secattr **attrs) { +#ifdef CONFIG_SECURITY_SELINUX_NETLABEL + struct sk_security_struct *sksec = selinux_sock(sock->sk); + + *attrs = sksec->nlbl_secattr; +#endif return sock_has_perm(current, sock->sk, SOCKET__WRITE); } diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 7984363..a2173f4 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -3616,7 +3616,7 @@ static int smack_unix_may_send(struct socket *sock, struct socket *other) * For IPv6 this is a check against the label of the port. */ static int smack_socket_sendmsg(struct socket *sock, struct msghdr *msg, - int size) + int size, struct netlbl_lsm_secattr **attrs) { struct sockaddr_in *sip = (struct sockaddr_in *) msg->msg_name; #if IS_ENABLED(CONFIG_IPV6) @@ -3628,6 +3628,7 @@ static int smack_socket_sendmsg(struct socket *sock, struct msghdr *msg, #endif int rc = 0; + *attrs = NULL; /* * Perfectly reasonable for this to be NULL */ @@ -3637,6 +3638,7 @@ static int smack_socket_sendmsg(struct socket *sock, struct msghdr *msg, switch (sock->sk->sk_family) { case AF_INET: rc = smack_netlabel_send(sock->sk, sip); + *attrs = &ssp->smk_out->smk_netlabel; break; case AF_INET6: #ifdef SMACK_IPV6_SECMARK_LABELING diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c index bbbf48d..f61039d 100644 --- a/security/tomoyo/tomoyo.c +++ b/security/tomoyo/tomoyo.c @@ -499,11 +499,12 @@ static int tomoyo_socket_bind(struct socket *sock, struct sockaddr *addr, * @sock: Pointer to "struct socket". * @msg: Pointer to "struct msghdr". * @size: Size of message. + * @attrs: unused * * Returns 0 on success, negative value otherwise. */ static int tomoyo_socket_sendmsg(struct socket *sock, struct msghdr *msg, - int size) + int size, struct netlbl_lsm_secattr **attrs) { return tomoyo_socket_sendmsg_permission(sock, msg, size); }
Subject: [PATCH RFC 10/9] LSM: netlabel control on sendmsg This is incomplete as it only addresses the sendmsg hook, but is presented to assess the viability of the approach. Create a netlabel KAPI to compare two netlabel security attribute structures. Use this to determine if all of the security modules agree on how a packet ought to be labeled in the send operation. Refuse the send if they don't agree. A module that does not report a netlabel security attribute is assumed to not care what, if any, attribute is attached to the packet. Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> --- include/linux/lsm_hooks.h | 5 +++- include/net/netlabel.h | 8 +++++++ net/netlabel/netlabel_kapi.c | 54 ++++++++++++++++++++++++++++++++++++++++++++ security/security.c | 23 ++++++++++++++++++- security/selinux/hooks.c | 7 +++++- security/smack/smack_lsm.c | 4 +++- security/tomoyo/tomoyo.c | 3 ++- 7 files changed, 99 insertions(+), 5 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html