Quicklz decompression memory corruption issue fix

[Chaloff]

There is a memory corruption issue inside the quicklz.c source file that ships with Percona XtraBackup. Specifically the problem happens on copying user-supplied binary data over heap allocated memory buffers of user-controlled size. This allows corruption of heap data structures and potential arbitrary code execution.

The code in question is inside the qlz_decompress function of quicklz.c file:

size_t qlz_decompress(const char *source, void *destination, qlz_state_decompress *state)
{
        size_t dsiz = qlz_size_decompressed(source);

        if (state->stream_counter + qlz_size_decompressed(source) - 1 >= QLZ_STREAMING_BUFFER)
        {
                if((*source & 1) == 1)
                {
                        reset_table_decompress(state);
                        dsiz = qlz_decompress_core((const unsigned char *)source, (unsigned char *)destination, dsiz, state, (const unsigned char *)destination);
                }
                else
                {
                        memcpy(destination, source + qlz_size_header(source), dsiz);
                }
                state->stream_counter = 0;
                reset_table_decompress(state);
        }
        else
        {
                unsigned char *dst = state->stream_buffer + state->stream_counter;
                if((*source & 1) == 1)
                {
                        dsiz = qlz_decompress_core((const unsigned char *)source, dst, dsiz, state, (const unsigned char *)state->stream_buffer);
                }
                else
                {
                        memcpy(dst, source + qlz_size_header(source), dsiz);
                        reset_table_decompress(state);
                }
                memcpy(destination, dst, dsiz);
                state->stream_counter += dsiz;
        }
        return dsiz;
}

Note the first memcpy invocation: that does copy data from user-provided compressed file into a heap-allocated buffer for which size is also controlled by the user via the compressed file header. This allows heap corruption with user-controlled data. Potentially this means arbitrary code execution for the processes that utilize the vulnerable function - one example is xbstream with —decompress flag.

Steps to reproduce:

• Create a compressed file, e.g. with qpress from some file larger than 65535 bytes.
• Edit compressed file so that the four bytes at offset 8 are changed to be less than 0x10000, for example set to 0x1000 instead.
• Edit the file so that the byte at offset 50 is an even value to pass the test: if((*source & 1) == 1)
• Replace the bytes of actual file with some recognizable pattern, e.g. 0x41 0x42 0x43 0x44
• Add the file to an xbstream file: xbstream -c Demo.qp > Demo.xbstream
• Now try to extract with decompression using xbstream under a debugger, e.g. gdb and observe the corruption: xbstream —decompress -x < Demo.xbstream

head -c 100000 </dev/urandom > payload.bin

qpress payload.bin payload.qp

ls -l payload.qp -rw-r--r-- 1 me me 100107 Feb 17 18:08 payload.qp

printf '\x00\x01\x00' | dd of=payload.qp bs=1 seek=8 count=3 conv=notrunc

printf '\x10' | dd of=payload.qp bs=1 seek=49 count=1 conv=notrunc

python -c 'import sys; sys.stdout.write("A"*100040)' | dd of=payload.qp bs=1
seek=50 count=100040 conv=notrunc

xbstream-80 -c payload.qp > corrupted.xbstream

