[PATCH] selinux: map RTM_GETLINK to a privileged permission

[PATCH] selinux: map RTM_GETLINK to a privileged permission

[Jeff Vander Stoep]

Persistent device identifiers like MAC addresses are sensitive
because they are (usually) unique and can be used to
identify/track a device or user [1]. The MAC address is
accessible via the RTM_GETLINK request message type of a netlink
route socket[2] which returns the RTM_NEWLINK message.
Mapping RTM_GETLINK to a separate permission enables restricting
access to the MAC address without changing the behavior for
other RTM_GET* message types.

[1] https://adamdrake.com/mac-addresses-udids-and-privacy.html
[2] Other access vectors like ioctl(SIOCGIFHWADDR) are already covered
by existing LSM hooks.

Signed-off-by: Jeff Vander Stoep <jeffv@google.com>
---
 security/selinux/include/classmap.h |  2 +-
 security/selinux/include/security.h |  9 +++++++++
 security/selinux/nlmsgtab.c         | 26 +++++++++++++++++++++++++-
 security/selinux/ss/services.c      |  4 +++-
 4 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 986f3ac14282..77ccd558890a 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -116,7 +116,7 @@ struct security_class_mapping secclass_map[] = {
 	  { COMMON_IPC_PERMS, NULL } },
 	{ "netlink_route_socket",
 	  { COMMON_SOCK_PERMS,
-	    "nlmsg_read", "nlmsg_write", NULL } },
+	    "nlmsg_read", "nlmsg_write", "nlmsg_readpriv", NULL } },
 	{ "netlink_tcpdiag_socket",
 	  { COMMON_SOCK_PERMS,
 	    "nlmsg_read", "nlmsg_write", NULL } },
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index a39f9565d80b..1671b418ddcb 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -79,6 +79,7 @@ enum {
 	POLICYDB_CAPABILITY_ALWAYSNETWORK,
 	POLICYDB_CAPABILITY_CGROUPSECLABEL,
 	POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION,
+	POLICYDB_CAPABILITY_NETLINK_ROUTE_GETLINK,
 	__POLICYDB_CAPABILITY_MAX
 };
 #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1)
@@ -209,6 +210,13 @@ static inline bool selinux_policycap_nnp_nosuid_transition(void)
 	return state->policycap[POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION];
 }
 
+static inline bool selinux_policycap_nlroute_getlink(void)
+{
+	struct selinux_state *state = &selinux_state;
+
+	return state->policycap[POLICYDB_CAPABILITY_NETLINK_ROUTE_GETLINK];
+}
+
 int security_mls_enabled(struct selinux_state *state);
 int security_load_policy(struct selinux_state *state,
 			 void *data, size_t len);
@@ -422,6 +430,7 @@ extern struct vfsmount *selinuxfs_mount;
 extern void selnl_notify_setenforce(int val);
 extern void selnl_notify_policyload(u32 seqno);
 extern int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm);
+extern void selinux_nlmsg_init(void);
 
 extern void avtab_cache_init(void);
 extern void ebitmap_cache_init(void);
diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
index c97fdae8f71b..aa7064a629a0 100644
--- a/security/selinux/nlmsgtab.c
+++ b/security/selinux/nlmsgtab.c
@@ -25,7 +25,7 @@ struct nlmsg_perm {
 	u32	perm;
 };
 
