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

[Jon M. <jm...@re...>]

On 10/21/20 10:55 AM, Ying Xue wrote:
> On 10/10/20 10:56 PM, jm...@re... wrote:
>> 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 types which were really in use
>> ([0,1]), and block all the rest ([2,63]).
>>
>> This is dangerous, 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.
>>
> Sorry, Jon. I could not understand how such scenarios described above
> happened. Can you please take an example to demonstrate a regular user
> can bind to the same service type as TIPC_TOP_SRV or TIPC_CFG_SRV under
> the current code logic?
>
> Thanks,
> Ying
That is extremely simple. Take the hello_server under 
tipcutils/demos/hello_world and change the server name to {1,1}.
It works! And it shouldn't of course, because it will steal traffic 
directed to the toplogy server.

///jon

>
>> 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 filtered out, since it is the very first
>> binding performed when the system is starting.
>>
>> We can hence block TIPC_CFG_SRV completely, and only allow TIPC_TOP_SRV
>> to be bound once, and the correct behavior is achieved. This is what we
>> do in this commit.
>>
>> 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 | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
>> index e795a8a2955b..67875a5761d0 100644
>> --- a/net/tipc/socket.c
>> +++ b/net/tipc/socket.c
>> @@ -665,6 +665,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,9 +692,10 @@ 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_RESERVED_TYPES &&
>> +	    (stype != TIPC_TOP_SRV ||
>> +	     tipc_nametbl_translate(sock_net(sk), stype, stype, &dnode))) {
>>   		res = -EACCES;
>>   		goto exit;
>>   	}
>>