Re: [PATCH v7 26/28] NET: Add SO_PEERCONTEXT for multiple LSMs

On 8/8/2019 3:21 PM, Kees Cook wrote:

On Wed, Aug 07, 2019 at 12:44:08PM -0700, Casey Schaufler wrote:

quoted

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

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
[...]

diff --git a/security/security.c b/security/security.c
index 2f4a430a1126..0ea7ee27e331 100644
--- a/security/security.c
+++ b/security/security.c

@@ -2117,8 +2117,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);
 	}

@@ -2342,17 +2342,91 @@ int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
 EXPORT_SYMBOL(security_sock_rcv_skb);
 
 int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
-				      int __user *optlen, unsigned len)
+				      int __user *optlen, unsigned len,
+				      int display)
 {
-	int display = lsm_task_display(current);
 	struct security_hook_list *hp;
+	char *final = NULL;
+	char *cp;
+	char *tp;
+	int rc = 0;
+	unsigned finallen = 0;
+	unsigned llen;
+	unsigned clen = 0;
+	unsigned tlen;

Please move the case-specific variables into the case scope, like (and
expand type names):

	case LSMBLOB_COMPOUND: {
		unsigned int clen ...;

I've never been a fan of that style. I'll probably make the
special cases into functions.

quoted

+
+	switch (display) {
+	case LSMBLOB_DISPLAY:
+		rc = -ENOPROTOOPT;
+		display = lsm_task_display(current);
+		hlist_for_each_entry(hp,
+				&security_hook_heads.socket_getpeersec_stream,
+				list)
+			if (display == LSMBLOB_INVALID ||
+			    display == hp->lsmid->slot) {
+				rc = hp->hook.socket_getpeersec_stream(sock,
+							&final, &finallen, len);
+				break;
+			}
+		break;
+	case LSMBLOB_COMPOUND:
+		/*
+		 * A compound context, in the form [lsm\0value\0]...
+		 */
+		hlist_for_each_entry(hp,
+				&security_hook_heads.socket_getpeersec_stream,
+				list) {
+			rc = hp->hook.socket_getpeersec_stream(sock, &cp, &clen,
+							       len);

Is passing "len" here useful at all? It's kind of a lie, but nothing
else wouldn't also be a lie. :)

I could be reducing the value of len for each component gathered.
It requires yet another temporary length variable, and exceeding the
len passed has to get checked in here anyway. I may be able to make
this more sensible.

quoted

+			if (rc == -EINVAL || rc == -ENOPROTOOPT) {
+				rc = 0;
+				continue;
+			}
+			if (rc) {
+				kfree(final);
+				return rc;
+			}
+			/*
+			 * Don't propogate trailing nul bytes.

typo: propagate

Not a typo, a spelling error. I'm real good at spelling errors.

quoted

+			 */
+			clen = strnlen(cp, clen) + 1;
+			llen = strlen(hp->lsmid->lsm) + 1;
+			tlen = llen + clen;
+			if (final)
+				tlen += finallen;

You can drop the "if (final)" since finallen is initialized to 0.

Right you are.

quoted

+			tp = kzalloc(tlen, GFP_KERNEL);

I'm not a huge fan of "c", "l", and "t" prefixes -- can you just make
these a little more self-documenting? cp and clen could be value and
value_len. llen could be lsm_name_len. tp and tlen could be tuple and
tuple_len. (And maybe final and finallen could be compound and
compound_len?)

You didn't spend much time using TTY33s, did you? Or 80 column punch cards
where the last 8 are reserved for collation numbers, I'd bet. Kids. OK,
I'll see what I can do since I will be fiddling about anyway.

quoted

+			if (tp == NULL) {
+				kfree(cp);
+				kfree(final);
+				return -ENOMEM;
+			}
+			if (final)
+				memcpy(tp, final, finallen);
+			memcpy(tp + finallen, hp->lsmid->lsm, llen);
+			memcpy(tp + finallen + llen, cp, clen);
+			kfree(cp);
+			if (final)
+				kfree(final);

Just kfree(final) is safe here -- kfree(NULL) is valid.

Just so. At some point there was more in the if condition.

quoted

+			final = tp;
+			finallen = tlen;
+		}
+		if (final == NULL)
+			return -EINVAL;
+		break;
+	default:
+		return -EINVAL;
+	}
 
-	hlist_for_each_entry(hp, &security_hook_heads.socket_getpeersec_stream,
-			     list)
-		if (display == LSMBLOB_INVALID || display == hp->lsmid->slot)
-			return hp->hook.socket_getpeersec_stream(sock, optval,
-								 optlen, len);
-	return -ENOPROTOOPT;
+	if (finallen > len)
+		rc = -ERANGE;
+	else if (copy_to_user(optval, final, finallen))
+		rc = -EFAULT;
+
+	if (put_user(finallen, optlen))
+		rc = -EFAULT;
+
+	kfree(final);
+	return rc;
 }

Otherwise, looks good.

Reviewed-by: Kees Cook <redacted>

-Kees

quoted

 
 int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb,

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index fcad2e3432d2..5e7d61754798 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c

@@ -4923,10 +4923,8 @@ static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
 	return err;
 }
 
-static int selinux_socket_getpeersec_stream(struct socket *sock,
-					    char __user *optval,
-					    int __user *optlen,
-					    unsigned int len)
+static int selinux_socket_getpeersec_stream(struct socket *sock, char **optval,
+					    int *optlen, unsigned int len)
 {
 	int err = 0;
 	char *scontext;

@@ -4946,18 +4944,12 @@ static int selinux_socket_getpeersec_stream(struct socket *sock,
 	if (err)
 		return err;
 
-	if (scontext_len > len) {
+	if (scontext_len > len)
 		err = -ERANGE;
-		goto out_len;
-	}
-
-	if (copy_to_user(optval, scontext, scontext_len))
-		err = -EFAULT;
+	else
+		*optval = scontext;
 
-out_len:
-	if (put_user(scontext_len, optlen))
-		err = -EFAULT;
-	kfree(scontext);
+	*optlen = scontext_len;
 	return err;
 }
 

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 7a30b8692b1e..40c75205a914 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c

@@ -3919,28 +3919,29 @@ static int smack_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
  *
  * returns zero on success, an error code otherwise
  */
-static int smack_socket_getpeersec_stream(struct socket *sock,
-					  char __user *optval,
-					  int __user *optlen, unsigned len)
+static int smack_socket_getpeersec_stream(struct socket *sock, char **optval,
+					  int *optlen, unsigned len)
 {
-	struct socket_smack *ssp;
-	char *rcp = "";
-	int slen = 1;
+	struct socket_smack *ssp = smack_sock(sock->sk);
+	char *rcp;
+	int slen;
 	int rc = 0;
 
-	ssp = smack_sock(sock->sk);
-	if (ssp->smk_packet != NULL) {
-		rcp = ssp->smk_packet->smk_known;
-		slen = strlen(rcp) + 1;
+	if (ssp->smk_packet == NULL) {
+		*optlen = 0;
+		return -EINVAL;
 	}
 
+	rcp = ssp->smk_packet->smk_known;
+	slen = strlen(rcp) + 1;
 	if (slen > len)
 		rc = -ERANGE;
-	else if (copy_to_user(optval, rcp, slen) != 0)
-		rc = -EFAULT;
-
-	if (put_user(slen, optlen) != 0)
-		rc = -EFAULT;
+	else {
+		*optval = kstrdup(rcp, GFP_KERNEL);
+		if (*optval == NULL)
+			rc = -ENOMEM;
+	}
+	*optlen = slen;
 
 	return rc;
 }

-- 
2.20.1