On 6/20/2024 2:05 PM, Paul Moore wrote:

On May 13, 2024 Casey Schaufler [off-list ref] wrote:

quoted

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.

quoted

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

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.

quoted

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.

quoted

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

quoted

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

quoted

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

quoted

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

"... set @opt_len ... "

Yes.

quoted

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

Do the @rc check above, not here.

quoted

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

quoted

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