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.

Sign up for GitHub

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

Jump to bottom

Heap buffer overflow in xrow_decode_error #9098

Closed
ligurio opened this issue Sep 6, 2023 · 1 comment · Fixed by #9114
Closed

Heap buffer overflow in xrow_decode_error #9098

ligurio opened this issue Sep 6, 2023 · 1 comment · Fixed by #9114
Assignees
@Gumix
Labels
2.11 Target is 2.11 and all newer release/master branches bug Something isn't working

Comments

@ligurio
Copy link
Member

ligurio commented Sep 6, 2023 •
edited

Bug description

On using function xrow_decode_error Address Sanitizers reports heap-buffer-overflow.
The bug was found by fuzzing test committed in a branch https://github.com/ligurio/tarantool/tree/ligurio/gh-xxxx-add-xrow_decode_error_fuzzer and PR #9022.

  • OS: Linux
  • OS Version: Ubuntu 22.04
  • Architecture: amd64
  • Compiler: Clang 17

Steps to reproduce

How to run:

Apply a patch below and run cmake -S . -B build -DENABLE_ASAN=ON -DENABLE_FUZZER=ON && cmake --build build --parallel --target xrow_decode_error_fuzzer && ./build/test/fuzz/xrow_decode_error_fuzzer <path to reproducer>.

Reproducer:
crash-97b93cdd0c8d06e7fadafb5feccfdbbd13f12da3.txt

Actual behavior

=================================================================
==27==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200001fbdf at pc 0x00000041f8ae bp 0x7ffd42f2ee10 sp 0x7ffd42f2e5e0
WRITE of size 16 at 0x60200001fbdf thread T0 (unknown)
SCARINESS: 45 (multi-byte-write-heap-buffer-overflow)
    #0 0x41f8ad in __asan_memset /src/llvm-project/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:26:3
    #1 0x51a775 in error_payload_destroy /src/tarantool/src/lib/core/error_payload.c:21:3
    #2 0x4be1c8 in mp_error_destroy(mp_error*) /src/tarantool/src/box/mp_error.cc:135:2
    #3 0x4b1cf1 in mp_decode_error_one(char const**) /src/tarantool/src/box/mp_error.cc:383:2
    #4 0x4b0440 in error_unpack_unsafe /src/tarantool/src/box/mp_error.cc:498:25
    #5 0x47ba7e in xrow_decode_error /src/tarantool/src/box/xrow.c:1715:22
    #6 0x45f31d in LLVMFuzzerTestOneInput /src/tarantool/test/fuzz/xrow_decode_error_fuzzer.c:38:2
    #7 0x32eec3 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:611:15
    #8 0x32e6aa in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:514:3
    #9 0x32fd79 in fuzzer::Fuzzer::MutateAndTestOne() /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:757:19
    #10 0x330a45 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, std::__Fuzzer::allocator<fuzzer::SizedFile> >&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:895:5
    #11 0x31f15f in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:912:6
    #12 0x349402 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #13 0x7f3e40753082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082) (BuildId: 1878e6b475720c7c51969e69ab2d276fae6d1dee)
    #14 0x30fb9d in _start (build-out/xrow_decode_error_fuzzer+0x30fb9d)

DEDUP_TOKEN: __asan_memset--error_payload_destroy--mp_error_destroy(mp_error*)
0x60200001fbdf is located 0 bytes to the right of 15-byte region [0x60200001fbd0,0x60200001fbdf)
allocated by thread T0 (unknown) here:
    #0 0x420236 in __interceptor_malloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:69:3
    #1 0x51b4fa in error_payload_prepare /src/tarantool/src/lib/core/error_payload.c:58:6
    #2 0x51d466 in error_payload_set_mp /src/tarantool/src/lib/core/error_payload.c:259:10
    #3 0x4bc545 in mp_decode_error_fields(char const**, mp_error*, region*) /src/tarantool/src/box/mp_error.cc:311:3
    #4 0x4b19cd in mp_decode_error_one(char const**) /src/tarantool/src/box/mp_error.cc:368:8
    #5 0x4b0440 in error_unpack_unsafe /src/tarantool/src/box/mp_error.cc:498:25
    #6 0x47ba7e in xrow_decode_error /src/tarantool/src/box/xrow.c:1715:22
    #7 0x45f31d in LLVMFuzzerTestOneInput /src/tarantool/test/fuzz/xrow_decode_error_fuzzer.c:38:2
    #8 0x32eec3 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:611:15
    #9 0x32e6aa in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:514:3
    #10 0x32fd79 in fuzzer::Fuzzer::MutateAndTestOne() /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:757:19
    #11 0x330a45 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, std::__Fuzzer::allocator<fuzzer::SizedFile> >&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:895:5
    #12 0x31f15f in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:912:6
    #13 0x349402 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #14 0x7f3e40753082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082) (BuildId: 1878e6b475720c7c51969e69ab2d276fae6d1dee)

