Bug 10871

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.