Re: [PATCH RFC] LSM, net: Add SO_PEERCONTEXT for peer LSM data

[Paul Moore <paul@paul-moore.com>]

On May 13, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote:
> 
> We recently introduced system calls to access process attributes that
> are used by Linux Security Modules (LSM). An important aspect of these
> system calls is that they provide the LSM attribute data in a format
> that identifies the LSM to which the data applies. Another aspect is that
> it can be used to provide multiple instances of the attribute for the
> case where more than one LSM supplies the attribute.
> 
> We wish to take advantage of this format for data about network peers.
> The existing mechanism, SO_PEERSEC, provides peer security data as a
> text string. This is sufficient when the LSM providing the information
> is known by the user of SO_PEERSEC, and there is only one LSM providing
> the information. It fails, however, if the user does not know which
> LSM is providing the information.
> 
> Discussions about extending SO_PEERSEC to accomodate either the new

Spelling nitpick -> "accommodate" :)

> format or some other encoding scheme invariably lead to the conclusion
> that doing so would lead to tears. Hence, we introduce SO_PEERCONTEXT
> which uses the same API data as the LSM system calls.
> 
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
>  arch/alpha/include/uapi/asm/socket.h  |  1 +
>  arch/mips/include/uapi/asm/socket.h   |  1 +
>  arch/parisc/include/uapi/asm/socket.h |  1 +
>  arch/sparc/include/uapi/asm/socket.h  |  1 +
>  include/linux/lsm_hook_defs.h         |  2 +
>  include/linux/security.h              | 18 ++++++++
>  include/uapi/asm-generic/socket.h     |  1 +
>  net/core/sock.c                       |  4 ++
>  security/apparmor/lsm.c               | 39 ++++++++++++++++
>  security/security.c                   | 86 +++++++++++++++++++++++++++++++++++
>  security/selinux/hooks.c              | 35 ++++++++++++++
>  security/smack/smack_lsm.c            | 25 ++++++++++
>  12 files changed, 214 insertions(+)

...

> diff --git a/include/uapi/asm-generic/socket.h b/include/uapi/asm-generic/socket.h
> index 8ce8a39a1e5f..e0166ff53670 100644
> --- a/include/uapi/asm-generic/socket.h
> +++ b/include/uapi/asm-generic/socket.h
> @@ -134,6 +134,7 @@
>  
>  #define SO_PASSPIDFD		76
>  #define SO_PEERPIDFD		77
> +#define SO_PEERCONTEXT		78

Bikeshed time ... how about SO_PEERLSMCTX since we are returning a
lsm_ctx struct?

> diff --git a/security/security.c b/security/security.c
> index e387614cb054..fd4919c28e8f 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -874,6 +874,64 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len,
>  	return rc;
>  }
>  
> +/**
> + * lsm_fill_socket_ctx - Fill a socket lsm_ctx structure
> + * @optval: a socket LSM context to be filled
> + * @optlen: uctx size

"@optlen: @optval size"

> + * @val: the new LSM context value
> + * @val_len: the size of the new LSM context value
> + * @id: LSM id
> + * @flags: LSM defined flags
> + *
> + * Fill all of the fields in a lsm_ctx structure.  If @optval is NULL
> + * simply calculate the required size to output via @optlen and return
> + * success.
> + *
> + * Returns 0 on success, -E2BIG if userspace buffer is not large enough,
> + * -EFAULT on a copyout error, -ENOMEM if memory can't be allocated.
> + */
> +int lsm_fill_socket_ctx(sockptr_t optval, sockptr_t optlen, void *val,
> +			size_t val_len, u64 id, u64 flags)
> +{
> +	struct lsm_ctx *nctx = NULL;
> +	unsigned int nctx_len;
> +	int loptlen;

u32?

> +	int rc = 0;
> +
> +	if (copy_from_sockptr(&loptlen, optlen, sizeof(int)))
> +		return -EFAULT;

It seems the current guidance prefers copy_safe_from_sockptr(), see
the note in include/linux/sockptr.h. 

> +	nctx_len = ALIGN(struct_size(nctx, ctx, val_len), sizeof(void *));
> +	if (nctx_len > loptlen && !sockptr_is_null(optval))
> +		rc = -E2BIG;

Why do we care if @optval is NULL or not?  We are in a -E2BIG state,
we're not copying anything into @optval anyway.  In fact, why are we
doing the @rc check below?  Do it here like we do in lsm_fill_user_ctx().

  if (nctx_len > loptlen) {
    rc = -E2BIG;
    goto out;
  }

> +	/* no buffer - return success/0 and set @uctx_len to the req size */

"... set @opt_len ... "

> +	if (sockptr_is_null(optval) || rc)
> +		goto out;

Do the @rc check above, not here.

> +	nctx = kzalloc(nctx_len, GFP_KERNEL);
> +	if (!nctx) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +	nctx->id = id;
> +	nctx->flags = flags;
> +	nctx->len = nctx_len;
> +	nctx->ctx_len = val_len;
> +	memcpy(nctx->ctx, val, val_len);
> +
> +	if (copy_to_sockptr(optval, nctx, nctx_len))
> +		rc = -EFAULT;

This is always going to copy to the start of @optval which means we
are going to keep overwriting previous values in the multi-LSM case.
I think we likely want copy_to_sockptr_offset(), or similar.  See my
comment in security_socket_getpeerctx_stream().

> +	kfree(nctx);
> +out:
> +	if (copy_to_sockptr(optlen, &nctx_len, sizeof(int)))
> +		rc = -EFAULT;
> +
> +	return rc;
> +}
> +
> +
>  /*
>   * The default value of the LSM hook is defined in linux/lsm_hook_defs.h and
>   * can be accessed with:
> @@ -4743,6 +4801,34 @@ int security_socket_getpeersec_stream(struct socket *sock, sockptr_t optval,
>  	return LSM_RET_DEFAULT(socket_getpeersec_stream);
>  }
>  
> +/**
> + * security_socket_getpeerctx_stream() - Get the remote peer label
> + * @sock: socket
> + * @optval: destination buffer
> + * @optlen: size of peer label copied into the buffer
> + * @len: maximum size of the destination buffer
> + *
> + * This hook allows the security module to provide peer socket security state
> + * for unix or connected tcp sockets to userspace via getsockopt
> + * SO_GETPEERCONTEXT.  For tcp sockets this can be meaningful if the socket
> + * is associated with an ipsec SA.
> + *
> + * Return: Returns 0 if all is well, otherwise, typical getsockopt return
> + *         values.
> + */
> +int security_socket_getpeerctx_stream(struct socket *sock, sockptr_t optval,
> +				      sockptr_t optlen, unsigned int len)
> +{
> +	struct security_hook_list *hp;
> +
> +	hlist_for_each_entry(hp, &security_hook_heads.socket_getpeerctx_stream,
> +			     list)
> +		return hp->hook.socket_getpeerctx_stream(sock, optval, optlen,
> +							 len);
> +
> +	return LSM_RET_DEFAULT(socket_getpeerctx_stream);
> +}

Don't we need the same magic that we have in security_getselfattr() to
handle the multi-LSM case?

--
paul-moore.com