DEDUP_TOKEN: __interceptor_malloc--error_payload_prepare--error_payload_set_mp
SUMMARY: AddressSanitizer: heap-buffer-overflow /src/llvm-project/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:26:3 in __asan_memset
Shadow bytes around the buggy address:
  0x0c047fffbf20: fa fa 04 fa fa fa 04 fa fa fa fd fa fa fa fd fd
  0x0c047fffbf30: fa fa fd fd fa fa fd fd fa fa fd fa fa fa fd fd
  0x0c047fffbf40: fa fa fd fd fa fa fd fa fa fa fd fd fa fa fd fd
  0x0c047fffbf50: fa fa fd fa fa fa fd fd fa fa fd fd fa fa fd fa
  0x0c047fffbf60: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
=>0x0c047fffbf70: fa fa 04 fa fa fa fd fa fa fa 00[07]fa fa fa fa
  0x0c047fffbf80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fd fd
  0x0c047fffbf90: fa fa fd fa fa fa fd fd fa fa fd fa fa fa fa fa
  0x0c047fffbfa0: fa fa fd fd fa fa fd fd fa fa 04 fa fa fa fd fd
  0x0c047fffbfb0: fa fa fd fd fa fa fa fa fa fa 00 fa fa fa fa fa
  0x0c047fffbfc0: fa fa fd fa fa fa fd fa fa fa fd fd fa fa 04 fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==27==ABORTING
MS: 3 InsertByte-CMP-CopyPart- DE: "\024\001"-; base unit: 49b7d27b17202cb6ea1042cc5995c50d51f510ba
0x8b,0xff,0xff,0x3f,0x0,0x0,0x31,0x21,0x32,0x52,0x88,0x10,0x0,0x0,0x93,0x8b,0x31,0x32,0x0,0xa1,0x31,0x30,0xc,0x0,0xa1,0x32,0x0,0xa4,0xa1,0x0,0x0,0x83,0x6,0x89,0xa1,0x31,0x30,0xc,0x0,0xa1,0x32,0x0,0xa4,0xa1,0x0,0x0,0x83,0x6,0x89,0xff,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x2f,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x14,0xff,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x2f,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x14,0x1,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x41,0x80,0xfe,0x32,0x0,0x0,0xa1,0x31,0x3,0xd2,0xd2,0xd2,0xd2,0xd2,0x30,0xc,0x3a,0x30,0x0,0x0,0x1,0x0,
\213\377\377?\000\0001!2R\210\020\000\000\223\21312\000\24110\014\000\2412\000\244\241\000\000\203\006\211\24110\014\000\2412\000\244\241\000\000\203\006\211\377\000\000\000\000\000\000\000/\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\024\377\000\000\000\000\000\000\000/\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\024\001\377\377\377\377\377\377\377\377\377\377\377\377A\200\3762\000\000\2411\003\322\322\322\322\3220\014:0\000\000\001\000
artifact_prefix='/tmp/tmphi3_xxd2/'; Test unit written to /tmp/tmphi3_xxd2/crash-ab0abeea579c234e396a14450af8d5d9194330e3
Base64: i///PwAAMSEyUogQAACTizEyAKExMAwAoTIApKEAAIMGiaExMAwAoTIApKEAAIMGif8AAAAAAAAAL/////////////////////8U/wAAAAAAAAAv/////////////////////xQB////////////////QYD+MgAAoTED0tLS0tIwDDowAAABAA==
stat::number_of_executed_units: 1796808
stat::average_exec_per_sec:     37433
stat::new_units_added:          3878
stat::slowest_unit_time_sec:    0
stat::peak_rss_mb:              66

Expected behavior

no heap-buffer-overflow
@ligurio ligurio added the bug Something isn't working label Sep 6, 2023
@ligurio ligurio mentioned this issue Sep 6, 2023
Merged
@Gumix Gumix assigned Gumix and unassigned alyapunov Sep 6, 2023
@Gumix
Copy link
Contributor

Gumix commented Sep 6, 2023 •
edited

We allocate 15 bytes for struct error_field in error_payload_prepare(), that's the correct size for the given data (data_offset = 14, value_size = 1, extra = 0).

tarantool/src/lib/core/error_payload.h

Lines 17 to 24 in 307f3c5

struct error_field {
/** MessagePack field value. */
char *data;
/** Data size. */
uint32_t size;
/** Zero terminated field name. */
char name[1];
};

tarantool/src/lib/core/error_payload.c

Line 58 in 307f3c5

