Re: [PATCH RFC] LSM, net: Add SO_PEERCONTEXT for peer LSM data
From: Paul Moore <paul@paul-moore.com>
Date: 2024-06-20 21:05:28
Also in:
linux-api, lkml, netdev
On May 13, 2024 Casey Schaufler [off-list ref] 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(+)
...
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
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().
quoted hunk ↗ jump to hunk
+ 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