From 28ad2041854a344e0d67e9a513ddd0231d5eac5d Mon Sep 17 00:00:00 2001 From: Serial <69764315+Serial-ATA@users.noreply.github.com> Date: Tue, 29 Oct 2024 20:26:01 -0400 Subject: [PATCH 1/6] APE/MPC/WV: Verify tag sizes in stream length calculation --- lofty/src/ape/read.rs | 27 +++++++++++------- lofty/src/musepack/read.rs | 9 ++++-- lofty/src/wavpack/read.rs | 16 +++++++---- ...h-c98d99ebce28b50b50eb2e96320f02e1e729e543 | Bin 0 -> 20 bytes lofty/tests/fuzz/mpcfile_read_from.rs | 8 ++++++ 5 files changed, 42 insertions(+), 18 deletions(-) create mode 100644 lofty/tests/fuzz/assets/mpcfile_read_from/crash-c98d99ebce28b50b50eb2e96320f02e1e729e543 diff --git a/lofty/src/ape/read.rs b/lofty/src/ape/read.rs index cfd4807ec..675cace47 100644 --- a/lofty/src/ape/read.rs +++ b/lofty/src/ape/read.rs @@ -8,7 +8,7 @@ use crate::id3::v1::tag::Id3v1Tag; use crate::id3::v2::read::parse_id3v2; use crate::id3::v2::tag::Id3v2Tag; use crate::id3::{find_id3v1, find_id3v2, find_lyrics3v2, FindId3v2Config, ID3FindResults}; -use crate::macros::decode_err; +use crate::macros::{decode_err, err}; use std::io::{Read, Seek, SeekFrom}; @@ -37,12 +37,12 @@ where if let ID3FindResults(Some(header), content) = find_id3v2(data, find_id3v2_config)? { log::warn!("Encountered an ID3v2 tag. This tag cannot be rewritten to the APE file!"); - stream_len -= u64::from(header.size); + let Some(new_stream_length) = stream_len.checked_sub(u64::from(header.full_tag_size())) + else { + err!(SizeMismatch); + }; - // Exclude the footer - if header.flags.footer { - stream_len -= 10; - } + stream_len = new_stream_length; if let Some(content) = content { let reader = &mut &*content; @@ -107,16 +107,21 @@ where let ID3FindResults(id3v1_header, id3v1) = find_id3v1(data, parse_options.read_tags)?; if id3v1_header.is_some() { - stream_len -= 128; id3v1_tag = id3v1; + let Some(new_stream_length) = stream_len.checked_sub(128) else { + err!(SizeMismatch); + }; + + stream_len = new_stream_length; } // Next, check for a Lyrics3v2 tag, and skip over it, as it's no use to us - let ID3FindResults(lyrics3_header, lyrics3v2_size) = find_lyrics3v2(data)?; + let ID3FindResults(_, lyrics3v2_size) = find_lyrics3v2(data)?; + let Some(new_stream_length) = stream_len.checked_sub(u64::from(lyrics3v2_size)) else { + err!(SizeMismatch); + }; - if lyrics3_header.is_some() { - stream_len -= u64::from(lyrics3v2_size) - } + stream_len = new_stream_length; // Next, search for an APE tag footer // diff --git a/lofty/src/musepack/read.rs b/lofty/src/musepack/read.rs index 510b71b41..1d03ce939 100644 --- a/lofty/src/musepack/read.rs +++ b/lofty/src/musepack/read.rs @@ -32,12 +32,17 @@ where // ID3v2 tags are unsupported in MPC files, but still possible #[allow(unused_variables)] if let ID3FindResults(Some(header), Some(content)) = find_id3v2(reader, find_id3v2_config)? { + let Some(new_stream_length) = stream_length.checked_sub(u64::from(header.full_tag_size())) + else { + err!(SizeMismatch); + }; + + stream_length = new_stream_length; + let reader = &mut &*content; let id3v2 = parse_id3v2(reader, header, parse_options)?; file.id3v2_tag = Some(id3v2); - - stream_length -= u64::from(header.full_tag_size()); } // Save the current position, so we can go back and read the properties after the tags diff --git a/lofty/src/wavpack/read.rs b/lofty/src/wavpack/read.rs index 2656587a6..a12ef20b8 100644 --- a/lofty/src/wavpack/read.rs +++ b/lofty/src/wavpack/read.rs @@ -4,6 +4,7 @@ use crate::config::ParseOptions; use crate::error::Result; use crate::id3::{find_id3v1, find_lyrics3v2, ID3FindResults}; +use crate::macros::err; use std::io::{Read, Seek, SeekFrom}; pub(super) fn read_from(reader: &mut R, parse_options: ParseOptions) -> Result @@ -20,16 +21,21 @@ where let ID3FindResults(id3v1_header, id3v1) = find_id3v1(reader, parse_options.read_tags)?; if id3v1_header.is_some() { - stream_length -= 128; id3v1_tag = id3v1; + let Some(new_stream_length) = stream_length.checked_sub(128) else { + err!(SizeMismatch); + }; + + stream_length = new_stream_length; } // Next, check for a Lyrics3v2 tag, and skip over it, as it's no use to us - let ID3FindResults(lyrics3_header, lyrics3v2_size) = find_lyrics3v2(reader)?; + let ID3FindResults(_, lyrics3v2_size) = find_lyrics3v2(reader)?; + let Some(new_stream_length) = stream_length.checked_sub(u64::from(lyrics3v2_size)) else { + err!(SizeMismatch); + }; - if lyrics3_header.is_some() { - stream_length -= u64::from(lyrics3v2_size); - } + stream_length = new_stream_length; // Next, search for an APE tag footer // diff --git a/lofty/tests/fuzz/assets/mpcfile_read_from/crash-c98d99ebce28b50b50eb2e96320f02e1e729e543 b/lofty/tests/fuzz/assets/mpcfile_read_from/crash-c98d99ebce28b50b50eb2e96320f02e1e729e543 new file mode 100644 index 0000000000000000000000000000000000000000..e8604ee407b9b1b97c76c6ae8f0560fecbc8b081 GIT binary patch literal 20 bcmeZtF=p1<%D}+DB*>7?P^Z99!jK98B_#ut literal 0 HcmV?d00001 diff --git a/lofty/tests/fuzz/mpcfile_read_from.rs b/lofty/tests/fuzz/mpcfile_read_from.rs index 98acc2441..99cb6656e 100644 --- a/lofty/tests/fuzz/mpcfile_read_from.rs +++ b/lofty/tests/fuzz/mpcfile_read_from.rs @@ -8,3 +8,11 @@ fn panic1() { let mut reader = crate::get_reader("mpcfile_read_from/output.aac"); let _ = MpcFile::read_from(&mut reader, ParseOptions::new()); } + +// Overflow on badly sized ID3v2 tag +#[test_log::test] +fn panic2() { + let mut reader = + crate::get_reader("mpcfile_read_from/crash-c98d99ebce28b50b50eb2e96320f02e1e729e543"); + let _ = MpcFile::read_from(&mut reader, ParseOptions::new()); +} From 4f2fcf66ee6a1272e2c792fcc1d277c3c4288e7a Mon Sep 17 00:00:00 2001 From: Serial <69764315+Serial-ATA@users.noreply.github.com> Date: Tue, 29 Oct 2024 20:31:36 -0400 Subject: [PATCH 2/6] WAV: Fix potential overflow in bit depth calculation --- lofty/src/iff/wav/properties.rs | 2 +- lofty/tests/fuzz/assets/wavfile_read_from/aa | Bin 0 -> 44 bytes lofty/tests/fuzz/wavfile_read_from.rs | 6 ++++++ 3 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 lofty/tests/fuzz/assets/wavfile_read_from/aa diff --git a/lofty/src/iff/wav/properties.rs b/lofty/src/iff/wav/properties.rs index 7788eb90a..e284af234 100644 --- a/lofty/src/iff/wav/properties.rs +++ b/lofty/src/iff/wav/properties.rs @@ -209,7 +209,7 @@ pub(super) fn read_properties( .. }) if valid_bits_per_sample > 0 => bit_depth = valid_bits_per_sample as u8, _ if bits_per_sample > 0 => bit_depth = bits_per_sample as u8, - _ => bit_depth = (bytes_per_sample * 8) as u8, + _ => bit_depth = bytes_per_sample.saturating_mul(8) as u8, }; let channel_mask = extensible_info.map(|info| info.channel_mask); diff --git a/lofty/tests/fuzz/assets/wavfile_read_from/aa b/lofty/tests/fuzz/assets/wavfile_read_from/aa new file mode 100644 index 0000000000000000000000000000000000000000..f0afb8ab8645886d5389c42349df120f4932c16c GIT binary patch literal 44 ycmWIYbaT_=U| Date: Tue, 29 Oct 2024 20:43:26 -0400 Subject: [PATCH 3/6] WV: Fix potential OOB index on wrongly sized blocks --- lofty/src/wavpack/properties.rs | 19 ++++++++++++++++++ .../fuzz/assets/wavpackfile_read_from/output | Bin 0 -> 189 bytes lofty/tests/fuzz/wavpackfile_read_from.rs | 8 ++++++++ 3 files changed, 27 insertions(+) create mode 100644 lofty/tests/fuzz/assets/wavpackfile_read_from/output diff --git a/lofty/src/wavpack/properties.rs b/lofty/src/wavpack/properties.rs index b2b4857bf..255c7c896 100644 --- a/lofty/src/wavpack/properties.rs +++ b/lofty/src/wavpack/properties.rs @@ -305,6 +305,10 @@ fn get_extended_meta_info( let mut index = 0; let block_size = block_content.len(); while index < block_size { + if block_size - index < 2 { + break; + } + let id = block_content[index]; index += 1; @@ -326,6 +330,7 @@ fn get_extended_meta_info( ID_NON_STANDARD_SAMPLE_RATE => { properties.sample_rate = (&mut &block_content[index..]).read_u24::()?; + index += 3; }, ID_DSD => { if size <= 1 { @@ -364,32 +369,46 @@ fn get_extended_meta_info( match s { 0 => { let channel_mask = reader.read_u8()?; + index += 1; + properties.channel_mask = ChannelMask(u32::from(channel_mask)); }, 1 => { let channel_mask = reader.read_u16::()?; + index += 2; + properties.channel_mask = ChannelMask(u32::from(channel_mask)); }, 2 => { let channel_mask = reader.read_u24::()?; + index += 3; + properties.channel_mask = ChannelMask(channel_mask); }, 3 => { let channel_mask = reader.read_u32::()?; + index += 4; + properties.channel_mask = ChannelMask(channel_mask); }, 4 => { properties.channels |= u16::from(reader.read_u8()? & 0xF) << 8; properties.channels += 1; + index += 1; let channel_mask = reader.read_u24::()?; + index += 3; + properties.channel_mask = ChannelMask(channel_mask); }, 5 => { properties.channels |= u16::from(reader.read_u8()? & 0xF) << 8; properties.channels += 1; + index += 1; let channel_mask = reader.read_u32::()?; + index += 4; + properties.channel_mask = ChannelMask(channel_mask); }, _ => decode_err!(@BAIL WavPack, "Encountered invalid channel info size"), diff --git a/lofty/tests/fuzz/assets/wavpackfile_read_from/output b/lofty/tests/fuzz/assets/wavpackfile_read_from/output new file mode 100644 index 0000000000000000000000000000000000000000..a7cfeefcdb93caec42ae21711c36d3602b16c517 GIT binary patch literal 189 zcmXRfE6Co;z`(%7P|Q%`>0->xu$F;=(ab8Ov9S>-3;_->2E}wL$Qqz2Aq@Y)07Q{( GzZC$)ffz9W literal 0 HcmV?d00001 diff --git a/lofty/tests/fuzz/wavpackfile_read_from.rs b/lofty/tests/fuzz/wavpackfile_read_from.rs index 7e827ab75..8419de7ef 100644 --- a/lofty/tests/fuzz/wavpackfile_read_from.rs +++ b/lofty/tests/fuzz/wavpackfile_read_from.rs @@ -1,4 +1,6 @@ use crate::oom_test; +use lofty::config::ParseOptions; +use lofty::file::AudioFile; use lofty::wavpack::WavPackFile; #[test_log::test] @@ -82,3 +84,9 @@ fn oom11() { fn oom12() { oom_test::("wavpackfile_read_from/oom-94867b6fefcd32cd5bc3bc298468cd3d65d93ff1"); } + +#[test_log::test] +fn panic1() { + let mut reader = crate::get_reader("wavpackfile_read_from/output"); + let _ = WavPackFile::read_from(&mut reader, ParseOptions::default()); +} From 0da7b1a84f31575ef8dd9cbcf3fb0915bde8d15f Mon Sep 17 00:00:00 2001 From: Serial <69764315+Serial-ATA@users.noreply.github.com> Date: Tue, 29 Oct 2024 20:46:05 -0400 Subject: [PATCH 4/6] WV: Don't allow zero-size blocks --- lofty/src/wavpack/properties.rs | 4 ++++ lofty/tests/fuzz/assets/wavpackfile_read_from/bb | Bin 0 -> 123 bytes lofty/tests/fuzz/wavpackfile_read_from.rs | 6 ++++++ 3 files changed, 10 insertions(+) create mode 100644 lofty/tests/fuzz/assets/wavpackfile_read_from/bb diff --git a/lofty/src/wavpack/properties.rs b/lofty/src/wavpack/properties.rs index 255c7c896..f23639992 100644 --- a/lofty/src/wavpack/properties.rs +++ b/lofty/src/wavpack/properties.rs @@ -322,6 +322,10 @@ fn get_extended_meta_info( index += 2; } + if size == 0 { + decode_err!(@BAIL WavPack, "Encountered a zero-sized block"); + } + if id & ID_FLAG_ODD_SIZE > 0 { size -= 1; } diff --git a/lofty/tests/fuzz/assets/wavpackfile_read_from/bb b/lofty/tests/fuzz/assets/wavpackfile_read_from/bb new file mode 100644 index 0000000000000000000000000000000000000000..01457ab6452ae0ba107c23f1ab45658e99833528 GIT binary patch literal 123 ucmXRfE66Tp0D=+_!O)5TAOHXV-wGBal}Kk`NMvB}VQ@y*2sPXrLIVJrzZE?I literal 0 HcmV?d00001 diff --git a/lofty/tests/fuzz/wavpackfile_read_from.rs b/lofty/tests/fuzz/wavpackfile_read_from.rs index 8419de7ef..dca46f7fb 100644 --- a/lofty/tests/fuzz/wavpackfile_read_from.rs +++ b/lofty/tests/fuzz/wavpackfile_read_from.rs @@ -90,3 +90,9 @@ fn panic1() { let mut reader = crate::get_reader("wavpackfile_read_from/output"); let _ = WavPackFile::read_from(&mut reader, ParseOptions::default()); } + +#[test_log::test] +fn panic2() { + let mut reader = crate::get_reader("wavpackfile_read_from/bb"); + let _ = WavPackFile::read_from(&mut reader, ParseOptions::default()); +} From 9e6186a5a5031ac4f68d8ab07af187a8790d46dd Mon Sep 17 00:00:00 2001 From: Serial <69764315+Serial-ATA@users.noreply.github.com> Date: Tue, 29 Oct 2024 20:50:02 -0400 Subject: [PATCH 5/6] MPEG: Verify the size of APE tags --- lofty/src/mpeg/read.rs | 6 +++++- ...b183cc14faf3ecc93bdf4a5e30b05f7a537d_minimized | Bin 0 -> 116 bytes lofty/tests/fuzz/mpegfile_read_from.rs | 7 +++++++ 3 files changed, 12 insertions(+), 1 deletion(-) create mode 100644 lofty/tests/fuzz/assets/mpegfile_read_from/crash-7b5eb183cc14faf3ecc93bdf4a5e30b05f7a537d_minimized diff --git a/lofty/src/mpeg/read.rs b/lofty/src/mpeg/read.rs index 77312f067..3fb3d5c10 100644 --- a/lofty/src/mpeg/read.rs +++ b/lofty/src/mpeg/read.rs @@ -163,7 +163,11 @@ where // Seek back to the start of the tag let pos = reader.stream_position()?; - reader.seek(SeekFrom::Start(pos - u64::from(header.size)))?; + let Some(start_of_tag) = pos.checked_sub(u64::from(header.size)) else { + err!(SizeMismatch); + }; + + reader.seek(SeekFrom::Start(start_of_tag))?; }, _ => { // Correct the position (APE header - Preamble) diff --git a/lofty/tests/fuzz/assets/mpegfile_read_from/crash-7b5eb183cc14faf3ecc93bdf4a5e30b05f7a537d_minimized b/lofty/tests/fuzz/assets/mpegfile_read_from/crash-7b5eb183cc14faf3ecc93bdf4a5e30b05f7a537d_minimized new file mode 100644 index 0000000000000000000000000000000000000000..c4e457c0dca9e40e37860ef5469a1a7018a0c4fa GIT binary patch literal 116 zcmaFo>agK+v+Vf;Lf1w7MOrp>=`b+(-Ye$JpZfczQbWd^n>k%a?be2s@-lODFX`+y z>Ul=BVDLP1d{-v%c{-L;b9d_TSD9nhrYi0-A$U5){+A&%~@5svO9nI#Mu G;6DJ|(=4("mpegfile_read_from/oom-f8730cbfa5682ab12343ccb70de9b71a061ef4d0"); From cfa80f3cad216be56b9b2fab23529e11eaa6b3fd Mon Sep 17 00:00:00 2001 From: Serial <69764315+Serial-ATA@users.noreply.github.com> Date: Tue, 29 Oct 2024 20:54:02 -0400 Subject: [PATCH 6/6] changelog: Add entries for fuzzing fixes --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5191f9ee4..9ee81c0ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - When skipping invalid frames in `ParsingMode::{BestAttempt, Relaxed}`, the parser will no longer be able to go out of the bounds of the frame content ([issue](https://github.com/Serial-ATA/lofty-rs/issues/458)) ([PR](https://github.com/Serial-ATA/lofty-rs/pull/459)) - **MP4**: Support for flag items (ex. `cpil`) of any size (not just 1 byte) ([issue](https://github.com/Serial-ATA/lofty-rs/issues/457)) ([PR](https://github.com/Serial-ATA/lofty-rs/pull/460)) +- **Fuzzing** (Thanks [@qarmin](https://github.com/qarmin)!) ([PR](https://github.com/Serial-ATA/lofty-rs/pull/476)): + - **MusePack**: Fix panic when ID3v2 tag sizes exceed the stream length ([issue](https://github.com/Serial-ATA/lofty-rs/issues/470)) + - **WAV**: Fix panic when calculating bit depth with abnormally large `bytes_per_sample` ([issue](https://github.com/Serial-ATA/lofty-rs/issues/471)) + - **WavPack***: Fix panic when encountering wrongly sized blocks ([issue](https://github.com/Serial-ATA/lofty-rs/issues/472)) + - **WavPack***: Fix panic when encountering zero-sized blocks ([issue](https://github.com/Serial-ATA/lofty-rs/issues/473)) + - **MPEG**: Fix panic when APE tags are incorrectly sized ([issue](https://github.com/Serial-ATA/lofty-rs/issues/474)) ## [0.21.1] - 2024-08-28