NetBSD Problem Report #56260

From www@netbsd.org  Fri Jun 18 10:04:30 2021
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 0A0BE1A921F
	for <gnats-bugs@gnats.NetBSD.org>; Fri, 18 Jun 2021 10:04:30 +0000 (UTC)
Message-Id: <20210618100428.8A9231A923D@mollari.NetBSD.org>
Date: Fri, 18 Jun 2021 10:04:28 +0000 (UTC)
From: arichardson@FreeBSD.org
Reply-To: arichardson@FreeBSD.org
To: gnats-bugs@NetBSD.org
Subject: [PATCH] Out-of-bounds stack read in lib/libc/gen/vis.c
X-Send-Pr-Version: www-1.0

>Number:         56260
>Category:       lib
>Synopsis:       [PATCH] Out-of-bounds stack read in lib/libc/gen/vis.c
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    lib-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Jun 18 10:05:00 +0000 2021
>Closed-Date:    Wed Jan 10 23:07:30 +0000 2024
>Last-Modified:  Wed Jan 10 23:07:30 +0000 2024
>Originator:     Alex Richardson
>Release:        N/A
>Organization:
FreeBSD
>Environment:
N/A
>Description:
I found an out-of-bounds stack read in the vis code when running FreeBSD compiled for CHERI. Since the vis code in FreeBSD is the same as NetBSD lib/libc/gen/vis.c the patch also applies here.

See https://cgit.freebsd.org/src/commit/?id=1a2f06d0f2905c9a18340b377cbbe772f2ca6844
>How-To-Repeat:
Call vis(3) without the VIS_NOLOCALE flag and two non-ASCII chars: it passes a 2 byte buffer to istrsenvisx, but the first loop iteration may attempt to decode up to MB_LEN_MAX bytes from the two char buffer.
>Fix:
Apply the patch from https://cgit.freebsd.org/src/commit/?id=1a2f06d0f2905c9a18340b377cbbe772f2ca6844 - replace MB_LEN_MAX with MIN(mbslength, MB_LEN_MAX)

>Release-Note:

>Audit-Trail:
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56260 CVS commit: src/lib/libc/gen
Date: Fri, 18 Jun 2021 06:57:14 -0400

 Module Name:	src
 Committed By:	christos
 Date:		Fri Jun 18 10:57:14 UTC 2021

 Modified Files:
 	src/lib/libc/gen: vis.c

 Log Message:
 PR/56260: Alex Richardson: Out-of-bounds stack read in lib/libc/gen/vis.c
 Also sync with other FreeBSD changes.


 To generate a diff of this commit:
 cvs rdiff -u -r1.74 -r1.75 src/lib/libc/gen/vis.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->needs-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Thu, 07 Dec 2023 15:16:12 +0000
State-Changed-Why:
This never got pulled up to 8 or 9.