-static const struct nlmsg_perm nlmsg_route_perms[] =
+static struct nlmsg_perm nlmsg_route_perms[] =
 {
 	{ RTM_NEWLINK,		NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
 	{ RTM_DELLINK,		NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
@@ -208,3 +208,27 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm)
 
 	return err;
 }
+
+static void nlmsg_set_getlink_perm(u32 perm)
+{
+	int i;
+
+	for (i = 0; i < sizeof(nlmsg_route_perms)/sizeof(nlmsg_perm); i++) {
+		if (nlmsg_route_perms[i].nlmsg_type == RTM_GETLINK) {
+			nlmsg_route_perms[i].perm = perm;
+			break;
+		}
+	}
+}
+
+/**
+ * The value permission guarding RTM_GETLINK changes if nlroute_getlink
+ * policy capability is set.
+ */
+void selinux_nlmsg_init(void)
+{
+	if (selinux_policycap_nlroute_getlink())
+		nlmsg_set_getlink_perm(NETLINK_ROUTE_SOCKET__NLMSG_READPRIV);
+	else
+		nlmsg_set_getlink_perm(NETLINK_ROUTE_SOCKET__NLMSG_READ);
+}
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 0e8b94e8e156..910b924fa715 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -73,7 +73,8 @@ const char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = {
 	"extended_socket_class",
 	"always_check_network",
 	"cgroup_seclabel",
-	"nnp_nosuid_transition"
+	"nnp_nosuid_transition",
+	"netlink_route_getlink"
 };
 
 static struct selinux_ss selinux_ss;
@@ -2223,6 +2224,7 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
 
 		state->ss->sidtab = newsidtab;
 		security_load_policycaps(state);
+		selinux_nlmsg_init();
 		selinux_mark_initialized(state);
 		seqno = ++state->ss->latest_granting;
 		selinux_complete_init();
-- 
2.25.0.rc1.283.g88dfdc4193-goog

Re: [PATCH] selinux: map RTM_GETLINK to a privileged permission

[Stephen Smalley]

On 1/16/20 9:26 AM, Jeff Vander Stoep wrote:
> Persistent device identifiers like MAC addresses are sensitive
> because they are (usually) unique and can be used to
> identify/track a device or user [1]. The MAC address is
> accessible via the RTM_GETLINK request message type of a netlink
> route socket[2] which returns the RTM_NEWLINK message.
> Mapping RTM_GETLINK to a separate permission enables restricting
> access to the MAC address without changing the behavior for
> other RTM_GET* message types.
> 
> [1] https://adamdrake.com/mac-addresses-udids-and-privacy.html
> [2] Other access vectors like ioctl(SIOCGIFHWADDR) are already covered
> by existing LSM hooks.
> 
> Signed-off-by: Jeff Vander Stoep <jeffv@google.com>
> ---
> diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
> index c97fdae8f71b..aa7064a629a0 100644
> --- a/security/selinux/nlmsgtab.c
> +++ b/security/selinux/nlmsgtab.c
> @@ -208,3 +208,27 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm)
>   
>   	return err;
>   }
> +
> +static void nlmsg_set_getlink_perm(u32 perm)
> +{
> +	int i;
> +
> +	for (i = 0; i < sizeof(nlmsg_route_perms)/sizeof(nlmsg_perm); i++) {

Usually we'd use ARRAY_SIZE(nlmsg_route_perms) here.

> +		if (nlmsg_route_perms[i].nlmsg_type == RTM_GETLINK) {
> +			nlmsg_route_perms[i].perm = perm;
> +			break;
> +		}
> +	}
> +}
> +
> +/**
> + * The value permission guarding RTM_GETLINK changes if nlroute_getlink

Doesn't quite parse, maybe "The value of the permission" or just "The 
permission".

> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 0e8b94e8e156..910b924fa715 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
>   static struct selinux_ss selinux_ss;
> @@ -2223,6 +2224,7 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
>   
>   		state->ss->sidtab = newsidtab;
>   		security_load_policycaps(state);
> +		selinux_nlmsg_init();
>   		selinux_mark_initialized(state);
>   		seqno = ++state->ss->latest_granting;
>   		selinux_complete_init();
> 

You also need to call it after the other later call to 
security_load_policycaps() for the policy reload case.

Re: [PATCH] selinux: map RTM_GETLINK to a privileged permission

[Paul Moore]

On Thu, Jan 16, 2020 at 9:27 AM Jeff Vander Stoep <jeffv@google.com> wrote:
> Persistent device identifiers like MAC addresses are sensitive
> because they are (usually) unique and can be used to
> identify/track a device or user [1]. The MAC address is
> accessible via the RTM_GETLINK request message type of a netlink
> route socket[2] which returns the RTM_NEWLINK message.
> Mapping RTM_GETLINK to a separate permission enables restricting
> access to the MAC address without changing the behavior for
> other RTM_GET* message types.
>
> [1] https://adamdrake.com/mac-addresses-udids-and-privacy.html
> [2] Other access vectors like ioctl(SIOCGIFHWADDR) are already covered
> by existing LSM hooks.
>
> Signed-off-by: Jeff Vander Stoep <jeffv@google.com>
> ---
>  security/selinux/include/classmap.h |  2 +-
>  security/selinux/include/security.h |  9 +++++++++
>  security/selinux/nlmsgtab.c         | 26 +++++++++++++++++++++++++-
>  security/selinux/ss/services.c      |  4 +++-
>  4 files changed, 38 insertions(+), 3 deletions(-)

...

> diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
> index c97fdae8f71b..aa7064a629a0 100644
> --- a/security/selinux/nlmsgtab.c
> +++ b/security/selinux/nlmsgtab.c
> @@ -25,7 +25,7 @@ struct nlmsg_perm {
>         u32     perm;
>  };
>
> -static const struct nlmsg_perm nlmsg_route_perms[] =
> +static struct nlmsg_perm nlmsg_route_perms[] =
>  {
>         { RTM_NEWLINK,          NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
>         { RTM_DELLINK,          NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
> @@ -208,3 +208,27 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm)
>
>         return err;
>  }
> +
> +static void nlmsg_set_getlink_perm(u32 perm)
> +{
> +       int i;
> +
> +       for (i = 0; i < sizeof(nlmsg_route_perms)/sizeof(nlmsg_perm); i++) {
> +               if (nlmsg_route_perms[i].nlmsg_type == RTM_GETLINK) {
> +                       nlmsg_route_perms[i].perm = perm;
> +                       break;
> +               }
> +       }
> +}
> +
> +/**
> + * The value permission guarding RTM_GETLINK changes if nlroute_getlink
> + * policy capability is set.
> + */
> +void selinux_nlmsg_init(void)
> +{
> +       if (selinux_policycap_nlroute_getlink())
> +               nlmsg_set_getlink_perm(NETLINK_ROUTE_SOCKET__NLMSG_READPRIV);
> +       else
> +               nlmsg_set_getlink_perm(NETLINK_ROUTE_SOCKET__NLMSG_READ);
> +}

