Re: [tipc-discussion] [net v2] tipc: add stricter control of reserved service types

[Xue, Y. <Yin...@wi...>]

Hi Jon,

I have one idea about how to block the vulnerability described below from happening. Although it's not so elegant, it's quite simple:

As you mentioned below, TIPC_TOP_SRV service is only published in tipc_topsrv_create_listener () from kernel space. So probably we can change the code as below:

tipc_bind()
{
...
	if ((addr->addr.nameseq.type < TIPC_RESERVED_TYPES) {
		res = -EACCES;
		goto exit;
	}
	__tipc_bind();
...
}

Int __tipc_bind()
{
        If (addr->scope >= 0)                
	tipc_sk_publish(tsk, addr->scope, &addr->addr.nameseq) :
         else
                tipc_sk_withdraw(tsk, -addr->scope, &addr->addr.nameseq);
}

tipc_topsrv_create_listener()
{
...
- rc = kernel_bind(lsock, (struct sockaddr *)&saddr, sizeof(saddr));
+ rc = __tipc_bind(lsock, (struct sockaddr *)&saddr, sizeof(saddr));
...
}

Thanks,
Ying

-----Original Message-----
From: jm...@re... <jm...@re...> 
Sent: Sunday, October 25, 2020 3:16 AM
To: tip...@li...
Cc: tun...@de...; hoa...@de...; tuo...@de...; jm...@re...; ma...@do...; xi...@re...; Xue, Ying <Yin...@wi...>; par...@gm...
Subject: [net v2] tipc: add stricter control of reserved service types

From: Jon Maloy <jm...@re...>

TIPC reserves 64 service types for current and future internal use.
Therefore, the bind() function is meant to block regular user sockets from being bound to these values, while it should let through such bindings from internal users.

However, since we at the design moment saw no way to distinguish between regular and internal users the filter function ended up with allowing all bindings of the reserved types which were really in use ([0,1]), and block all the rest ([2,63]).

This is risky, since a regular user may bind to the service type representing the topology server (TIPC_TOP_SRV == 1) or the one used for indicating neigboring node status (TIPC_CFG_SRV == 0), and wreak havoc for users of those services, i.e., practically all users.

The reality is however that TIPC_CFG_SRV never is bound through the
bind() function, since it doesn't represent a regular socket, and TIPC_TOP_SRV can easily be singled out, since it is published from kernel mode and is the very first binding performed when the system is starting.

We now introduce a 'privileged' mode for sockets, marking which of those are entitled to bind to reserved service type values. The only such socket we have so far is the topology server's listener socket, which is identified the way described above. All other bindings to reserved service types are rejected.

It should be noted that, although this is a change of the API semantics, there is no risk we will break any currently working applications by doing this. Any application trying to bind to the values in question would be badly broken from the outset, so there is no chance we would find any such applications in real-world production systems.

Signed-off-by: Jon Maloy <jm...@re...>
---
 net/tipc/socket.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c index e795a8a2955b..a0a144ff84fd 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1,7 +1,8 @@
 /*
  * net/tipc/socket.c: TIPC socket API
  *
- * Copyright (c) 2001-2007, 2012-2017, Ericsson AB
+ * Copyright (c) 2020, Red Hat Inc
+ * Copyright (c) 2001-2007, 2012-2019, Ericsson AB
  * Copyright (c) 2004-2008, 2010-2013, Wind River Systems
  * All rights reserved.
  *
@@ -127,6 +128,8 @@ struct tipc_sock {
 	bool expect_ack;
 	bool nodelay;
 	bool group_is_open;
+	bool privileged;
+	bool kernel;
 };
 
 static int tipc_sk_backlog_rcv(struct sock *sk, struct sk_buff *skb); @@ -479,6 +482,7 @@ static int tipc_sk_create(struct net *net, struct socket *sock,
 	tsk->max_pkt = MAX_PKT_DEFAULT;
 	tsk->maxnagle = 0;
 	tsk->nagle_start = NAGLE_START_INIT;
+	tsk->kernel = !!kern;
 	INIT_LIST_HEAD(&tsk->publications);
 	INIT_LIST_HEAD(&tsk->cong_links);
 	msg = &tsk->phdr;
@@ -665,6 +669,7 @@ static int tipc_bind(struct socket *sock, struct sockaddr *uaddr,
 	struct sockaddr_tipc *addr = (struct sockaddr_tipc *)uaddr;
 	struct tipc_sock *tsk = tipc_sk(sk);
 	int res = -EINVAL;
+	u32 stype, dnode;
 
 	lock_sock(sk);
 	if (unlikely(!uaddr_len)) {
@@ -691,13 +696,15 @@ static int tipc_bind(struct socket *sock, struct sockaddr *uaddr,
 		goto exit;
 	}
 
-	if ((addr->addr.nameseq.type < TIPC_RESERVED_TYPES) &&
-	    (addr->addr.nameseq.type != TIPC_TOP_SRV) &&
-	    (addr->addr.nameseq.type != TIPC_CFG_SRV)) {
+	stype = addr->addr.nameseq.type;
+	if (stype == TIPC_TOP_SRV && tsk->kernel &&
+	    !tipc_nametbl_translate(sock_net(sk), stype, stype, &dnode))
+		tsk->privileged = true;
+
+	if (stype < TIPC_RESERVED_TYPES && !tsk->privileged) {
 		res = -EACCES;
 		goto exit;
 	}
-
 	res = (addr->scope >= 0) ?
 		tipc_sk_publish(tsk, addr->scope, &addr->addr.nameseq) :
 		tipc_sk_withdraw(tsk, -addr->scope, &addr->addr.nameseq);
--
2.25.4