On 6/20/2024 2:05 PM, Paul Moore wrote:
> On May 13, 2024 Casey Schaufler <casey@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
>> ---
>> 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.
