| Message ID | 20240820002723.1345639-1-tweek@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: Add netlink xperm support | expand |
On Mon, Aug 19, 2024 at 8:27 PM Thiébaud Weksteen <tweek@google.com> wrote: > > Reuse the existing extended permissions infrastructure to support > policies based on the netlink message types. > > A new policy capability "netlink_xperm" is introduced. When disabled, > the previous behaviour is preserved. That is, netlink_send will rely on > the permission mappings defined in nlmsgtab.c (e.g, nlmsg_read for > RTM_GETADDR on NETLINK_ROUTE). When enabled, the mappings are ignored > and the generic "nlmsg" permission is used instead. > > The new "nlmsg" permission is an extended permission. The 16 bits of the > extended permission are mapped to the nlmsg_type field. > > 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 > }; > 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 > }; > > The constants in the example above (e.g., RTM_GETLINK) are explicitly > defined in the policy. > > It is possible to generate policies to support kernels that may or > may not have the capability enabled by generating a rule for each > scenario. For instance: > > allow domain self:netlink_audit_socket nlmsg_read; > allow domain self:netlink_audit_socket nlmsg; > allowxperm domain self:netlink_audit_socket nlmsg { AUDIT_GET }; > > The approach of defining a new permission ("nlmsg") instead of relying > on the existing permissions (e.g., "nlmsg_read", "nlmsg_readpriv" or > "nlmsg_tty_audit") has been preferred because: > 1. This is similar to the other extended permission ("ioctl"); > 2. With the new extended permission, the coarse-grained mapping is not > necessary anymore. It could eventually be removed, which would be > impossible if the extended permission was defined below these. > 3. Having a single extra extended permission considerably simplifies > the implementation here and in libselinux. > > The class NETLINK_GENERIC is excluded from using this extended > permission because the nlmsg_type field is referencing the family id > which is dynamically associated. > > Signed-off-by: Thiébaud Weksteen <tweek@google.com> > Signed-off-by: Bram Bonné <brambonne@google.com> Thank you for reviving this patch. Do you have a corresponding userspace patch? And for extra credit, a selinux-testsuite patch? A minor comment below. > --- > diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h > index 7229c9bf6c27..c95bf89c9ce5 100644 > --- a/security/selinux/include/classmap.h > +++ b/security/selinux/include/classmap.h > @@ -96,17 +96,17 @@ const struct security_class_mapping secclass_map[] = { > { "shm", { COMMON_IPC_PERMS, "lock", NULL } }, > { "ipc", { COMMON_IPC_PERMS, NULL } }, > { "netlink_route_socket", > - { COMMON_SOCK_PERMS, "nlmsg_read", "nlmsg_write", NULL } }, > + { COMMON_SOCK_PERMS, "nlmsg", "nlmsg_read", "nlmsg_write", NULL } }, I would just add the "nlmsg" permission to the end of the list before the NULL for each class. Doesn't matter as much anymore since the dynamic class/perm mapping support was added but generally we avoid perturbing the class/permission bit assignments unless there is a good reason to do so. Feel free to wait to see if Paul agrees since your code will work as is.
On Tue, Aug 20, 2024 at 2:02 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > On Mon, Aug 19, 2024 at 8:27 PM Thiébaud Weksteen <tweek@google.com> wrote: > > --- > > diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h > > index 7229c9bf6c27..c95bf89c9ce5 100644 > > --- a/security/selinux/include/classmap.h > > +++ b/security/selinux/include/classmap.h > > @@ -96,17 +96,17 @@ const struct security_class_mapping secclass_map[] = { > > { "shm", { COMMON_IPC_PERMS, "lock", NULL } }, > > { "ipc", { COMMON_IPC_PERMS, NULL } }, > > { "netlink_route_socket", > > - { COMMON_SOCK_PERMS, "nlmsg_read", "nlmsg_write", NULL } }, > > + { COMMON_SOCK_PERMS, "nlmsg", "nlmsg_read", "nlmsg_write", NULL } }, > > I would just add the "nlmsg" permission to the end of the list before > the NULL for each class. > Doesn't matter as much anymore since the dynamic class/perm mapping > support was added but generally we avoid perturbing the > class/permission bit assignments unless there is a good reason to do > so. Feel free to wait to see if Paul agrees since your code will work > as is. I haven't had a chance to look at the rest of the patch yet, but I agree with Stephen. Generally speaking we should strive to only add new perms to the end of the list, I'd hate to hit some odd corner case on someone's system simply because we thought we'd tempt fate and something to the front of the list ;)
On Wed, Aug 21, 2024 at 5:54 AM Paul Moore <paul@paul-moore.com> wrote: > > On Tue, Aug 20, 2024 at 2:02 PM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: > > Thank you for reviving this patch. > Do you have a corresponding userspace patch? And for extra credit, a > selinux-testsuite patch? > Thank you for the quick response and initial feedback. I've just sent the libsepol patches for userland on this mailing list. For selinux-testsuite, an issue I came across while testing is that the policy capabilities cannot be updated (that is, only the capabilities from the original host policy are active). I am not sure if I got that right or if there is any obvious solution (except toggling on the new capability in Fedora). I'm still hoping to get the extra credits by: updating the selinux notebook documentation as well as updating setools (for sesearch support). :) I will send pull requests if these patches get accepted. > > On Mon, Aug 19, 2024 at 8:27 PM Thiébaud Weksteen <tweek@google.com> wrote: > > > --- > > > diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h > > > index 7229c9bf6c27..c95bf89c9ce5 100644 > > > --- a/security/selinux/include/classmap.h > > > +++ b/security/selinux/include/classmap.h > > > @@ -96,17 +96,17 @@ const struct security_class_mapping secclass_map[] = { > > > { "shm", { COMMON_IPC_PERMS, "lock", NULL } }, > > > { "ipc", { COMMON_IPC_PERMS, NULL } }, > > > { "netlink_route_socket", > > > - { COMMON_SOCK_PERMS, "nlmsg_read", "nlmsg_write", NULL } }, > > > + { COMMON_SOCK_PERMS, "nlmsg", "nlmsg_read", "nlmsg_write", NULL } }, > > > > I would just add the "nlmsg" permission to the end of the list before > > the NULL for each class. > > Doesn't matter as much anymore since the dynamic class/perm mapping > > support was added but generally we avoid perturbing the > > class/permission bit assignments unless there is a good reason to do > > so. Feel free to wait to see if Paul agrees since your code will work > > as is. > > I haven't had a chance to look at the rest of the patch yet, but I > agree with Stephen. Generally speaking we should strive to only add > new perms to the end of the list, I'd hate to hit some odd corner case > on someone's system simply because we thought we'd tempt fate and > something to the front of the list ;) > Ack, I'll fix that in the next version. I'll wait for a bit in case you or someone else has more feedback. Thanks
On Wed, Aug 21, 2024 at 8:56 PM Thiébaud Weksteen <tweek@google.com> wrote: > > On Wed, Aug 21, 2024 at 5:54 AM Paul Moore <paul@paul-moore.com> wrote: > > > > On Tue, Aug 20, 2024 at 2:02 PM Stephen Smalley > > <stephen.smalley.work@gmail.com> wrote: > > > > Thank you for reviving this patch. > > Do you have a corresponding userspace patch? And for extra credit, a > > selinux-testsuite patch? > > > > Thank you for the quick response and initial feedback. I've just sent > the libsepol patches for userland on this mailing list. > For selinux-testsuite, an issue I came across while testing is that > the policy capabilities cannot be updated (that is, only the > capabilities from the original host policy are active). I am not sure > if I got that right or if there is any obvious solution (except > toggling on the new capability in Fedora). > I'm still hoping to get the extra credits by: updating the selinux > notebook documentation as well as updating setools (for sesearch > support). :) I will send pull requests if these patches get accepted. With your userspace patches, can't you just do this: $ cat netlink_xperm.cil (policycap netlink_xperm) $ sudo semodule -i netlink_xperm.cil If so, then you can add that along with corresponding allowxperm rules to the test policy to exercise this.
On Thu, Aug 22, 2024 at 10:37 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Wed, Aug 21, 2024 at 8:56 PM Thiébaud Weksteen <tweek@google.com> wrote: > > > > On Wed, Aug 21, 2024 at 5:54 AM Paul Moore <paul@paul-moore.com> wrote: > > > > > > On Tue, Aug 20, 2024 at 2:02 PM Stephen Smalley > > > <stephen.smalley.work@gmail.com> wrote: > > > > > > Thank you for reviving this patch. > > > Do you have a corresponding userspace patch? And for extra credit, a > > > selinux-testsuite patch? > > > > > > > Thank you for the quick response and initial feedback. I've just sent > > the libsepol patches for userland on this mailing list. > > For selinux-testsuite, an issue I came across while testing is that > > the policy capabilities cannot be updated (that is, only the > > capabilities from the original host policy are active). I am not sure > > if I got that right or if there is any obvious solution (except > > toggling on the new capability in Fedora). > > I'm still hoping to get the extra credits by: updating the selinux > > notebook documentation as well as updating setools (for sesearch > > support). :) I will send pull requests if these patches get accepted. > > With your userspace patches, can't you just do this: > $ cat netlink_xperm.cil > (policycap netlink_xperm) > $ sudo semodule -i netlink_xperm.cil > > If so, then you can add that along with corresponding allowxperm rules > to the test policy to exercise this. NB you may need to also allow { domain -testsuite_domain } the new nlmsg permission for all the netlink socket classes to avoid breaking the other processes running on the test system
On Thu, Aug 22, 2024 at 11:37 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Thu, Aug 22, 2024 at 10:37 AM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: > > > > On Wed, Aug 21, 2024 at 8:56 PM Thiébaud Weksteen <tweek@google.com> wrote: > > > > > > On Wed, Aug 21, 2024 at 5:54 AM Paul Moore <paul@paul-moore.com> wrote: > > > > > > > > On Tue, Aug 20, 2024 at 2:02 PM Stephen Smalley > > > > <stephen.smalley.work@gmail.com> wrote: > > > > > > > > Thank you for reviving this patch. > > > > Do you have a corresponding userspace patch? And for extra credit, a > > > > selinux-testsuite patch? > > > > > > > > > > Thank you for the quick response and initial feedback. I've just sent > > > the libsepol patches for userland on this mailing list. > > > For selinux-testsuite, an issue I came across while testing is that > > > the policy capabilities cannot be updated (that is, only the > > > capabilities from the original host policy are active). I am not sure > > > if I got that right or if there is any obvious solution (except > > > toggling on the new capability in Fedora). > > > I'm still hoping to get the extra credits by: updating the selinux > > > notebook documentation as well as updating setools (for sesearch > > > support). :) I will send pull requests if these patches get accepted. > > > > With your userspace patches, can't you just do this: > > $ cat netlink_xperm.cil > > (policycap netlink_xperm) > > $ sudo semodule -i netlink_xperm.cil > > > > If so, then you can add that along with corresponding allowxperm rules > > to the test policy to exercise this. > > NB you may need to also allow { domain -testsuite_domain } the new > nlmsg permission for all the netlink socket classes to avoid breaking > the other processes running on the test system. Sorry, never mind - you would still need to define the nlmsg permission for each of the netlink socket classes and that doesn't appear to be something one can do in anything other than the original definition in the base module. Oh well.
On Mon, Aug 19, 2024 at 8:27 PM Thiébaud Weksteen <tweek@google.com> wrote: > > Reuse the existing extended permissions infrastructure to support > policies based on the netlink message types. > > A new policy capability "netlink_xperm" is introduced. When disabled, > the previous behaviour is preserved. That is, netlink_send will rely on > the permission mappings defined in nlmsgtab.c (e.g, nlmsg_read for > RTM_GETADDR on NETLINK_ROUTE). When enabled, the mappings are ignored > and the generic "nlmsg" permission is used instead. > > The new "nlmsg" permission is an extended permission. The 16 bits of the > extended permission are mapped to the nlmsg_type field. > > 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 > }; > 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 > }; > > The constants in the example above (e.g., RTM_GETLINK) are explicitly > defined in the policy. > > It is possible to generate policies to support kernels that may or > may not have the capability enabled by generating a rule for each > scenario. For instance: > > allow domain self:netlink_audit_socket nlmsg_read; > allow domain self:netlink_audit_socket nlmsg; > allowxperm domain self:netlink_audit_socket nlmsg { AUDIT_GET }; > > The approach of defining a new permission ("nlmsg") instead of relying > on the existing permissions (e.g., "nlmsg_read", "nlmsg_readpriv" or > "nlmsg_tty_audit") has been preferred because: > 1. This is similar to the other extended permission ("ioctl"); > 2. With the new extended permission, the coarse-grained mapping is not > necessary anymore. It could eventually be removed, which would be > impossible if the extended permission was defined below these. > 3. Having a single extra extended permission considerably simplifies > the implementation here and in libselinux. > > The class NETLINK_GENERIC is excluded from using this extended > permission because the nlmsg_type field is referencing the family id > which is dynamically associated. > > Signed-off-by: Thiébaud Weksteen <tweek@google.com> > Signed-off-by: Bram Bonné <brambonne@google.com> Expect you will re-spin with nlmsg permission added to the end of the permissions lists, but since the patches work regardless and we have previously added permissions (e.g. map permission) that displaced other permission definitions since we introduced the dynamic class/perm support, I'll go ahead and add my: Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com> Tested-by: Stephen Smalley <stephen.smalley.work@gmail.com> > --- > I previously suggested to rename the constants AVTAB_XPERMS_IOCTLDRIVER > and AVTAB_XPERMS_IOCTLFUNCTION as they have a similar purpose to the new > AVTAB_XPERMS_NLMSG. However, there is no other information in the > avtab_datum structure to link the extended permission to the original > permission. So it is still necessary to capture which permission is > specified in struct avtab_extended_perms. > > security/selinux/hooks.c | 56 +++++++++++++--- > security/selinux/include/classmap.h | 10 +-- > security/selinux/include/policycap.h | 1 + > security/selinux/include/policycap_names.h | 1 + > security/selinux/include/security.h | 6 ++ > security/selinux/nlmsgtab.c | 21 ++++++ > security/selinux/ss/avtab.h | 5 +- > security/selinux/ss/services.c | 78 ++++++++++++---------- > 8 files changed, 126 insertions(+), 52 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 55c78c318ccd..c30fad37c013 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -4582,14 +4582,10 @@ static int socket_sockcreate_sid(const struct task_security_struct *tsec, > secclass, NULL, socksid); > } > > -static int sock_has_perm(struct sock *sk, u32 perms) > +static bool sock_skip_has_perm(u32 sid) > { > - struct sk_security_struct *sksec = sk->sk_security; > - struct common_audit_data ad; > - struct lsm_network_audit net; > - > - if (sksec->sid == SECINITSID_KERNEL) > - return 0; > + if (sid == SECINITSID_KERNEL) > + return true; > > /* > * Before POLICYDB_CAP_USERSPACE_INITIAL_CONTEXT, sockets that > @@ -4603,7 +4599,19 @@ static int sock_has_perm(struct sock *sk, u32 perms) > * setting. > */ > if (!selinux_policycap_userspace_initial_context() && > - sksec->sid == SECINITSID_INIT) > + sid == SECINITSID_INIT) > + return true; > + return false; > +} > + > + > +static int sock_has_perm(struct sock *sk, u32 perms) > +{ > + struct sk_security_struct *sksec = sk->sk_security; > + struct common_audit_data ad; > + struct lsm_network_audit net; > + > + if (sock_skip_has_perm(sksec->sid)) > return 0; > > ad_net_init_from_sk(&ad, &net, sk); > @@ -5929,6 +5937,26 @@ static unsigned int selinux_ip_postroute(void *priv, > } > #endif /* CONFIG_NETFILTER */ > > +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; > + u8 driver; > + u8 xperm; > + > + if (sock_skip_has_perm(sksec->sid)) > + return 0; > + > + ad_net_init_from_sk(&ad, &net, sk); > + > + driver = nlmsg_type >> 8; > + xperm = nlmsg_type & 0xff; > + > + return avc_has_extended_perms(current_sid(), sksec->sid, sksec->sclass, > + perms, driver, xperm, &ad); > +} > + > static int selinux_netlink_send(struct sock *sk, struct sk_buff *skb) > { > int rc = 0; > @@ -5954,7 +5982,17 @@ 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); > + /* For Generic Netlink, nlmsg_type is mapped to the > + * family id which is dynamically assigned. > + * Ignore extended permissions for this type. > + */ > + if (selinux_policycap_netlink_xperm() && > + (sclass != SECCLASS_NETLINK_GENERIC_SOCKET)) { > + rc = nlmsg_sock_has_extended_perms( > + sk, perm, nlh->nlmsg_type); > + } else { > + rc = sock_has_perm(sk, perm); > + } > if (rc) > return rc; > } else if (rc == -EINVAL) { > diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h > index 7229c9bf6c27..c95bf89c9ce5 100644 > --- a/security/selinux/include/classmap.h > +++ b/security/selinux/include/classmap.h > @@ -96,17 +96,17 @@ const struct security_class_mapping secclass_map[] = { > { "shm", { COMMON_IPC_PERMS, "lock", NULL } }, > { "ipc", { COMMON_IPC_PERMS, NULL } }, > { "netlink_route_socket", > - { COMMON_SOCK_PERMS, "nlmsg_read", "nlmsg_write", NULL } }, > + { COMMON_SOCK_PERMS, "nlmsg", "nlmsg_read", "nlmsg_write", NULL } }, > { "netlink_tcpdiag_socket", > - { COMMON_SOCK_PERMS, "nlmsg_read", "nlmsg_write", NULL } }, > + { COMMON_SOCK_PERMS, "nlmsg", "nlmsg_read", "nlmsg_write", NULL } }, > { "netlink_nflog_socket", { COMMON_SOCK_PERMS, NULL } }, > { "netlink_xfrm_socket", > - { COMMON_SOCK_PERMS, "nlmsg_read", "nlmsg_write", NULL } }, > + { COMMON_SOCK_PERMS, "nlmsg", "nlmsg_read", "nlmsg_write", NULL } }, > { "netlink_selinux_socket", { COMMON_SOCK_PERMS, NULL } }, > { "netlink_iscsi_socket", { COMMON_SOCK_PERMS, NULL } }, > { "netlink_audit_socket", > - { COMMON_SOCK_PERMS, "nlmsg_read", "nlmsg_write", "nlmsg_relay", > - "nlmsg_readpriv", "nlmsg_tty_audit", NULL } }, > + { COMMON_SOCK_PERMS, "nlmsg", "nlmsg_read", "nlmsg_write", > + "nlmsg_relay", "nlmsg_readpriv", "nlmsg_tty_audit", NULL } }, > { "netlink_fib_lookup_socket", { COMMON_SOCK_PERMS, NULL } }, > { "netlink_connector_socket", { COMMON_SOCK_PERMS, NULL } }, > { "netlink_netfilter_socket", { COMMON_SOCK_PERMS, NULL } }, > diff --git a/security/selinux/include/policycap.h b/security/selinux/include/policycap.h > index dc3674eb29c1..079679fe7254 100644 > --- a/security/selinux/include/policycap.h > +++ b/security/selinux/include/policycap.h > @@ -14,6 +14,7 @@ enum { > POLICYDB_CAP_GENFS_SECLABEL_SYMLINKS, > POLICYDB_CAP_IOCTL_SKIP_CLOEXEC, > POLICYDB_CAP_USERSPACE_INITIAL_CONTEXT, > + POLICYDB_CAP_NETLINK_XPERM, > __POLICYDB_CAP_MAX > }; > #define POLICYDB_CAP_MAX (__POLICYDB_CAP_MAX - 1) > diff --git a/security/selinux/include/policycap_names.h b/security/selinux/include/policycap_names.h > index 2cffcc1ce851..e080827408c4 100644 > --- a/security/selinux/include/policycap_names.h > +++ b/security/selinux/include/policycap_names.h > @@ -17,6 +17,7 @@ const char *const selinux_policycap_names[__POLICYDB_CAP_MAX] = { > "genfs_seclabel_symlinks", > "ioctl_skip_cloexec", > "userspace_initial_context", > + "netlink_xperm", > }; > /* clang-format on */ > > diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h > index 289bf9233f71..c7f2731abd03 100644 > --- a/security/selinux/include/security.h > +++ b/security/selinux/include/security.h > @@ -195,6 +195,12 @@ static inline bool selinux_policycap_userspace_initial_context(void) > selinux_state.policycap[POLICYDB_CAP_USERSPACE_INITIAL_CONTEXT]); > } > > +static inline bool selinux_policycap_netlink_xperm(void) > +{ > + return READ_ONCE( > + selinux_state.policycap[POLICYDB_CAP_NETLINK_XPERM]); > +} > + > struct selinux_policy_convert_data; > > struct selinux_load_state { > diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c > index 8ff670cf1ee5..0dac942156d6 100644 > --- a/security/selinux/nlmsgtab.c > +++ b/security/selinux/nlmsgtab.c > @@ -170,6 +170,27 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm) > { > int err = 0; > > + if (selinux_policycap_netlink_xperm()) { > + switch (sclass) { > + case SECCLASS_NETLINK_ROUTE_SOCKET: > + *perm = NETLINK_ROUTE_SOCKET__NLMSG; > + break; > + case SECCLASS_NETLINK_TCPDIAG_SOCKET: > + *perm = NETLINK_TCPDIAG_SOCKET__NLMSG; > + break; > + case SECCLASS_NETLINK_XFRM_SOCKET: > + *perm = NETLINK_XFRM_SOCKET__NLMSG; > + break; > + case SECCLASS_NETLINK_AUDIT_SOCKET: > + *perm = NETLINK_AUDIT_SOCKET__NLMSG; > + break; > + default: > + err = -ENOENT; > + break; > + } > + return err; > + } > + > switch (sclass) { > case SECCLASS_NETLINK_ROUTE_SOCKET: > /* RTM_MAX always points to RTM_SETxxxx, ie RTM_NEWxxx + 3. > diff --git a/security/selinux/ss/avtab.h b/security/selinux/ss/avtab.h > index 8e8820484c55..f4407185401c 100644 > --- a/security/selinux/ss/avtab.h > +++ b/security/selinux/ss/avtab.h > @@ -53,8 +53,9 @@ struct avtab_key { > */ > 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_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 e33e55384b75..48d5748f03da 100644 > --- a/security/selinux/ss/services.c > +++ b/security/selinux/ss/services.c > @@ -582,8 +582,7 @@ static void type_attribute_bounds_av(struct policydb *policydb, > } > > /* > - * flag which drivers have permissions > - * only looking for ioctl based extended permissions > + * Flag which drivers have permissions. > */ > void services_compute_xperms_drivers( > struct extended_perms *xperms, > @@ -591,14 +590,18 @@ void services_compute_xperms_drivers( > { > unsigned int i; > > - if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLDRIVER) { > + switch (node->datum.u.xperms->specified) { > + case AVTAB_XPERMS_IOCTLDRIVER: > /* if one or more driver has all permissions allowed */ > for (i = 0; i < ARRAY_SIZE(xperms->drivers.p); i++) > xperms->drivers.p[i] |= node->datum.u.xperms->perms.p[i]; > - } else if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLFUNCTION) { > + break; > + case AVTAB_XPERMS_IOCTLFUNCTION: > + case AVTAB_XPERMS_NLMSG: > /* if allowing permissions within a driver */ > security_xperm_set(xperms->drivers.p, > node->datum.u.xperms->driver); > + break; > } > > xperms->len = 1; > @@ -942,55 +945,58 @@ static void avd_init(struct selinux_policy *policy, struct av_decision *avd) > avd->flags = 0; > } > > -void services_compute_xperms_decision(struct extended_perms_decision *xpermd, > - struct avtab_node *node) > +static void update_xperms_extended_data(u8 specified, > + struct extended_perms_data *from, > + struct extended_perms_data *xp_data) > { > unsigned int i; > > - if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLFUNCTION) { > + switch (specified) { > + case AVTAB_XPERMS_IOCTLDRIVER: > + memset(xp_data->p, 0xff, sizeof(xp_data->p)); > + break; > + case AVTAB_XPERMS_IOCTLFUNCTION: > + case AVTAB_XPERMS_NLMSG: > + for (i = 0; i < ARRAY_SIZE(xp_data->p); i++) > + xp_data->p[i] |= from->p[i]; > + break; > + } > + > +} > + > +void services_compute_xperms_decision(struct extended_perms_decision *xpermd, > + struct avtab_node *node) > +{ > + switch (node->datum.u.xperms->specified) { > + case AVTAB_XPERMS_IOCTLFUNCTION: > + case AVTAB_XPERMS_NLMSG: > if (xpermd->driver != node->datum.u.xperms->driver) > return; > - } else if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLDRIVER) { > + break; > + case AVTAB_XPERMS_IOCTLDRIVER: > if (!security_xperm_test(node->datum.u.xperms->perms.p, > xpermd->driver)) > return; > - } else { > + break; > + default: > BUG(); > } > > if (node->key.specified == AVTAB_XPERMS_ALLOWED) { > xpermd->used |= XPERMS_ALLOWED; > - if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLDRIVER) { > - memset(xpermd->allowed->p, 0xff, > - sizeof(xpermd->allowed->p)); > - } > - if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLFUNCTION) { > - for (i = 0; i < ARRAY_SIZE(xpermd->allowed->p); i++) > - xpermd->allowed->p[i] |= > - node->datum.u.xperms->perms.p[i]; > - } > + update_xperms_extended_data(node->datum.u.xperms->specified, > + &node->datum.u.xperms->perms, > + xpermd->allowed); > } else if (node->key.specified == AVTAB_XPERMS_AUDITALLOW) { > xpermd->used |= XPERMS_AUDITALLOW; > - if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLDRIVER) { > - memset(xpermd->auditallow->p, 0xff, > - sizeof(xpermd->auditallow->p)); > - } > - if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLFUNCTION) { > - for (i = 0; i < ARRAY_SIZE(xpermd->auditallow->p); i++) > - xpermd->auditallow->p[i] |= > - node->datum.u.xperms->perms.p[i]; > - } > + update_xperms_extended_data(node->datum.u.xperms->specified, > + &node->datum.u.xperms->perms, > + xpermd->auditallow); > } else if (node->key.specified == AVTAB_XPERMS_DONTAUDIT) { > xpermd->used |= XPERMS_DONTAUDIT; > - if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLDRIVER) { > - memset(xpermd->dontaudit->p, 0xff, > - sizeof(xpermd->dontaudit->p)); > - } > - if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLFUNCTION) { > - for (i = 0; i < ARRAY_SIZE(xpermd->dontaudit->p); i++) > - xpermd->dontaudit->p[i] |= > - node->datum.u.xperms->perms.p[i]; > - } > + update_xperms_extended_data(node->datum.u.xperms->specified, > + &node->datum.u.xperms->perms, > + xpermd->dontaudit); > } else { > BUG(); > } > -- > 2.46.0.184.g6999bdac58-goog >
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 55c78c318ccd..c30fad37c013 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -4582,14 +4582,10 @@ static int socket_sockcreate_sid(const struct task_security_struct *tsec, secclass, NULL, socksid); } -static int sock_has_perm(struct sock *sk, u32 perms) +static bool sock_skip_has_perm(u32 sid) { - struct sk_security_struct *sksec = sk->sk_security; - struct common_audit_data ad; - struct lsm_network_audit net; - - if (sksec->sid == SECINITSID_KERNEL) - return 0; + if (sid == SECINITSID_KERNEL) + return true; /* * Before POLICYDB_CAP_USERSPACE_INITIAL_CONTEXT, sockets that @@ -4603,7 +4599,19 @@ static int sock_has_perm(struct sock *sk, u32 perms) * setting. */ if (!selinux_policycap_userspace_initial_context() && - sksec->sid == SECINITSID_INIT) + sid == SECINITSID_INIT) + return true; + return false; +} + + +static int sock_has_perm(struct sock *sk, u32 perms) +{ + struct sk_security_struct *sksec = sk->sk_security; + struct common_audit_data ad; + struct lsm_network_audit net; + + if (sock_skip_has_perm(sksec->sid)) return 0; ad_net_init_from_sk(&ad, &net, sk); @@ -5929,6 +5937,26 @@ static unsigned int selinux_ip_postroute(void *priv, } #endif /* CONFIG_NETFILTER */ +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; + u8 driver; + u8 xperm; + + if (sock_skip_has_perm(sksec->sid)) + return 0; + + ad_net_init_from_sk(&ad, &net, sk); + + driver = nlmsg_type >> 8; + xperm = nlmsg_type & 0xff; + + return avc_has_extended_perms(current_sid(), sksec->sid, sksec->sclass, + perms, driver, xperm, &ad); +} + static int selinux_netlink_send(struct sock *sk, struct sk_buff *skb) { int rc = 0; @@ -5954,7 +5982,17 @@ 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); + /* For Generic Netlink, nlmsg_type is mapped to the + * family id which is dynamically assigned. + * Ignore extended permissions for this type. + */ + if (selinux_policycap_netlink_xperm() && + (sclass != SECCLASS_NETLINK_GENERIC_SOCKET)) { + rc = nlmsg_sock_has_extended_perms( + sk, perm, nlh->nlmsg_type); + } else { + rc = sock_has_perm(sk, perm); + } if (rc) return rc; } else if (rc == -EINVAL) { diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h index 7229c9bf6c27..c95bf89c9ce5 100644 --- a/security/selinux/include/classmap.h +++ b/security/selinux/include/classmap.h @@ -96,17 +96,17 @@ const struct security_class_mapping secclass_map[] = { { "shm", { COMMON_IPC_PERMS, "lock", NULL } }, { "ipc", { COMMON_IPC_PERMS, NULL } }, { "netlink_route_socket", - { COMMON_SOCK_PERMS, "nlmsg_read", "nlmsg_write", NULL } }, + { COMMON_SOCK_PERMS, "nlmsg", "nlmsg_read", "nlmsg_write", NULL } }, { "netlink_tcpdiag_socket", - { COMMON_SOCK_PERMS, "nlmsg_read", "nlmsg_write", NULL } }, + { COMMON_SOCK_PERMS, "nlmsg", "nlmsg_read", "nlmsg_write", NULL } }, { "netlink_nflog_socket", { COMMON_SOCK_PERMS, NULL } }, { "netlink_xfrm_socket", - { COMMON_SOCK_PERMS, "nlmsg_read", "nlmsg_write", NULL } }, + { COMMON_SOCK_PERMS, "nlmsg", "nlmsg_read", "nlmsg_write", NULL } }, { "netlink_selinux_socket", { COMMON_SOCK_PERMS, NULL } }, { "netlink_iscsi_socket", { COMMON_SOCK_PERMS, NULL } }, { "netlink_audit_socket", - { COMMON_SOCK_PERMS, "nlmsg_read", "nlmsg_write", "nlmsg_relay", - "nlmsg_readpriv", "nlmsg_tty_audit", NULL } }, + { COMMON_SOCK_PERMS, "nlmsg", "nlmsg_read", "nlmsg_write", + "nlmsg_relay", "nlmsg_readpriv", "nlmsg_tty_audit", NULL } }, { "netlink_fib_lookup_socket", { COMMON_SOCK_PERMS, NULL } }, { "netlink_connector_socket", { COMMON_SOCK_PERMS, NULL } }, { "netlink_netfilter_socket", { COMMON_SOCK_PERMS, NULL } }, diff --git a/security/selinux/include/policycap.h b/security/selinux/include/policycap.h index dc3674eb29c1..079679fe7254 100644 --- a/security/selinux/include/policycap.h +++ b/security/selinux/include/policycap.h @@ -14,6 +14,7 @@ enum { POLICYDB_CAP_GENFS_SECLABEL_SYMLINKS, POLICYDB_CAP_IOCTL_SKIP_CLOEXEC, POLICYDB_CAP_USERSPACE_INITIAL_CONTEXT, + POLICYDB_CAP_NETLINK_XPERM, __POLICYDB_CAP_MAX }; #define POLICYDB_CAP_MAX (__POLICYDB_CAP_MAX - 1) diff --git a/security/selinux/include/policycap_names.h b/security/selinux/include/policycap_names.h index 2cffcc1ce851..e080827408c4 100644 --- a/security/selinux/include/policycap_names.h +++ b/security/selinux/include/policycap_names.h @@ -17,6 +17,7 @@ const char *const selinux_policycap_names[__POLICYDB_CAP_MAX] = { "genfs_seclabel_symlinks", "ioctl_skip_cloexec", "userspace_initial_context", + "netlink_xperm", }; /* clang-format on */ diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h index 289bf9233f71..c7f2731abd03 100644 --- a/security/selinux/include/security.h +++ b/security/selinux/include/security.h @@ -195,6 +195,12 @@ static inline bool selinux_policycap_userspace_initial_context(void) selinux_state.policycap[POLICYDB_CAP_USERSPACE_INITIAL_CONTEXT]); } +static inline bool selinux_policycap_netlink_xperm(void) +{ + return READ_ONCE( + selinux_state.policycap[POLICYDB_CAP_NETLINK_XPERM]); +} + struct selinux_policy_convert_data; struct selinux_load_state { diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c index 8ff670cf1ee5..0dac942156d6 100644 --- a/security/selinux/nlmsgtab.c +++ b/security/selinux/nlmsgtab.c @@ -170,6 +170,27 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm) { int err = 0; + if (selinux_policycap_netlink_xperm()) { + switch (sclass) { + case SECCLASS_NETLINK_ROUTE_SOCKET: + *perm = NETLINK_ROUTE_SOCKET__NLMSG; + break; + case SECCLASS_NETLINK_TCPDIAG_SOCKET: + *perm = NETLINK_TCPDIAG_SOCKET__NLMSG; + break; + case SECCLASS_NETLINK_XFRM_SOCKET: + *perm = NETLINK_XFRM_SOCKET__NLMSG; + break; + case SECCLASS_NETLINK_AUDIT_SOCKET: + *perm = NETLINK_AUDIT_SOCKET__NLMSG; + break; + default: + err = -ENOENT; + break; + } + return err; + } + switch (sclass) { case SECCLASS_NETLINK_ROUTE_SOCKET: /* RTM_MAX always points to RTM_SETxxxx, ie RTM_NEWxxx + 3. diff --git a/security/selinux/ss/avtab.h b/security/selinux/ss/avtab.h index 8e8820484c55..f4407185401c 100644 --- a/security/selinux/ss/avtab.h +++ b/security/selinux/ss/avtab.h @@ -53,8 +53,9 @@ struct avtab_key { */ 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_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 e33e55384b75..48d5748f03da 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -582,8 +582,7 @@ static void type_attribute_bounds_av(struct policydb *policydb, } /* - * flag which drivers have permissions - * only looking for ioctl based extended permissions + * Flag which drivers have permissions. */ void services_compute_xperms_drivers( struct extended_perms *xperms, @@ -591,14 +590,18 @@ void services_compute_xperms_drivers( { unsigned int i; - if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLDRIVER) { + switch (node->datum.u.xperms->specified) { + case AVTAB_XPERMS_IOCTLDRIVER: /* if one or more driver has all permissions allowed */ for (i = 0; i < ARRAY_SIZE(xperms->drivers.p); i++) xperms->drivers.p[i] |= node->datum.u.xperms->perms.p[i]; - } else if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLFUNCTION) { + break; + case AVTAB_XPERMS_IOCTLFUNCTION: + case AVTAB_XPERMS_NLMSG: /* if allowing permissions within a driver */ security_xperm_set(xperms->drivers.p, node->datum.u.xperms->driver); + break; } xperms->len = 1; @@ -942,55 +945,58 @@ static void avd_init(struct selinux_policy *policy, struct av_decision *avd) avd->flags = 0; } -void services_compute_xperms_decision(struct extended_perms_decision *xpermd, - struct avtab_node *node) +static void update_xperms_extended_data(u8 specified, + struct extended_perms_data *from, + struct extended_perms_data *xp_data) { unsigned int i; - if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLFUNCTION) { + switch (specified) { + case AVTAB_XPERMS_IOCTLDRIVER: + memset(xp_data->p, 0xff, sizeof(xp_data->p)); + break; + case AVTAB_XPERMS_IOCTLFUNCTION: + case AVTAB_XPERMS_NLMSG: + for (i = 0; i < ARRAY_SIZE(xp_data->p); i++) + xp_data->p[i] |= from->p[i]; + break; + } + +} + +void services_compute_xperms_decision(struct extended_perms_decision *xpermd, + struct avtab_node *node) +{ + switch (node->datum.u.xperms->specified) { + case AVTAB_XPERMS_IOCTLFUNCTION: + case AVTAB_XPERMS_NLMSG: if (xpermd->driver != node->datum.u.xperms->driver) return; - } else if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLDRIVER) { + break; + case AVTAB_XPERMS_IOCTLDRIVER: if (!security_xperm_test(node->datum.u.xperms->perms.p, xpermd->driver)) return; - } else { + break; + default: BUG(); } if (node->key.specified == AVTAB_XPERMS_ALLOWED) { xpermd->used |= XPERMS_ALLOWED; - if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLDRIVER) { - memset(xpermd->allowed->p, 0xff, - sizeof(xpermd->allowed->p)); - } - if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLFUNCTION) { - for (i = 0; i < ARRAY_SIZE(xpermd->allowed->p); i++) - xpermd->allowed->p[i] |= - node->datum.u.xperms->perms.p[i]; - } + update_xperms_extended_data(node->datum.u.xperms->specified, + &node->datum.u.xperms->perms, + xpermd->allowed); } else if (node->key.specified == AVTAB_XPERMS_AUDITALLOW) { xpermd->used |= XPERMS_AUDITALLOW; - if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLDRIVER) { - memset(xpermd->auditallow->p, 0xff, - sizeof(xpermd->auditallow->p)); - } - if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLFUNCTION) { - for (i = 0; i < ARRAY_SIZE(xpermd->auditallow->p); i++) - xpermd->auditallow->p[i] |= - node->datum.u.xperms->perms.p[i]; - } + update_xperms_extended_data(node->datum.u.xperms->specified, + &node->datum.u.xperms->perms, + xpermd->auditallow); } else if (node->key.specified == AVTAB_XPERMS_DONTAUDIT) { xpermd->used |= XPERMS_DONTAUDIT; - if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLDRIVER) { - memset(xpermd->dontaudit->p, 0xff, - sizeof(xpermd->dontaudit->p)); - } - if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLFUNCTION) { - for (i = 0; i < ARRAY_SIZE(xpermd->dontaudit->p); i++) - xpermd->dontaudit->p[i] |= - node->datum.u.xperms->perms.p[i]; - } + update_xperms_extended_data(node->datum.u.xperms->specified, + &node->datum.u.xperms->perms, + xpermd->dontaudit); } else { BUG(); }
Reuse the existing extended permissions infrastructure to support policies based on the netlink message types. A new policy capability "netlink_xperm" is introduced. When disabled, the previous behaviour is preserved. That is, netlink_send will rely on the permission mappings defined in nlmsgtab.c (e.g, nlmsg_read for RTM_GETADDR on NETLINK_ROUTE). When enabled, the mappings are ignored and the generic "nlmsg" permission is used instead. The new "nlmsg" permission is an extended permission. The 16 bits of the extended permission are mapped to the nlmsg_type field. 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 }; 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 }; The constants in the example above (e.g., RTM_GETLINK) are explicitly defined in the policy. It is possible to generate policies to support kernels that may or may not have the capability enabled by generating a rule for each scenario. For instance: allow domain self:netlink_audit_socket nlmsg_read; allow domain self:netlink_audit_socket nlmsg; allowxperm domain self:netlink_audit_socket nlmsg { AUDIT_GET }; The approach of defining a new permission ("nlmsg") instead of relying on the existing permissions (e.g., "nlmsg_read", "nlmsg_readpriv" or "nlmsg_tty_audit") has been preferred because: 1. This is similar to the other extended permission ("ioctl"); 2. With the new extended permission, the coarse-grained mapping is not necessary anymore. It could eventually be removed, which would be impossible if the extended permission was defined below these. 3. Having a single extra extended permission considerably simplifies the implementation here and in libselinux. The class NETLINK_GENERIC is excluded from using this extended permission because the nlmsg_type field is referencing the family id which is dynamically associated. Signed-off-by: Thiébaud Weksteen <tweek@google.com> Signed-off-by: Bram Bonné <brambonne@google.com> --- I previously suggested to rename the constants AVTAB_XPERMS_IOCTLDRIVER and AVTAB_XPERMS_IOCTLFUNCTION as they have a similar purpose to the new AVTAB_XPERMS_NLMSG. However, there is no other information in the avtab_datum structure to link the extended permission to the original permission. So it is still necessary to capture which permission is specified in struct avtab_extended_perms. security/selinux/hooks.c | 56 +++++++++++++--- security/selinux/include/classmap.h | 10 +-- security/selinux/include/policycap.h | 1 + security/selinux/include/policycap_names.h | 1 + security/selinux/include/security.h | 6 ++ security/selinux/nlmsgtab.c | 21 ++++++ security/selinux/ss/avtab.h | 5 +- security/selinux/ss/services.c | 78 ++++++++++++---------- 8 files changed, 126 insertions(+), 52 deletions(-)