On 8/12/23 10:05, Taylor R Campbell wrote:
The following reply was made to PR lib/57573; it has been noted by GNATS.
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: kevans%FreeBSD.org@localhost
Cc: gnats-bugs%NetBSD.org@localhost
Subject: Re: lib/57573: Overflow possibilities in vis(3)
Date: Sat, 12 Aug 2023 15:01:36 +0000
This is a multi-part message in MIME format.
--=_La7nkjIJilZyzQ04m7iYdJZjSuvrF8DG
Turns out there's another buffer overrun lurking, when you do
strnvis(dst, 0, "", ...) -- it shouldn't write to dst[0], but it does.
I added an atf test for that, a branch to prevent it, and an assertion
to catch it.
Ahh, whoops, we didn't look quite that far up- our immediate concern was
a consumer that wasn't providing a buffer quite large enough (missing
space for a NUL byte); I convinced them to fix the size and switch to
strnvis(), but then realized that wouldn't quite be sufficient for the
scenario they were hitting in the first place where we were apparently
needing to encode every single byte in the buffer (thus, writing into a
redzone with the NUL terminator). I'm quite glad you did a more careful
analysis here.
I adapted your patch for the other overruns with some minor changes:
Thanks!
1. `char mbbuf[MB_LEN_MAX]' instead of `char mbbuf[MB_CUR_MAX]', since
MB_CUR_MAX is not a compile-time constant.
This avoids what is formally a variable-length array, which gets in
the way of stack protectors, &c. It may not pose a practical
problem for stack overflow, because MB_CUR_MAX <= MB_LEN_MAX, but
it's easier to keep measures that detect real problems elsewhere
happy this way.
Ahh, good idea- I had contemplated making it MB_LEN_MAX in case someone
does something squirrely and swaps out the process locale in the middle
of a strvis(3), but hadn't yet convinced myself that that wasn't
problematic in many other ways yet.
2. Changed mbslength from ssize_t to size_t, since it can't go
negative, and added some assertions to support this.
This obviates the need to worry about what happens when mblength >
SIZE_MAX -- maybe impossible but I don't need to think about
proving that now!
3. Added another bounds check upstream of the callocs, in case
16*mbslength + 1 would overflow.
4. Eliminated a duplicate call wcslen(start).
5. Kept the #ifdef VIS_NOLOCALE in t_vis.c updated. Not sure why this
is here, but easier to just keep it updated than to investigate.
Attached is the patch series I committed to NetBSD for convenience if
you want to take a look. Tried to avoid unnecessary style differences
from yours to make merging easier if you want.
Perfect, thanks! I'll get this merged downstream as soon as I can.
(I feel like this code shouldn't need to allocate multiple temporary
copies of everything -- should be able to work incrementally in a
stream from src to dst with only a constant-size intermediate buffer
like mbbuf, but I'm not feeling inclined to rewrite this now, so. >
I agree with that sentiment... istrsenvisx in general kind of scares me
a bit still. :-)