Two comments, with the first being rather trivial:

It might be nice to rename this to selinux_policycaps_init() or
something similar; that way we have some hope of collecting similar
policycaps related tweaks in one place.

Our current handling of netlink messages is rather crude, especially
when you consider the significance of the netlink messages and the
rather coarse granularity when compared to other SELinux object
classes.  I believe some (most? all?) of this is due to the number of
netlink messages compared to the maximum number of permissions in an
object class.  Back when xperms were added, one of the motivations for
making it a general solution was to potentially use them for netlink;
we obviously haven't made the change in the netlink controls, but I
think this might be the right time to do it.

--
paul moore
www.paul-moore.com

Re: [PATCH] selinux: map RTM_GETLINK to a privileged permission

[Jeffrey Vander Stoep]

On Fri, Jan 17, 2020 at 1:32 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Thu, Jan 16, 2020 at 9:27 AM Jeff Vander Stoep <jeffv@google.com> wrote:
> > Persistent device identifiers like MAC addresses are sensitive
> > because they are (usually) unique and can be used to
> > identify/track a device or user [1]. The MAC address is
> > accessible via the RTM_GETLINK request message type of a netlink
> > route socket[2] which returns the RTM_NEWLINK message.
> > Mapping RTM_GETLINK to a separate permission enables restricting
> > access to the MAC address without changing the behavior for
> > other RTM_GET* message types.
> >
> > [1] https://adamdrake.com/mac-addresses-udids-and-privacy.html
> > [2] Other access vectors like ioctl(SIOCGIFHWADDR) are already covered
> > by existing LSM hooks.
> >
> > Signed-off-by: Jeff Vander Stoep <jeffv@google.com>
> > ---
> >  security/selinux/include/classmap.h |  2 +-
> >  security/selinux/include/security.h |  9 +++++++++
> >  security/selinux/nlmsgtab.c         | 26 +++++++++++++++++++++++++-
> >  security/selinux/ss/services.c      |  4 +++-
> >  4 files changed, 38 insertions(+), 3 deletions(-)
>
> ...
>
> > diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
> > index c97fdae8f71b..aa7064a629a0 100644
> > --- a/security/selinux/nlmsgtab.c
> > +++ b/security/selinux/nlmsgtab.c
> > @@ -25,7 +25,7 @@ struct nlmsg_perm {
> >         u32     perm;
> >  };
> >
> > -static const struct nlmsg_perm nlmsg_route_perms[] =
> > +static struct nlmsg_perm nlmsg_route_perms[] =
> >  {
> >         { RTM_NEWLINK,          NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
> >         { RTM_DELLINK,          NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
> > @@ -208,3 +208,27 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm)
> >
> >         return err;
> >  }
> > +
> > +static void nlmsg_set_getlink_perm(u32 perm)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < sizeof(nlmsg_route_perms)/sizeof(nlmsg_perm); i++) {
> > +               if (nlmsg_route_perms[i].nlmsg_type == RTM_GETLINK) {
> > +                       nlmsg_route_perms[i].perm = perm;
> > +                       break;
> > +               }
> > +       }
> > +}
> > +
> > +/**
> > + * The value permission guarding RTM_GETLINK changes if nlroute_getlink
> > + * policy capability is set.
> > + */
> > +void selinux_nlmsg_init(void)
> > +{
> > +       if (selinux_policycap_nlroute_getlink())
> > +               nlmsg_set_getlink_perm(NETLINK_ROUTE_SOCKET__NLMSG_READPRIV);
> > +       else
> > +               nlmsg_set_getlink_perm(NETLINK_ROUTE_SOCKET__NLMSG_READ);
> > +}
>
> Two comments, with the first being rather trivial:
>
> It might be nice to rename this to selinux_policycaps_init() or
> something similar; that way we have some hope of collecting similar
> policycaps related tweaks in one place.
>
> Our current handling of netlink messages is rather crude, especially
> when you consider the significance of the netlink messages and the
> rather coarse granularity when compared to other SELinux object
> classes.  I believe some (most? all?) of this is due to the number of
> netlink messages compared to the maximum number of permissions in an
> object class.  Back when xperms were added, one of the motivations for
> making it a general solution was to potentially use them for netlink;
> we obviously haven't made the change in the netlink controls, but I
> think this might be the right time to do it.

