selinux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] rtnetlink: gate MAC address with an LSM hook
@ 2019-08-21 13:45 Jeff Vander Stoep
  2019-08-21 13:55 ` Jeffrey Vander Stoep
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Jeff Vander Stoep @ 2019-08-21 13:45 UTC (permalink / raw)
  To: netdev, linux-security-module, selinux; +Cc: Jeff Vander Stoep

MAC addresses are often considered 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_NEWLINK message type of a
netlink route socket[2]. Ideally we could grant/deny access to the
MAC address on a case-by-case basis without blocking the entire
RTM_NEWLINK message type which contains a lot of other useful
information. This can be achieved using a new LSM hook on the netlink
message receive path. Using this new hook, individual LSMs can select
which processes are allowed access to the real MAC, otherwise a
default value of zeros is returned. Offloading access control
decisions like this to an LSM is convenient because it preserves the
status quo for most Linux users while giving the various LSMs
flexibility to make finer grained decisions on access to sensitive
data based on policy.

[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>
---
 include/linux/lsm_hooks.h |  8 ++++++++
 include/linux/security.h  |  6 ++++++
 net/core/rtnetlink.c      | 12 ++++++++++--
 security/security.c       |  5 +++++
 4 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index df1318d85f7d..dfcb2e11ff43 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -728,6 +728,12 @@
  *
  * Security hooks for Netlink messaging.
  *
+ * @netlink_receive
+ *	Check permissions on a netlink message field before populating it.
+ *	@sk associated sock of task receiving the message.
+ *	@skb contains the sk_buff structure for the netlink message.
+ *	Return 0 if the data should be included in the message.
+ *
  * @netlink_send:
  *	Save security information for a netlink message so that permission
  *	checking can be performed when the message is processed.  The security
@@ -1673,6 +1679,7 @@ union security_list_options {
 	int (*sem_semop)(struct kern_ipc_perm *perm, struct sembuf *sops,
 				unsigned nsops, int alter);
 
+	int (*netlink_receive)(struct sock *sk, struct sk_buff *skb);
 	int (*netlink_send)(struct sock *sk, struct sk_buff *skb);
 
 	void (*d_instantiate)(struct dentry *dentry, struct inode *inode);
@@ -1952,6 +1959,7 @@ struct security_hook_heads {
 	struct hlist_head sem_associate;
 	struct hlist_head sem_semctl;
 	struct hlist_head sem_semop;
+	struct hlist_head netlink_receive;
 	struct hlist_head netlink_send;
 	struct hlist_head d_instantiate;
 	struct hlist_head getprocattr;
diff --git a/include/linux/security.h b/include/linux/security.h
index 5f7441abbf42..46b5af6de59e 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -382,6 +382,7 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
 			 char **value);
 int security_setprocattr(const char *lsm, const char *name, void *value,
 			 size_t size);
+int security_netlink_receive(struct sock *sk, struct sk_buff *skb);
 int security_netlink_send(struct sock *sk, struct sk_buff *skb);
 int security_ismaclabel(const char *name);
 int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
@@ -1162,6 +1163,11 @@ static inline int security_setprocattr(const char *lsm, char *name,
 	return -EINVAL;
 }
 
+static inline int security_netlink_receive(struct sock *sk, struct sk_buff *skb)
+{
+	return 0;
+}
+
 static inline int security_netlink_send(struct sock *sk, struct sk_buff *skb)
 {
 	return 0;
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 1ee6460f8275..7d69fcb8d22e 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1650,8 +1650,16 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
 		goto nla_put_failure;
 
 	if (dev->addr_len) {
-		if (nla_put(skb, IFLA_ADDRESS, dev->addr_len, dev->dev_addr) ||
-		    nla_put(skb, IFLA_BROADCAST, dev->addr_len, dev->broadcast))
+		if (skb->sk && security_netlink_receive(skb->sk, skb)) {
+			if (!nla_reserve(skb, IFLA_ADDRESS, dev->addr_len))
+				goto nla_put_failure;
+
+		} else {
+			if (nla_put(skb, IFLA_ADDRESS, dev->addr_len,
+				    dev->dev_addr))
+				goto nla_put_failure;
+		}
+		if (nla_put(skb, IFLA_BROADCAST, dev->addr_len, dev->broadcast))
 			goto nla_put_failure;
 	}
 
diff --git a/security/security.c b/security/security.c
index 250ee2d76406..35c5929921b2 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1861,6 +1861,11 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
 	return -EINVAL;
 }
 
+int security_netlink_receive(struct sock *sk, struct sk_buff *skb)
+{
+	return call_int_hook(netlink_receive, 0, sk, skb);
+}
+
 int security_netlink_send(struct sock *sk, struct sk_buff *skb)
 {
 	return call_int_hook(netlink_send, 0, sk, skb);
-- 
2.23.0.rc1.153.gdeed80330f-goog


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] rtnetlink: gate MAC address with an LSM hook
  2019-08-21 13:45 [PATCH 1/2] rtnetlink: gate MAC address with an LSM hook Jeff Vander Stoep
@ 2019-08-21 13:55 ` Jeffrey Vander Stoep
  2019-08-21 14:34 ` Casey Schaufler
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 11+ messages in thread
From: Jeffrey Vander Stoep @ 2019-08-21 13:55 UTC (permalink / raw)
  To: netdev, LSM List, selinux

On Wed, Aug 21, 2019 at 3:45 PM Jeff Vander Stoep <jeffv@google.com> wrote:
>
> MAC addresses are often considered 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_NEWLINK message type of a
> netlink route socket[2]. Ideally we could grant/deny access to the
> MAC address on a case-by-case basis without blocking the entire
> RTM_NEWLINK message type which contains a lot of other useful
> information. This can be achieved using a new LSM hook on the netlink
> message receive path. Using this new hook, individual LSMs can select
> which processes are allowed access to the real MAC, otherwise a
> default value of zeros is returned. Offloading access control
> decisions like this to an LSM is convenient because it preserves the
> status quo for most Linux users while giving the various LSMs
> flexibility to make finer grained decisions on access to sensitive
> data based on policy.
>
> [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>
> ---
>  include/linux/lsm_hooks.h |  8 ++++++++
>  include/linux/security.h  |  6 ++++++
>  net/core/rtnetlink.c      | 12 ++++++++++--
>  security/security.c       |  5 +++++
>  4 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index df1318d85f7d..dfcb2e11ff43 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -728,6 +728,12 @@
>   *
>   * Security hooks for Netlink messaging.
>   *
> + * @netlink_receive
> + *     Check permissions on a netlink message field before populating it.
> + *     @sk associated sock of task receiving the message.
> + *     @skb contains the sk_buff structure for the netlink message.
> + *     Return 0 if the data should be included in the message.
> + *
>   * @netlink_send:
>   *     Save security information for a netlink message so that permission
>   *     checking can be performed when the message is processed.  The security
> @@ -1673,6 +1679,7 @@ union security_list_options {
>         int (*sem_semop)(struct kern_ipc_perm *perm, struct sembuf *sops,
>                                 unsigned nsops, int alter);
>
> +       int (*netlink_receive)(struct sock *sk, struct sk_buff *skb);
>         int (*netlink_send)(struct sock *sk, struct sk_buff *skb);
>
>         void (*d_instantiate)(struct dentry *dentry, struct inode *inode);
> @@ -1952,6 +1959,7 @@ struct security_hook_heads {
>         struct hlist_head sem_associate;
>         struct hlist_head sem_semctl;
>         struct hlist_head sem_semop;
> +       struct hlist_head netlink_receive;
>         struct hlist_head netlink_send;
>         struct hlist_head d_instantiate;
>         struct hlist_head getprocattr;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 5f7441abbf42..46b5af6de59e 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -382,6 +382,7 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
>                          char **value);
>  int security_setprocattr(const char *lsm, const char *name, void *value,
>                          size_t size);
> +int security_netlink_receive(struct sock *sk, struct sk_buff *skb);
>  int security_netlink_send(struct sock *sk, struct sk_buff *skb);
>  int security_ismaclabel(const char *name);
>  int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
> @@ -1162,6 +1163,11 @@ static inline int security_setprocattr(const char *lsm, char *name,
>         return -EINVAL;
>  }
>
> +static inline int security_netlink_receive(struct sock *sk, struct sk_buff *skb)
> +{
> +       return 0;
> +}
> +
>  static inline int security_netlink_send(struct sock *sk, struct sk_buff *skb)
>  {
>         return 0;
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 1ee6460f8275..7d69fcb8d22e 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1650,8 +1650,16 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
>                 goto nla_put_failure;
>
>         if (dev->addr_len) {
> -               if (nla_put(skb, IFLA_ADDRESS, dev->addr_len, dev->dev_addr) ||
> -                   nla_put(skb, IFLA_BROADCAST, dev->addr_len, dev->broadcast))
> +               if (skb->sk && security_netlink_receive(skb->sk, skb)) {
> +                       if (!nla_reserve(skb, IFLA_ADDRESS, dev->addr_len))
> +                               goto nla_put_failure;


Is populating the field with zeros the right approach or should I just
omit it entirely?
Even though this change will only impact LSM users I would still like to
minimize the potential for breakage of userspace processes. Returning the same
packet size and format seems like the least fragile thing to do.


>
> +
> +               } else {
> +                       if (nla_put(skb, IFLA_ADDRESS, dev->addr_len,
> +                                   dev->dev_addr))
> +                               goto nla_put_failure;
> +               }
> +               if (nla_put(skb, IFLA_BROADCAST, dev->addr_len, dev->broadcast))
>                         goto nla_put_failure;
>         }
>
> diff --git a/security/security.c b/security/security.c
> index 250ee2d76406..35c5929921b2 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1861,6 +1861,11 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
>         return -EINVAL;
>  }
>
> +int security_netlink_receive(struct sock *sk, struct sk_buff *skb)
> +{
> +       return call_int_hook(netlink_receive, 0, sk, skb);
> +}
> +
>  int security_netlink_send(struct sock *sk, struct sk_buff *skb)
>  {
>         return call_int_hook(netlink_send, 0, sk, skb);
> --
> 2.23.0.rc1.153.gdeed80330f-goog
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] rtnetlink: gate MAC address with an LSM hook
  2019-08-21 13:45 [PATCH 1/2] rtnetlink: gate MAC address with an LSM hook Jeff Vander Stoep
  2019-08-21 13:55 ` Jeffrey Vander Stoep