From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56260 CVS commit: [netbsd-9] src
Date: Sat, 9 Dec 2023 13:09:04 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Sat Dec  9 13:09:04 UTC 2023

 Modified Files:
 	src/lib/libc/gen [netbsd-9]: vis.c
 	src/tests/lib/libc/gen [netbsd-9]: t_vis.c

 Log Message:
 Pull up following revision(s) (requested by riastradh in ticket #1774):

 	lib/libc/gen/vis.c: revision 1.75-1.86
 	tests/lib/libc/gen/t_vis.c: revision 1.10-1.14

 PR 56260: fix out-of-bounds stack read.

 vis(3): Avoid nonportable MIN in portable code.

 vis(3) tests: Add xfail test for encoding overflow.

 From Kyle Evans <kevans%FreeBSD.org@localhost>.
 PR lib/57573

 vis(3) tests: Expand tests and diagnostic outputs on failure.
 PR lib/57573

 vis(3) tests: Test another overflow edge case.
 Related to PR lib/57573.

 vis(3): Make maxolen unsigned size_t, not ssize_t.
 It is initialized once either to *dlen, which is unsigned size_t, or
 to wcslen(start) * MB_MAX_LEN + 1, and wcslen returns unsigned size_t
 too.  So there appears to have never been any reason for this to be
 signed.
 Part of PR lib/57573.

 vis(3): Make mbslength unsigned.
 Sprinkle assertions and comments justifying the proposition that it
 would never go negative if signed.
 Obviates need to worry about mblength > SSIZE_MAX.
 Prompted by PR lib/57573.

 vis(3): Avoid arithmetic overflow before calloc(3).
 Prompted by PR lib/57573.

 vis(3): Call wcslen(start) only once.
 It had better not change between these two times!
 Prompted by PR lib/57573.

 vis(3): Avoid potential arithmetic overflow in maxolen.
 Can't easily prove that this overflow is impossible, so let's add a
 check.
 Prompted by PR lib/57573.

 vis(3): Fix main part of PR lib/57573.
 From Kyle Evans <kevans%FreeBSD.org@localhost>.

 vis(3): Fix one more buffer overrun in an edge case.
 PR lib/57573

 vis(3): Sort includes.  No functional change intended.
 Prompted by PR lib/57573.

 vis(3): Need <stdint.h> for SIZE_MAX, per C standard.
 From Kyle Evans <kevans%FreeBSD.org@localhost>.
 Followup to PR lib/57573.

 vis(3): Per KNF, sys/param.h comes before sys/types.h.
 Which is nice because that's also lexicographic.


 To generate a diff of this commit:
 cvs rdiff -u -r1.74 -r1.74.6.1 src/lib/libc/gen/vis.c
 cvs rdiff -u -r1.9 -r1.9.16.1 src/tests/lib/libc/gen/t_vis.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56260 CVS commit: [netbsd-8] src
Date: Sat, 9 Dec 2023 13:10:16 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Sat Dec  9 13:10:16 UTC 2023

 Modified Files:
 	src/lib/libc/gen [netbsd-8]: vis.c
 	src/tests/lib/libc/gen [netbsd-8]: t_vis.c

 Log Message:
 Pull up following revision(s) (requested by riastradh in ticket #1923):

 	lib/libc/gen/vis.c: revision 1.75-1.86
 	tests/lib/libc/gen/t_vis.c: revision 1.10-1.14

 PR 56260: fix out-of-bounds stack read.

 vis(3): Avoid nonportable MIN in portable code.

 vis(3) tests: Add xfail test for encoding overflow.

 From Kyle Evans <kevans%FreeBSD.org@localhost>.
 PR lib/57573

 vis(3) tests: Expand tests and diagnostic outputs on failure.
 PR lib/57573

 vis(3) tests: Test another overflow edge case.
 Related to PR lib/57573.

 vis(3): Make maxolen unsigned size_t, not ssize_t.
 It is initialized once either to *dlen, which is unsigned size_t, or
 to wcslen(start) * MB_MAX_LEN + 1, and wcslen returns unsigned size_t
 too.  So there appears to have never been any reason for this to be
 signed.
 Part of PR lib/57573.

 vis(3): Make mbslength unsigned.
 Sprinkle assertions and comments justifying the proposition that it
 would never go negative if signed.
 Obviates need to worry about mblength > SSIZE_MAX.
 Prompted by PR lib/57573.

 vis(3): Avoid arithmetic overflow before calloc(3).
 Prompted by PR lib/57573.

 vis(3): Call wcslen(start) only once.
 It had better not change between these two times!
 Prompted by PR lib/57573.

 vis(3): Avoid potential arithmetic overflow in maxolen.
 Can't easily prove that this overflow is impossible, so let's add a
 check.
 Prompted by PR lib/57573.

 vis(3): Fix main part of PR lib/57573.
 From Kyle Evans <kevans%FreeBSD.org@localhost>.

 vis(3): Fix one more buffer overrun in an edge case.
 PR lib/57573

 vis(3): Sort includes.  No functional change intended.
 Prompted by PR lib/57573.

 vis(3): Need <stdint.h> for SIZE_MAX, per C standard.
 From Kyle Evans <kevans%FreeBSD.org@localhost>.
 Followup to PR lib/57573.

 vis(3): Per KNF, sys/param.h comes before sys/types.h.
 Which is nice because that's also lexicographic.


 To generate a diff of this commit:
 cvs rdiff -u -r1.73.4.1 -r1.73.4.2 src/lib/libc/gen/vis.c
 cvs rdiff -u -r1.9 -r1.9.6.1 src/tests/lib/libc/gen/t_vis.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: needs-pullups->closed
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Wed, 10 Jan 2024 23:07:30 +0000
State-Changed-Why:
fixed in HEAD, pulled up to 10, 9, 8


>Unformatted:

Home	PR Database Search