Thread (69 messages) 69 messages, 6 authors, 2019-12-19

Re: [PATCH v12 23/25] NET: Add SO_PEERCONTEXT for multiple LSMs

From: Stephen Smalley <hidden>
Date: 2019-12-18 18:29:13
Also in: selinux

Possibly related (same subject, not in this thread)

On 12/16/19 5:36 PM, Casey Schaufler wrote:
The getsockopt SO_PEERSEC provides the LSM based security
information for a single module, but for reasons of backward
compatibility cannot include the information for multiple
modules. A new option SO_PEERCONTEXT is added to report the
security "context" of multiple modules using a "compound" format

         lsm1\0value\0lsm2\0value\0

This is expected to be used by system services, including dbus-daemon.
The exact format of a compound context has been the subject of
considerable debate. This format was suggested by Simon McVittie,
a dbus maintainer with a significant stake in the format being
usable.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
cc: netdev@vger.kernel.org
Requires ack by netdev and linux-api.  A couple of comments below.
---
quoted hunk ↗ jump to hunk
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 2bf82e1cf347..2ae10e7f81a7 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -880,8 +880,8 @@
   *	SO_GETPEERSEC.  For tcp sockets this can be meaningful if the
   *	socket is associated with an ipsec SA.
   *	@sock is the local socket.
- *	@optval userspace memory where the security state is to be copied.
- *	@optlen userspace int where the module should copy the actual length
+ *	@optval memory where the security state is to be copied.
This is misleading; it suggests that the caller is providing an 
allocated buffer into which the security module copies its data. Instead 
it is just a pointer to a pointer that is then set by the security 
module to a buffer the module allocates.
quoted hunk ↗ jump to hunk
diff --git a/include/linux/security.h b/include/linux/security.h
index 536db4dbfcbb..b72bb90b1903 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -181,7 +181,7 @@ struct lsmblob {
  #define LSMBLOB_NEEDED		-2	/* Slot requested on initialization */
  #define LSMBLOB_NOT_NEEDED	-3	/* Slot not requested */
  #define LSMBLOB_DISPLAY		-4	/* Use the "display" slot */
-#define LSMBLOB_FIRST		-5	/* Use the default "display" slot */
+#define LSMBLOB_COMPOUND	-5	/* A compound "display" */
I'm puzzled by the removal of LSMBLOB_FIRST by this patch; it suggests 
it was never needed in the first place by the patch that introduced it. 
But more below.
quoted hunk ↗ jump to hunk
diff --git a/security/security.c b/security/security.c
index d0b57a7c3b31..1afe245f3246 100644
--- a/security/security.c
+++ b/security/security.c
@@ -723,6 +723,42 @@ static void __init lsm_early_task(struct task_struct *task)
  		panic("%s: Early task alloc failed.\n", __func__);
  }
  
+/**
+ * append_ctx - append a lsm/context pair to a compound context
+ * @ctx: the existing compound context
+ * @ctxlen: size of the old context, including terminating nul byte
+ * @lsm: new lsm name, nul terminated
+ * @new: new context, possibly nul terminated
+ * @newlen: maximum size of @new
+ *
+ * replace @ctx with a new compound context, appending @newlsm and @new
+ * to @ctx. On exit the new data replaces the old, which is freed.
+ * @ctxlen is set to the new size, which includes a trailing nul byte.
+ *
+ * Returns 0 on success, -ENOMEM if no memory is available.
+ */
+static int append_ctx(char **ctx, int *ctxlen, const char *lsm, char *new,
+		      int newlen)
+{
+	char *final;
+	int llen;
+
+	llen = strlen(lsm) + 1;
+	newlen = strnlen(new, newlen) + 1;
+
+	final = kzalloc(*ctxlen + llen + newlen, GFP_KERNEL);
+	if (final == NULL)
+		return -ENOMEM;
+	if (*ctxlen)
+		memcpy(final, *ctx, *ctxlen);
+	memcpy(final + *ctxlen, lsm, llen);
+	memcpy(final + *ctxlen + llen, new, newlen);
+	kfree(*ctx);
+	*ctx = final;
+	*ctxlen = *ctxlen + llen + newlen;
+	return 0;
+}
You should likely take some precautions against integer overflows in the 
above code?
quoted hunk ↗ jump to hunk
+
  /*
   * Hook list operation macros.
   *
@@ -2164,8 +2200,8 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
  	hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
  		if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
  			continue;
-		if (lsm == NULL && *display != LSMBLOB_INVALID &&
-		    *display != hp->lsmid->slot)
+		if (lsm == NULL && display != NULL &&
+		    *display != LSMBLOB_INVALID && *display != hp->lsmid->slot)
  			continue;
  		return hp->hook.setprocattr(name, value, size);
  	}
Is this a bug fix that should be folded into the earlier patch that 
introduced it?
quoted hunk ↗ jump to hunk
@@ -2196,7 +2232,7 @@ int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp,
  	 */
  	if (display == LSMBLOB_DISPLAY)
  		display = lsm_task_display(current);
-	else if (display == LSMBLOB_FIRST)
+	else if (display == 0)
  		display = LSMBLOB_INVALID;
  	else if (display < 0) {
  		WARN_ONCE(true,
Why is it necessary to re-map display 0 in this manner? Previously if 
display 0 was specified, it would require it to match the lsmid->slot 
value.  Won't it match anyway?
quoted hunk ↗ jump to hunk
@@ -2246,6 +2282,15 @@ void security_release_secctx(struct lsmcontext *cp)
  	struct security_hook_list *hp;
  	bool found = false;
  
+	if (cp->slot == LSMBLOB_INVALID)
+		return;
+
+	if (cp->slot == LSMBLOB_COMPOUND) {
+		kfree(cp->context);
+		found = true;
+		goto clear_out;
+	}
+
If you re-order your pr_warn() below with your memset() to address the 
earlier comment, you'll end up trying to print the freed memory.  Not a 
problem if you just drop the pr_warn() altogether.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help