That's a very large change with some unanswered questions - like how to handle
generic netlink. I will have time later this year to make that change.

In the meantime, this change is small (ideal for backporting) and
consistent with
how we differentiate between levels of sensitivity on netlink_audit messages.
Would you consider taking v3 of this change with your suggested adjustment to
selinux_policycaps_init()?

(Apologies for the resend, gmail switched out of "plain text" mode so my initial
response wasn't delivered to the mailing list).

>
>
> --
> paul moore
> www.paul-moore.com

Re: [PATCH] selinux: map RTM_GETLINK to a privileged permission

[Dominick Grift]

Jeffrey Vander Stoep <jeffv@google.com> writes:

> On Fri, Jan 17, 2020 at 1:32 AM Paul Moore <paul@paul-moore.com> wrote:
>>
>> On Thu, Jan 16, 2020 at 9:27 AM Jeff Vander Stoep <jeffv@google.com> wrote:
>> > Persistent device identifiers like MAC addresses are sensitive
>> > because they are (usually) unique and can be used to
>> > identify/track a device or user [1]. The MAC address is
>> > accessible via the RTM_GETLINK request message type of a netlink
>> > route socket[2] which returns the RTM_NEWLINK message.
>> > Mapping RTM_GETLINK to a separate permission enables restricting
>> > access to the MAC address without changing the behavior for
>> > other RTM_GET* message types.
>> >
>> > [1] https://adamdrake.com/mac-addresses-udids-and-privacy.html
>> > [2] Other access vectors like ioctl(SIOCGIFHWADDR) are already covered
>> > by existing LSM hooks.
>> >
>> > Signed-off-by: Jeff Vander Stoep <jeffv@google.com>

Pardon my intrusion but I am trying to determine whether I would be able
to leverage this functionality and I would appreciate any comments,
suggestions etc.

I have two commits:

1. Adding nlmsg_readpriv to netlink_route_socket, and adding the
netlink_route_getlink policy capability.

This commit effectively changes nothing whether I have the polcap
enabled or not.

https://defensec.nl/gitweb/dssp2.git/commitdiff/83162d18c6f829de418921339269fa41b4e61882

2. leveraging nlmsg_readpriv

This adds a permissionx for "all netlink_route_socket ioctl except
SIOCGIFHWADDR and two classpermissions that are basically the
r_netlink_route_socket_perms and create_netlink_route_socket_perms
equivalents but without ioctl and nlmsg_readpriv.

https://defensec.nl/gitweb/dssp2.git/commit/1ab25105ede7a085f85c1b11b3abbc8e5b80dae5

The idea is that domains that shouldnt have access to mac addresses (I
suppose the majority) will use for example ...

(allow mydomain self r_netlink_route_except_ioctl_and_nlmsg_readpriv_socket_perms)
(allowx mydomain self netlink_route_socket_ioctl_except_SIOCGIFHWADDR)

... whereas everything else will keep using the existing
r_netlink_route_socket_perms or create_netlink_route_socket_perms

Does this make sense to you, and are these all the *direct* access
vectors to get mac addresses?

I guess there would be indirect ways to get it from an entity that does
have access to netlink_route_socket nlmsg_readpriv and SIOCGIFHWADDR but
that is a different story.

>> > ---
>> >  security/selinux/include/classmap.h |  2 +-
>> >  security/selinux/include/security.h |  9 +++++++++
>> >  security/selinux/nlmsgtab.c         | 26 +++++++++++++++++++++++++-
>> >  security/selinux/ss/services.c      |  4 +++-
>> >  4 files changed, 38 insertions(+), 3 deletions(-)
>>
>> ...
>>
>> > diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
>> > index c97fdae8f71b..aa7064a629a0 100644
>> > --- a/security/selinux/nlmsgtab.c
>> > +++ b/security/selinux/nlmsgtab.c
>> > @@ -25,7 +25,7 @@ struct nlmsg_perm {
>> >         u32     perm;
>> >  };
>> >
>> > -static const struct nlmsg_perm nlmsg_route_perms[] =
>> > +static struct nlmsg_perm nlmsg_route_perms[] =
>> >  {
>> >         { RTM_NEWLINK,          NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
>> >         { RTM_DELLINK,          NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
>> > @@ -208,3 +208,27 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm)
>> >
>> >         return err;
>> >  }
>> > +
>> > +static void nlmsg_set_getlink_perm(u32 perm)
>> > +{
>> > +       int i;
>> > +
>> > +       for (i = 0; i < sizeof(nlmsg_route_perms)/sizeof(nlmsg_perm); i++) {
>> > +               if (nlmsg_route_perms[i].nlmsg_type == RTM_GETLINK) {
>> > +                       nlmsg_route_perms[i].perm = perm;
>> > +                       break;
>> > +               }
>> > +       }
>> > +}
>> > +
>> > +/**
>> > + * The value permission guarding RTM_GETLINK changes if nlroute_getlink
>> > + * policy capability is set.
>> > + */
>> > +void selinux_nlmsg_init(void)
>> > +{
>> > +       if (selinux_policycap_nlroute_getlink())
>> > +               nlmsg_set_getlink_perm(NETLINK_ROUTE_SOCKET__NLMSG_READPRIV);
>> > +       else
>> > +               nlmsg_set_getlink_perm(NETLINK_ROUTE_SOCKET__NLMSG_READ);
>> > +}
>>
>> Two comments, with the first being rather trivial:
>>
>> It might be nice to rename this to selinux_policycaps_init() or
>> something similar; that way we have some hope of collecting similar
>> policycaps related tweaks in one place.
>>
>> Our current handling of netlink messages is rather crude, especially
>> when you consider the significance of the netlink messages and the
>> rather coarse granularity when compared to other SELinux object
>> classes.  I believe some (most? all?) of this is due to the number of
>> netlink messages compared to the maximum number of permissions in an
>> object class.  Back when xperms were added, one of the motivations for
>> making it a general solution was to potentially use them for netlink;
>> we obviously haven't made the change in the netlink controls, but I
>> think this might be the right time to do it.
>
> That's a very large change with some unanswered questions - like how to handle
> generic netlink. I will have time later this year to make that change.
>
> In the meantime, this change is small (ideal for backporting) and
> consistent with
> how we differentiate between levels of sensitivity on netlink_audit messages.
> Would you consider taking v3 of this change with your suggested adjustment to
> selinux_policycaps_init()?
>
> (Apologies for the resend, gmail switched out of "plain text" mode so my initial
> response wasn't delivered to the mailing list).
>
>>
>>
>> --
>> paul moore
>> www.paul-moore.com

-- 
Key fingerprint = 5F4D 3CDB D3F8 3652 FBD8 02D5 3B6C 5F1D 2C7B 6B02
https://sks-keyservers.net/pks/lookup?op=get&search=0x3B6C5F1D2C7B6B02
Dominick Grift

Re: [PATCH] selinux: map RTM_GETLINK to a privileged permission

[Jeffrey Vander Stoep]

On Fri, Jan 17, 2020 at 1:38 PM Dominick Grift <dac.override@gmail.com> wrote:
>
> Jeffrey Vander Stoep <jeffv@google.com> writes:
>
> > On Fri, Jan 17, 2020 at 1:32 AM Paul Moore <paul@paul-moore.com> wrote:
> >>
> >> On Thu, Jan 16, 2020 at 9:27 AM Jeff Vander Stoep <jeffv@google.com> wrote:
> >> > Persistent device identifiers like MAC addresses are sensitive
> >> > because they are (usually) unique and can be used to
> >> > identify/track a device or user [1]. The MAC address is
> >> > accessible via the RTM_GETLINK request message type of a netlink
> >> > route socket[2] which returns the RTM_NEWLINK message.
> >> > Mapping RTM_GETLINK to a separate permission enables restricting
> >> > access to the MAC address without changing the behavior for
> >> > other RTM_GET* message types.
> >> >
> >> > [1] https://adamdrake.com/mac-addresses-udids-and-privacy.html
> >> > [2] Other access vectors like ioctl(SIOCGIFHWADDR) are already covered
> >> > by existing LSM hooks.
> >> >
> >> > Signed-off-by: Jeff Vander Stoep <jeffv@google.com>
>
> Pardon my intrusion but I am trying to determine whether I would be able
> to leverage this functionality and I would appreciate any comments,
> suggestions etc.
>
> I have two commits:
>
> 1. Adding nlmsg_readpriv to netlink_route_socket, and adding the
> netlink_route_getlink policy capability.
>
> This commit effectively changes nothing whether I have the polcap
> enabled or not.

Yes, this change is necessary but not sufficient. You must also block other
access vectors.

>
> https://defensec.nl/gitweb/dssp2.git/commitdiff/83162d18c6f829de418921339269fa41b4e61882
>
> 2. leveraging nlmsg_readpriv
>
> This adds a permissionx for "all netlink_route_socket ioctl except
> SIOCGIFHWADDR and two classpermissions that are basically the
> r_netlink_route_socket_perms and create_netlink_route_socket_perms
> equivalents but without ioctl and nlmsg_readpriv.
>
> https://defensec.nl/gitweb/dssp2.git/commit/1ab25105ede7a085f85c1b11b3abbc8e5b80dae5
>
> The idea is that domains that shouldnt have access to mac addresses (I
> suppose the majority) will use for example ...
>
> (allow mydomain self r_netlink_route_except_ioctl_and_nlmsg_readpriv_socket_perms)
> (allowx mydomain self netlink_route_socket_ioctl_except_SIOCGIFHWADDR)
>
> ... whereas everything else will keep using the existing
> r_netlink_route_socket_perms or create_netlink_route_socket_perms
>
> Does this make sense to you, and are these all the *direct* access
> vectors to get mac addresses?

I restrict three vectors
1. RTM_GETLINK on netlink_route sockets
2. bind() on netlink_route sockets.
3. SIOCGIFHWADDR ioctl for all sockets

That's sufficient on Android.
>
> I guess there would be indirect ways to get it from an entity that does
> have access to netlink_route_socket nlmsg_readpriv and SIOCGIFHWADDR but
> that is a different story.

Yes, laundering of permissions is a separate issue unrelated to this patch.

>
> >> > ---
> >> >  security/selinux/include/classmap.h |  2 +-
> >> >  security/selinux/include/security.h |  9 +++++++++
> >> >  security/selinux/nlmsgtab.c         | 26 +++++++++++++++++++++++++-
> >> >  security/selinux/ss/services.c      |  4 +++-
> >> >  4 files changed, 38 insertions(+), 3 deletions(-)
> >>
> >> ...
> >>
> >> > diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
> >> > index c97fdae8f71b..aa7064a629a0 100644
> >> > --- a/security/selinux/nlmsgtab.c
> >> > +++ b/security/selinux/nlmsgtab.c
> >> > @@ -25,7 +25,7 @@ struct nlmsg_perm {
> >> >         u32     perm;
> >> >  };
> >> >
> >> > -static const struct nlmsg_perm nlmsg_route_perms[] =
> >> > +static struct nlmsg_perm nlmsg_route_perms[] =
> >> >  {
> >> >         { RTM_NEWLINK,          NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
> >> >         { RTM_DELLINK,          NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
> >> > @@ -208,3 +208,27 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm)
> >> >
> >> >         return err;
> >> >  }
> >> > +
> >> > +static void nlmsg_set_getlink_perm(u32 perm)
> >> > +{
> >> > +       int i;
> >> > +
> >> > +       for (i = 0; i < sizeof(nlmsg_route_perms)/sizeof(nlmsg_perm); i++) {
> >> > +               if (nlmsg_route_perms[i].nlmsg_type == RTM_GETLINK) {
> >> > +                       nlmsg_route_perms[i].perm = perm;
> >> > +                       break;
> >> > +               }
> >> > +       }
> >> > +}
> >> > +
> >> > +/**
> >> > + * The value permission guarding RTM_GETLINK changes if nlroute_getlink
> >> > + * policy capability is set.
> >> > + */
> >> > +void selinux_nlmsg_init(void)
> >> > +{
> >> > +       if (selinux_policycap_nlroute_getlink())
> >> > +               nlmsg_set_getlink_perm(NETLINK_ROUTE_SOCKET__NLMSG_READPRIV);
> >> > +       else
> >> > +               nlmsg_set_getlink_perm(NETLINK_ROUTE_SOCKET__NLMSG_READ);
> >> > +}
> >>
> >> Two comments, with the first being rather trivial:
> >>
> >> It might be nice to rename this to selinux_policycaps_init() or
> >> something similar; that way we have some hope of collecting similar
> >> policycaps related tweaks in one place.
> >>
> >> Our current handling of netlink messages is rather crude, especially
> >> when you consider the significance of the netlink messages and the
> >> rather coarse granularity when compared to other SELinux object
> >> classes.  I believe some (most? all?) of this is due to the number of
> >> netlink messages compared to the maximum number of permissions in an
> >> object class.  Back when xperms were added, one of the motivations for
> >> making it a general solution was to potentially use them for netlink;
> >> we obviously haven't made the change in the netlink controls, but I
> >> think this might be the right time to do it.
> >
> > That's a very large change with some unanswered questions - like how to handle
> > generic netlink. I will have time later this year to make that change.
> >
> > In the meantime, this change is small (ideal for backporting) and
> > consistent with
> > how we differentiate between levels of sensitivity on netlink_audit messages.
> > Would you consider taking v3 of this change with your suggested adjustment to
> > selinux_policycaps_init()?
> >
> > (Apologies for the resend, gmail switched out of "plain text" mode so my initial
> > response wasn't delivered to the mailing list).
> >
> >>
> >>
> >> --
> >> paul moore
> >> www.paul-moore.com
>
> --
> Key fingerprint = 5F4D 3CDB D3F8 3652 FBD8 02D5 3B6C 5F1D 2C7B 6B02
> https://sks-keyservers.net/pks/lookup?op=get&search=0x3B6C5F1D2C7B6B02
> Dominick Grift

Re: [PATCH] selinux: map RTM_GETLINK to a privileged permission

[Paul Moore]

On January 17, 2020 3:21:10 AM Jeffrey Vander Stoep <jeffv@google.com> wrote:
> On Fri, Jan 17, 2020 at 1:32 AM Paul Moore <paul@paul-moore.com> wrote:
>> Our current handling of netlink messages is rather crude, especially
>> when you consider the significance of the netlink messages and the
>> rather coarse granularity when compared to other SELinux object
>> classes.  I believe some (most? all?) of this is due to the number of
>> netlink messages compared to the maximum number of permissions in an
>> object class.  Back when xperms were added, one of the motivations for
>> making it a general solution was to potentially use them for netlink;
>> we obviously haven't made the change in the netlink controls, but I
>> think this might be the right time to do it.
> That's a very large change with some unanswered questions - like how to
> handle
> generic netlink. I will have time later this year to make that change.
>
> In the meantime, this change is small (ideal for backporting) and
> consistent with
> how we differentiate between levels of sensitivity on netlink_audit
> messages.
> Would you consider taking v3 of this change with your suggested adjustment
> to
> selinux_policycaps_init()?

Yes, it is a big change and there are some open questions, but both of the changes we are discussing here are exposed to userspace so there is a need to make sure we get this as right as possible the first time.  I am not a fan of exposing a change to userspace knowing that we will be replacing it in the future.

If we need to update the netlink controls, and I think we do, let's do it properly and not one message at a time.

--
paul moore
www.paul-moore.com

Re: [PATCH] selinux: map RTM_GETLINK to a privileged permission

[Jeffrey Vander Stoep]

OK. I'll put something together, but it'll be in a couple of months.

On Fri, Jan 17, 2020 at 4:19 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On January 17, 2020 3:21:10 AM Jeffrey Vander Stoep <jeffv@google.com> wrote:
> > On Fri, Jan 17, 2020 at 1:32 AM Paul Moore <paul@paul-moore.com> wrote:
> >> Our current handling of netlink messages is rather crude, especially
> >> when you consider the significance of the netlink messages and the
> >> rather coarse granularity when compared to other SELinux object
> >> classes.  I believe some (most? all?) of this is due to the number of
> >> netlink messages compared to the maximum number of permissions in an
> >> object class.  Back when xperms were added, one of the motivations for
> >> making it a general solution was to potentially use them for netlink;
> >> we obviously haven't made the change in the netlink controls, but I
> >> think this might be the right time to do it.
> > That's a very large change with some unanswered questions - like how to
> > handle
> > generic netlink. I will have time later this year to make that change.
> >
> > In the meantime, this change is small (ideal for backporting) and
> > consistent with
> > how we differentiate between levels of sensitivity on netlink_audit
> > messages.
> > Would you consider taking v3 of this change with your suggested adjustment
> > to
> > selinux_policycaps_init()?
>
> Yes, it is a big change and there are some open questions, but both of the changes we are discussing here are exposed to userspace so there is a need to make sure we get this as right as possible the first time.  I am not a fan of exposing a change to userspace knowing that we will be replacing it in the future.
>
> If we need to update the netlink controls, and I think we do, let's do it properly and not one message at a time.
>
> --
> paul moore
> www.paul-moore.com
>
>
>
>