mirror of
git://git.code.sf.net/p/cdesktopenv/code
synced 2025-02-12 19:22:41 +00:00
Fix multiple buffer overflows with justified strings (-L/-R/-Z)
ksh crashed in various different and operating system-dependent
ways when attempting to create or apply justification strings
using typeset -L/-R/-Z, especially if large sizes are used.
The crashes had two immediate causes:
- In nv_newattr(), when applying justification attributes, a buffer
was allocated for the justified string that was exactly 8 bytes
longer than the original string. Any larger justification string
caused a buffer overflow (!!!).
- In nv_putval(), when applying existing attributes to a new value,
the corresponding memmove() either did not zero-terminate the
justified string (if the original string was longer than the
justified string) or could read memory past the original string
(if the original string was shorter than the justified string).
Both scenarios can cause a crash.
This commit fixes other minor issues as well, such as a mysterious
8 extra bytes allocated by several malloc/realloc calls. This may
have been some naive attempt to paper over the above bugs. It seems
no one can make any other kind of sense of it.
A readjustment bug with zero-filling was also fixed.
src/cmd/ksh93/sh/name.c:
- nv_putval():
. Get rid of the magical +8 bytes for malloc and realloc. Just
allocate one extra byte for the terminating zero.
. Fix the memmove operation to use strncpy instead, so that
buffer overflows are avoided in both scenarios described above.
Also make it conditional upon a size adjustment actually
happening (i.e. if 'dot' is nonzero).
. Mild refactoring: combine two 'if(sp)' blocks into one;
declare variables only used there locally for legibility.
- nv_newattr():
* Replace the fatally broken "let's allocate string length + 8
bytes no matter the size of the adjustment" routine with a new
one based on work by @hyenias (see comments in #142). It is
efficient with memory, taking into account numeric types,
growing strings, and shrinking strings.
* Fix zero-filling in readjustment after changing the initial
size of a -Z attribute. If the number was zero, all zeros were
still skipped, leaving an empty string.
Thanks to @hyenias for originally identifying this breakage and
laying the groundwork for fixing nv_newattr(), and to @lijog for
the crash analysis that revealed the key to the nv_putval() fix.
Resolves: https://github.com/ksh93/ksh/issues/142
Resolves: https://github.com/ksh93/ksh/issues/181
This commit is contained in:
Martijn Dekker
parent
a959a35291
commit
bdb997415d
3 changed files with 70 additions and 25 deletions
6
NEWS
6
NEWS
|
|
@ -8,6 +8,12 @@ Any uppercase BUG_* names are modernish shell bug IDs.
|
|||
- Fixed a bug introduced on 2021-01-20: if a DEBUG trap action yielded exit
|
||||
status 2, the execution of the next command was not skipped as documented.
|
||||
|
||||
- Fixed multiple buffer overflows causing crashes in typeset -L/-R-/-Z.
|
||||
|
||||
- Fixed typeset -Z zero-filling: if the number was zero, all zeros
|
||||
were skipped when changing the initial size value of the -Z attribute,
|
||||
leaving an empty string.
|
||||
|
||||
2021-02-18:
|
||||
|
||||
- A bug was fixed in the 'read' builtin that caused it to fail to process
|
||||
|
|
|
|||
|
|
@ -1573,9 +1573,7 @@ void nv_putval(register Namval_t *np, const char *string, int flags)
|
|||
Shell_t *shp = sh_getinterp();
|
||||
register const char *sp=string;
|
||||
register union Value *up;
|
||||
register char *cp;
|
||||
register int size = 0;
|
||||
register int dot;
|
||||
int was_local = nv_local;
|
||||
union Value u;
|
||||
#if SHOPT_FIXEDARRAY
|
||||
|
|
@ -1788,7 +1786,7 @@ void nv_putval(register Namval_t *np, const char *string, int flags)
|
|||
else
|
||||
{
|
||||
const char *tofree=0;
|
||||
int offset=0,append;
|
||||
int offset = 0;
|
||||
#if _lib_pathnative
|
||||
char buff[PATH_MAX];
|
||||
#endif /* _lib_pathnative */
|
||||
|
|
@ -1871,9 +1869,13 @@ void nv_putval(register Namval_t *np, const char *string, int flags)
|
|||
tofree = 0;
|
||||
if(nv_isattr(np,NV_LJUST|NV_RJUST) && nv_isattr(np,NV_LJUST|NV_RJUST)!=(NV_LJUST|NV_RJUST))
|
||||
tofree = 0;
|
||||
if (sp)
|
||||
if(!sp)
|
||||
up->cp = NIL(char*);
|
||||
else
|
||||
{
|
||||
append=0;
|
||||
char *cp = NIL(char*); /* pointer to new string */
|
||||
int dot; /* attribute or type length; defaults to string length */
|
||||
int append = 0; /* offset for appending */
|
||||
if(sp==up->cp && !(flags&NV_APPEND))
|
||||
return;
|
||||
dot = strlen(sp);
|
||||
|
|
@ -1953,25 +1955,22 @@ void nv_putval(register Namval_t *np, const char *string, int flags)
|
|||
{
|
||||
if(tofree && tofree!=Empty && tofree!=Null)
|
||||
{
|
||||
cp = (char*)realloc((void*)tofree,((unsigned)dot+append+8));
|
||||
cp = (char*)realloc((void*)tofree,((unsigned)dot+append+1));
|
||||
tofree = 0;
|
||||
}
|
||||
else
|
||||
cp = (char*)malloc(((unsigned)dot+8));
|
||||
cp = (char*)malloc(((unsigned)dot+append+1));
|
||||
cp[dot+append] = 0;
|
||||
nv_offattr(np,NV_NOFREE);
|
||||
}
|
||||
}
|
||||
|
||||
}
|
||||
else
|
||||
cp = 0;
|
||||
up->cp = cp;
|
||||
if(sp)
|
||||
{
|
||||
int c = cp[dot+append];
|
||||
memmove(cp+append,sp,dot);
|
||||
cp[dot+append] = c;
|
||||
if(dot)
|
||||
{
|
||||
char c = cp[dot+append];
|
||||
strncpy(cp+append, sp, dot+1);
|
||||
cp[dot+append] = c;
|
||||
}
|
||||
up->cp = cp;
|
||||
if(nv_isattr(np, NV_RJUST) && nv_isattr(np, NV_ZFILL))
|
||||
rightjust(cp,size,'0');
|
||||
else if(nv_isattr(np, NV_LJUST|NV_RJUST)==NV_RJUST)
|
||||
|
|
@ -3029,9 +3028,41 @@ void nv_newattr (register Namval_t *np, unsigned newatts, int size)
|
|||
if (sp = nv_getval(np))
|
||||
{
|
||||
if(nv_isattr(np,NV_ZFILL))
|
||||
while(*sp=='0') sp++;
|
||||
cp = (char*)malloc((n=strlen (sp)) + 8);
|
||||
strcpy(cp, sp);
|
||||
{
|
||||
while(*sp=='0') sp++; /* skip initial zeros */
|
||||
if(!*sp) sp--; /* if number was 0, leave one zero */
|
||||
}
|
||||
n = strlen(sp);
|
||||
if(size==0 || (newatts&(NV_INTEGER|NV_BINARY)))
|
||||
{
|
||||
/* allocate to match existing value for numerics and auto length assignment for -L/R/Z */
|
||||
cp = (char*)malloc((size_t)n + 1);
|
||||
strcpy(cp, sp);
|
||||
}
|
||||
else if(size>=n)
|
||||
{
|
||||
/* growing string */
|
||||
cp = (char*)malloc((size_t)size + 1);
|
||||
strcpy(cp, sp);
|
||||
}
|
||||
else
|
||||
{
|
||||
/* shrinking string */
|
||||
cp = (char*)malloc((size_t)size + 1);
|
||||
if(newatts&NV_RJUST)
|
||||
strncpy(cp, n - size + sp, size);
|
||||
else
|
||||
{
|
||||
/* NV_LJUST and the rest */
|
||||
if(newatts&NV_ZFILL)
|
||||
{
|
||||
while(*sp=='0') sp++; /* skip initial zeros */
|
||||
if(!*sp) sp--; /* if number was 0, leave one zero */
|
||||
}
|
||||
strncpy(cp, sp, size);
|
||||
}
|
||||
cp[size] = '\0';
|
||||
}
|
||||
if(sp && (mp=nv_opensub(np)))
|
||||
{
|
||||
if(trans)
|
||||
|
|
|
|||
|
|
@ -526,11 +526,11 @@ typeset -A expect=(
|
|||
[F12]='typeset -x -r -F 12 foo'
|
||||
[H]='typeset -x -r -H foo'
|
||||
[L]='typeset -x -r -L 0 foo'
|
||||
# [L17]='typeset -x -r -L 17 foo' # TODO: outputs '-L 0'
|
||||
# [L17]='typeset -x -r -L 17 foo' # TODO: outputs 'typeset -x -r -L 0 foo'
|
||||
[Mtolower]='typeset -x -r -l foo'
|
||||
[Mtoupper]='typeset -x -r -u foo'
|
||||
[R]='typeset -x -r -R 0 foo'
|
||||
# [R17]='typeset -x -r -R 17 foo' # TODO: outputs '-L 0'
|
||||
# [R17]='typeset -x -r -R 17 foo' # TODO: outputs 'typeset -x -r -R 0 foo'
|
||||
[X17]='typeset -x -r -X 17 foo'
|
||||
[S]='typeset -x -r foo'
|
||||
[T]='typeset -x -r foo'
|
||||
|
|
@ -615,19 +615,27 @@ done
|
|||
unset expect
|
||||
|
||||
# ======
|
||||
# Bug introduced in 0e4c4d61: could not alter the size of an existing justified string attribute
|
||||
# https://github.com/ksh93/ksh/issues/142#issuecomment-780931533
|
||||
got=$(unset s; typeset -L 100 s=h; typeset +p s; typeset -L 5 s; typeset +p s)
|
||||
exp=$'typeset -L 100 s\ntypeset -L 5 s'
|
||||
[[ $got == "$exp" ]] || err_exit 'cannot alter size of existing justified string attribute' \
|
||||
"expected $(printf %q "$exp"), got $(printf %q "$got")"
|
||||
"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
|
||||
got=$(unset s; typeset -L 100 s=h; typeset +p s; typeset -R 5 s; typeset +p s)
|
||||
exp=$'typeset -L 100 s\ntypeset -R 5 s'
|
||||
[[ $got == "$exp" ]] || err_exit 'cannot set -R after setting -L' \
|
||||
"expected $(printf %q "$exp"), got $(printf %q "$got")"
|
||||
"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
|
||||
got=$(unset s; typeset -L 100 s=h; typeset +p s; typeset -Z 5 s; typeset +p s)
|
||||
exp=$'typeset -L 100 s\ntypeset -Z 5 -R 5 s'
|
||||
[[ $got == "$exp" ]] || err_exit 'cannot set -Z after setting -L' \
|
||||
"expected $(printf %q "$exp"), got $(printf %q "$got")"
|
||||
"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
|
||||
|
||||
# Old bug: zero-filling zero would cause the zero number to disappear
|
||||
# https://github.com/ksh93/ksh/issues/142#issuecomment-782013674
|
||||
got=$(typeset -i x=0; typeset -Z5 x; echo $x; typeset -Z3 x; echo $x; typeset -Z7 x; echo $x)
|
||||
exp=$'00000\n000\n0000000'
|
||||
[[ $got == "$exp" ]] || err_exit 'failed to zero-fill zero' \
|
||||
"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
|
||||
|
||||
# ======
|
||||
exit $((Errors<125?Errors:125))
|
||||
|
|
|
|||
Loading…
Reference in a new issue