Created attachment 7531 [details] defconfig Heap overflow in decompress_unlzma It is possible to trigger buffer overflow in (archival/libarchive/decompress_unlzma.c line 459) using files from [bug #10436] and specific configuration (see attached defconfig). There is a general problem handling files. With specific defconfig attached unzip fails to check zip fileheader magic (archival/unzip.c line 695) and uses (archival/libarchive/decompress_unlzma.c) for decompression which leads to segmentation fault. Please find attached strace and gdb log leading to the segmentation fault alongside defconfig. $ gdb --args ../busybox.nosuid unzip id_000008,sig_11,src_000775,op_havoc,rep_8 -oqd /tmp (gdb) run Starting program: /home/busybox.nosuid unzip id_000008,sig_11,src_000775,op_havoc,rep_8 -oqd /tmp unzip: removing leading '/' from member names Program received signal SIGSEGV, Segmentation fault. 0x000a6b9c in unpack_lzma_stream (xstate=0xbefff9d8) at /usr/src/debug/busybox/1.27.2-r0/busybox-1.27.2/archival/libarchive/decompress_unlzma.c:459 459 previous_byte = buffer[pos];
Created attachment 7536 [details] strace
Created attachment 7541 [details] gdb log
(In reply to Radovan Scasny from comment #2) $ gdb --args ../busybox.nosuid unzip id_000008,sig_11,src_000775,op_havoc,rep_8 -oqd /tmp (gdb) run Starting program: /home/busybox.nosuid unzip id_000008,sig_11,src_000775,op_havoc,rep_8 -oqd /tmp unzip: removing leading '/' from member names Program received signal SIGSEGV, Segmentation fault. 0x000a6b9c in unpack_lzma_stream (xstate=0xbefff9d8) at /usr/src/debug/busybox/1.27.2-r0/busybox-1.27.2/archival/libarchive/decompress_unlzma.c:459 459 previous_byte = buffer[pos]; The line 459 in actual 1.27.2 source is different: do { uint32_t pos = buffer_pos - rep0; if ((int32_t)pos < 0) pos += header.dict_size; previous_byte = buffer[pos]; IF_NOT_FEATURE_LZMA_FAST(one_byte2:) buffer[buffer_pos++] = previous_byte; if (buffer_pos == header.dict_size) { LINE 459 =====> buffer_pos = 0; global_pos += header.dict_size; I have hard time debugging a problem for which I have no reproducer. Can you give me the archive which causes crash on unpack? Also, please check whether current git crashes too.
(In reply to Radovan Scasny from comment #0) Your config: CONFIG_UNZIP=y # CONFIG_FEATURE_UNZIP_CDF is not set # CONFIG_FEATURE_UNZIP_BZIP2 is not set # CONFIG_FEATURE_UNZIP_LZMA is not set # CONFIG_FEATURE_UNZIP_XZ is not set > There is a general problem handling files. With specific defconfig attached unzip fails to check zip fileheader magic (archival/unzip.c line 695) and uses (archival/libarchive/decompress_unlzma.c) for decompression which leads to segmentation fault. I don't see how unzip.c can possibly call unlzma decompressor if the call sits in this ifdef: #if ENABLE_FEATURE_UNZIP_LZMA else if (zip->fmt.method == 14) { /* Not tested yet */ xstate.bytes_out = unpack_lzma_stream(&xstate); if (xstate.bytes_out < 0) bb_error_msg_and_die("inflate error"); } #endif It is disabled in your config.
*** Bug 10436 has been marked as a duplicate of this bug. ***
Created attachment 7576 [details] New defconfig Previous defconfing file was wrong
Reproducer file from (https://bugs.busybox.net/attachment.cgi?id=7306) $ busybox unzip id_000008,sig_11,src_000775,op_havoc,rep_8 -oqd /tmp archival/libarchive/decompress_unlzma.c 256: buffer = xmalloc(MIN(header.dst_size, header.dict_size)); - header.dst_size = 744 - header.dict_size = 1694695433 header.dst_size < header.dict_size, so buffer is allocated to header.dst_size(477) 462: do { uint32_t pos = buffer_pos - rep0; // buffer_pos = 25, rep0 = 25227794 => pos = 4269739527 if ((int32_t)pos < 0) { // 4269739527 += 1694695433 => pos = 1669467664 pos += header.dict_size; if ((int32_t)pos < 0) goto bad; } previous_byte = buffer[pos]; !!! Here is the problem, buffer is allocated to 744, but pos as index has value 1669467664. I think, the root cause comes more earlier, but a segmentation is only the result.
Fixed in git, thanks
(In reply to Denys Vlasenko from comment #8) Fine, thank you, but what about another lines? I mean 295 and 355. Are those lines ok? From first lookup, there is a similar problem.
(In reply to Andrej Valek from comment #9) I have checked these mentioned lines and they needs to be fixed too.
Do you have an example file which makes bbox crash?
(In reply to Denys Vlasenko from comment #11) I have made some additional testing with the same testing file. additional config enabled: CONFIG_UNLZMA=y CONFIG_FEATURE_LZMA_FAST=y 256: // header.dst_size = 744 // header.dict_size = 1694695433 buffer_size = MIN(header.dst_size, header.dict_size); // buffer is allocated to 744 buffer = xmalloc(buffer_size); 272: // loop while (global_pos + buffer_pos) < 744 while (global_pos + buffer_pos < header.dst_size) { int pos_state = (buffer_pos + global_pos) & pos_state_mask; uint16_t *prob = p + LZMA_IS_MATCH + (state << LZMA_NUM_POS_BITS_MAX) + pos_state; // this condition is valid 24times if (!rc_is_bit_1(rc, prob)) { static const char next_state[LZMA_NUM_STATES] = { 0, 0, 0, 0, 1, 2, 3, 4, 5, 6, 4, 5 }; ... // check if state is >= 7. In this case is never >= 7. if (state >= LZMA_NUM_LIT_STATES) { 292: // !! potential vulnerable part pos = buffer_pos - rep0; if ((int32_t)pos < 0) pos += header.dict_size; match_byte = buffer[pos]; 313: #if ENABLE_FEATURE_LZMA_FAST one_byte1: // buffer_pos increased every loop buffer[buffer_pos++] = previous_byte; // !!! check if buffer_pos is equal to 1694695433. In this case will never be reached if (buffer_pos == header.dict_size) { 333: prob2 = p + LZMA_IS_REP + state; // condition is valid if (!rc_is_bit_1(rc, prob2)) { 352: // !! potential vulnerable part pos = buffer_pos - rep0; if ((int32_t)pos < 0) pos += header.dict_size; ... len = 1; // where is this label defined? goto string; 463: // Why was this macro entered, when CONFIG_FEATURE_LZMA_FAST was enabled? IF_NOT_FEATURE_LZMA_FAST(string:) do { uint32_t pos = buffer_pos - rep0; if ((int32_t)pos < 0) { pos += header.dict_size; /* bug 10436 has an example file where this triggers: */ //if ((int32_t)pos < 0) // goto bad; /* more stringent test (see unzip_bad_lzma_1.zip): */ if (pos >= buffer_size) goto bad; // file is reported as bad, what is ok. Maybe these cases never happens, but I want to be sure. Please, clarify my open points.
Created attachment 7586 [details] new_reproducer
Created attachment 7591 [details] fix_10871 Regarding my previous comment #12, I have found the reproducer for it and many others. First, your fix in #8 is not so good. There is way to access out of buffer even if the pos > 0. (Founded 23 reproducer files.) Right fix should be: 464: do { uint32_t pos = buffer_pos - rep0; if ((int32_t)pos < 0) pos += header.dict_size; /* bug 10436 has an example file where this triggers: */ //if ((int32_t)pos < 0) // goto bad; /* more stringent test (see unzip_bad_lzma_1.zip): */ if (pos >= buffer_size) goto bad; previous_byte = buffer[pos]; Now back into my report. It's necessary to do the same fix for my mentioned lines. (Reproducer attachment 7586 [details]) I have added fixing patch (see attachment).
Added a fix for the second reproducer: $ ./runtest unzip PASS: unzip (subdir only) PASS: unzip (bad archive) PASS: unzip (archive with corrupted lzma 1) PASS: unzip (archive with corrupted lzma 2) <===THIS Thanks!
Reopen if you have another reproducer.
As I have already said in comment #14 . I have about 20 reproducers. Why didn't You use my fixing patch? Your fix doesn't make any sense. Putting 'if (pos >= buffer_size)' into 'if ((int32_t)pos < 0)' is totally wrong. You can't check buffer index only if pos overflowed. I have explained it in the mentioned comment. So, please use my fixing patch. After applying it, you can close this without any pending issues.
Please share your reproducers which still fail. I want to have them in the testsuite.
(In reply to Denys Vlasenko from comment #18) Seems to be, that everything is working. But why did you put the buffer_size checking nested into if? I guess, If pos overflow, index will be wrong.
Because speed. When the check is inside other if(), it usually does not execute. if ((int32_t)pos < 0) { pos += header.dict_size; /* see unzip_bad_lzma_2.zip: */ if (pos >= buffer_size) goto bad; } I understand your point that it looks like the second condition might also happen when pos does not overflow, but (1) I want a proof that it's really possible before I sacrifice decompression speed, and (2) I prefer to have testcase for the testsuite.
OK, understood. I will reopen it, if I found some new test-case, but for now we can close it. Anyway, thanks for clarification.