linux-kernel - Re: [PATCH net-next] af_unix: Refine UNIX domain sockets autobind identifier length

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]

Message-ID: <20250206040118.77016-1-kuniyu@amazon.com>
Date: Thu, 6 Feb 2025 13:01:18 +0900
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <buaajxlj@....com>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <horms@...nel.org>,
	<kuba@...nel.org>, <kuniyu@...zon.com>, <liangjie@...iang.com>,
	<linux-kernel@...r.kernel.org>, <mhal@...x.co>, <netdev@...r.kernel.org>,
	<pabeni@...hat.com>
Subject: Re: [PATCH net-next] af_unix: Refine UNIX domain sockets autobind identifier length

From: Liang Jie <buaajxlj@....com>
Date: Wed,  5 Feb 2025 18:09:04 +0800
> The logs from 'netdev/build_allmodconfig_warn' indicate that the patch has
> given rise to the following warning:
> 
>  - ../net/unix/af_unix.c: In function ‘unix_autobind’:
>  - ../net/unix/af_unix.c:1227:48: warning: ‘sprintf’ writing a terminating nul past the end of the destination [-Wformat-overflow=]
>  -  1227 |         sprintf(addr->name->sun_path + 1, "%0*x", AUTOBIND_LEN - 1, ordernum);
>  -       |                                                ^
>  - ../net/unix/af_unix.c:1227:9: note: ‘sprintf’ output 6 bytes into a destination of size 5
>  -  1227 |         sprintf(addr->name->sun_path + 1, "%0*x", AUTOBIND_LEN - 1, ordernum);
>  -       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> It appears that the 'sprintf' call attempts to write a terminating null
> byte past the end of the 'sun_path' array, potentially causing an overflow.
> 
> To address this issue, I am considering the following approach:
> 
> 	char orderstring[6];
> 
> 	sprintf(orderstring, "%05x", ordernum);
> 	memcpy(addr->name->sun_path + 1, orderstring, 5);
> 
> This would prevent the buffer overflow by using 'memcpy' to safely copy the
> formatted string into 'sun_path'.

Finally new hard-coded values are introduced..

I'm not sure this is worth saving just 10 bytes, which is not excessive,
vs extra 5 bytes memcpy(), so I'd rather not touch here.


> 
> Before proceeding with a patch submission, I wanted to consult with you to
> see if you have any suggestions for a better or more elegant solution to
> this problem.

An elegant option might be add a variant of snprintf without terminating
string by \0 ?