Re: Overflow in out_limit calculation

[Miroslav Lichvar]

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

> (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