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" :)
> 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?
> 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
