Re: [PATCH] lsm: make security_socket_getpeersec_stream() sockptr_t safe

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/10/2022 2:58 PM, Paul Moore wrote:
> Commit 4ff09db1b79b ("bpf: net: Change sk_getsockopt() to take the
> sockptr_t argument") made it possible to call sk_getsockopt()
> with both user and kernel address space buffers through the use of
> the sockptr_t type.  Unfortunately at the time of conversion the
> security_socket_getpeersec_stream() LSM hook was written to only
> accept userspace buffers, and in a desire to avoid having to change
> the LSM hook the commit author simply passed the sockptr_t's
> userspace buffer pointer.  Since the only sk_getsockopt() callers
> at the time of conversion which used kernel sockptr_t buffers did
> not allow SO_PEERSEC, and hence the
> security_socket_getpeersec_stream() hook, this was acceptable but
> also very fragile as future changes presented the possibility of
> silently passing kernel space pointers to the LSM hook.
>
> There are several ways to protect against this, including careful
> code review of future commits, but since relying on code review to
> catch bugs is a recipe for disaster and the upstream eBPF maintainer
> is "strongly against defensive programming", this patch updates the
> LSM hook, and all of the implementations to support sockptr_t and
> safely handle both user and kernel space buffers.
>
> Signed-off-by: Paul Moore <paul@xxxxxxxxxxxxxx>

Smack part looks ok, I haven't had the opportunity to test it.
Will do so as I crunch through the backlog.

Acked-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>

> ---
>  include/linux/lsm_hook_defs.h |    2 +-
>  include/linux/lsm_hooks.h     |    4 ++--
>  include/linux/security.h      |   11 +++++++----
>  net/core/sock.c               |    3 ++-
>  security/apparmor/lsm.c       |   29 +++++++++++++----------------
>  security/security.c           |    6 +++---
>  security/selinux/hooks.c      |   13 ++++++-------
>  security/smack/smack_lsm.c    |   19 ++++++++++---------
>  8 files changed, 44 insertions(+), 43 deletions(-)
>
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index ec119da1d89b4..6abde829b6e5e 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -302,7 +302,7 @@ LSM_HOOK(int, 0, socket_setsockopt, struct socket *sock, int level, int optname)
>  LSM_HOOK(int, 0, socket_shutdown, struct socket *sock, int how)
>  LSM_HOOK(int, 0, socket_sock_rcv_skb, struct sock *sk, struct sk_buff *skb)
>  LSM_HOOK(int, 0, socket_getpeersec_stream, struct socket *sock,
> -	 char __user *optval, int __user *optlen, unsigned len)
> +	 sockptr_t optval, sockptr_t optlen, unsigned int len)
>  LSM_HOOK(int, 0, socket_getpeersec_dgram, struct socket *sock,
>  	 struct sk_buff *skb, u32 *secid)
>  LSM_HOOK(int, 0, sk_alloc_security, struct sock *sk, int family, gfp_t priority)
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 4ec80b96c22e7..883f0f252f062 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -962,8 +962,8 @@
>   *	SO_GETPEERSEC.  For tcp sockets this can be meaningful if the
>   *	socket is associated with an ipsec SA.
>   *	@sock is the local socket.
> - *	@optval userspace memory where the security state is to be copied.
> - *	@optlen userspace int where the module should copy the actual length
> + *	@optval memory where the security state is to be copied.
> + *	@optlen memory where the module should copy the actual length
>   *	of the security state.
>   *	@len as input is the maximum length to copy to userspace provided
>   *	by the caller.
> diff --git a/include/linux/security.h b/include/linux/security.h
> index ca1b7109c0dbb..0e419c595cee5 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -31,6 +31,7 @@
>  #include <linux/err.h>
>  #include <linux/string.h>
>  #include <linux/mm.h>
> +#include <linux/sockptr.h>
>  
>  struct linux_binprm;
>  struct cred;
> @@ -1411,8 +1412,8 @@ int security_socket_getsockopt(struct socket *sock, int level, int optname);
>  int security_socket_setsockopt(struct socket *sock, int level, int optname);
>  int security_socket_shutdown(struct socket *sock, int how);
>  int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb);
> -int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> -				      int __user *optlen, unsigned len);
> +int security_socket_getpeersec_stream(struct socket *sock, sockptr_t optval,
> +				      sockptr_t optlen, unsigned int len);
>  int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid);
>  int security_sk_alloc(struct sock *sk, int family, gfp_t priority);
>  void security_sk_free(struct sock *sk);
> @@ -1548,8 +1549,10 @@ static inline int security_sock_rcv_skb(struct sock *sk,
>  	return 0;
>  }
>  
> -static inline int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> -						    int __user *optlen, unsigned len)
> +static inline int security_socket_getpeersec_stream(struct socket *sock,
> +						    sockptr_t optval,
> +						    sockptr_t optlen,
> +						    unsigned int len)
>  {
>  	return -ENOPROTOOPT;
>  }
> diff --git a/net/core/sock.c b/net/core/sock.c
> index eeb6cbac6f499..70064415349d6 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1793,7 +1793,8 @@ int sk_getsockopt(struct sock *sk, int level, int optname,
>  		break;
>  
>  	case SO_PEERSEC:
> -		return security_socket_getpeersec_stream(sock, optval.user, optlen.user, len);
> +		return security_socket_getpeersec_stream(sock,
> +							 optval, optlen, len);
>  
>  	case SO_MARK:
>  		v.val = sk->sk_mark;
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index f56070270c69d..89e84ef54e8eb 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1103,11 +1103,10 @@ static struct aa_label *sk_peer_label(struct sock *sk)
>   * Note: for tcp only valid if using ipsec or cipso on lan
>   */
>  static int apparmor_socket_getpeersec_stream(struct socket *sock,
> -					     char __user *optval,
> -					     int __user *optlen,
> +					     sockptr_t optval, sockptr_t optlen,
>  					     unsigned int len)
>  {
> -	char *name;
> +	char *name = NULL;
>  	int slen, error = 0;
>  	struct aa_label *label;
>  	struct aa_label *peer;
> @@ -1124,23 +1123,21 @@ static int apparmor_socket_getpeersec_stream(struct socket *sock,
>  	/* don't include terminating \0 in slen, it breaks some apps */
>  	if (slen < 0) {
>  		error = -ENOMEM;
> -	} else {
> -		if (slen > len) {
> -			error = -ERANGE;
> -		} else if (copy_to_user(optval, name, slen)) {
> -			error = -EFAULT;
> -			goto out;
> -		}
> -		if (put_user(slen, optlen))
> -			error = -EFAULT;
> -out:
> -		kfree(name);
> -
> +		goto done;
> +	}
> +	if (slen > len) {
> +		error = -ERANGE;
> +		goto done_len;
>  	}
>  
> +	if (copy_to_sockptr(optval, name, slen))
> +		error = -EFAULT;
> +done_len:
> +	if (copy_to_sockptr(optlen, &slen, sizeof(slen)))
> +		error = -EFAULT;
>  done:
>  	end_current_label_crit_section(label);
> -
> +	kfree(name);
>  	return error;
>  }
>  
> diff --git a/security/security.c b/security/security.c
> index 79d82cb6e4696..f27c885ee98db 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2267,11 +2267,11 @@ int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
>  }
>  EXPORT_SYMBOL(security_sock_rcv_skb);
>  
> -int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> -				      int __user *optlen, unsigned len)
> +int security_socket_getpeersec_stream(struct socket *sock, sockptr_t optval,
> +				      sockptr_t optlen, unsigned int len)
>  {
>  	return call_int_hook(socket_getpeersec_stream, -ENOPROTOOPT, sock,
> -				optval, optlen, len);
> +			     optval, optlen, len);
>  }
>  
>  int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f553c370397ee..0bdddeba90a6c 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5119,11 +5119,12 @@ static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
>  	return err;
>  }
>  
> -static int selinux_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> -					    int __user *optlen, unsigned len)
> +static int selinux_socket_getpeersec_stream(struct socket *sock,
> +					    sockptr_t optval, sockptr_t optlen,
> +					    unsigned int len)
>  {
>  	int err = 0;
> -	char *scontext;
> +	char *scontext = NULL;
>  	u32 scontext_len;
>  	struct sk_security_struct *sksec = sock->sk->sk_security;
>  	u32 peer_sid = SECSID_NULL;
> @@ -5139,17 +5140,15 @@ static int selinux_socket_getpeersec_stream(struct socket *sock, char __user *op
>  				      &scontext_len);
>  	if (err)
>  		return err;
> -
>  	if (scontext_len > len) {
>  		err = -ERANGE;
>  		goto out_len;
>  	}
>  
> -	if (copy_to_user(optval, scontext, scontext_len))
> +	if (copy_to_sockptr(optval, scontext, scontext_len))
>  		err = -EFAULT;
> -
>  out_len:
> -	if (put_user(scontext_len, optlen))
> +	if (copy_to_sockptr(optlen, &scontext_len, sizeof(scontext_len)))
>  		err = -EFAULT;
>  	kfree(scontext);
>  	return err;
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index b6306d71c9088..2bd7fadf7fb4c 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -4006,12 +4006,12 @@ static int smack_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
>   * returns zero on success, an error code otherwise
>   */
>  static int smack_socket_getpeersec_stream(struct socket *sock,
> -					  char __user *optval,
> -					  int __user *optlen, unsigned len)
> +					  sockptr_t optval, sockptr_t optlen,
> +					  unsigned int len)
>  {
>  	struct socket_smack *ssp;
>  	char *rcp = "";
> -	int slen = 1;
> +	u32 slen = 1;
>  	int rc = 0;
>  
>  	ssp = sock->sk->sk_security;
> @@ -4019,15 +4019,16 @@ static int smack_socket_getpeersec_stream(struct socket *sock,
>  		rcp = ssp->smk_packet->smk_known;
>  		slen = strlen(rcp) + 1;
>  	}
> -
> -	if (slen > len)
> +	if (slen > len) {
>  		rc = -ERANGE;
> -	else if (copy_to_user(optval, rcp, slen) != 0)
> -		rc = -EFAULT;
> +		goto out_len;
> +	}
>  
> -	if (put_user(slen, optlen) != 0)
> +	if (copy_to_sockptr(optval, rcp, slen))
> +		rc = -EFAULT;
> +out_len:
> +	if (copy_to_sockptr(optlen, &slen, sizeof(slen)))
>  		rc = -EFAULT;
> -
>  	return rc;
>  }
>  
>



[Index of Archives]     [Selinux Refpolicy]     [Linux SGX]     [Fedora Users]     [Fedora Desktop]     [Yosemite Photos]     [Yosemite Camping]     [Yosemite Campsites]     [KDE Users]     [Gnome Users]

  Powered by Linux