Re: lib/57573: Overflow possibilities in vis(3)

[Kyle Evans <kevans%FreeBSD.org@localhost>]

The following reply was made to PR lib/57573; it has been noted by GNATS.

From: Kyle Evans <kevans%FreeBSD.org@localhost>
To: gnats-bugs%netbsd.org@localhost, lib-bug-people%netbsd.org@localhost, gnats-admin%netbsd.org@localhost,
 netbsd-bugs%netbsd.org@localhost
Cc: 
Subject: Re: lib/57573: Overflow possibilities in vis(3)
Date: Sun, 13 Aug 2023 00:30:32 -0500

 On 8/12/23 22:35, Kyle Evans wrote:
 > 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. :-)
 > 
 
 One tiny nit: SIZE_MAX is formally defined in <stdint.h>, so I think 
 there's some header pollution afoot in NetBSD; I've added the include to 
 vis.c downstream.
 
 Thanks,
 
 Kyle Evans