Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PXB-2854 - Quicklz decompression memory corruption issue fix #1366

Closed
wants to merge 1 commit into from

Conversation

Chaloff
Copy link
Contributor

@Chaloff Chaloff commented Aug 19, 2022

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
Copy link

it-percona-cla commented Aug 19, 2022

CLA assistant check
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
Copy link

ottok commented Aug 19, 2022

Related to this we have also submitted PierreLvx/qpress#6

@ottok
Copy link

ottok commented Aug 22, 2022

@ottok
Copy link

ottok commented Sep 16, 2022

Any possibility to get a review on this one?

@altmannmarcelo
Copy link
Contributor

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
Copy link
Contributor

@Chaloff I am working on reviewing this fix and merging it to our next release branch. Can you please sign the CLA agreement at #1366 (comment)

@ottok
Copy link

ottok commented Oct 5, 2022 via email

Copy link
Contributor

@altmannmarcelo altmannmarcelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will get back on the license once I hear back internally.

For now, I can see that the provided patch breaks the software functionality:

xtrabackup --backup --port=3306 --stream=xbstream --parallel=16 --compress --compress-threads=4 --encrypt=AES256 --encrypt-key='percona_xtrabackup_is_awesome___' --encrypt-threads=4 --encrypt-chunk-size=8K > backup.out

mkdir out

xbstream -xv --parallel=1 --decompress --decompress-threads=1 --decrypt=AES256 --encrypt-key='percona_xtrabackup_is_awesome___' --encrypt-threads=1 -C out < backup.out 

This produces an error:

sys/sys_config.ibd.qp.xbcrypt
Error: compressed file was corrupted - header data size and actual data size mismatch - can’t decompress 
decompress: error running decompression.
decrypt: write to destination failed.
xbstream: my_write() failed.
exit code: 1

@altmannmarcelo
Copy link
Contributor

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

@Chaloff
Copy link
Contributor Author

Chaloff commented Oct 24, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

Chaloff commented Oct 28, 2022

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 altmannmarcelo changed the title Quicklz decompression memory corruption issue fix PXB-2854 - Quicklz decompression memory corruption issue fix Nov 15, 2022
@altmannmarcelo
Copy link
Contributor

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
Copy link
Contributor Author

Chaloff commented Nov 15, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

Chaloff commented Dec 7, 2022

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...

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 ``` We noticed that
quicklz already has a mechanism to check if the data was corrupted and prevent
reading and writing outside array bounds. Programm should be compiled against
QLZ_MEMORY_SAFE flag. So we uncommented the flag and add the error message.

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.
@dlenski
Copy link

dlenski commented Dec 9, 2022

@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
Copy link
Contributor Author

Chaloff commented Dec 9, 2022

@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
Copy link
Contributor

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants