On Tue, Nov 16, 2021 at 10:41 AM Stephen Smalley
<stephen.smalley.work@xxxxxxxxx> wrote:
>
> On Wed, Nov 10, 2021 at 6:13 AM Bram Bonne <brambonne@xxxxxxxxxx> 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@xxxxxxxxxxxxxx/
> >
> > Signed-off-by: Bram Bonne <brambonne@xxxxxxxxxx>
> > ---
> > 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
