Heap-based Buffer Overflow in squell/id3

Valid

Reported on

Jun 3rd 2021


✍️ Description

When running the id3 app built from commit 51e738 with Clang 13 (+ASan) on Ubuntu 20.04.2 against the test data file encoding.tag, a heap-buffer-overflow is triggered at https://github.com/squell/id3/blob/51e738e7575c54fd7fdd54c931a155b25c3f2d30/id3v2.c#L104.

static ulong ul4(uchar n[4])
{
    return (ulong)n[0]<<24  <<< L104
         | (ulong)n[1]<<16
         | (ulong)n[2]<< 8
         | (ulong)n[3]<< 0;
}

🕵️‍♂️ Proof of Concept

./id3 test/encoding.tag returns this ASan stack trace:

File: test/encoding.tag
Metadata: ID3v2.3
=================================================================
==5831==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6130000001b8 at pc 0x000000510184 bp 0x7ffd87d1a370 sp 0x7ffd87d1a368
READ of size 1 at 0x6130000001b8 thread T0
    #0 0x510183 in ul4 /root/id3/id3v2.c:104:19
    #1 0x512027 in ID3_frame /root/id3/id3v2.c:421:25
    #2 0x50a6fa in getframe(_ID3FRAME*, char const*, unsigned long) /root/id3/getid3v2.cpp:112:11
    #3 0x509fad in tag::read::ID3v2::operator[](tag::ID3field) const /root/id3/getid3v2.cpp:72:20
    #4 0x4d9ed7 in listtag::file(char const*, fileexp::record const&) /root/id3/main.cpp:318:42
    #5 0x4e57d1 in fileexp::filefind::nested(auto_dir, char*, char*) /root/id3/fileexp.cpp
    #6 0x4e44be in fileexp::find::glob(char const*, bool) /root/id3/fileexp.cpp:63:14
    #7 0x4d01d7 in process_(fileexp::find&, char**, bool) /root/id3/main.cpp:339:19
    #8 0x4d4619 in main_(int, char**) /root/id3/main.cpp:577:24
    #9 0x4d7ed9 in main /root/id3/main.cpp:631:16
    #10 0x7fa9fb7490b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16
    #11 0x420c1d in _start (/root/id3/id3+0x420c1d)

0x6130000001b8 is located 0 bytes to the right of 376-byte region [0x613000000040,0x6130000001b8)
allocated by thread T0 here:
    #0 0x49c0ed in malloc (/root/id3/id3+0x49c0ed)
    #1 0x50f2e7 in ID3_readf /root/id3/id3v2.c:216:11
    #2 0x509af7 in tag::read::ID3v2::ID3v2(char const*) /root/id3/getid3v2.cpp:31:36
    #3 0x500c2c in tag::write::ID3v2::read(char const*) const /root/id3/setid3v2.cpp:281:16
    #4 0x500c2c in non-virtual thunk to tag::write::ID3v2::read(char const*) const /root/id3/setid3v2.cpp
    #5 0x4d8f78 in tag::combined<tag::reader>::read(char const*) const /root/id3/./setgroup.h:95:52
    #6 0x4d9d51 in listtag::file(char const*, fileexp::record const&) /root/id3/main.cpp:312:49
    #7 0x4e57d1 in fileexp::filefind::nested(auto_dir, char*, char*) /root/id3/fileexp.cpp
    #8 0x4e44be in fileexp::find::glob(char const*, bool) /root/id3/fileexp.cpp:63:14
    #9 0x4d01d7 in process_(fileexp::find&, char**, bool) /root/id3/main.cpp:339:19
    #10 0x4d4619 in main_(int, char**) /root/id3/main.cpp:577:24
    #11 0x4d7ed9 in main /root/id3/main.cpp:631:16
    #12 0x7fa9fb7490b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16

SUMMARY: AddressSanitizer: heap-buffer-overflow /root/id3/id3v2.c:104:19 in ul4

💥 Impact

This vulnerability is capable of crashing the software, heap corruption, data corruption, and other unintended consequences of reading outside of the buffer.

Occurrences

Jamie Slome
a year ago

Admin


I have contacted the maintainer via a GitHub Issue and we will await a response from them.

Marc
a year ago

Maintainer


Cool. I think it would save me a lot of time if you can provide me with the encoding.tag file!

Marc
a year ago

Maintainer


(Also, I would be interested, as an academic, which method was used to find this vulnerability--some static analysis tool? fuzzing? greybox testing? manual inspection? etc etc.)

Marc
a year ago

Maintainer


Confirmed independently (assuming I found the same problem): the ID3v2 tag is semantically checked upon reading, and its end is marked with a sentinel value, but the size-information (not allocated in this sentinel object) will be accessed even for a sentinel value. This can go out-of-heap-bounds if a tag has less than 4 bytes of padding.

I would say the severity of this is "low" however: as long as the program doesn't segfault, https://github.com/squell/id3/blob/master/id3v2.c#L445 will indicate that no correct frame has been read, and the (incorrect) size information parsed by ul4(). Still, it would be more appropriate if ID3_frame just is a no-op if sees the end-of-tag sentinel.

Nice find!

Marc R. Schoolderman validated this vulnerability a year ago
geeknik has been awarded the disclosure bounty
The fix bounty is now up for grabs
Jamie Slome
a year ago

Admin


@Marc, feel free to confirm the fix and the bounty mentioned above is yours!

Marc
a year ago

Maintainer


Test-file with this hex-dump triggered the problem in commit 51e738e7575c54fd7fdd54c931a155b25c3f2d30:

00000000: 4944 3303 0000 0000 000c 5450 4531 0000 ID3.......TPE1.. 00000010: 0002 0000 0041 .....A

Commit c7b729db3cb5e346b2f839932f445f479ec5cbc7 fixes this issue.

Marc R. Schoolderman confirmed that a fix has been merged on c7b729 a year ago
has been awarded the fix bounty
Jamie Slome
a year ago

Admin


Great job all!

Marc
a year ago

Maintainer


Now that this is public---because I dislike the idea that vulnerabilities in open source code should be financially more rewarding to a developer than writing correct code, any fix bounties I receive for vulnerabilities in this tool will be donated to archive.org.

Jamie Slome
a year ago

Admin


Hey @Marc, do you think an automated donation system would be valuable?

geeknik
a year ago

Researcher


Sorry for the late response, the encoding.tag file was located in the test directory of your public repository. This particular bug was found just by running the compiled example program against your test data. Any bugs reported in the future will likely have been found using a combination of American Fuzzy Lop, Honggfuzz and libFuzzer. Thank you!

Marc
a year ago

Maintainer


@geeknik Geez! I forgot I had that test file ;-) Looking forward to discovering more interesting bugs, it's fun.

@Jamie Slome: For me personally, not that much, but I can imagine more projects would use such a feature if it was there. Thanks!

to join this conversation