linux-kernel - Re: [PATCH] can: ucan: fix out of bound read in strscpy() source

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] [day] [month] [year] [list]

Message-ID: <20250218-pompous-diamond-bumblebee-dd4293-mkl@pengutronix.de>
Date: Tue, 18 Feb 2025 16:12:43 +0100
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Vincent Mailhol <mailhol.vincent@...adoo.fr>
Cc: Matt Jan <zoo868e@...il.com>, Xu Panda <xu.panda@....com.cn>, 
	Yang Yang <yang.yang29@....com>, linux-can@...r.kernel.org, linux-kernel@...r.kernel.org, 
	syzbot+d7d8c418e8317899e88c@...kaller.appspotmail.com
Subject: Re: [PATCH] can: ucan: fix out of bound read in strscpy() source

On 18.02.2025 23:32:28, Vincent Mailhol wrote:
> Commit 7fdaf8966aae ("can: ucan: use strscpy() to instead of strncpy()")
> unintentionally introduced a one byte out of bound read on strscpy()'s
> source argument (which is kind of ironic knowing that strscpy() is meant
> to be a more secure alternative :)).
> 
> Let's consider below buffers:
> 
>   dest[len + 1]; /* will be NUL terminated */
>   src[len]; /* may not be NUL terminated */
> 
> When doing:
> 
>   strncpy(dest, src, len);
>   dest[len] = '\0';
> 
> strncpy() will read up to len bytes from src.
> 
> On the other hand:
> 
>   strscpy(dest, src, len + 1);
> 
> will read up to len + 1 bytes from src, that is to say, an out of bound
> read of one byte will occur on src if it is not NUL terminated. Note
> that the src[len] byte is never copied, but strscpy() still needs to
> read it to check whether a truncation occurred or not.
> 
> This exact pattern happened in ucan.
> 
> The root cause is that the source is not NUL terminated. Instead of
> doing a copy in a local buffer, directly NUL terminate it as soon as
> usb_control_msg() returns. With this, the local firmware_str[] variable
> can be removed.
> 
> On top of this do a couple refactors:
> 
>   - ucan_ctl_payload->raw is only used for the firmware string, so
>     rename it to ucan_ctl_payload->fw_str and change its type from u8 to
>     char.
> 
>   - ucan_device_request_in() is only used to retrieve the firmware
>     string, so rename it to ucan_get_fw_str() and refactor it to make it
>     directly handle all the string termination logic.
> 
> Reported-by: syzbot+d7d8c418e8317899e88c@...kaller.appspotmail.com
> Closes: https://lore.kernel.org/linux-can/67b323a4.050a0220.173698.002b.GAE@google.com/
> Fixes: 7fdaf8966aae ("can: ucan: use strscpy() to instead of strncpy()")
> Signed-off-by: Vincent Mailhol <mailhol.vincent@...adoo.fr>

Wow! Thanks for the detailed analysis and description of the problem!

Applied to linux-can.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)