@ 2019-08-21 14:34 ` Casey Schaufler
  2019-08-21 14:52   ` Jeffrey Vander Stoep
  2019-08-22 23:19 ` David Miller
  2019-08-23  4:24 ` kbuild test robot
  3 siblings, 1 reply; 11+ messages in thread
From: Casey Schaufler @ 2019-08-21 14:34 UTC (permalink / raw)
  To: Jeff Vander Stoep, netdev, linux-security-module, selinux

On 8/21/2019 6:45 AM, Jeff Vander Stoep wrote:
> MAC addresses are often considered 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_NEWLINK message type of a
> netlink route socket[2]. Ideally we could grant/deny access to the
> MAC address on a case-by-case basis without blocking the entire
> RTM_NEWLINK message type which contains a lot of other useful
> information. This can be achieved using a new LSM hook on the netlink
> message receive path. Using this new hook, individual LSMs can select
> which processes are allowed access to the real MAC, otherwise a
> default value of zeros is returned. Offloading access control
> decisions like this to an LSM is convenient because it preserves the
> status quo for most Linux users while giving the various LSMs
> flexibility to make finer grained decisions on access to sensitive
> data based on policy.

Is the MAC address the only bit of skb data that you might
want to control with MAC? ( Sorry, couldn't help it ;) )
Just musing, but might it make more sense to leave the core
code unmodified and clear the MAC address in the skb inside
the LSM? If you did it that way you could address any other
data you want to control using the same hook. I would hate
to see separate LSM hooks for each of several bits of data. 
On the other hand, I wouldn't want you to violate any layering
policies in the networking code. That would be wrong.

>
> [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>
> ---
>  include/linux/lsm_hooks.h |  8 ++++++++
>  include/linux/security.h  |  6 ++++++
>  net/core/rtnetlink.c      | 12 ++++++++++--
>  security/security.c       |  5 +++++
>  4 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index df1318d85f7d..dfcb2e11ff43 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -728,6 +728,12 @@
>   *
>   * Security hooks for Netlink messaging.
>   *
> + * @netlink_receive
> + *	Check permissions on a netlink message field before populating it.
> + *	@sk associated sock of task receiving the message.
> + *	@skb contains the sk_buff structure for the netlink message.
> + *	Return 0 if the data should be included in the message.
> + *
>   * @netlink_send:
>   *	Save security information for a netlink message so that permission
>   *	checking can be performed when the message is processed.  The security
> @@ -1673,6 +1679,7 @@ union security_list_options {
>  	int (*sem_semop)(struct kern_ipc_perm *perm, struct sembuf *sops,
>  				unsigned nsops, int alter);
>  
> +	int (*netlink_receive)(struct sock *sk, struct sk_buff *skb);
>  	int (*netlink_send)(struct sock *sk, struct sk_buff *skb);
>  
>  	void (*d_instantiate)(struct dentry *dentry, struct inode *inode);
> @@ -1952,6 +1959,7 @@ struct security_hook_heads {
>  	struct hlist_head sem_associate;
>  	struct hlist_head sem_semctl;
>  	struct hlist_head sem_semop;
> +	struct hlist_head netlink_receive;
>  	struct hlist_head netlink_send;
>  	struct hlist_head d_instantiate;
>  	struct hlist_head getprocattr;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 5f7441abbf42..46b5af6de59e 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -382,6 +382,7 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
>  			 char **value);
>  int security_setprocattr(const char *lsm, const char *name, void *value,
>  			 size_t size);
> +int security_netlink_receive(struct sock *sk, struct sk_buff *skb);
>  int security_netlink_send(struct sock *sk, struct sk_buff *skb);
>  int security_ismaclabel(const char *name);
>  int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
> @@ -1162,6 +1163,11 @@ static inline int security_setprocattr(const char *lsm, char *name,
>  	return -EINVAL;
>  }
>  
> +static inline int security_netlink_receive(struct sock *sk, struct sk_buff *skb)
> +{
> +	return 0;
> +}
> +
>  static inline int security_netlink_send(struct sock *sk, struct sk_buff *skb)
>  {
>  	return 0;
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 1ee6460f8275..7d69fcb8d22e 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1650,8 +1650,16 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
>  		goto nla_put_failure;
>  
>  	if (dev->addr_len) {
> -		if (nla_put(skb, IFLA_ADDRESS, dev->addr_len, dev->dev_addr) ||
> -		    nla_put(skb, IFLA_BROADCAST, dev->addr_len, dev->broadcast))
> +		if (skb->sk && security_netlink_receive(skb->sk, skb)) {
> +			if (!nla_reserve(skb, IFLA_ADDRESS, dev->addr_len))
> +				goto nla_put_failure;
> +
> +		} else {
> +			if (nla_put(skb, IFLA_ADDRESS, dev->addr_len,
> +				    dev->dev_addr))
> +				goto nla_put_failure;
> +		}
> +		if (nla_put(skb, IFLA_BROADCAST, dev->addr_len, dev->broadcast))
>  			goto nla_put_failure;
>  	}
>  
> diff --git a/security/security.c b/security/security.c
> index 250ee2d76406..35c5929921b2 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1861,6 +1861,11 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
>  	return -EINVAL;
>  }
>  
> +int security_netlink_receive(struct sock *sk, struct sk_buff *skb)
> +{
> +	return call_int_hook(netlink_receive, 0, sk, skb);
> +}
> +
>  int security_netlink_send(struct sock *sk, struct sk_buff *skb)
>  {
>  	return call_int_hook(netlink_send, 0, sk, skb);


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] rtnetlink: gate MAC address with an LSM hook
  2019-08-21 14:34 ` Casey Schaufler
@ 2019-08-21 14:52   ` Jeffrey Vander Stoep
  0 siblings, 0 replies; 11+ messages in thread
From: Jeffrey Vander Stoep @ 2019-08-21 14:52 UTC (permalink / raw)
  To: Casey Schaufler; +Cc: netdev, LSM List, selinux

On Wed, Aug 21, 2019 at 4:34 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 8/21/2019 6:45 AM, Jeff Vander Stoep wrote:
> > MAC addresses are often considered 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_NEWLINK message type of a
> > netlink route socket[2]. Ideally we could grant/deny access to the
> > MAC address on a case-by-case basis without blocking the entire
> > RTM_NEWLINK message type which contains a lot of other useful
> > information. This can be achieved using a new LSM hook on the netlink
> > message receive path. Using this new hook, individual LSMs can select
> > which processes are allowed access to the real MAC, otherwise a
> > default value of zeros is returned. Offloading access control
> > decisions like this to an LSM is convenient because it preserves the
> > status quo for most Linux users while giving the various LSMs
> > flexibility to make finer grained decisions on access to sensitive
> > data based on policy.
>
> Is the MAC address the only bit of skb data that you might
> want to control with MAC? ( Sorry, couldn't help it ;) )
> Just musing, but might it make more sense to leave the core
> code unmodified and clear the MAC address in the skb inside
> the LSM? If you did it that way you could address any other
> data you want to control using the same hook. I would hate
> to see separate LSM hooks for each of several bits of data.
> On the other hand, I wouldn't want you to violate any layering
> policies in the networking code. That would be wrong.

I considered that approach, but having the LSM modifying the skb
like that without the networking code's knowledge did seem like a layering
violation, and fragile. It's also different than how LSM hooks typically
operate - generally they return decisions and the calling code is
responsible for taking appropriate action.

I'm currently only interested in the MAC, but this approach can be extended
to other fields. The selinux patch just splits up the read permission into two
levels, privileged and unprivileged which is consistent with how netlink
audit sockets are handled.



>
> >
> > [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>
> > ---
> >  include/linux/lsm_hooks.h |  8 ++++++++
> >  include/linux/security.h  |  6 ++++++
> >  net/core/rtnetlink.c      | 12 ++++++++++--
> >  security/security.c       |  5 +++++
> >  4 files changed, 29 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index df1318d85f7d..dfcb2e11ff43 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -728,6 +728,12 @@
> >   *
> >   * Security hooks for Netlink messaging.
> >   *
> > + * @netlink_receive
> > + *   Check permissions on a netlink message field before populating it.
> > + *   @sk associated sock of task receiving the message.
> > + *   @skb contains the sk_buff structure for the netlink message.
> > + *   Return 0 if the data should be included in the message.
> > + *
> >   * @netlink_send:
> >   *   Save security information for a netlink message so that permission
> >   *   checking can be performed when the message is processed.  The security
> > @@ -1673,6 +1679,7 @@ union security_list_options {
> >       int (*sem_semop)(struct kern_ipc_perm *perm, struct sembuf *sops,
> >                               unsigned nsops, int alter);
> >
> > +     int (*netlink_receive)(struct sock *sk, struct sk_buff *skb);
> >       int (*netlink_send)(struct sock *sk, struct sk_buff *skb);
> >
> >       void (*d_instantiate)(struct dentry *dentry, struct inode *inode);
> > @@ -1952,6 +1959,7 @@ struct security_hook_heads {
> >       struct hlist_head sem_associate;
> >       struct hlist_head sem_semctl;
> >       struct hlist_head sem_semop;
> > +     struct hlist_head netlink_receive;
> >       struct hlist_head netlink_send;
> >       struct hlist_head d_instantiate;
> >       struct hlist_head getprocattr;
> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index 5f7441abbf42..46b5af6de59e 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -382,6 +382,7 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
> >                        char **value);
> >  int security_setprocattr(const char *lsm, const char *name, void *value,
> >                        size_t size);
> > +int security_netlink_receive(struct sock *sk, struct sk_buff *skb);
> >  int security_netlink_send(struct sock *sk, struct sk_buff *skb);
> >  int security_ismaclabel(const char *name);
> >  int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
> > @@ -1162,6 +1163,11 @@ static inline int security_setprocattr(const char *lsm, char *name,
> >       return -EINVAL;
> >  }
> >
> > +static inline int security_netlink_receive(struct sock *sk, struct sk_buff *skb)
> > +{
> > +     return 0;
> > +}
> > +
> >  static inline int security_netlink_send(struct sock *sk, struct sk_buff *skb)
> >  {
> >       return 0;
> > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> > index 1ee6460f8275..7d69fcb8d22e 100644
> > --- a/net/core/rtnetlink.c
> > +++ b/net/core/rtnetlink.c
> > @@ -1650,8 +1650,16 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
> >               goto nla_put_failure;
> >
> >       if (dev->addr_len) {
> > -             if (nla_put(skb, IFLA_ADDRESS, dev->addr_len, dev->dev_addr) ||
> > -                 nla_put(skb, IFLA_BROADCAST, dev->addr_len, dev->broadcast))
> > +             if (skb->sk && security_netlink_receive(skb->sk, skb)) {
> > +                     if (!nla_reserve(skb, IFLA_ADDRESS, dev->addr_len))
> > +                             goto nla_put_failure;
> > +
> > +             } else {
> > +                     if (nla_put(skb, IFLA_ADDRESS, dev->addr_len,
> > +                                 dev->dev_addr))
> > +                             goto nla_put_failure;
> > +             }
> > +             if (nla_put(skb, IFLA_BROADCAST, dev->addr_len, dev->broadcast))
> >                       goto nla_put_failure;
> >       }
> >
> > diff --git a/security/security.c b/security/security.c
> > index 250ee2d76406..35c5929921b2 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -1861,6 +1861,11 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
> >       return -EINVAL;
> >  }
> >
> > +int security_netlink_receive(struct sock *sk, struct sk_buff *skb)
> > +{
> > +     return call_int_hook(netlink_receive, 0, sk, skb);
> > +}
> > +
> >  int security_netlink_send(struct sock *sk, struct sk_buff *skb)
> >  {
> >       return call_int_hook(netlink_send, 0, sk, skb);
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] rtnetlink: gate MAC address with an LSM hook
  2019-08-21 13:45 [PATCH 1/2] rtnetlink: gate MAC address with an LSM hook Jeff Vander Stoep
  2019-08-21 13:55 ` Jeffrey Vander Stoep
  2019-08-21 14:34 ` Casey Schaufler
@ 2019-08-22 23:19 ` David Miller
  2019-08-23 11:41   ` Jeffrey Vander Stoep
  2019-08-23  4:24 ` kbuild test robot
  3 siblings, 1 reply; 11+ messages in thread
From: David Miller @ 2019-08-22 23:19 UTC (permalink / raw)
  To: jeffv; +Cc: netdev, linux-security-module, selinux

From: Jeff Vander Stoep <jeffv@google.com>
Date: Wed, 21 Aug 2019 15:45:47 +0200

> MAC addresses are often considered 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_NEWLINK message type of a
> netlink route socket[2]. Ideally we could grant/deny access to the
> MAC address on a case-by-case basis without blocking the entire
> RTM_NEWLINK message type which contains a lot of other useful
> information. This can be achieved using a new LSM hook on the netlink
> message receive path. Using this new hook, individual LSMs can select
> which processes are allowed access to the real MAC, otherwise a
> default value of zeros is returned. Offloading access control
> decisions like this to an LSM is convenient because it preserves the
> status quo for most Linux users while giving the various LSMs
> flexibility to make finer grained decisions on access to sensitive
> data based on policy.
> 
> [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>

I'm sure the MAC address will escape into userspace via other means,
dumping pieces of networking config in other contexts, etc.  I mean,
if I can get a link dump, I can dump the neighbor table as well.

I kinda think this is all very silly whack-a-mole kind of stuff, to
be quite honest.

And like others have said, tomorrow you'll be like "oh crap, we should
block X too" and we'll get another hook, another config knob, another
rulset update, etc.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] rtnetlink: gate MAC address with an LSM hook
  2019-08-21 13:45 [PATCH 1/2] rtnetlink: gate MAC address with an LSM hook Jeff Vander Stoep
                   ` (2 preceding siblings ...)
  2019-08-22 23:19 ` David Miller
@ 2019-08-23  4:24 ` kbuild test robot
  3 siblings, 0 replies; 11+ messages in thread
From: kbuild test robot @ 2019-08-23  4:24 UTC (permalink / raw)
  To: Jeff Vander Stoep
  Cc: kbuild-all, netdev, linux-security-module, selinux, Jeff Vander Stoep

[-- Attachment #1: Type: text/plain, Size: 13961 bytes --]

Hi Jeff,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[cannot apply to v5.3-rc5 next-20190822]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jeff-Vander-Stoep/rtnetlink-gate-MAC-address-with-an-LSM-hook/20190823-071253
reproduce: make htmldocs

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   Warning: The Sphinx 'sphinx_rtd_theme' HTML theme was not found. Make sure you have the theme installed to produce pretty HTML output. Falling back to the default theme.
   WARNING: dot(1) not found, for better output quality install graphviz from http://www.graphviz.org
   WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org)
   include/linux/w1.h:272: warning: Function parameter or member 'of_match_table' not described in 'w1_family'
   include/linux/lsm_hooks.h:1818: warning: Function parameter or member 'quotactl' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1818: warning: Function parameter or member 'quota_on' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1818: warning: Function parameter or member 'sb_free_mnt_opts' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1818: warning: Function parameter or member 'sb_eat_lsm_opts' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1818: warning: Function parameter or member 'sb_kern_mount' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1818: warning: Function parameter or member 'sb_show_options' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1818: warning: Function parameter or member 'sb_add_mnt_opt' not described in 'security_list_options'
>> include/linux/lsm_hooks.h:1818: warning: Function parameter or member 'netlink_receive' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1818: warning: Function parameter or member 'd_instantiate' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1818: warning: Function parameter or member 'getprocattr' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1818: warning: Function parameter or member 'setprocattr' not described in 'security_list_options'
   lib/genalloc.c:1: warning: 'gen_pool_add_virt' not found
   lib/genalloc.c:1: warning: 'gen_pool_alloc' not found
   lib/genalloc.c:1: warning: 'gen_pool_free' not found
   lib/genalloc.c:1: warning: 'gen_pool_alloc_algo' not found
   include/linux/i2c.h:337: warning: Function parameter or member 'init_irq' not described in 'i2c_client'
   fs/direct-io.c:258: warning: Excess function parameter 'offset' description in 'dio_complete'
   fs/libfs.c:496: warning: Excess function parameter 'available' description in 'simple_write_end'
   fs/posix_acl.c:647: warning: Function parameter or member 'inode' not described in 'posix_acl_update_mode'
   fs/posix_acl.c:647: warning: Function parameter or member 'mode_p' not described in 'posix_acl_update_mode'
   fs/posix_acl.c:647: warning: Function parameter or member 'acl' not described in 'posix_acl_update_mode'
   include/linux/spi/spi.h:190: warning: Function parameter or member 'driver_override' not described in 'spi_device'
   drivers/usb/typec/bus.c:1: warning: 'typec_altmode_unregister_driver' not found
   drivers/usb/typec/bus.c:1: warning: 'typec_altmode_register_driver' not found
   drivers/usb/typec/class.c:1: warning: 'typec_altmode_register_notifier' not found
   drivers/usb/typec/class.c:1: warning: 'typec_altmode_unregister_notifier' not found
   include/linux/regulator/machine.h:196: warning: Function parameter or member 'max_uV_step' not described in 'regulation_constraints'
   include/linux/regulator/driver.h:223: warning: Function parameter or member 'resume' not described in 'regulator_ops'
   include/linux/input/sparse-keymap.h:43: warning: Function parameter or member 'sw' not described in 'key_entry'
   include/linux/skbuff.h:893: warning: Function parameter or member 'dev_scratch' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'list' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'ip_defrag_offset' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'skb_mstamp_ns' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member '__cloned_offset' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'head_frag' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member '__pkt_type_offset' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'encapsulation' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'encap_hdr_csum' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'csum_valid' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member '__pkt_vlan_present_offset' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'vlan_present' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'csum_complete_sw' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'csum_level' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'inner_protocol_type' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'remcsum_offload' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'sender_cpu' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'reserved_tailroom' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'inner_ipproto' not described in 'sk_buff'
   include/net/sock.h:233: warning: Function parameter or member 'skc_addrpair' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_portpair' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_ipv6only' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_net_refcnt' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_v6_daddr' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_v6_rcv_saddr' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_cookie' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_listener' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_tw_dr' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_rcv_wnd' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_tw_rcv_nxt' not described in 'sock_common'
   include/net/sock.h:515: warning: Function parameter or member 'sk_rx_skb_cache' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'sk_wq_raw' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'tcp_rtx_queue' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'sk_tx_skb_cache' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'sk_route_forced_caps' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'sk_txtime_report_errors' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'sk_validate_xmit_skb' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'sk_bpf_storage' not described in 'sock'
   include/net/sock.h:2439: warning: Function parameter or member 'tcp_rx_skb_cache_key' not described in 'DECLARE_STATIC_KEY_FALSE'
   include/net/sock.h:2439: warning: Excess function parameter 'sk' description in 'DECLARE_STATIC_KEY_FALSE'
   include/net/sock.h:2439: warning: Excess function parameter 'skb' description in 'DECLARE_STATIC_KEY_FALSE'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'gso_partial_features' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'l3mdev_ops' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'xfrmdev_ops' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'tlsdev_ops' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'name_assign_type' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'ieee802154_ptr' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'mpls_ptr' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'xdp_prog' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'gro_flush_timeout' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'nf_hooks_ingress' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member '____cacheline_aligned_in_smp' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'qdisc_hash' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'xps_cpus_map' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'xps_rxqs_map' not described in 'net_device'
   include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising' not described in 'phylink_link_state'
   include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising' not described in 'phylink_link_state'
   drivers/net/phy/phylink.c:595: warning: Function parameter or member 'config' not described in 'phylink_create'
   drivers/net/phy/phylink.c:595: warning: Excess function parameter 'ndev' description in 'phylink_create'
   include/net/mac80211.h:2006: warning: Function parameter or member 'txpwr' not described in 'ieee80211_sta'
   mm/util.c:1: warning: 'get_user_pages_fast' not found
   mm/slab.c:4215: warning: Function parameter or member 'objp' not described in '__ksize'
   include/net/cfg80211.h:1092: warning: Function parameter or member 'txpwr' not described in 'station_parameters'
   include/net/mac80211.h:4043: warning: Function parameter or member 'sta_set_txpwr' not described in 'ieee80211_ops'
   drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c:142: warning: Function parameter or member 'blockable' not described in 'amdgpu_mn_read_lock'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:347: warning: cannot understand function prototype: 'struct amdgpu_vm_pt_cursor '
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:348: warning: cannot understand function prototype: 'struct amdgpu_vm_pt_cursor '
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:494: warning: Function parameter or member 'start' not described in 'amdgpu_vm_pt_first_dfs'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'adev' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'vm' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'start' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'cursor' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'entry' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:823: warning: Function parameter or member 'level' not described in 'amdgpu_vm_bo_param'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'params' not described in 'amdgpu_vm_update_flags'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'bo' not described in 'amdgpu_vm_update_flags'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'level' not described in 'amdgpu_vm_update_flags'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'pe' not described in 'amdgpu_vm_update_flags'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'addr' not described in 'amdgpu_vm_update_flags'

vim +1818 include/linux/lsm_hooks.h

3c4ed7bdf5997d Casey Schaufler 2015-05-02 @1818  

:::::: The code at line 1818 was first introduced by commit
:::::: 3c4ed7bdf5997d8020cbb8d4abbef2fcfb9f1284 LSM: Split security.h

:::::: TO: Casey Schaufler <casey@schaufler-ca.com>
:::::: CC: James Morris <james.l.morris@oracle.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 7282 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] rtnetlink: gate MAC address with an LSM hook
  2019-08-22 23:19 ` David Miller
@ 2019-08-23 11:41   ` Jeffrey Vander Stoep
  2019-08-23 21:41     ` David Miller
  2019-08-27 20:47     ` Paul Moore
  0 siblings, 2 replies; 11+ messages in thread
From: Jeffrey Vander Stoep @ 2019-08-23 11:41 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, LSM List, selinux

On Fri, Aug 23, 2019 at 1:19 AM David Miller <davem@davemloft.net> wrote:
>
> From: Jeff Vander Stoep <jeffv@google.com>
> Date: Wed, 21 Aug 2019 15:45:47 +0200
>
> > MAC addresses are often considered 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_NEWLINK message type of a
> > netlink route socket[2]. Ideally we could grant/deny access to the
> > MAC address on a case-by-case basis without blocking the entire
> > RTM_NEWLINK message type which contains a lot of other useful
> > information. This can be achieved using a new LSM hook on the netlink
> > message receive path. Using this new hook, individual LSMs can select
> > which processes are allowed access to the real MAC, otherwise a
> > default value of zeros is returned. Offloading access control
> > decisions like this to an LSM is convenient because it preserves the
> > status quo for most Linux users while giving the various LSMs
> > flexibility to make finer grained decisions on access to sensitive
> > data based on policy.
> >
> > [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>
>
> I'm sure the MAC address will escape into userspace via other means,
> dumping pieces of networking config in other contexts, etc.  I mean,
> if I can get a link dump, I can dump the neighbor table as well.

These are already gated by existing LSM hooks and capability checks.
They are not allowed on mandatory access control systems unless explicitly
granted.

>
> I kinda think this is all very silly whack-a-mole kind of stuff, to
> be quite honest.

We evaluated mechanisms for the MAC to reach unprivileged apps.
A number of researchers have published on this as well such as:
https://www.usenix.org/conference/usenixsecurity19/presentation/reardon

Three "leaks" were identified, two have already been fixed.
-ioctl(SIOCGIFHWADDR). Fixed using finer grained LSM checks
on socket ioctls (similar to this change).
-IPv6 IP addresses. Fixed by no longer including the MAC as part
of the IP address.
-RTM_NEWLINK netlink route messages. The last mole to be whacked.

>
> And like others have said, tomorrow you'll be like "oh crap, we should
> block X too" and we'll get another hook, another config knob, another
> rulset update, etc.

This seems like an issue inherent with permissions/capabilities. I don’t
think we should abandon the concept of permissions because someone
can forget to add a check.  Likewise, if someone adds new code to the
kernel and omits a capable(CAP_NET_*) check, I would expect it to be
fixed like any other bug without the idea of capability checks being tossed
out.

We need to do something because this information is being abused. Any
recommendations? This seemed like the simplest approach, but I can
definitely appreciate that it has downsides.

I could make this really generic by adding a single hook to the end of
sock_msgrecv() which would allow an LSM to modify the message to omit
the MAC address and any other information that we deem as sensitive in the
future. Basically what Casey was suggesting. Thoughts on that approach?

Thanks for your help on this.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] rtnetlink: gate MAC address with an LSM hook
  2019-08-23 11:41   ` Jeffrey Vander Stoep
@ 2019-08-23 21:41     ` David Miller
  2019-08-27 20:47     ` Paul Moore
  1 sibling, 0 replies; 11+ messages in thread
From: David Miller @ 2019-08-23 21:41 UTC (permalink / raw)
  To: jeffv; +Cc: netdev, linux-security-module, selinux

From: Jeffrey Vander Stoep <jeffv@google.com>
Date: Fri, 23 Aug 2019 13:41:38 +0200

> I could make this really generic by adding a single hook to the end of
> sock_msgrecv() which would allow an LSM to modify the message to omit
> the MAC address and any other information that we deem as sensitive in the
> future. Basically what Casey was suggesting. Thoughts on that approach?

Editing the SKB in place is generally frowned upon, and it could be cloned
and in used by other code paths even, so would need to be copied or COW'd.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] rtnetlink: gate MAC address with an LSM hook
  2019-08-23 11:41   ` Jeffrey Vander Stoep
  2019-08-23 21:41     ` David Miller
@ 2019-08-27 20:47     ` Paul Moore
  2019-08-29  7:45       ` Michal Kubecek
  1 sibling, 1 reply; 11+ messages in thread
From: Paul Moore @ 2019-08-27 20:47 UTC (permalink / raw)
  To: Jeffrey Vander Stoep; +Cc: David Miller, netdev, LSM List, selinux

On Fri, Aug 23, 2019 at 7:41 AM Jeffrey Vander Stoep <jeffv@google.com> wrote:
> On Fri, Aug 23, 2019 at 1:19 AM David Miller <davem@davemloft.net> wrote:
> > From: Jeff Vander Stoep <jeffv@google.com>
> > Date: Wed, 21 Aug 2019 15:45:47 +0200
> >
> > > MAC addresses are often considered 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_NEWLINK message type of a
> > > netlink route socket[2]. Ideally we could grant/deny access to the
> > > MAC address on a case-by-case basis without blocking the entire
> > > RTM_NEWLINK message type which contains a lot of other useful
> > > information. This can be achieved using a new LSM hook on the netlink
> > > message receive path. Using this new hook, individual LSMs can select
> > > which processes are allowed access to the real MAC, otherwise a
> > > default value of zeros is returned. Offloading access control
> > > decisions like this to an LSM is convenient because it preserves the
> > > status quo for most Linux users while giving the various LSMs
> > > flexibility to make finer grained decisions on access to sensitive
> > > data based on policy.
> > >
> > > [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>
> >
> > I'm sure the MAC address will escape into userspace via other means,
> > dumping pieces of networking config in other contexts, etc.  I mean,
> > if I can get a link dump, I can dump the neighbor table as well.
>
> These are already gated by existing LSM hooks and capability checks.
> They are not allowed on mandatory access control systems unless explicitly
> granted.
>
> > I kinda think this is all very silly whack-a-mole kind of stuff, to
> > be quite honest.
>
> We evaluated mechanisms for the MAC to reach unprivileged apps.
> A number of researchers have published on this as well such as:
> https://www.usenix.org/conference/usenixsecurity19/presentation/reardon
>
> Three "leaks" were identified, two have already been fixed.
> -ioctl(SIOCGIFHWADDR). Fixed using finer grained LSM checks
> on socket ioctls (similar to this change).
> -IPv6 IP addresses. Fixed by no longer including the MAC as part
> of the IP address.
> -RTM_NEWLINK netlink route messages. The last mole to be whacked.
>
> > And like others have said, tomorrow you'll be like "oh crap, we should
> > block X too" and we'll get another hook, another config knob, another
> > rulset update, etc.
>
> This seems like an issue inherent with permissions/capabilities. I don’t
> think we should abandon the concept of permissions because someone
> can forget to add a check.  Likewise, if someone adds new code to the
> kernel and omits a capable(CAP_NET_*) check, I would expect it to be
> fixed like any other bug without the idea of capability checks being tossed
> out.
>
> We need to do something because this information is being abused. Any
> recommendations? This seemed like the simplest approach, but I can
> definitely appreciate that it has downsides.
>
> I could make this really generic by adding a single hook to the end of
> sock_msgrecv() which would allow an LSM to modify the message to omit
> the MAC address and any other information that we deem as sensitive in the
> future. Basically what Casey was suggesting. Thoughts on that approach?

I apologize for the delay in responding; I'm blaming LSS-NA travel.

I'm also not a big fan of inserting the hook in rtnl_fill_ifinfo(); as
presented it is way too specific for a LSM hook for me to be happy.
However, I do agree that giving the LSMs some control over netlink
messages makes sense.  As others have pointed out, it's all a matter
of where to place the hook.

If we only care about netlink messages which leverage nlattrs I
suppose one option that I haven't seen mentioned would be to place a
hook in nla_put().  While it is a bit of an odd place for a hook, it
would allow the LSM easy access to the skb and attribute type to make
decisions, and all of the callers should already be checking the
return code (although we would need to verify this).  One notable
drawback (not the only one) is that the hook is going to get hit
multiple times for each message.

--
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] rtnetlink: gate MAC address with an LSM hook
  2019-08-27 20:47     ` Paul Moore
@ 2019-08-29  7:45       ` Michal Kubecek
  2019-08-30 21:46         ` Paul Moore
  0 siblings, 1 reply; 11+ messages in thread
From: Michal Kubecek @ 2019-08-29  7:45 UTC (permalink / raw)
  To: netdev; +Cc: Paul Moore, Jeffrey Vander Stoep, David Miller, LSM List, selinux

On Tue, Aug 27, 2019 at 04:47:04PM -0400, Paul Moore wrote:
> 
> I'm also not a big fan of inserting the hook in rtnl_fill_ifinfo(); as
> presented it is way too specific for a LSM hook for me to be happy.
> However, I do agree that giving the LSMs some control over netlink
> messages makes sense.  As others have pointed out, it's all a matter
> of where to place the hook.
> 
> If we only care about netlink messages which leverage nlattrs I
> suppose one option that I haven't seen mentioned would be to place a
> hook in nla_put().  While it is a bit of an odd place for a hook, it
> would allow the LSM easy access to the skb and attribute type to make
> decisions, and all of the callers should already be checking the
> return code (although we would need to verify this).  One notable
> drawback (not the only one) is that the hook is going to get hit
> multiple times for each message.

For most messages, "multiple times" would mean tens, for many even
hundreds of calls. For each, you would have to check corresponding
socket (and possibly also genetlink header) to see which netlink based
protocol it is and often even parse existing part of the message to get
the context (because the same numeric attribute type can mean something
completely different if it appears in a nested attribute).

Also, nla_put() (or rather __nla_put()) is not used for all attributes,
one may also use nla_reserve() and then compose the attribute date in
place.

Michal Kubecek

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] rtnetlink: gate MAC address with an LSM hook
  2019-08-29  7:45       ` Michal Kubecek
@ 2019-08-30 21:46         ` Paul Moore
  0 siblings, 0 replies; 11+ messages in thread
From: Paul Moore @ 2019-08-30 21:46 UTC (permalink / raw)
  To: Michal Kubecek
  Cc: netdev, Jeffrey Vander Stoep, David Miller, LSM List, selinux

On Thu, Aug 29, 2019 at 3:45 AM Michal Kubecek <mkubecek@suse.cz> wrote:
> On Tue, Aug 27, 2019 at 04:47:04PM -0400, Paul Moore wrote:
> >
> > I'm also not a big fan of inserting the hook in rtnl_fill_ifinfo(); as
> > presented it is way too specific for a LSM hook for me to be happy.
> > However, I do agree that giving the LSMs some control over netlink
> > messages makes sense.  As others have pointed out, it's all a matter
> > of where to place the hook.
> >
> > If we only care about netlink messages which leverage nlattrs I
> > suppose one option that I haven't seen mentioned would be to place a
> > hook in nla_put().  While it is a bit of an odd place for a hook, it
> > would allow the LSM easy access to the skb and attribute type to make
> > decisions, and all of the callers should already be checking the
> > return code (although we would need to verify this).  One notable
> > drawback (not the only one) is that the hook is going to get hit
> > multiple times for each message.
>
> For most messages, "multiple times" would mean tens, for many even
> hundreds of calls. For each, you would have to check corresponding
> socket (and possibly also genetlink header) to see which netlink based
> protocol it is and often even parse existing part of the message to get
> the context (because the same numeric attribute type can mean something
> completely different if it appears in a nested attribute).
>
> Also, nla_put() (or rather __nla_put()) is not used for all attributes,
> one may also use nla_reserve() and then compose the attribute date in
> place.

I never said it was a great idea, just an idea ;)

Honestly I'm just trying to spur some discussion on this so we can
hopefully arrive at a solution which allows a LSM to control kernel
generated netlink messages that we can all accept.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-08-30 21:46 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-21 13:45 [PATCH 1/2] rtnetlink: gate MAC address with an LSM hook Jeff Vander Stoep
2019-08-21 13:55 ` Jeffrey Vander Stoep
2019-08-21 14:34 ` Casey Schaufler
2019-08-21 14:52   ` Jeffrey Vander Stoep
2019-08-22 23:19 ` David Miller
2019-08-23 11:41   ` Jeffrey Vander Stoep
2019-08-23 21:41     ` David Miller
2019-08-27 20:47     ` Paul Moore
2019-08-29  7:45       ` Michal Kubecek
2019-08-30 21:46         ` Paul Moore
2019-08-23  4:24 ` kbuild test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).