f = xmalloc(total);

But in error_payload_destroy() the TRASH() macro calculates sizeof(*p->fields[0]) as 16, because the structure is not packed.

See also: https://gitlab.inria.fr/gustedt/p99/-/blob/master/p99/p99_new.h#L425

 ** The above formula for the exact size is only valid for larger
 ** values of @c N. We must also ensure that we allocate at least @c
 ** sizeof(package_head) bytes. So the complete formula looks
 ** something like
 ** @code
 ** #define P99_FSIZEOF(T, F, N) P99_MAXOF(sizeof(T), offsetof(T, F) + P99_SIZEOF(T, F[0]) * N)
 ** @endcode

@Gumix Gumix added the 2.11 Target is 2.11 and all newer release/master branches label Sep 7, 2023
Gumix added a commit to Gumix/tarantool that referenced this issue Sep 7, 2023
@Gumix
box: fix out of bound write in error_payload_destroy()
d927d3a 
If `strlen(name)` is 1, `value_size` is 1, and `extra` is 0, then 15 bytes
are allocated for `struct error_field` in error_payload_prepare(). However,
the size of this structure is 16 because of the padding for the alignment.
Thus TRASH() in error_payload_destroy() writes 1 byte beyond the structure.

Closes tarantool#9098

NO_DOC=bugfix
@Gumix Gumix mentioned this issue Sep 7, 2023
Merged
Gumix added a commit to Gumix/tarantool that referenced this issue Sep 8, 2023
@Gumix
box: fix out of bound write in error_payload_destroy()
077d23d 
If `strlen(name)` is 1, `value_size` is 1, and `extra` is 0, then 15 bytes
are allocated for `struct error_field` in error_payload_prepare(). However,
the size of this structure is 16 because of the padding for the alignment.
Thus TRASH() in error_payload_destroy() writes 1 byte beyond the structure.

Closes tarantool#9098

NO_DOC=bugfix
locker pushed a commit that referenced this issue Sep 11, 2023
@Gumix @locker
box: fix out of bound write in error_payload_destroy()
454ffd1 
If `strlen(name)` is 1, `value_size` is 1, and `extra` is 0, then 15 bytes
are allocated for `struct error_field` in error_payload_prepare(). However,
the size of this structure is 16 because of the padding for the alignment.
Thus TRASH() in error_payload_destroy() writes 1 byte beyond the structure.

Closes #9098

NO_DOC=bugfix
locker pushed a commit that referenced this issue Sep 11, 2023
@Gumix @locker
box: fix out of bound write in error_payload_destroy()
fb4e8dd 
If `strlen(name)` is 1, `value_size` is 1, and `extra` is 0, then 15 bytes
are allocated for `struct error_field` in error_payload_prepare(). However,
the size of this structure is 16 because of the padding for the alignment.
Thus TRASH() in error_payload_destroy() writes 1 byte beyond the structure.

Closes #9098

NO_DOC=bugfix

(cherry picked from commit 454ffd1)
ligurio added a commit to ligurio/tarantool that referenced this issue Sep 12, 2023
@ligurio
test/fuzz: add fuzzing test for xrow_decode_error
a3dbf6d 
The patch adds a fuzzing test for IPROTO decoding function
xrow_decode_error().

Follows up tarantool#8921
Follows up tarantool#9098

NO_DOC=testing
NO_CHANGELOG=testing
ligurio added a commit to ligurio/tarantool that referenced this issue Sep 12, 2023
@ligurio
test/fuzz: add fuzzing test for xrow_decode_error
78549cf 
The patch adds a fuzzing test for IPROTO decoding function
xrow_decode_error().

Follows up tarantool#8921
Follows up tarantool#9098

NO_DOC=testing
NO_CHANGELOG=testing
kyukhin pushed a commit that referenced this issue Sep 12, 2023
@ligurio @kyukhin
test/fuzz: add fuzzing test for xrow_decode_error
2c700aa 
The patch adds a fuzzing test for IPROTO decoding function
xrow_decode_error().

Follows up #8921
Follows up #9098

NO_DOC=testing
NO_CHANGELOG=testing
kyukhin pushed a commit that referenced this issue Sep 12, 2023
@ligurio @kyukhin
test/fuzz: add fuzzing test for xrow_decode_error
3602698 
The patch adds a fuzzing test for IPROTO decoding function
xrow_decode_error().

Follows up #8921
Follows up #9098

NO_DOC=testing
NO_CHANGELOG=testing

(cherry picked from commit 2c700aa)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Gumix Gumix

Labels
2.11 Target is 2.11 and all newer release/master branches bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

box: fix out of bound write in error_payload_destroy() Gumix/tarantool
3 participants
@ligurio @alyapunov @Gumix