heap-use-after-free via the function flvmeta/src/flv.c:375:21 in flv_close was detected

hanxuer commented 8 months ago

Hello, I would like to bring to your attention that I have encountered a potential issue in the new version about the flvmeta 1.2.2 0d3eb28.I'm not sure if this is. This observation was made during testing on Ubuntu 18.04. Thank you for your understanding.

compiler with asan

mkdir build
cd build
cmake .. -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_FLAGS="-fsanitize=address"
make

poc：https://github.com/hanxuer/crashes/raw/main/flvmeta/01/poc reproduce： ./flvmeta/build/src/flvmeta ./poc

Asan report

==129869==ERROR: AddressSanitizer: heap-use-after-free on address 0x604000000010 at pc 0x000000539f2c bp 0x7ffc37b92ee0 sp 0x7ffc37b92ed8
READ of size 8 at 0x604000000010 thread T0
    #0 0x539f2b in flv_close /flvmeta/flvmeta/src/flv.c:375:21
    #1 0x53bce3 in flv_parse /flvmeta/flvmeta/src/flv.c:525:17
    #2 0x52c487 in dump_metadata /flvmeta/flvmeta/src/dump.c:180:14
    #3 0x53c9cb in main /flvmeta/flvmeta/src/flvmeta.c:385:50
    #4 0x7faea13ddc86 in __libc_start_main /build/glibc-CVJwZb/glibc-2.27/csu/../csu/libc-start.c:310
    #5 0x41c7f9 in _start (/flvmeta/flvmeta/build/src/flvmeta+0x41c7f9)

0x604000000010 is located 0 bytes inside of 40-byte region [0x604000000010,0x604000000038)
freed by thread T0 here:
    #0 0x4dc4e0 in __interceptor_free.localalias.0 (/flvmeta/flvmeta-cov/build/src/flvmeta+0x4dc4e0)
    #1 0x539f79 in flv_close /flvmeta/flvmeta/src/flv.c:378:9
    #2 0x539e83 in flv_read_video_tag /flvmeta/flvmeta/src/flv.c:250:13
    #3 0x53bc93 in flv_parse /flvmeta/flvmeta/src/flv.c:523:22
    #4 0x52c487 in dump_metadata /flvmeta/flvmeta/src/dump.c:180:14
    #5 0x53c9cb in main /flvmeta/flvmeta/src/flvmeta.c:385:50
    #6 0x7faea13ddc86 in __libc_start_main /build/glibc-CVJwZb/glibc-2.27/csu/../csu/libc-start.c:310

previously allocated by thread T0 here:
    #0 0x4dc6b0 in malloc (/flvmeta/flvmeta/build/src/flvmeta+0x4dc6b0)
    #1 0x53771a in flv_open /flvmeta/flvmeta/src/flv.c:52:42
    #2 0x53b5d9 in flv_parse /flvmeta/flvmeta/src/flv.c:480:22
    #3 0x52c487 in dump_metadata /flvmeta/flvmeta/src/dump.c:180:14
    #4 0x53c9cb in main /flvmeta/flvmeta/src/flvmeta.c:385:50
    #5 0x7faea13ddc86 in __libc_start_main /build/glibc-CVJwZb/glibc-2.27/csu/../csu/libc-start.c:310

SUMMARY: AddressSanitizer: heap-use-after-free /flvmeta/flvmeta-cov/src/flv.c:375:21 in flv_close
Shadow bytes around the buggy address:
  0x0c087fff7fb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c087fff7fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c087fff7fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c087fff7fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c087fff7ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c087fff8000: fa fa[fd]fd fd fd fd fa fa fa fa fa fa fa fa fa
  0x0c087fff8010: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c087fff8020: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c087fff8030: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c087fff8040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c087fff8050: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 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
==129869==ABORTING

gdb backtrace

