From 89b12bc296c4f33df6ed13a527e731e4053a5417 Mon Sep 17 00:00:00 2001 From: Colin Murphy Date: Thu, 12 Dec 2024 15:09:11 -0500 Subject: [PATCH] fix: prevent panics from malformed id3 metadata * mllt overflow * invalid frame id length * decode content size underflow * decode MLLT deviation overflow * stream tag frame bytes underflow --- src/stream/frame/content.rs | 77 ++++++++++++++++++++++++++++++++++++- src/stream/frame/v4.rs | 57 ++++++++++++++++++++++++++- src/stream/tag.rs | 15 +++++++- 3 files changed, 144 insertions(+), 5 deletions(-) diff --git a/src/stream/frame/content.rs b/src/stream/frame/content.rs index e730a187b..fb8cfffd4 100644 --- a/src/stream/frame/content.rs +++ b/src/stream/frame/content.rs @@ -247,7 +247,9 @@ impl Encoder { &mut self, content: &MpegLocationLookupTable, ) -> crate::Result<()> { - let ref_packed_size = content.bits_for_bytes + content.bits_for_millis; + let ref_packed_size = content + .bits_for_bytes + .saturating_add(content.bits_for_millis); if ref_packed_size % 4 != 0 { return Err(Error::new( ErrorKind::InvalidInput, @@ -886,7 +888,9 @@ impl<'a> Decoder<'a> { ), )); } - deviations[i] = u32::try_from(carry >> (64 - bits_us)).unwrap(); + deviations[i] = u32::try_from(carry >> (64 - bits_us)).map_err(|_| { + Error::new(ErrorKind::InvalidInput, "MLLT deviation field overflow") + })?; carry <<= bits_us; carry_bits -= bits_us; } @@ -1039,6 +1043,7 @@ mod tests { use crate::frame::Content; use crate::frame::{self, Picture, PictureType}; use std::collections::HashMap; + use std::io::Cursor; fn bytes_for_encoding(text: &str, encoding: Encoding) -> Vec { encoding.encode(text) @@ -1835,4 +1840,72 @@ mod tests { ) .is_none()); } + + #[test] + fn test_encode_mllt_overflow() { + let mllt = Content::MpegLocationLookupTable(MpegLocationLookupTable { + frames_between_reference: 1, + bytes_between_reference: 418, + millis_between_reference: 15, + bits_for_bytes: 252, + bits_for_millis: 252, // This would cause an overflow if not for saturating_add + references: vec![ + MpegLocationLookupTableReference { + deviate_bytes: 0x1, + deviate_millis: 0x2, + }, + MpegLocationLookupTableReference { + deviate_bytes: 0x3, + deviate_millis: 0x4, + }, + MpegLocationLookupTableReference { + deviate_bytes: 0x5, + deviate_millis: 0x6, + }, + ], + }); + + let mut data_out = Vec::new(); + let result = encode(&mut data_out, &mllt, Version::Id3v23, Encoding::UTF8); + + // Error since 255 is not a multiple of 4, but no panic. + assert!(result.is_err()); + if let Err(e) = result { + assert!(matches!(e.kind, ErrorKind::InvalidInput)); + assert_eq!( + e.description, + "MLLT bits_for_bytes + bits_for_millis must be a multiple of 4" + ); + } + } + + #[test] + fn test_decode_mllt_deviation_overflow() { + // Create a payload with large deviation values that would overflow u32 + let payload = [ + 0xFF, 0xFF, // frames_between_reference (u16::MAX) + 0xFF, 0xFF, 0xFF, // bytes_between_reference (u24::MAX) + 0xFF, 0xFF, 0xFF, // millis_between_reference (u24::MAX) + 0x38, // bits_for_bytes (56) + 0x1C, // bits_for_millis (28) + 0xFF, 0xFF, 0xFF, 0xFF, // deviate_bytes (u32::MAX) + 0xFF, 0xFF, 0xFF, 0xFF, // deviate_millis (u32::MAX) + ]; + + // Combine header and payload into a single data stream + let mut data = Vec::new(); + data.extend_from_slice(&payload); + + let mut reader = Cursor::new(data); + + // Attempt to decode the frame + let result = decode("MLLT", Version::Id3v23, &mut reader); + + // Ensure that the result is an error due to overflow + assert!(result.is_err()); + if let Err(e) = result { + assert!(matches!(e.kind, ErrorKind::InvalidInput)); + assert_eq!(e.description, "MLLT deviation field overflow"); + } + } } diff --git a/src/stream/frame/v4.rs b/src/stream/frame/v4.rs index 355c244f0..d3042de4f 100644 --- a/src/stream/frame/v4.rs +++ b/src/stream/frame/v4.rs @@ -45,7 +45,7 @@ pub fn decode(mut reader: impl io::Read) -> crate::Result let read_size = if flags.contains(Flags::DATA_LENGTH_INDICATOR) { let _decompressed_size = unsynch::decode_u32(reader.read_u32::()?); - content_size - 4 + content_size.saturating_sub(4) } else { content_size }; @@ -94,7 +94,12 @@ pub fn encode(mut writer: impl io::Write, frame: &Frame, flags: Flags) -> crate: writer.write_all({ let id = frame.id().as_bytes(); - assert_eq!(4, id.len()); + if id.len() != 4 { + return Err(Error::new( + ErrorKind::InvalidInput, + "Frame ID must be 4 bytes long", + )); + } id })?; writer.write_u32::(unsynch::encode_u32( @@ -109,3 +114,51 @@ pub fn encode(mut writer: impl io::Write, frame: &Frame, flags: Flags) -> crate: writer.write_all(&content_buf)?; Ok(10 + comp_hint_delta + content_buf.len()) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::frame::Content; + use std::io::Cursor; + + #[test] + fn test_encode_with_invalid_frame_id() { + let frame = Frame::with_content("TST", Content::Text("Test content".to_string())); + let flags = Flags::empty(); + let mut writer = Cursor::new(Vec::new()); + + let result = encode(&mut writer, &frame, flags); + + assert!(result.is_err()); + if let Err(e) = result { + assert!(matches!(e.kind, ErrorKind::InvalidInput)); + assert_eq!(e.description, "Frame ID must be 4 bytes long"); + } + } + + #[test] + fn test_decode_with_underflow() { + // Create a frame header with DATA_LENGTH_INDICATOR flag set and a content size of 3 + let frame_header = [ + b'T', b'E', b'S', b'T', // Frame ID + 0x00, 0x00, 0x00, 0x03, // Content size (3 bytes) + 0x00, 0x01, // Flags (DATA_LENGTH_INDICATOR) + ]; + // Create a reader with the frame header followed by 4 bytes for the decompressed size + let mut data = Vec::new(); + data.extend_from_slice(&frame_header); + data.extend_from_slice(&[0x00, 0x00, 0x00, 0x04]); // Decompressed size (4 bytes) + + let mut reader = Cursor::new(data); + + // Attempt to decode the frame + let result = decode(&mut reader); + + // Ensure that the result is an error due to underflow + assert!(result.is_err()); + if let Err(e) = result { + assert!(matches!(e.kind, ErrorKind::Parsing)); + assert_eq!(e.description, "Insufficient data to decode bytes"); + } + } +} diff --git a/src/stream/tag.rs b/src/stream/tag.rs index 05359b9b7..c3a5ee676 100644 --- a/src/stream/tag.rs +++ b/src/stream/tag.rs @@ -65,7 +65,7 @@ impl Header { } fn frame_bytes(&self) -> u64 { - u64::from(self.tag_size) - u64::from(self.ext_header_size) + u64::from(self.tag_size).saturating_sub(u64::from(self.ext_header_size)) } fn tag_size(&self) -> u64 { @@ -1091,4 +1091,17 @@ mod tests { assert_eq!(tag, tag_read); } + + #[test] + fn test_frame_bytes_underflow() { + let header = Header { + version: Version::Id3v24, + flags: Flags::empty(), + tag_size: 10, + ext_header_size: 20, + }; + + // Without saturating_sub, this would underflow and cause a panic. + assert_eq!(header.frame_bytes(), 0); + } }