linux-kernel - RE: [PATCH] tipc: avoid possible string overflow

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 28 Mar 2018 14:41:50 +0000
From:   Jon Maloy <jon.maloy@...csson.com>
To:     Arnd Bergmann <arnd@...db.de>, Ying Xue <ying.xue@...driver.com>,
        "David S. Miller" <davem@...emloft.net>
CC:     Parthasarathy Bhuvaragan <parthasarathy.bhuvaragan@...csson.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "tipc-discussion@...ts.sourceforge.net" 
        <tipc-discussion@...ts.sourceforge.net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] tipc: avoid possible string overflow



> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@...db.de]
> Sent: Wednesday, March 28, 2018 10:02
> To: Jon Maloy <jon.maloy@...csson.com>; Ying Xue
> <ying.xue@...driver.com>; David S. Miller <davem@...emloft.net>
> Cc: Arnd Bergmann <arnd@...db.de>; Parthasarathy Bhuvaragan
> <parthasarathy.bhuvaragan@...csson.com>; netdev@...r.kernel.org; tipc-
> discussion@...ts.sourceforge.net; linux-kernel@...r.kernel.org
> Subject: [PATCH] tipc: avoid possible string overflow
> 
> gcc points out that the combined length of the fixed-length inputs to
> l->name is larger than the destination buffer size:
> 
> net/tipc/link.c: In function 'tipc_link_create':
> net/tipc/link.c:465:26: error: '%s' directive writing up to 32 bytes into a region
> of size between 26 and 58 [-Werror=format-overflow=]
>   sprintf(l->name, "%s:%s-%s:unknown", self_str, if_name, peer_str);
>                           ^~                              ~~~~~~~~
> net/tipc/link.c:465:2: note: 'sprintf' output 11 or more bytes (assuming 75)
> into a destination of size 60
>   sprintf(l->name, "%s:%s-%s:unknown", self_str, if_name, peer_str);
> 
> Using snprintf() ensures that the destination is still a nul-terminated string in
> all cases. It's still theoretically possible that the string gets trunctated though,
> so this patch should be carefully reviewed to ensure that either truncation is
> impossible in practice, or that we're ok with the truncation.

Theoretically, maximum bearer name is MAX_BEARER_NAME - 3  = 29  (because if_name is only the part after the ":"  in a bearer name, and is zero-terminated.
The lines just above in the code reveals that the maximum length of self_str and peer_str is 16.
This taken together means that the theoretically max length of a link name becomes:
16  + 1 + 29 + 1 + 16 + 1 + 29 = 93.  Since we also need room for a terminating zero, we need to extend the tipc_link::name array to 96 bytes.

I'll fix that.

Thank you to for reporting this.
///jon

> 
> Fixes: 25b0b9c4e835 ("tipc: handle collisions of 32-bit node address hash
> values")
> Signed-off-by: Arnd Bergmann <arnd@...db.de>
> ---
>  net/tipc/link.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/tipc/link.c b/net/tipc/link.c index 1289b4ba404f..c195ba036035
> 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -462,7 +462,8 @@ bool tipc_link_create(struct net *net, char *if_name,
> int bearer_id,
>  			sprintf(peer_str, "%x", peer);
>  	}
>  	/* Peer i/f name will be completed by reset/activate message */
> -	sprintf(l->name, "%s:%s-%s:unknown", self_str, if_name, peer_str);
> +	snprintf(l->name, sizeof(l->name), "%s:%s-%s:unknown",
> +		 self_str, if_name, peer_str);
> 
>  	strcpy(l->if_name, if_name);
>  	l->addr = peer;
> --
> 2.9.0