$ xbstream-80 --decompress -x < corrupted.xbstream Segmentation fault ```

Fix by prevent XtraBackup read/write outside array bounds

All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.

[it-percona-cla]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[ottok]

Related to this we have also submitted https://github.com/PierreLvx/qpress/pull/6

[ottok]

Related blog post: https://lavaux.lv/2022/08/21/qpress-file-archiver-security-update.html

[ottok]

Any possibility to get a review on this one?

[altmannmarcelo]

Hi @ottok and @Chaloff .

First of all, thanks for providing the patch for this issue. We have raised an internal bug to keep track of it https://jira.percona.com/browse/PXB-2854.

This issue is currently a blocker for our next release. We are in the process of working on the issues that will be part of the release and this PR will get reviewed soon.

Thanks

[altmannmarcelo]

@Chaloff I am working on reviewing this fix and merging it to our next release branch. Can you please sign the CLA agreement at https://github.com/percona/percona-xtrabackup/pull/1366#issuecomment-1220960754

[ottok]

AWS does not sign CLAs. We contribute this with the open source license of the project.

[altmannmarcelo]

Hi @Chaloff @ottok - Did not hear any feedback in a few weeks. Are you interested in continue working on this PR?

[Chaloff]

Hi @Chaloff @ottok - Did not hear any feedback in a few weeks. Are you interested in continue working on this PR?

Yes, sorry - was busy, will proceed with the PR this week

[altmannmarcelo]

Hi @Chaloff . I am not sure if your last force push is intended to fix the encrypt issue. I tested it and I can still see the error:

 ╰▶ $ xbstream -xv --parallel=1 --decompress --decompress-threads=1 --decrypt=AES256 --encrypt-key='percona_xtrabackup_is_awesome___' --encrypt-threads=1 -C out < backup.out 
sys/sys_config.ibd.qp.xbcrypt
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Assertion "threads[i].to_len > 0" failed at /work/pxb/src/8.0/storage/innobase/xtrabackup/src/ds_decompress.cc:241
Aborted (core dumped)

[Chaloff]

Hi @Chaloff . I am not sure if your last force push is intended to fix the encrypt issue. I tested it and I can still see the error:

 ╰▶ $ xbstream -xv --parallel=1 --decompress --decompress-threads=1 --decrypt=AES256 --encrypt-key='percona_xtrabackup_is_awesome___' --encrypt-threads=1 -C out < backup.out 
sys/sys_config.ibd.qp.xbcrypt
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Assertion "threads[i].to_len > 0" failed at /work/pxb/src/8.0/storage/innobase/xtrabackup/src/ds_decompress.cc:241
Aborted (core dumped)

Checking...

[altmannmarcelo]

Hi @Chaloff

Using latest commit the same issue still happening:

 🐬 marcelo  📂 /tmp  ▶ 
 ╰▶ $ xbstream --version
xbstream  Ver 8.0.29-22 for Linux (x86_64) (revision id: 906fec986e5)

 🐬 marcelo  📂 /tmp  ▶ 
 ╰▶ $ xbstream -xv --parallel=1 --decompress --decompress-threads=1 --decrypt=AES256 --encrypt-key='percona_xtrabackup_is_awesome___' --encrypt-threads=1 -C out < backup.out 
sys/sys_config.ibd.qp.xbcrypt
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Assertion "threads[i].to_len > 0" failed at /work/pxb/src/8.0/storage/innobase/xtrabackup/src/ds_decompress.cc:241
Aborted (core dumped)

[Chaloff]

Hi @Chaloff

Using latest commit the same issue still happening:

 🐬 marcelo  📂 /tmp  ▶ 
 ╰▶ $ xbstream --version
xbstream  Ver 8.0.29-22 for Linux (x86_64) (revision id: 906fec986e5)

 🐬 marcelo  📂 /tmp  ▶ 
 ╰▶ $ xbstream -xv --parallel=1 --decompress --decompress-threads=1 --decrypt=AES256 --encrypt-key='percona_xtrabackup_is_awesome___' --encrypt-threads=1 -C out < backup.out 
sys/sys_config.ibd.qp.xbcrypt
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Assertion "threads[i].to_len > 0" failed at /work/pxb/src/8.0/storage/innobase/xtrabackup/src/ds_decompress.cc:241
Aborted (core dumped)

I probably need some assistance here if you don't mind. The fix in qpress are pretty simple and well tested - it just check boundaries of two arrays (source and target) before decompress. The problem seems to be in calling this qpress function - qlz_decompress(...) - we need to pass the allocated size of source and target arrays to be able to check against it. I do it like this: thd->to_alloc_size = decomp_file->decomp_ctxt->chunk_size; thd->from_alloc_size = qlz_size_compressed(decomp_file->header); Looks like it's incorrect way. Can you advise me here how to do it correctly? Thanks in advance

[altmannmarcelo]

Hi @Chaloff . I spent sometime investigating this issue. I have a particular file that is a copy of my.cnf during backup:

🐬 marcelo  📂 /work/issues/PXB-2854/individual/qpress  ▶ 
 ╰▶ $ cat backup-my.cnf 
# This MySQL options file was generated by innobackupex.

# The MySQL server
[mysqld]
innodb_checksum_algorithm=crc32
innodb_log_checksums=1
innodb_data_file_path=ibdata1:12M:autoextend
innodb_log_file_size=1073741824
innodb_page_size=16384
innodb_undo_directory=./
innodb_undo_tablespaces=2
server_id=0
innodb_log_checksums=ON
innodb_redo_log_encrypt=OFF
innodb_undo_log_encrypt=OFF
plugin_load=keyring_file.so
server_uuid=e84bec9f-7623-11ed-ab99-60a4b722b33a
master_key_id=0

I am sharing the file compressed with qpress and compressed with qpress via xbstream (on xbstream format).

When checking the same file with qpress under GDB I see that it produces the from_alloc_size as 403 (for reference, we are here):

(gdb) n
692	        if (QLZ_SIZE_COMPRESSED(src[thread_id]) > compress_chunk_size + QLZ_SIZE_OVERHEAD)
(gdb) p src[thread_id]
$8 = 0x5555555af280 "G\223\001"
(gdb) call QLZ_SIZE_COMPRESSED(src[thread_id])
$9 = 403

Adjusting xbstream code to use decomp_file->nbytes_expected when we have the state as STATE_PROCESS_DIRECT_BLOCK_DATA gives us the same source_alloc_size :

Thread 2 "xbstream" hit Breakpoint 2, qlz_decompress_core (source=0x7ffff0000c09 "G\223\001", destination=0x7ffff000a1b0 "# This MySQL options file was generated by innobackupex.\n\n#", size=477, state=0x7ffff0000ff0, history=0x7ffff000a1b0 "# This MySQL options file was generated by innobackupex.\n\n#", source_alloc_size=403, dest_alloc_size=65536) at /work/pxb/src/8.0/storage/innobase/xtrabackup/src/quicklz/quicklz.c:677

However, the second condition to the if is returning true:

Thread 2 "xbstream" hit Breakpoint 2, qlz_decompress_core (source=0x7ffff0000c09 "G\223\001", destination=0x7ffff000a1b0 "# This MySQL options file was generated by innobackupex.\n\n#", size=477, state=0x7ffff0000ff0, history=0x7ffff000a1b0 "# This MySQL options file was generated by innobackupex.\n\n#", source_alloc_size=403, dest_alloc_size=65536) at /work/pxb/src/8.0/storage/innobase/xtrabackup/src/quicklz/quicklz.c:677
677				if((offset2 < source || offset2 + matchlen > source + source_alloc_size) || (dst < destination || dst + matchlen > destination + dest_alloc_size))

(gdb) p offset2
$32 = (const unsigned char *) 0x7ffff000a1b1 " This MySQL options file was generated by innobackupex.\n\n#"
(gdb) p source
$33 = (const unsigned char *) 0x7ffff0000c09 "G\223\001"
(gdb) p offset2 < source
$34 = 0
(gdb) p matchlen
$35 = 3
(gdb) p source_alloc_size
$36 = 403
(gdb) p offset2 + matchlen > source + source_alloc_size <-- THIS ONE -->
$37 = 1
(gdb) p dst
$38 = (unsigned char *) 0x7ffff000a1eb ""
(gdb) p destination
$39 = (unsigned char *) 0x7ffff000a1b0 "# This MySQL options file was generated by innobackupex.\n\n#"
(gdb) p dst < destination
$40 = 0
(gdb) p dest_alloc_size
$41 = 65536
(gdb) p dst + matchlen > destination + dest_alloc_size
$42 = 0
(gdb) 

By disabling the error (return 0) on that if, I can see that the check fails multiple times for this file. However, it does produce the correct file as result and also the file has the same resulting size of 477 bytes as we computed when calling qlz_decompress_core (Thread 2 "xbstream" hit Breakpoint 2, qlz_decompress_core (source=0x7ffff0000c09 "G\223\001", destination=0x7ffff000a1b0 "# This MySQL options file was generated by innobackupex.\n\n#", size=477, ) :

🐬 marcelo  📂 /work/issues/PXB-2854/individual/xbstream  ▶ 
 ╰▶ $ xbstream -x --decompress < ../backup-my.cnf.qp.xbs
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 

 🐬 marcelo  📂 /work/issues/PXB-2854/individual/xbstream  ▶ 
 ╰▶ $ ls -la
total 12
drwxrwxr-x 2 marcelo marcelo 4096 dez  7 14:13 .
drwxrwxr-x 7 marcelo marcelo 4096 dez  7 11:33 ..
-rw-r----- 1 marcelo marcelo  477 dez  7 14:13 backup-my.cnf

 🐬 marcelo  📂 /work/issues/PXB-2854/individual/xbstream  ▶ 
 ╰▶ $ cat backup-my.cnf
# This MySQL options file was generated by innobackupex.

# The MySQL server
[mysqld]
innodb_checksum_algorithm=crc32
innodb_log_checksums=1
innodb_data_file_path=ibdata1:12M:autoextend
innodb_log_file_size=1073741824
innodb_page_size=16384
innodb_undo_directory=./
innodb_undo_tablespaces=2
server_id=0
innodb_log_checksums=ON
innodb_redo_log_encrypt=OFF
innodb_undo_log_encrypt=OFF
plugin_load=keyring_file.so
server_uuid=e84bec9f-7623-11ed-ab99-60a4b722b33a
master_key_id=0

 🐬 marcelo  📂 /work/issues/PXB-2854/individual/xbstream  ▶ 

Files I used:

# backup-my.cnf.qp
curl https://gist.githubusercontent.com/altmannmarcelo/af0660cea41affc20b515c0993c77899/raw/45936d3a25a022a507c46f5b136b60b8ce39e72a/backup-my.cnf.qp | base64 -d > backup-my.cnf.qp

# backup-my.cnf.qp.xbs
curl https://gist.githubusercontent.com/altmannmarcelo/22e61a4abf48ae790774be9b7c88ee4b/raw/155da4a480915577813242f63c541ee5df165677/backup-my.cnf.qp.xbs | base64 -d > backup-my.cnf.qp.xbs

[Chaloff]

Hi @Chaloff . I spent sometime investigating this issue. I have a particular file that is a copy of my.cnf during backup:

Thanks for the support - looking...

[dlenski]

@Chaloff, it appears that in the latest version of your code (dac3d82), it is no longer checking the data size in the header against the size of the actual data region of the file.

That means it is no longer actually addressing the vulnerability it is intended to address (= qpress trusts the user-supplied header, when it shouldn't).

Am I missing something?

[Chaloff]

@Chaloff, it appears that in the latest version of your code (dac3d82), it is no longer checking the data size in the header against the size of the actual data region of the file.

That means it is no longer actually addressing the vulnerability it is intended to address (= qpress trusts the user-supplied header, when it shouldn't).

Am I missing something?

Yes, you are missing something but thanks anyway for the comment :) Please see the change on line 31 in quicklz.h This simple change triggers the code on lines 576, 584, 669 and 711 in quicklz.c which does the boundary check. Additionally we can corruption check on line 814 as suggested in this article - see "Tackling the last alarms" - we only need to write it correctly - I suppose the correct code should looks like: if(qlz_size_decompressed(source) != qlz_size_compressed(source) + qlz_size_header(source)) return 0;

[altmannmarcelo]

Hi @Chaloff . I confirm that all tests are passing now including a new test case added to cover the memory corruption issue. Code has been incorporated into 8.0 trunk.

Thanks for your contribution!