Re: Overflow in out_limit calculation

[Thomas Dickey]

On Tue, May 07, 2024 at 08:35:29AM +0200, Miroslav Lichvar wrote:
> On Mon, May 06, 2024 at 03:18:13PM -0400, Thomas Dickey wrote:
> > On Mon, May 06, 2024 at 03:07:24PM +0200, Miroslav Lichvar wrote:
> > > A static analyzer reports a possible integer overflow in
> > > _nc_setupscreen(): 
> > > 
> > >   sp->out_limit = (size_t) ((2 + slines) * (6 + scolumns));
> > > 
> > > slines and scolumns are ints, possibly parsed from the well-known
> > > environment variables, before they are validated in newwin()->...->
> > > dimension_limit(), which might accept any value if NCURSES_SIZE_T is
> > > int.

I use int in my test package, but don't recall any packager doing that,
because it affects binary compatibility.

> > In a quick read/review, it seems that this is executed after
> > calling _nc_get_screensize, which calls _nc_default_screensize,
> > which ensures that those values are positive.
> 
> They are positive, but can be large enough to make the multiplication
> of signed integers overflow, which is undefined behavior. I can
> reproduce it by setting COLUMNS=100000 LINES=100000.

I see (first time I recall this one being pointed out - a fix is needed).
I was thinking of sign-extension in the cast, rather than just out-of-range
size.

Even with 32k by 32k (the limit before extended numbers), that's excessive.

Occasionally I encounter someone who claims they need something like 500x500
(really?), which led to the SGR mouse feature in xterm.

I suppose I'll start with 512x512 and see who complains.

But a different limit is needed for pads (which can be larger than a window).
 
> > (The static analyzer might give better info, with the caveat that
> > I've seen a significant number of false positives over the past year
> > from both gcc and clang - and oddly enough, sometimes the same false
> > positive).
> 
> Here is the report. It seems to be concerned about the malloc, but
> that part seems fine as the other code using the buffer checks
> out_limit instead of assuming it's columns*lines large. (Does the
> buffer actually need to have a char for each cell of the screen?)
> 
> Error: INTEGER_OVERFLOW (CWE-190):
> ncurses-6.4-20240127/ncurses/base/lib_set_term.c:390: tainted_data_argument: 
> The value returned in "scolumns" is considered tainted.
> ncurses-6.4-20240127/ncurses/base/lib_set_term.c:444: overflow: The tainted 
> expression "scolumns" is used in an arithmetic operation. The expression "6 + 
> scolumns" is considered to have possibly overflowed.
> ncurses-6.4-20240127/ncurses/base/lib_set_term.c:444: overflow: The 
> expression "(2 + slines) * (6 + scolumns)" is deemed overflowed because at 
> least one of its arguments has overflowed.
> ncurses-6.4-20240127/ncurses/base/lib_set_term.c:444: cast_overflow: An 
> assign that casts to a different type, which might trigger an overflow.
> ncurses-6.4-20240127/ncurses/base/lib_set_term.c:445: overflow_sink: 
> "sp->out_limit", which might have underflowed, is passed to 
> "malloc(sp->out_limit)".
> #  443|   #endif
> #  444|       sp->out_limit = (size_t) ((2 + slines) * (6 + scolumns));
> #  445|->     if ((sp->out_buffer = malloc(sp->out_limit)) == 0)
> #  446|           sp->out_limit = 0;
> #  447|       sp->out_inuse = 0;
> 
> -- 
> Miroslav Lichvar
> 

-- 
Thomas E. Dickey <dickey@invisible-island.net>
https://invisible-island.net

signature.asc
Description: PGP signature