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@xxxxxxxxxxxxxxxx>
cc: netdev@xxxxxxxxxxxxxxx
Requires ack by netdev and linux-api. A couple of comments below.
---
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.
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.
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?
+
/*
* 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?
@@ -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?
@@ -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.
