emo/busybox
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

hush: in some cases, expand_on_ifs() relied of uninitialized memory

Browse Source 
The n > 0 check to prevent access to the last byte of non-existing argv[-1]
wasn't enough. Switched to making sure there are initialized (zero) bytes there.

A predictable testcase is rather hard to construct, unfortunately,
contents of memory depends on allocator behavior and whatnot.

function                                             old     new   delta
o_save_ptr_helper                                    119     137     +18
expand_on_ifs                                        345     339      -6
------------------------------------------------------------------------------
(add/remove: 0/0 grow/shrink: 1/1 up/down: 18/-6)              Total: 12 bytes

Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
This commit is contained in:
Denys Vlasenko 2018-07-27 12:14:39 +02:00
parent 7c5f18a3ba
commit 186cf49767
1 changed files with 13 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
shell/hush.c
View File

@ -3069,6 +3069,13 @@ static int o_save_ptr_helper(o_string *o, int n)
o->data = xrealloc(o->data, o->maxlen + 1); o->data = xrealloc(o->data, o->maxlen + 1);
list = (char**)o->data; list = (char**)o->data;
memmove(list + n + 0x10, list + n, string_len); memmove(list + n + 0x10, list + n, string_len);
/*
* expand_on_ifs() has a "previous argv[] ends in IFS?"
* check. (grep for -prev-ifs-check-).
* Ensure that argv[-1][last] is not garbage
* but zero bytes, to save index check there.
*/
list[n + 0x10 - 1] = 0;
o->length += 0x10 * sizeof(list[0]); o->length += 0x10 * sizeof(list[0]);
} else { } else {
debug_printf_list("list[%d]=%d string_start=%d\n", debug_printf_list("list[%d]=%d string_start=%d\n",
@ -5797,12 +5804,16 @@ static int expand_on_ifs(o_string *output, int n, const char *str)
/* Start new word... but not always! */ /* Start new word... but not always! */
/* Case "v=' a'; echo ''$v": we do need to finalize empty word: */ /* Case "v=' a'; echo ''$v": we do need to finalize empty word: */
if (output->has_quoted_part if (output->has_quoted_part
/* Case "v=' a'; echo $v": /*
* Case "v=' a'; echo $v":
* here nothing precedes the space in $v expansion, * here nothing precedes the space in $v expansion,
* therefore we should not finish the word * therefore we should not finish the word
* (IOW: if there *is* word to finalize, only then do it): * (IOW: if there *is* word to finalize, only then do it):
* It's okay if this accesses the byte before first argv[]:
* past call to o_save_ptr() cleared it to zero byte
* (grep for -prev-ifs-check-).
*/ */
|| (n > 0 && output->data[output->length - 1]) || output->data[output->length - 1]
) { ) {
new_word: new_word:
o_addchr(output, '\0'); o_addchr(output, '\0');