gef➤  bt
#0  __GI_raise (sig=sig@entry=0x6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007ffff76847f1 in __GI_abort () at abort.c:79
#2  0x00007ffff76cd837 in __libc_message (action=action@entry=do_abort, fmt=fmt@entry=0x7ffff77faa7b "%s\n") at ../sysdeps/posix/libc_fatal.c:181
#3  0x00007ffff76d48ba in malloc_printerr (str=str@entry=0x7ffff77fc6e8 "free(): double free detected in tcache 2") at malloc.c:5342
#4  0x00007ffff76dc0ed in _int_free (have_lock=0x0, p=0x693250, av=0x7ffff7a2fc40 <main_arena>) at malloc.c:4195
#5  __GI___libc_free (mem=0x693260) at malloc.c:3134
#6  0x00000000004223d3 in flv_close (stream=0x693260) at /home/hanxuerr/myfuzz/target_program/flvmeta/flvmeta/src/flv.c:378
#7  0x000000000042394c in flv_parse (file=<optimized out>, parser=<optimized out>) at /flvmeta/flvmeta/src/flv.c:525
#8  0x000000000041a41d in dump_metadata (options=0x652530 <main.options>) at /flvmeta/flvmeta/src/dump.c:180
#9  0x0000000000424937 in main (argc=<optimized out>, argv=<optimized out>) at /flvmeta/flvmeta/src/flvmeta.c:385

source code

void flv_close(flv_stream * stream) {
    if (stream != NULL) {
        if (stream->flvin != NULL) {
            fclose(stream->flvin);
        }
        free(stream); // flv.c:378
    }
}

else if (tag.type == FLV_TAG_TYPE_VIDEO) {
            retval = flv_read_video_tag(parser->stream, &vt);
            if (retval == FLV_ERROR_EOF) {
                flv_close(parser->stream);  //flv.c:525
                return retval;
            }

noirotm commented 8 months ago

Hi! Thanks for the bug report. I will try to reproduce, and have an in-depth look at this. At first glance, I don't believe this has been introduced recently as the code you're pointing to hasn't been changed much.

noirotm commented 8 months ago

I can reproduce a crash when trying to display metadata.

Also the check command gives this output, which demonstrates that we can see that there's something wrong in the file.

$ flvmeta.exe -C poc.flv
0x00000003: error E11003: header version should be 1, 121 found instead
0x00000004: error E11007: header reserved flags are not zero
0x00000005: error E11008: header offset should be 9, 3825205257 found instead
0x00000009: error E12010: first previous tag size should be 0, 3909091698 found instead
0x0000000e: fatal F20016: tag body length (14617727 bytes) exceeds file size
5 error(s), 0 warning(s)

noirotm commented 8 months ago

At first glance, I don't believe this has been introduced recently as the code you're pointing to hasn't been changed much.

Nevermind, I found the bug.

It has been introduced in 5ad3d503. Problem is at https://github.com/noirotm/flvmeta/blob/0d3eb281dd4bc17344fa53b05a78294f599c9327/src/flv.c#L250

The flv_read_video_tag function should not call flv_close as it violates the ownership contract that only the owner of the flv_stream should be able to close it, and not the flv_stream itself.

I unfortunately overlooked this in my code review.

The more I review code in this project, the more I want to rewrite it in Rust 😄

hanxuer commented 8 months ago

At first glance, I don't believe this has been introduced recently as the code you're pointing to hasn't been changed much.

Nevermind, I found the bug.

It has been introduced in 5ad3d50. Problem is at

https://github.com/noirotm/flvmeta/blob/0d3eb281dd4bc17344fa53b05a78294f599c9327/src/flv.c#L250

The flv_read_video_tag function should not call flv_close as it violates the ownership contract that only the owner of the flv_stream should be able to close it, and not the flv_stream itself.

I unfortunately overlooked this in my code review.

The more I review code in this project, the more I want to rewrite it in Rust 😄

Thank you for attention to this issue.I am confident that you work will significantly bolster the project's stability 😊

noirotm commented 8 months ago

Reopening issue to make sure the fix actually does the job.

noirotm commented 8 months ago

@hanxuer Could you please confirm if https://github.com/noirotm/flvmeta/commit/b54861c940f2ebc67bf88c4f6841256dba8fb0ac fixed the issue?

hanxuer commented 8 months ago

@hanxuer Could you please confirm if b54861c fixed the issue? There is no longer a use-after-free vulnerability. Thank you for your attention.😊

noirotm / flvmeta