| Message ID | 20211110111308.3463879-1-brambonne@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [RFC] selinux: Add netlink xperm support | expand |
On Wed, Nov 10, 2021 at 6:13 AM Bram Bonne <brambonne@google.com> wrote: > > Reuse the existing extended permissions infrastructure to support > sepolicy for different netlink message types. > > When individual netlink message types are omitted only the existing > permissions are checked. As is the case for ioctl xperms, this feature > is intended to provide finer granularity for nlmsg_read and nlmsg_write > permissions, as they may be too imprecise. For example, a single > NETLINK_ROUTE socket may provide access to both an interface's IP > address and to its ARP table, which might have different privacy > implications. In addition, the set of message types has grown over time, > so even if the current list is acceptable, future additions might not be. > It was suggested before on the mailing list [1] that extended permissions > would be a good fit for this purpose. > > Existing policy using the nlmsg_read and nlmsg_write permissions will > continue to work as-is. Similar to ioctl xperms, netlink xperms allow > for a more fine-grained policy where needed. > > Example policy on Android, preventing regular apps from accessing the > device's MAC address and ARP table, but allowing this access to > privileged apps, looks as follows: > > allow netdomain self:netlink_route_socket { > create read getattr write setattr lock append connect getopt > setopt shutdown nlmsg_read > }; > allowxperm netdomain self:netlink_route_socket nlmsg ~{ > RTM_GETLINK RTM_GETNEIGH RTM_GETNEIGHTBL > }; > allowxperm priv_app self:netlink_route_socket nlmsg { > RTM_GETLINK RTM_GETNEIGH RTM_GETNEIGHTBL > }; > > Android currently uses code similar to [1] as a temporary workaround to > limit access to certain netlink message types; our hope is that this patch > will allow us to move back to upstream code with an approach that works for > everyone. > > [1] https://lore.kernel.org/selinux/CAHC9VhRSUhozBycHMZcMaJsibJDxNMsTsKVT2zOnW=5H4R4mdg@mail.gmail.com/ > > Signed-off-by: Bram Bonne <brambonne@google.com> > --- > security/selinux/hooks.c | 24 +++++++++++++++++++++++- > security/selinux/ss/avtab.h | 1 + > security/selinux/ss/services.c | 23 ++++++++++++++++++++++- > 3 files changed, 46 insertions(+), 2 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index e7ebd45ca345..a503865fabed 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -4662,6 +4662,28 @@ static int sock_has_perm(struct sock *sk, u32 perms) > &ad); > } > > +static int nlmsg_sock_has_extended_perms(struct sock *sk, u32 perms, u16 nlmsg_type) > +{ > + struct sk_security_struct *sksec = sk->sk_security; > + struct common_audit_data ad; > + struct lsm_network_audit net = {0,}; > + u8 xperm; > + > + if (sksec->sid == SECINITSID_KERNEL) > + return 0; > + > + ad.type = LSM_AUDIT_DATA_NET; > + ad.u.net = &net; > + ad.u.net->sk = sk; > + > + // nlmsg_types comfortably fit into a single driver, see RTM_MAX in uapi/linux/rtnetlink.h > + xperm = nlmsg_type & 0xff; This seems like a dangerous assumption; obviously not all netlink users are rtnetlink. Even if all existing netlink users follow this, nothing prevents userspace from creating a netlink message that violates it AFAIK, at which point you will just silently discard the higher bits. If we think we can get away with this restriction, then we need to enforce it here (i.e. return an error if they do not fit); if not, then we likely need to support multiple drivers with a simple mapping of the upper bits to driver. > + > + return avc_has_extended_perms(&selinux_state, > + current_sid(), sksec->sid, sksec->sclass, perms, 0, xperm, > + &ad); > +} > + > static int selinux_socket_create(int family, int type, > int protocol, int kern) > { > @@ -6037,7 +6059,7 @@ static int selinux_netlink_send(struct sock *sk, struct sk_buff *skb) > > rc = selinux_nlmsg_lookup(sclass, nlh->nlmsg_type, &perm); > if (rc == 0) { > - rc = sock_has_perm(sk, perm); > + rc = nlmsg_sock_has_extended_perms(sk, perm, nlh->nlmsg_type); > if (rc) > return rc; > } else if (rc == -EINVAL) { > diff --git a/security/selinux/ss/avtab.h b/security/selinux/ss/avtab.h > index d3ebea8d146f..f69aa7bc3dc3 100644 > --- a/security/selinux/ss/avtab.h > +++ b/security/selinux/ss/avtab.h > @@ -55,6 +55,7 @@ struct avtab_extended_perms { > /* These are not flags. All 256 values may be used */ > #define AVTAB_XPERMS_IOCTLFUNCTION 0x01 > #define AVTAB_XPERMS_IOCTLDRIVER 0x02 > +#define AVTAB_XPERMS_NLMSG 0x03 > /* extension of the avtab_key specified */ > u8 specified; /* ioctl, netfilter, ... */ > /* > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > index e5f1b2757a83..7bbb070e9ff5 100644 > --- a/security/selinux/ss/services.c > +++ b/security/selinux/ss/services.c > @@ -591,7 +591,7 @@ static void type_attribute_bounds_av(struct policydb *policydb, > > /* > * flag which drivers have permissions > - * only looking for ioctl based extended permssions > + * only looking for ioctl/netlink based extended permissions > */ > void services_compute_xperms_drivers( > struct extended_perms *xperms, > @@ -607,6 +607,9 @@ void services_compute_xperms_drivers( > /* if allowing permissions within a driver */ > security_xperm_set(xperms->drivers.p, > node->datum.u.xperms->driver); > + } else if (node->datum.u.xperms->specified == AVTAB_XPERMS_NLMSG) { > + /* all netlink permissions are included in driver 0 */ > + xperms->drivers.p[0] |= 1; > } > > xperms->len = 1; > @@ -970,6 +973,9 @@ void services_compute_xperms_decision(struct extended_perms_decision *xpermd, > if (!security_xperm_test(node->datum.u.xperms->perms.p, > xpermd->driver)) > return; > + } else if (node->datum.u.xperms->specified == AVTAB_XPERMS_NLMSG) { > + if (xpermd->driver != node->datum.u.xperms->driver) > + return; > } else { > BUG(); > } > @@ -985,6 +991,11 @@ void services_compute_xperms_decision(struct extended_perms_decision *xpermd, > xpermd->allowed->p[i] |= > node->datum.u.xperms->perms.p[i]; > } > + if (node->datum.u.xperms->specified == AVTAB_XPERMS_NLMSG) { > + for (i = 0; i < ARRAY_SIZE(xpermd->allowed->p); i++) > + xpermd->allowed->p[i] |= > + node->datum.u.xperms->perms.p[i]; > + } > } else if (node->key.specified == AVTAB_XPERMS_AUDITALLOW) { > xpermd->used |= XPERMS_AUDITALLOW; > if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLDRIVER) { > @@ -996,6 +1007,11 @@ void services_compute_xperms_decision(struct extended_perms_decision *xpermd, > xpermd->auditallow->p[i] |= > node->datum.u.xperms->perms.p[i]; > } > + if (node->datum.u.xperms->specified == AVTAB_XPERMS_NLMSG) { > + for (i = 0; i < ARRAY_SIZE(xpermd->auditallow->p); i++) > + xpermd->auditallow->p[i] |= > + node->datum.u.xperms->perms.p[i]; > + } > } else if (node->key.specified == AVTAB_XPERMS_DONTAUDIT) { > xpermd->used |= XPERMS_DONTAUDIT; > if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLDRIVER) { > @@ -1007,6 +1023,11 @@ void services_compute_xperms_decision(struct extended_perms_decision *xpermd, > xpermd->dontaudit->p[i] |= > node->datum.u.xperms->perms.p[i]; > } > + if (node->datum.u.xperms->specified == AVTAB_XPERMS_NLMSG) { > + for (i = 0; i < ARRAY_SIZE(xpermd->dontaudit->p); i++) > + xpermd->dontaudit->p[i] |= > + node->datum.u.xperms->perms.p[i]; > + } > } else { > BUG(); > } > -- > 2.34.0.rc0.344.g81b53c2807-goog >
On Tue, Nov 16, 2021 at 10:41 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Wed, Nov 10, 2021 at 6:13 AM Bram Bonne <brambonne@google.com> wrote: > > > > Reuse the existing extended permissions infrastructure to support > > sepolicy for different netlink message types. > > > > When individual netlink message types are omitted only the existing > > permissions are checked. As is the case for ioctl xperms, this feature > > is intended to provide finer granularity for nlmsg_read and nlmsg_write > > permissions, as they may be too imprecise. For example, a single > > NETLINK_ROUTE socket may provide access to both an interface's IP > > address and to its ARP table, which might have different privacy > > implications. In addition, the set of message types has grown over time, > > so even if the current list is acceptable, future additions might not be. > > It was suggested before on the mailing list [1] that extended permissions > > would be a good fit for this purpose. > > > > Existing policy using the nlmsg_read and nlmsg_write permissions will > > continue to work as-is. Similar to ioctl xperms, netlink xperms allow > > for a more fine-grained policy where needed. > > > > Example policy on Android, preventing regular apps from accessing the > > device's MAC address and ARP table, but allowing this access to > > privileged apps, looks as follows: > > > > allow netdomain self:netlink_route_socket { > > create read getattr write setattr lock append connect getopt > > setopt shutdown nlmsg_read > > }; > > allowxperm netdomain self:netlink_route_socket nlmsg ~{ > > RTM_GETLINK RTM_GETNEIGH RTM_GETNEIGHTBL > > }; > > allowxperm priv_app self:netlink_route_socket nlmsg { > > RTM_GETLINK RTM_GETNEIGH RTM_GETNEIGHTBL > > }; > > > > Android currently uses code similar to [1] as a temporary workaround to > > limit access to certain netlink message types; our hope is that this patch > > will allow us to move back to upstream code with an approach that works for > > everyone. > > > > [1] https://lore.kernel.org/selinux/CAHC9VhRSUhozBycHMZcMaJsibJDxNMsTsKVT2zOnW=5H4R4mdg@mail.gmail.com/ > > > > Signed-off-by: Bram Bonne <brambonne@google.com> > > --- > > security/selinux/hooks.c | 24 +++++++++++++++++++++++- > > security/selinux/ss/avtab.h | 1 + > > security/selinux/ss/services.c | 23 ++++++++++++++++++++++- > > 3 files changed, 46 insertions(+), 2 deletions(-) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index e7ebd45ca345..a503865fabed 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -4662,6 +4662,28 @@ static int sock_has_perm(struct sock *sk, u32 perms) > > &ad); > > } > > > > +static int nlmsg_sock_has_extended_perms(struct sock *sk, u32 perms, u16 nlmsg_type) > > +{ > > + struct sk_security_struct *sksec = sk->sk_security; > > + struct common_audit_data ad; > > + struct lsm_network_audit net = {0,}; > > + u8 xperm; > > + > > + if (sksec->sid == SECINITSID_KERNEL) > > + return 0; > > + > > + ad.type = LSM_AUDIT_DATA_NET; > > + ad.u.net = &net; > > + ad.u.net->sk = sk; > > + > > + // nlmsg_types comfortably fit into a single driver, see RTM_MAX in uapi/linux/rtnetlink.h > > + xperm = nlmsg_type & 0xff; > > This seems like a dangerous assumption; obviously not all netlink > users are rtnetlink. Even if all existing netlink users follow this, > nothing prevents userspace from creating a netlink message that > violates it AFAIK, at which point you will just silently discard the > higher bits. If we think we can get away with this restriction, then > we need to enforce it here (i.e. return an error if they do not fit); > if not, > then we likely need to support multiple drivers with a simple mapping > of the upper bits to driver. Also, it would be very helpful if you were to add a test to the selinux-testsuite [1] for this new checking. There are some tests of the ioctl xperms checking there [2]. [1] https://github.com/selinuxproject/selinux-testsuite [2] https://github.com/SELinuxProject/selinux-testsuite/commit/b6e5e01a282582322185d67eb628569ac1a9f2dc
On Tue, Nov 16, 2021 at 10:41 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Wed, Nov 10, 2021 at 6:13 AM Bram Bonne <brambonne@google.com> wrote: > > > > Reuse the existing extended permissions infrastructure to support > > sepolicy for different netlink message types. > > > > When individual netlink message types are omitted only the existing > > permissions are checked. As is the case for ioctl xperms, this feature > > is intended to provide finer granularity for nlmsg_read and nlmsg_write > > permissions, as they may be too imprecise. For example, a single > > NETLINK_ROUTE socket may provide access to both an interface's IP > > address and to its ARP table, which might have different privacy > > implications. In addition, the set of message types has grown over time, > > so even if the current list is acceptable, future additions might not be. > > It was suggested before on the mailing list [1] that extended permissions > > would be a good fit for this purpose. > > > > Existing policy using the nlmsg_read and nlmsg_write permissions will > > continue to work as-is. Similar to ioctl xperms, netlink xperms allow > > for a more fine-grained policy where needed. > > > > Example policy on Android, preventing regular apps from accessing the > > device's MAC address and ARP table, but allowing this access to > > privileged apps, looks as follows: > > > > allow netdomain self:netlink_route_socket { > > create read getattr write setattr lock append connect getopt > > setopt shutdown nlmsg_read > > }; > > allowxperm netdomain self:netlink_route_socket nlmsg ~{ > > RTM_GETLINK RTM_GETNEIGH RTM_GETNEIGHTBL > > }; > > allowxperm priv_app self:netlink_route_socket nlmsg { > > RTM_GETLINK RTM_GETNEIGH RTM_GETNEIGHTBL > > }; > > > > Android currently uses code similar to [1] as a temporary workaround to > > limit access to certain netlink message types; our hope is that this patch > > will allow us to move back to upstream code with an approach that works for > > everyone. > > > > [1] https://lore.kernel.org/selinux/CAHC9VhRSUhozBycHMZcMaJsibJDxNMsTsKVT2zOnW=5H4R4mdg@mail.gmail.com/ > > > > Signed-off-by: Bram Bonne <brambonne@google.com> > > --- > > security/selinux/hooks.c | 24 +++++++++++++++++++++++- > > security/selinux/ss/avtab.h | 1 + > > security/selinux/ss/services.c | 23 ++++++++++++++++++++++- > > 3 files changed, 46 insertions(+), 2 deletions(-) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index e7ebd45ca345..a503865fabed 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -4662,6 +4662,28 @@ static int sock_has_perm(struct sock *sk, u32 perms) > > &ad); > > } > > > > +static int nlmsg_sock_has_extended_perms(struct sock *sk, u32 perms, u16 nlmsg_type) > > +{ > > + struct sk_security_struct *sksec = sk->sk_security; > > + struct common_audit_data ad; > > + struct lsm_network_audit net = {0,}; > > + u8 xperm; > > + > > + if (sksec->sid == SECINITSID_KERNEL) > > + return 0; > > + > > + ad.type = LSM_AUDIT_DATA_NET; > > + ad.u.net = &net; > > + ad.u.net->sk = sk; > > + > > + // nlmsg_types comfortably fit into a single driver, see RTM_MAX in uapi/linux/rtnetlink.h > > + xperm = nlmsg_type & 0xff; > > This seems like a dangerous assumption; obviously not all netlink > users are rtnetlink. Even if all existing netlink users follow this, > nothing prevents userspace from creating a netlink message that > violates it AFAIK, at which point you will just silently discard the > higher bits. If we think we can get away with this restriction, then > we need to enforce it here (i.e. return an error if they do not fit); > if not, > then we likely need to support multiple drivers with a simple mapping > of the upper bits to driver. Looks like generic netlink puts the family id into the message type field, with the actual command in the separate generic netlink header in the payload. generic netlink family ids appear to be dynamically allocated, with GENL_MAX_ID defined as 1023. genl-ctrl-list on a sample Linux system reports ids from 0x10 through 0x1f so those would fit but there isn't anything in the code to prevent higher ids from being allocated up to the max. And if someday you want to be able to filter generic netlink messages at the per-command level, you'd further need to deal with the separate cmd field.
On Thu, Nov 18, 2021 at 9:46 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > On Tue, Nov 16, 2021 at 10:41 AM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: > > > > On Wed, Nov 10, 2021 at 6:13 AM Bram Bonne <brambonne@google.com> wrote: > > > > > > Reuse the existing extended permissions infrastructure to support > > > sepolicy for different netlink message types. > > > > > > When individual netlink message types are omitted only the existing > > > permissions are checked. As is the case for ioctl xperms, this feature > > > is intended to provide finer granularity for nlmsg_read and nlmsg_write > > > permissions, as they may be too imprecise. For example, a single > > > NETLINK_ROUTE socket may provide access to both an interface's IP > > > address and to its ARP table, which might have different privacy > > > implications. In addition, the set of message types has grown over time, > > > so even if the current list is acceptable, future additions might not be. > > > It was suggested before on the mailing list [1] that extended permissions > > > would be a good fit for this purpose. > > > > > > Existing policy using the nlmsg_read and nlmsg_write permissions will > > > continue to work as-is. Similar to ioctl xperms, netlink xperms allow > > > for a more fine-grained policy where needed. > > > > > > Example policy on Android, preventing regular apps from accessing the > > > device's MAC address and ARP table, but allowing this access to > > > privileged apps, looks as follows: > > > > > > allow netdomain self:netlink_route_socket { > > > create read getattr write setattr lock append connect getopt > > > setopt shutdown nlmsg_read > > > }; > > > allowxperm netdomain self:netlink_route_socket nlmsg ~{ > > > RTM_GETLINK RTM_GETNEIGH RTM_GETNEIGHTBL > > > }; > > > allowxperm priv_app self:netlink_route_socket nlmsg { > > > RTM_GETLINK RTM_GETNEIGH RTM_GETNEIGHTBL > > > }; > > > > > > Android currently uses code similar to [1] as a temporary workaround to > > > limit access to certain netlink message types; our hope is that this patch > > > will allow us to move back to upstream code with an approach that works for > > > everyone. > > > > > > [1] https://lore.kernel.org/selinux/CAHC9VhRSUhozBycHMZcMaJsibJDxNMsTsKVT2zOnW=5H4R4mdg@mail.gmail.com/ > > > > > > Signed-off-by: Bram Bonne <brambonne@google.com> > > > --- > > > security/selinux/hooks.c | 24 +++++++++++++++++++++++- > > > security/selinux/ss/avtab.h | 1 + > > > security/selinux/ss/services.c | 23 ++++++++++++++++++++++- > > > 3 files changed, 46 insertions(+), 2 deletions(-) > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > index e7ebd45ca345..a503865fabed 100644 > > > --- a/security/selinux/hooks.c > > > +++ b/security/selinux/hooks.c > > > @@ -4662,6 +4662,28 @@ static int sock_has_perm(struct sock *sk, u32 perms) > > > &ad); > > > } > > > > > > +static int nlmsg_sock_has_extended_perms(struct sock *sk, u32 perms, u16 nlmsg_type) > > > +{ > > > + struct sk_security_struct *sksec = sk->sk_security; > > > + struct common_audit_data ad; > > > + struct lsm_network_audit net = {0,}; > > > + u8 xperm; > > > + > > > + if (sksec->sid == SECINITSID_KERNEL) > > > + return 0; > > > + > > > + ad.type = LSM_AUDIT_DATA_NET; > > > + ad.u.net = &net; > > > + ad.u.net->sk = sk; > > > + > > > + // nlmsg_types comfortably fit into a single driver, see RTM_MAX in uapi/linux/rtnetlink.h > > > + xperm = nlmsg_type & 0xff; > > > > This seems like a dangerous assumption; obviously not all netlink > > users are rtnetlink. Even if all existing netlink users follow this, > > nothing prevents userspace from creating a netlink message that > > violates it AFAIK, at which point you will just silently discard the > > higher bits. If we think we can get away with this restriction, then > > we need to enforce it here (i.e. return an error if they do not fit); > > if not, > > then we likely need to support multiple drivers with a simple mapping > > of the upper bits to driver. > > Looks like generic netlink puts the family id into the message type > field, with the actual command in the separate generic netlink header > in the payload. generic netlink family ids appear to be dynamically > allocated, with GENL_MAX_ID defined as 1023. genl-ctrl-list on a > sample Linux system reports ids from 0x10 through 0x1f so those would > fit but there isn't anything in the code to prevent higher ids from > being allocated up to the max. And if someday you want to be able to > filter generic netlink messages at the per-command level, you'd > further need to deal with the separate cmd field. There is also NETLINK_AUDIT which currently has message types defined up to 2000. The netlink message header format allows for 16 bit message types (look at the nlmsghdr struct) and I think it would be a mistake if the SELinux netlink/xperms code didn't support a full 16 bits for the message type. As far as NETLINK_GENERIC is concerned, yes, that's a bit of a nuisance both with the dynamic family IDs and buried message types. On the plus side, there are existing kernel functions that will resolve the generic netlink family IDs to a genl_family struct but those are currently private to the generic netlink code; I imagine if there was a need those functions (or something similar) could be made available outside of the genetlink.c. Once you've matched on NETLINK_GENERIC and done the family resolution, it should just be a matter of doing the generic netlink command lookup in the context of the family, which really shouldn't be much different than looking up the message type of a conventional netlink message.
> > > > Reuse the existing extended permissions infrastructure to support > > > > sepolicy for different netlink message types. (Following up on this patch). > > > > > > > > When individual netlink message types are omitted only the existing > > > > permissions are checked. As is the case for ioctl xperms, this feature > > > > is intended to provide finer granularity for nlmsg_read and nlmsg_write > > > > permissions, as they may be too imprecise. For example, a single > > > > NETLINK_ROUTE socket may provide access to both an interface's IP > > > > address and to its ARP table, which might have different privacy > > > > implications. In addition, the set of message types has grown over time, > > > > so even if the current list is acceptable, future additions might not be. > > > > It was suggested before on the mailing list [1] that extended permissions > > > > would be a good fit for this purpose. > > > > > > > > Existing policy using the nlmsg_read and nlmsg_write permissions will > > > > continue to work as-is. Similar to ioctl xperms, netlink xperms allow > > > > for a more fine-grained policy where needed. > > > > > > > > Example policy on Android, preventing regular apps from accessing the > > > > device's MAC address and ARP table, but allowing this access to > > > > privileged apps, looks as follows: > > > > > > > > allow netdomain self:netlink_route_socket { > > > > create read getattr write setattr lock append connect getopt > > > > setopt shutdown nlmsg_read > > > > }; > > > > allowxperm netdomain self:netlink_route_socket nlmsg ~{ > > > > RTM_GETLINK RTM_GETNEIGH RTM_GETNEIGHTBL > > > > }; > > > > allowxperm priv_app self:netlink_route_socket nlmsg { > > > > RTM_GETLINK RTM_GETNEIGH RTM_GETNEIGHTBL > > > > }; > > > > > > > > Android currently uses code similar to [1] as a temporary workaround to > > > > limit access to certain netlink message types; our hope is that this patch > > > > will allow us to move back to upstream code with an approach that works for > > > > everyone. > > > > > > > > [1] https://lore.kernel.org/selinux/CAHC9VhRSUhozBycHMZcMaJsibJDxNMsTsKVT2zOnW=5H4R4mdg@mail.gmail.com/ > > > > > > > > Signed-off-by: Bram Bonne <brambonne@google.com> > > > > --- > > > > security/selinux/hooks.c | 24 +++++++++++++++++++++++- > > > > security/selinux/ss/avtab.h | 1 + > > > > security/selinux/ss/services.c | 23 ++++++++++++++++++++++- > > > > 3 files changed, 46 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > > index e7ebd45ca345..a503865fabed 100644 > > > > --- a/security/selinux/hooks.c > > > > +++ b/security/selinux/hooks.c > > > > @@ -4662,6 +4662,28 @@ static int sock_has_perm(struct sock *sk, u32 perms) > > > > &ad); > > > > } > > > > > > > > +static int nlmsg_sock_has_extended_perms(struct sock *sk, u32 perms, u16 nlmsg_type) > > > > +{ > > > > + struct sk_security_struct *sksec = sk->sk_security; > > > > + struct common_audit_data ad; > > > > + struct lsm_network_audit net = {0,}; > > > > + u8 xperm; > > > > + > > > > + if (sksec->sid == SECINITSID_KERNEL) > > > > + return 0; > > > > + > > > > + ad.type = LSM_AUDIT_DATA_NET; > > > > + ad.u.net = &net; > > > > + ad.u.net->sk = sk; > > > > + > > > > + // nlmsg_types comfortably fit into a single driver, see RTM_MAX in uapi/linux/rtnetlink.h > > > > + xperm = nlmsg_type & 0xff; > > > > > > This seems like a dangerous assumption; obviously not all netlink > > > users are rtnetlink. Even if all existing netlink users follow this, > > > nothing prevents userspace from creating a netlink message that > > > violates it AFAIK, at which point you will just silently discard the > > > higher bits. If we think we can get away with this restriction, then > > > we need to enforce it here (i.e. return an error if they do not fit); > > > if not, > > > then we likely need to support multiple drivers with a simple mapping > > > of the upper bits to driver. Agreed. Mapping the upper bits to the driver field would work. In this case, AVTAB_XPERMS_NLMSG proposed here would have the exact same implementation as AVTAB_XPERMS_IOCTLFUNCTION. An option could be to rename the AVTAB_XPERMS_IOCTLFUNCTION constant (for example, to AVTAB_XPERMS_PERMS, and similarly moving AVTAB_XPERMS_IOCTLDRIVER to AVTAB_XPERMS_DRIVER). This is describing more how struct avtab_extended_perms is being used rather than focusing on its original meaning. (That is, the driver and perms fields are historically related to ioctl, but they have their own meanings in the xperm implementation). The alternative is simply to have that new constant (AVTAB_XPERMS_NLMSG, maybe renamed to AVTAB_XPERMS_NETLINK_MSGTYPE, to be more explicit?). > > > > Looks like generic netlink puts the family id into the message type > > field, with the actual command in the separate generic netlink header > > in the payload. generic netlink family ids appear to be dynamically > > allocated, with GENL_MAX_ID defined as 1023. genl-ctrl-list on a > > sample Linux system reports ids from 0x10 through 0x1f so those would > > fit but there isn't anything in the code to prevent higher ids from > > being allocated up to the max. And if someday you want to be able to > > filter generic netlink messages at the per-command level, you'd > > further need to deal with the separate cmd field. > > There is also NETLINK_AUDIT which currently has message types defined > up to 2000. The netlink message header format allows for 16 bit > message types (look at the nlmsghdr struct) and I think it would be a > mistake if the SELinux netlink/xperms code didn't support a full 16 > bits for the message type. > > As far as NETLINK_GENERIC is concerned, yes, that's a bit of a > nuisance both with the dynamic family IDs and buried message types. > On the plus side, there are existing kernel functions that will > resolve the generic netlink family IDs to a genl_family struct but > those are currently private to the generic netlink code; I imagine if > there was a need those functions (or something similar) could be made > available outside of the genetlink.c. Once you've matched on > NETLINK_GENERIC and done the family resolution, it should just be a > matter of doing the generic netlink command lookup in the context of > the family, which really shouldn't be much different than looking up > the message type of a conventional netlink message. I can see how the resolution for cmd can be done, but I am not sure how the family should be handled. I don't think it is ok to not correlate the family to some data in the policy. Otherwise, it would be possible to reach a different generic netlink family (with the same operation number of an operation granted for a generic family in the policy). This means we should at least match on the family name, but I doubt we can use the current struct avtab_extended_perms for that. For now, an option is to fallback to regular permission checks (avc_has_perm) if the class is NETLINK_GENERIC. (This would mean that we don't make a call now on how NETLINK_GENERIC should be handled, and keep the options open for a decision in the future). In this case, an error would be raised if trying to load a policy with extended permissions for that class (as nlmsg_type has no static meaning).
On Mon, Jul 22, 2024 at 2:21 AM Thiébaud Weksteen <tweek@google.com> wrote: > > > > > > Reuse the existing extended permissions infrastructure to support > > > > > sepolicy for different netlink message types. > > (Following up on this patch). > > > > > > > > > > > When individual netlink message types are omitted only the existing > > > > > permissions are checked. As is the case for ioctl xperms, this feature > > > > > is intended to provide finer granularity for nlmsg_read and nlmsg_write > > > > > permissions, as they may be too imprecise. For example, a single > > > > > NETLINK_ROUTE socket may provide access to both an interface's IP > > > > > address and to its ARP table, which might have different privacy > > > > > implications. In addition, the set of message types has grown over time, > > > > > so even if the current list is acceptable, future additions might not be. > > > > > It was suggested before on the mailing list [1] that extended permissions > > > > > would be a good fit for this purpose. > > > > > > > > > > Existing policy using the nlmsg_read and nlmsg_write permissions will > > > > > continue to work as-is. Similar to ioctl xperms, netlink xperms allow > > > > > for a more fine-grained policy where needed. > > > > > > > > > > Example policy on Android, preventing regular apps from accessing the > > > > > device's MAC address and ARP table, but allowing this access to > > > > > privileged apps, looks as follows: > > > > > > > > > > allow netdomain self:netlink_route_socket { > > > > > create read getattr write setattr lock append connect getopt > > > > > setopt shutdown nlmsg_read > > > > > }; > > > > > allowxperm netdomain self:netlink_route_socket nlmsg ~{ > > > > > RTM_GETLINK RTM_GETNEIGH RTM_GETNEIGHTBL > > > > > }; > > > > > allowxperm priv_app self:netlink_route_socket nlmsg { > > > > > RTM_GETLINK RTM_GETNEIGH RTM_GETNEIGHTBL > > > > > }; > > > > > > > > > > Android currently uses code similar to [1] as a temporary workaround to > > > > > limit access to certain netlink message types; our hope is that this patch > > > > > will allow us to move back to upstream code with an approach that works for > > > > > everyone. > > > > > > > > > > [1] https://lore.kernel.org/selinux/CAHC9VhRSUhozBycHMZcMaJsibJDxNMsTsKVT2zOnW=5H4R4mdg@mail.gmail.com/ > > > > > > > > > > Signed-off-by: Bram Bonne <brambonne@google.com> > > > > > --- > > > > > security/selinux/hooks.c | 24 +++++++++++++++++++++++- > > > > > security/selinux/ss/avtab.h | 1 + > > > > > security/selinux/ss/services.c | 23 ++++++++++++++++++++++- > > > > > 3 files changed, 46 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > > > index e7ebd45ca345..a503865fabed 100644 > > > > > --- a/security/selinux/hooks.c > > > > > +++ b/security/selinux/hooks.c > > > > > @@ -4662,6 +4662,28 @@ static int sock_has_perm(struct sock *sk, u32 perms) > > > > > &ad); > > > > > } > > > > > > > > > > +static int nlmsg_sock_has_extended_perms(struct sock *sk, u32 perms, u16 nlmsg_type) > > > > > +{ > > > > > + struct sk_security_struct *sksec = sk->sk_security; > > > > > + struct common_audit_data ad; > > > > > + struct lsm_network_audit net = {0,}; > > > > > + u8 xperm; > > > > > + > > > > > + if (sksec->sid == SECINITSID_KERNEL) > > > > > + return 0; > > > > > + > > > > > + ad.type = LSM_AUDIT_DATA_NET; > > > > > + ad.u.net = &net; > > > > > + ad.u.net->sk = sk; > > > > > + > > > > > + // nlmsg_types comfortably fit into a single driver, see RTM_MAX in uapi/linux/rtnetlink.h > > > > > + xperm = nlmsg_type & 0xff; > > > > > > > > This seems like a dangerous assumption; obviously not all netlink > > > > users are rtnetlink. Even if all existing netlink users follow this, > > > > nothing prevents userspace from creating a netlink message that > > > > violates it AFAIK, at which point you will just silently discard the > > > > higher bits. If we think we can get away with this restriction, then > > > > we need to enforce it here (i.e. return an error if they do not fit); > > > > if not, > > > > then we likely need to support multiple drivers with a simple mapping > > > > of the upper bits to driver. > > Agreed. Mapping the upper bits to the driver field would work. > > In this case, AVTAB_XPERMS_NLMSG proposed here would have the exact same > implementation as AVTAB_XPERMS_IOCTLFUNCTION. An option could be to rename the > AVTAB_XPERMS_IOCTLFUNCTION constant (for example, to AVTAB_XPERMS_PERMS, and > similarly moving AVTAB_XPERMS_IOCTLDRIVER to AVTAB_XPERMS_DRIVER). This is > describing more how struct avtab_extended_perms is being used rather than > focusing on its original meaning. (That is, the driver and perms fields are > historically related to ioctl, but they have their own meanings in the xperm > implementation). > > The alternative is simply to have that new constant (AVTAB_XPERMS_NLMSG, maybe > renamed to AVTAB_XPERMS_NETLINK_MSGTYPE, to be more explicit?). Considering that the original patch is almost three years old at this point, if you're interested in picking up this work I'd suggest starting by picking one of the options above that you like the most and preparing a patch(set) that works on the current development trees. At this point all I remember about this patchset is that it existed. If I'm going to spend the time to go back and re-review something, I'd rather it be a bit more recent than three years ;) > > > Looks like generic netlink puts the family id into the message type > > > field, with the actual command in the separate generic netlink header > > > in the payload. generic netlink family ids appear to be dynamically > > > allocated, with GENL_MAX_ID defined as 1023. genl-ctrl-list on a > > > sample Linux system reports ids from 0x10 through 0x1f so those would > > > fit but there isn't anything in the code to prevent higher ids from > > > being allocated up to the max. And if someday you want to be able to > > > filter generic netlink messages at the per-command level, you'd > > > further need to deal with the separate cmd field. > > > > There is also NETLINK_AUDIT which currently has message types defined > > up to 2000. The netlink message header format allows for 16 bit > > message types (look at the nlmsghdr struct) and I think it would be a > > mistake if the SELinux netlink/xperms code didn't support a full 16 > > bits for the message type. > > > > As far as NETLINK_GENERIC is concerned, yes, that's a bit of a > > nuisance both with the dynamic family IDs and buried message types. > > On the plus side, there are existing kernel functions that will > > resolve the generic netlink family IDs to a genl_family struct but > > those are currently private to the generic netlink code; I imagine if > > there was a need those functions (or something similar) could be made > > available outside of the genetlink.c. Once you've matched on > > NETLINK_GENERIC and done the family resolution, it should just be a > > matter of doing the generic netlink command lookup in the context of > > the family, which really shouldn't be much different than looking up > > the message type of a conventional netlink message. > > I can see how the resolution for cmd can be done, but I am not sure how the > family should be handled. I don't think it is ok to not correlate the family to > some data in the policy. Yes, the family needs to be represented in the policy and using precedence as a guide I would suggest encoding the netlink family types as object classes. You'll need a generic netlink "default" for those generic netlink families that aren't explicitly defined, but that shouldn't be too hard to implement.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index e7ebd45ca345..a503865fabed 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -4662,6 +4662,28 @@ static int sock_has_perm(struct sock *sk, u32 perms) &ad); } +static int nlmsg_sock_has_extended_perms(struct sock *sk, u32 perms, u16 nlmsg_type) +{ + struct sk_security_struct *sksec = sk->sk_security; + struct common_audit_data ad; + struct lsm_network_audit net = {0,}; + u8 xperm; + + if (sksec->sid == SECINITSID_KERNEL) + return 0; + + ad.type = LSM_AUDIT_DATA_NET; + ad.u.net = &net; + ad.u.net->sk = sk; + + // nlmsg_types comfortably fit into a single driver, see RTM_MAX in uapi/linux/rtnetlink.h + xperm = nlmsg_type & 0xff; + + return avc_has_extended_perms(&selinux_state, + current_sid(), sksec->sid, sksec->sclass, perms, 0, xperm, + &ad); +} + static int selinux_socket_create(int family, int type, int protocol, int kern) { @@ -6037,7 +6059,7 @@ static int selinux_netlink_send(struct sock *sk, struct sk_buff *skb) rc = selinux_nlmsg_lookup(sclass, nlh->nlmsg_type, &perm); if (rc == 0) { - rc = sock_has_perm(sk, perm); + rc = nlmsg_sock_has_extended_perms(sk, perm, nlh->nlmsg_type); if (rc) return rc; } else if (rc == -EINVAL) { diff --git a/security/selinux/ss/avtab.h b/security/selinux/ss/avtab.h index d3ebea8d146f..f69aa7bc3dc3 100644 --- a/security/selinux/ss/avtab.h +++ b/security/selinux/ss/avtab.h @@ -55,6 +55,7 @@ struct avtab_extended_perms { /* These are not flags. All 256 values may be used */ #define AVTAB_XPERMS_IOCTLFUNCTION 0x01 #define AVTAB_XPERMS_IOCTLDRIVER 0x02 +#define AVTAB_XPERMS_NLMSG 0x03 /* extension of the avtab_key specified */ u8 specified; /* ioctl, netfilter, ... */ /* diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index e5f1b2757a83..7bbb070e9ff5 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -591,7 +591,7 @@ static void type_attribute_bounds_av(struct policydb *policydb, /* * flag which drivers have permissions - * only looking for ioctl based extended permssions + * only looking for ioctl/netlink based extended permissions */ void services_compute_xperms_drivers( struct extended_perms *xperms, @@ -607,6 +607,9 @@ void services_compute_xperms_drivers( /* if allowing permissions within a driver */ security_xperm_set(xperms->drivers.p, node->datum.u.xperms->driver); + } else if (node->datum.u.xperms->specified == AVTAB_XPERMS_NLMSG) { + /* all netlink permissions are included in driver 0 */ + xperms->drivers.p[0] |= 1; } xperms->len = 1; @@ -970,6 +973,9 @@ void services_compute_xperms_decision(struct extended_perms_decision *xpermd, if (!security_xperm_test(node->datum.u.xperms->perms.p, xpermd->driver)) return; + } else if (node->datum.u.xperms->specified == AVTAB_XPERMS_NLMSG) { + if (xpermd->driver != node->datum.u.xperms->driver) + return; } else { BUG(); } @@ -985,6 +991,11 @@ void services_compute_xperms_decision(struct extended_perms_decision *xpermd, xpermd->allowed->p[i] |= node->datum.u.xperms->perms.p[i]; } + if (node->datum.u.xperms->specified == AVTAB_XPERMS_NLMSG) { + for (i = 0; i < ARRAY_SIZE(xpermd->allowed->p); i++) + xpermd->allowed->p[i] |= + node->datum.u.xperms->perms.p[i]; + } } else if (node->key.specified == AVTAB_XPERMS_AUDITALLOW) { xpermd->used |= XPERMS_AUDITALLOW; if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLDRIVER) { @@ -996,6 +1007,11 @@ void services_compute_xperms_decision(struct extended_perms_decision *xpermd, xpermd->auditallow->p[i] |= node->datum.u.xperms->perms.p[i]; } + if (node->datum.u.xperms->specified == AVTAB_XPERMS_NLMSG) { + for (i = 0; i < ARRAY_SIZE(xpermd->auditallow->p); i++) + xpermd->auditallow->p[i] |= + node->datum.u.xperms->perms.p[i]; + } } else if (node->key.specified == AVTAB_XPERMS_DONTAUDIT) { xpermd->used |= XPERMS_DONTAUDIT; if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLDRIVER) { @@ -1007,6 +1023,11 @@ void services_compute_xperms_decision(struct extended_perms_decision *xpermd, xpermd->dontaudit->p[i] |= node->datum.u.xperms->perms.p[i]; } + if (node->datum.u.xperms->specified == AVTAB_XPERMS_NLMSG) { + for (i = 0; i < ARRAY_SIZE(xpermd->dontaudit->p); i++) + xpermd->dontaudit->p[i] |= + node->datum.u.xperms->perms.p[i]; + } } else { BUG(); }
Reuse the existing extended permissions infrastructure to support sepolicy for different netlink message types. When individual netlink message types are omitted only the existing permissions are checked. As is the case for ioctl xperms, this feature is intended to provide finer granularity for nlmsg_read and nlmsg_write permissions, as they may be too imprecise. For example, a single NETLINK_ROUTE socket may provide access to both an interface's IP address and to its ARP table, which might have different privacy implications. In addition, the set of message types has grown over time, so even if the current list is acceptable, future additions might not be. It was suggested before on the mailing list [1] that extended permissions would be a good fit for this purpose. Existing policy using the nlmsg_read and nlmsg_write permissions will continue to work as-is. Similar to ioctl xperms, netlink xperms allow for a more fine-grained policy where needed. Example policy on Android, preventing regular apps from accessing the device's MAC address and ARP table, but allowing this access to privileged apps, looks as follows: allow netdomain self:netlink_route_socket { create read getattr write setattr lock append connect getopt setopt shutdown nlmsg_read }; allowxperm netdomain self:netlink_route_socket nlmsg ~{ RTM_GETLINK RTM_GETNEIGH RTM_GETNEIGHTBL }; allowxperm priv_app self:netlink_route_socket nlmsg { RTM_GETLINK RTM_GETNEIGH RTM_GETNEIGHTBL }; Android currently uses code similar to [1] as a temporary workaround to limit access to certain netlink message types; our hope is that this patch will allow us to move back to upstream code with an approach that works for everyone. [1] https://lore.kernel.org/selinux/CAHC9VhRSUhozBycHMZcMaJsibJDxNMsTsKVT2zOnW=5H4R4mdg@mail.gmail.com/ Signed-off-by: Bram Bonne <brambonne@google.com> --- security/selinux/hooks.c | 24 +++++++++++++++++++++++- security/selinux/ss/avtab.h | 1 + security/selinux/ss/services.c | 23 ++++++++++++++++++++++- 3 files changed, 46 insertions(+), 2 deletions(-)