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

[Casey Schaufler <casey@schaufler-ca.com>]

On 6/20/2024 2:05 PM, Paul Moore wrote:
> 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" :)

Thanks.

>> 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?

Sure.


>> 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"

Thank you.


>> + * @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?

Probably. I'll revise in line with your comment below.

>> +	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.

Always a good idea to follow guidance.

>> +	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;
>   }

That's a bit sloppy on my part. I'll clean it up.


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

Yes.

>> +	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.

The multiple LSM case isn't handled in this version. I don't want this
patch to depend on multiple LSM support.

> 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?

Yes. I would like to move this ahead independently of the multi-LSM support.
Putting the multi-LSM magic in is unnecessary and rather pointless until then.

> --
> paul-moore.com

Thank you for the review. Expect v2 before very long.