Re: ncurses buffer overflows (fwd)

[Thomas E. Dickey]

On Mon, 2 Oct 2000, [iso-8859-1] Jouko Pynn?nen wrote:

> 
> On Mon, 2 Oct 2000, Thomas E. Dickey wrote:
> 
> > ...and does it break if we limit the strcpy's in lib_mvcur.c ?
> 
> My sense says yes, but I haven't tried yet.
> 
> > (it's a small change to relatively stable code: it would make more sense
> > to distribute the patch than rely on someone else to deduce it).
> 
> Hmm, there are many strcpy's in that file, one in repeated_append (doesn't
> seem so dangerous, at least tries to check bounds), a few in relative_move
> (dangerous), a few in onscreen_mvcur (dangerous).

they all go to fixed-size buffers (OPT_SIZE).  basically the fix would be
to truncate the result from tparm to that limit and then copy that
(verifying that's all that would be needed is the real work).
 
> Fix alternatives IMHO:
> 1) As far as I can see, it would be safe to change them all to 
> strncpy(a,b,OPT_SIZE-1) and do null-termination. They all seem to have
> char use[OPT_SIZE] as the destination.

strncpy is inefficient because it fills the destination with that many
bytes (so I look for other solutions).

> 2) The loading routines could (should?) limit the entry length, that would

they already do that...

> fix using things like /dev/zero to consume resources. It's not enough
> alone though. Even a short terminfo entry having things like %09999d could
> still overflow the buffer.
> 
> 3) tparm() could limit the length of the returned string to OPT_SIZE-1, so

no - it's only the use in mvcur that should be limited, since that has
the fixed-size buffers.

> its result could never overflow a buffer (except it the application is
> stupid enough to strcpy() them to a smaller buffer, or similar).
> 
> 4) The terminfo loaded could skip TERMINFO_PATHS and $HOME/.terminfo/ if
> the program runs setuid/setgid. How many people need a "customized"
> terminfo anyway? Almost none, and how many setuid/setgid programs need

probably true for those cases ($TERM, no).

> ncurses? Almost none. This would also fix possible unrevealed problems in
> parsing a terminfo file or other library functions...

that isn't clear.
 
> One more thing: in parse_format() or something like that, there are two
> variables for the %123.456d type field length and precision. They're
> declared int; if someone uses length that's greater than 0x7fffffff and a
> small precision value, space will be allocated for the small number of
> bytes, because (int)0x7fffffff < small number. And sprintf() will overflow
> the buffer if libc parses the number as unsigned int.

actually, a numeric overflow check is needed (I did not spot that in my
recent change).
 
>   Jouko
> 
> 

-- 
T.E.Dickey <address@hidden>
http://dickey.his.com
ftp://dickey.his.com