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

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

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. :-)

>   Thanks for the report!
>   

Thanks,

Kyle Evans