Re: [PATCH v7 26/28] NET: Add SO_PEERCONTEXT for multiple LSMs

[Kees Cook <keescook@chromium.org>]

On Wed, Aug 07, 2019 at 12:44:08PM -0700, Casey Schaufler wrote:
> The getsockopt SO_PEERSEC provides the LSM based security
> information for a single module, but for reasons of backward
> compatibility cannot include the information for multiple
> modules. A new option SO_PEERCONTEXT is added to report the
> security "context" of multiple modules using a "compound" format
> 
> 	lsm1\0value\0lsm2\0value\0
> 
> This is expected to be used by system services, including dbus-daemon.
> The exact format of a compound context has been the subject of
> considerable debate. This format was suggested by Simon McVittie,
> a dbus maintainer with a significant stake in the format being
> uasable.
> 
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> [...]
> diff --git a/security/security.c b/security/security.c
> index 2f4a430a1126..0ea7ee27e331 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2117,8 +2117,8 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
>  	hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
>  		if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
>  			continue;
> -		if (lsm == NULL && *display != LSMBLOB_INVALID &&
> -		    *display != hp->lsmid->slot)
> +		if (lsm == NULL && display != NULL &&
> +		    *display != LSMBLOB_INVALID && *display != hp->lsmid->slot)
>  			continue;
>  		return hp->hook.setprocattr(name, value, size);
>  	}
> @@ -2342,17 +2342,91 @@ 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 __user *optlen, unsigned len,
> +				      int display)
>  {
> -	int display = lsm_task_display(current);
>  	struct security_hook_list *hp;
> +	char *final = NULL;
> +	char *cp;
> +	char *tp;
> +	int rc = 0;
> +	unsigned finallen = 0;
> +	unsigned llen;
> +	unsigned clen = 0;
> +	unsigned tlen;

Please move the case-specific variables into the case scope, like (and
expand type names):

	case LSMBLOB_COMPOUND: {
		unsigned int clen ...;

> +
> +	switch (display) {
> +	case LSMBLOB_DISPLAY:
> +		rc = -ENOPROTOOPT;
> +		display = lsm_task_display(current);
> +		hlist_for_each_entry(hp,
> +				&security_hook_heads.socket_getpeersec_stream,
> +				list)
> +			if (display == LSMBLOB_INVALID ||
> +			    display == hp->lsmid->slot) {
> +				rc = hp->hook.socket_getpeersec_stream(sock,
> +							&final, &finallen, len);
> +				break;
> +			}
> +		break;
> +	case LSMBLOB_COMPOUND:
> +		/*
> +		 * A compound context, in the form [lsm\0value\0]...
> +		 */
> +		hlist_for_each_entry(hp,
> +				&security_hook_heads.socket_getpeersec_stream,
> +				list) {
> +			rc = hp->hook.socket_getpeersec_stream(sock, &cp, &clen,
> +							       len);

Is passing "len" here useful at all? It's kind of a lie, but nothing
else wouldn't also be a lie. :)

> +			if (rc == -EINVAL || rc == -ENOPROTOOPT) {
> +				rc = 0;
> +				continue;
> +			}
> +			if (rc) {
> +				kfree(final);
> +				return rc;
> +			}
> +			/*
> +			 * Don't propogate trailing nul bytes.

typo: propagate

> +			 */
> +			clen = strnlen(cp, clen) + 1;
> +			llen = strlen(hp->lsmid->lsm) + 1;
> +			tlen = llen + clen;
> +			if (final)
> +				tlen += finallen;

You can drop the "if (final)" since finallen is initialized to 0.

> +			tp = kzalloc(tlen, GFP_KERNEL);

I'm not a huge fan of "c", "l", and "t" prefixes -- can you just make
these a little more self-documenting? cp and clen could be value and
value_len. llen could be lsm_name_len. tp and tlen could be tuple and
tuple_len. (And maybe final and finallen could be compound and
compound_len?)

> +			if (tp == NULL) {
> +				kfree(cp);
> +				kfree(final);
> +				return -ENOMEM;
> +			}
> +			if (final)
> +				memcpy(tp, final, finallen);
> +			memcpy(tp + finallen, hp->lsmid->lsm, llen);
> +			memcpy(tp + finallen + llen, cp, clen);
> +			kfree(cp);
> +			if (final)
> +				kfree(final);

Just kfree(final) is safe here -- kfree(NULL) is valid.

> +			final = tp;
> +			finallen = tlen;
> +		}
> +		if (final == NULL)
> +			return -EINVAL;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
>  
> -	hlist_for_each_entry(hp, &security_hook_heads.socket_getpeersec_stream,
> -			     list)
> -		if (display == LSMBLOB_INVALID || display == hp->lsmid->slot)
> -			return hp->hook.socket_getpeersec_stream(sock, optval,
> -								 optlen, len);
> -	return -ENOPROTOOPT;
> +	if (finallen > len)
> +		rc = -ERANGE;
> +	else if (copy_to_user(optval, final, finallen))
> +		rc = -EFAULT;
> +
> +	if (put_user(finallen, optlen))
> +		rc = -EFAULT;
> +
> +	kfree(final);
> +	return rc;
>  }

Otherwise, looks good.

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

>  
>  int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb,
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index fcad2e3432d2..5e7d61754798 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4923,10 +4923,8 @@ 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 int len)
> +static int selinux_socket_getpeersec_stream(struct socket *sock, char **optval,
> +					    int *optlen, unsigned int len)
>  {
>  	int err = 0;
>  	char *scontext;
> @@ -4946,18 +4944,12 @@ static int selinux_socket_getpeersec_stream(struct socket *sock,
>  	if (err)
>  		return err;
>  
> -	if (scontext_len > len) {
> +	if (scontext_len > len)
>  		err = -ERANGE;
> -		goto out_len;
> -	}
> -
> -	if (copy_to_user(optval, scontext, scontext_len))
> -		err = -EFAULT;
> +	else
> +		*optval = scontext;
>  
> -out_len:
> -	if (put_user(scontext_len, optlen))
> -		err = -EFAULT;
> -	kfree(scontext);
> +	*optlen = scontext_len;
>  	return err;
>  }
>  
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 7a30b8692b1e..40c75205a914 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3919,28 +3919,29 @@ 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)
> +static int smack_socket_getpeersec_stream(struct socket *sock, char **optval,
> +					  int *optlen, unsigned len)
>  {
> -	struct socket_smack *ssp;
> -	char *rcp = "";
> -	int slen = 1;
> +	struct socket_smack *ssp = smack_sock(sock->sk);
> +	char *rcp;
> +	int slen;
>  	int rc = 0;
>  
> -	ssp = smack_sock(sock->sk);
> -	if (ssp->smk_packet != NULL) {
> -		rcp = ssp->smk_packet->smk_known;
> -		slen = strlen(rcp) + 1;
> +	if (ssp->smk_packet == NULL) {
> +		*optlen = 0;
> +		return -EINVAL;
>  	}
>  
> +	rcp = ssp->smk_packet->smk_known;
> +	slen = strlen(rcp) + 1;
>  	if (slen > len)
>  		rc = -ERANGE;
> -	else if (copy_to_user(optval, rcp, slen) != 0)
> -		rc = -EFAULT;
> -
> -	if (put_user(slen, optlen) != 0)
> -		rc = -EFAULT;
> +	else {
> +		*optval = kstrdup(rcp, GFP_KERNEL);
> +		if (*optval == NULL)
> +			rc = -ENOMEM;
> +	}
> +	*optlen = slen;
>  
>  	return rc;
>  }
> -- 
> 2.20.1
> 

-- 
Kees Cook