openSUSE patch: https://build.opensuse.org/package/view_file/openSUSE:Factory/mp3gain/0001-fix-security-bugs.patch?expand=1 Gentoo bug: https://bugs.gentoo.org/717940 ---- From: Jason Craig Date: Mon, 30 Mar 2020 12:43:20 -0600 Subject: [PATCH] Fix various security issues including CVE-2019-18359 References: boo#1154971 Upstream: dead Multiple POCs at https://github.com/zjuchenyuan/fuzzpoc were fixed. --- a/apetag.c +++ b/apetag.c @@ -16,6 +16,13 @@ #define _stricmp strcasecmp #endif /* WIN32 */ +// Min and max values for gain and peak in order to fit in allotted space in the APE tags. For gain that is nine chars, +// including a + or -. For peak that is eight chars, including a - but no +. Both will always have six precision digits. +#define MIN_GAIN -9.999999 +#define MAX_GAIN 9.999999 +#define MIN_PEAK -9.99999 +#define MAX_PEAK 9.999999 + int ReadMP3ID3v1Tag(FILE *fi, unsigned char **tagbuff, long *tag_offset) { char tmp[128]; @@ -102,9 +109,9 @@ static int ReadMP3Lyrics3v2Tag ( FILE *fp, unsigned char **tagbuff, unsigned lon if ( fseek (fp, *tag_offset - 128 - (long)sizeof (T) - len, SEEK_SET) ) return 0; if ( fread (tmp, 1, 11, fp) != 11 ) return 0; if ( memcmp (tmp, "LYRICSBEGIN", 11) ) return 0; - + taglen = 128 + Lyrics3GetNumber6(T.Length) + sizeof(T); - + *tag_offset -= taglen; if (*tagbuff != NULL) { free(*tagbuff); @@ -142,7 +149,7 @@ enum { unsigned long strlen_max(const char * ptr, unsigned long max) { unsigned long n = 0; - while (ptr[n] && n < max) n++; + while (n < max && ptr[n]) n++; return n; } @@ -234,6 +241,14 @@ int ReadMP3APETag ( FILE *fp, struct MP3GainTagInfo *info, struct APETagStruct info->albumPeak = atof(value); } else if (!_stricmp(name,"MP3GAIN_UNDO")) { /* value should be something like "+003,+003,W" */ + /* If the file didn't specify enough bytes for the value (at least 11...see above), skip the tag. */ + if(vsize < 11) + { + free(value); + free(name); + p += isize + 1 + vsize; + continue; + } info->haveUndo = !0; vp = value; memcpy(tmpString,vp,4); @@ -251,6 +266,14 @@ int ReadMP3APETag ( FILE *fp, struct MP3GainTagInfo *info, struct APETagStruct } } else if (!_stricmp(name,"MP3GAIN_MINMAX")) { /* value should be something like "001,153" */ + /* If the file didn't specify enough bytes for the value (at least 7...see above), skip the tag. */ + if(vsize < 7) + { + free(value); + free(name); + p += isize + 1 + vsize; + continue; + } info->haveMinMaxGain = !0; vp = value; memcpy(tmpString,vp,3); @@ -289,7 +312,7 @@ int ReadMP3APETag ( FILE *fp, struct MP3GainTagInfo *info, struct APETagStruct } free (buff); - + *tag_offset -= TagLen; (*apeTag)->originalTagSize = TagLen; @@ -318,7 +341,7 @@ int ReadMP3APETag ( FILE *fp, struct MP3GainTagInfo *info, struct APETagStruct int truncate_file (char *filename, long truncLength) { #ifdef WIN32 - + int fh, result; /* Open a file */ @@ -370,10 +393,10 @@ int ReadMP3GainAPETag (char *filename, struct MP3GainTagInfo *info, struct FileT fi = fopen(filename, "rb"); if (fi == NULL) return 0; - + fseek(fi, 0, SEEK_END); tag_offset = file_size = ftell(fi); - + fileTags->lyrics3TagSize = 0; do { @@ -515,7 +538,7 @@ int WriteMP3GainAPETag (char *filename, struct MP3GainTagInfo *info, struct File Write_LE_Uint32(newFooter.Flags,1<<31); /* tag has header */ memset(newFooter.Reserved,0,sizeof(newFooter.Reserved)); } - + if (info->haveMinMaxGain) { /* 8 bytes + "MP3GAIN_MINMAX" + '/0' + "123,123" = 30 bytes */ Write_LE_Uint32(mp3gainTagData,7); @@ -575,7 +598,10 @@ int WriteMP3GainAPETag (char *filename, struct MP3GainTagInfo *info, struct File mp3gainTagData += 4; strcpy(mp3gainTagData, "REPLAYGAIN_TRACK_GAIN"); mp3gainTagData += 22; - sprintf(valueString,"%-+9.6f", info->trackGain); + // Clamp the gain value to ensure that sprintf won't put more than 9 chars in valueString. In cases of very + // large trackGain value, valueString could overflow. + sprintf(valueString, "%-+9.6f", info->trackGain < MIN_GAIN ? MIN_GAIN + : (info->trackGain > MAX_GAIN ? MAX_GAIN : info->trackGain)); memcpy(mp3gainTagData, valueString, 9); mp3gainTagData += 9; memcpy(mp3gainTagData, " dB", 3); @@ -589,7 +615,10 @@ int WriteMP3GainAPETag (char *filename, struct MP3GainTagInfo *info, struct File mp3gainTagData += 4; strcpy(mp3gainTagData, "REPLAYGAIN_TRACK_PEAK"); mp3gainTagData += 22; - sprintf(valueString,"%-8.6f", info->trackPeak); + // Clamp the peak value to ensure that sprintf won't put more than 8 chars in valueString. In cases of very + // large trackPeak value, valueString could overflow. + sprintf(valueString,"%-8.6f", info->trackPeak < MIN_PEAK ? MIN_PEAK + : (info->trackPeak > MAX_PEAK ? MAX_PEAK : info->trackPeak)); memcpy(mp3gainTagData, valueString, 8); mp3gainTagData += 8; } @@ -601,7 +630,9 @@ int WriteMP3GainAPETag (char *filename, struct MP3GainTagInfo *info, struct File mp3gainTagData += 4; strcpy(mp3gainTagData, "REPLAYGAIN_ALBUM_GAIN"); mp3gainTagData += 22; - sprintf(valueString,"%-+9.6f", info->albumGain); + // Clamp the gain value, see haveTrackGain if above. + sprintf(valueString,"%-+9.6f", info->albumGain < MIN_GAIN ? MIN_GAIN + : (info->albumGain > MAX_GAIN ? MAX_GAIN : info->albumGain)); memcpy(mp3gainTagData, valueString, 9); mp3gainTagData += 9; memcpy(mp3gainTagData, " dB", 3); @@ -615,7 +646,9 @@ int WriteMP3GainAPETag (char *filename, struct MP3GainTagInfo *info, struct File mp3gainTagData += 4; strcpy(mp3gainTagData, "REPLAYGAIN_ALBUM_PEAK"); mp3gainTagData += 22; - sprintf(valueString,"%-8.6f", info->albumPeak); + // Clamp the peak value, see haveTrackPeak if above. + sprintf(valueString,"%-8.6f", info->albumPeak < MIN_PEAK ? MIN_PEAK + : (info->albumPeak > MAX_PEAK ? MAX_PEAK : info->albumPeak)); memcpy(mp3gainTagData, valueString, 8); mp3gainTagData += 8; } @@ -641,7 +674,7 @@ int WriteMP3GainAPETag (char *filename, struct MP3GainTagInfo *info, struct File } //no Lyrics3 tag fclose(outputFile); - + if (saveTimeStamp) fileTime(filename,setStoredTime); @@ -666,7 +699,7 @@ int RemoveMP3GainAPETag (char *filename, int saveTimeStamp) { info.haveMinMaxGain = 0; info.haveAlbumMinMaxGain = 0; info.haveUndo = 0; - + fileTags.apeTag = NULL; fileTags.id31tag = NULL; fileTags.lyrics3tag = NULL;