ID3v2: Restrict frame skipping to the bounds of the frame content

CHANGELOG.md

@@ -7,15 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased]
 
 ### Added
-- **ItemKey**: `ItemKey::TrackArtists`, available for ID3v2, Vorbis Comments, APE, and MP4 Ilst ([PR](https://github.com/Serial-ATA/lofty-rs/issues/454))
+- **ItemKey**: `ItemKey::TrackArtists`, available for ID3v2, Vorbis Comments, APE, and MP4 Ilst ([PR](https://github.com/Serial-ATA/lofty-rs/pull/454))
   - This is a multi-value item that stores each artist for a track. It should be retrieved with `Tag::get_strings` or `Tag::take_strings`.
   - For example, a track has `ItemKey::TrackArtist` = "Foo & Bar", then `ItemKey::TrackArtists` = ["Foo", "Bar"].
   - See <https://picard-docs.musicbrainz.org/en/appendices/tag_mapping.html#artists>
+- **UnsynchronizedStream**: `UnsynchronizedStream::get_ref()` ([PR](https://github.com/Serial-ATA/lofty-rs/pull/459))
 
 ### Fixed
 - **MusePack**: Fix potential panic when the beginning silence makes up the entire sample count ([PR](https://github.com/Serial-ATA/lofty-rs/pull/449))
 - **Timestamp**: Support timestamps without separators (ex. "20240906" vs "2024-09-06") ([issue](https://github.com/Serial-ATA/lofty-rs/issues/452)) ([PR](https://github.com/Serial-ATA/lofty-rs/issues/453))
-- **ID3v2**: `ItemKey::Director` will now be written correctly as a TXXX frame ([PR](https://github.com/Serial-ATA/lofty-rs/issues/454))
+- **ID3v2**:
+  - `ItemKey::Director` will now be written correctly as a TXXX frame ([PR](https://github.com/Serial-ATA/lofty-rs/issues/454))
+  - 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))
 
 ## [0.21.1] - 2024-08-28
 

lofty/src/id3/v2/frame/content.rs

@@ -21,6 +21,8 @@ pub(super) fn parse_content<R: Read>(
     version: Id3v2Version,
 	parse_mode: ParsingMode,
 ) -> Result<Option<Frame<'static>>> {
+	log::trace!("Parsing frame content for ID: {}", id);
+
 	Ok(match id.as_str() {
 		// The ID was previously upgraded, but the content remains unchanged, so version is necessary
 		"APIC" => {

lofty/src/id3/v2/frame/read.rs

@@ -4,18 +4,18 @@ use crate::config::{ParseOptions, ParsingMode};
 use crate::error::{Id3v2Error, Id3v2ErrorKind, Result};
 use crate::id3::v2::frame::content::parse_content;
 use crate::id3::v2::header::Id3v2Version;
+use crate::id3::v2::tag::ATTACHED_PICTURE_ID;
 use crate::id3::v2::util::synchsafe::{SynchsafeInteger, UnsynchronizedStream};
 use crate::id3::v2::{BinaryFrame, FrameFlags, FrameHeader, FrameId};
 use crate::macros::try_vec;
 
 use std::io::Read;
 
-use crate::id3::v2::tag::ATTACHED_PICTURE_ID;
 use byteorder::{BigEndian, ReadBytesExt};
 
 pub(crate) enum ParsedFrame<'a> {
 	Next(Frame<'a>),
-	Skip { size: u32 },
+	Skip,
 	Eof,
 }
 
@@ -46,16 +46,19 @@ impl<'a> ParsedFrame<'a> {
 				match parse_options.parsing_mode {
 					ParsingMode::Strict => return Err(err),
 					ParsingMode::BestAttempt | ParsingMode::Relaxed => {
+						log::warn!("Failed to read frame header, skipping: {}", err);
+
 						// Skip this frame and continue reading
-						// TODO: Log error?
-						return Ok(Self::Skip { size });
+						skip_frame(reader, size)?;
+						return Ok(Self::Skip);
 					},
 				}
 			},
 		};
 
 		if !parse_options.read_cover_art && id == ATTACHED_PICTURE_ID {
-			return Ok(Self::Skip { size });
+			skip_frame(reader, size)?;
+			return Ok(Self::Skip);
 		}
 
 		if size == 0 {
@@ -64,7 +67,9 @@ impl<'a> ParsedFrame<'a> {
 			}
 
 			log::debug!("Encountered a zero length frame, skipping");
-			return Ok(Self::Skip { size });
+
+			skip_frame(reader, size)?;
+			return Ok(Self::Skip);
 		}
 
 		// Get the encryption method symbol
@@ -252,6 +257,26 @@ fn parse_frame<R: Read>(
 ) -> Result<ParsedFrame<'static>> {
 	match parse_content(reader, id, flags, version, parse_mode)? {
 		Some(frame) => Ok(ParsedFrame::Next(frame)),
-		None => Ok(ParsedFrame::Skip { size }),
+		None => {
+			skip_frame(reader, size)?;
+			Ok(ParsedFrame::Skip)
+		},
 	}
 }
+
+// Note that this is only ever given the full frame size.
+//
+// In the context of `ParsedFrame::read`, the reader is restricted to the frame content, so this
+// is a safe operation, regardless of where we are in parsing the frame.
+//
+// This assumption *CANNOT* be made in other contexts.
+fn skip_frame(reader: &mut impl Read, size: u32) -> Result<()> {
+	log::trace!("Skipping frame of size {}", size);
+
+	let size = u64::from(size);
+	let mut reader = reader.take(size);
+	let skipped = std::io::copy(&mut reader, &mut std::io::sink())?;
+	debug_assert!(skipped <= size);
+
+	Ok(())
+}

lofty/src/id3/v2/read.rs

@@ -2,7 +2,7 @@ use super::frame::read::ParsedFrame;
 use super::header::Id3v2Header;
 use super::tag::Id3v2Tag;
 use crate::config::ParseOptions;
-use crate::error::{Id3v2Error, Id3v2ErrorKind, Result};
+use crate::error::Result;
 use crate::id3::v2::util::synchsafe::UnsynchronizedStream;
 use crate::id3::v2::{Frame, FrameId, Id3v2Version, TimestampFrame};
 use crate::tag::items::Timestamp;
@@ -130,19 +130,6 @@ fn construct_tdrc_from_v3(tag: &mut Id3v2Tag) {
 	}
 }
 
-fn skip_frame(reader: &mut impl Read, size: u32) -> Result<()> {
-	log::trace!("Skipping frame of size {}", size);
-
-	let size = u64::from(size);
-	let mut reader = reader.take(size);
-	let skipped = std::io::copy(&mut reader, &mut std::io::sink())?;
-	debug_assert!(skipped <= size);
-	if skipped != size {
-		return Err(Id3v2Error::new(Id3v2ErrorKind::BadFrameLength).into());
-	}
-	Ok(())
-}
-
 fn read_all_frames_into_tag<R>(
 	reader: &mut R,
 	header: Id3v2Header,
@@ -181,9 +168,7 @@ where
 				}
 			},
 			// No frame content found or ignored due to errors, but we can expect more frames
-			ParsedFrame::Skip { size } => {
-				skip_frame(reader, size)?;
-			},
+			ParsedFrame::Skip => {},
 			// No frame content found, and we can expect there are no more frames
 			ParsedFrame::Eof => break,
 		}

lofty/src/id3/v2/tag.rs

@@ -574,7 +574,7 @@ pub(crate) struct GenresIter<'a> {
 }
 
 impl<'a> GenresIter<'a> {
-	pub fn new(value: &'a str, preserve_indexes: bool) -> GenresIter<'_> {
+	pub fn new(value: &'a str, preserve_indexes: bool) -> GenresIter<'a> {
 		GenresIter {
 			value,
 			pos: 0,

lofty/src/id3/v2/tag/tests.rs

@@ -1554,3 +1554,32 @@ fn artists_tag_conversion() {
 
 	assert_eq!(id3v2_artists, ARTISTS);
 }
+
+#[test_log::test]
+fn ensure_frame_skipping_within_bounds() {
+	// This tag has an invalid `TDEN` frame, but it is skippable in BestAttempt/Relaxed parsing mode.
+	// We should be able to continue reading the tag as normal, reaching the other `TDTG` frame.
+
+	let path = "tests/tags/assets/id3v2/skippable_frame_otherwise_valid.id3v24";
+	let tag = read_tag_with_options(
+		&read_path(path),
+		ParseOptions::new().parsing_mode(ParsingMode::BestAttempt),
+	);
+
+	assert_eq!(tag.len(), 1);
+	assert_eq!(
+		tag.get(&FrameId::Valid(Cow::Borrowed("TDTG"))),
+		Some(&Frame::Timestamp(TimestampFrame::new(
+			FrameId::Valid(Cow::Borrowed("TDTG")),
+			TextEncoding::Latin1,
+			Timestamp {
+				year: 2014,
+				month: Some(6),
+				day: Some(10),
+				hour: Some(2),
+				minute: Some(16),
+				second: Some(10),
+			},
+		)))
+	);
+}

lofty/src/id3/v2/util/synchsafe.rs

@@ -78,6 +78,25 @@ impl<R> UnsynchronizedStream<R> {
 	pub fn into_inner(self) -> R {
 		self.reader
 	}
+
+	/// Get a reference to the inner reader
+	///
+	/// # Examples
+	///
+	/// ```rust
+	/// use lofty::id3::v2::util::synchsafe::UnsynchronizedStream;
+	/// use std::io::Cursor;
+	///
+	/// # fn main() -> lofty::error::Result<()> {
+	/// let reader = Cursor::new([0xFF, 0x00, 0x1A]);
+	/// let unsynchronized_reader = UnsynchronizedStream::new(reader);
+	///
+	/// let reader = unsynchronized_reader.get_ref();
+	/// assert_eq!(reader.position(), 0);
+	/// # Ok(()) }
+	pub fn get_ref(&self) -> &R {
+		&self.reader
+	}
 }
 
 impl<R: Read> Read for UnsynchronizedStream<R> {

lofty/src/ogg/tag.rs

@@ -210,7 +210,7 @@ impl VorbisComments {
 	/// let all_artists = vorbis_comments.get_all("ARTIST").collect::<Vec<&str>>();
 	/// assert_eq!(all_artists, vec!["Foo artist", "Bar artist", "Baz artist"]);
 	/// ```
-	pub fn get_all<'a>(&'a self, key: &'a str) -> impl Iterator<Item = &'a str> + Clone + '_ {
+	pub fn get_all<'a>(&'a self, key: &'a str) -> impl Iterator<Item = &'a str> + Clone + 'a {
 		self.items
 			.iter()
 			.filter_map(move |(k, v)| (k.eq_ignore_ascii_case(key)).then_some(v.as_str()))

lofty/src/ogg/vorbis/properties.rs

@@ -121,7 +121,7 @@ where
 		let last_page_abgp = last_page.header().abgp;
 
 		if properties.sample_rate > 0 {
-			let total_samples = last_page_abgp.saturating_sub(first_page_abgp) as u128;
+			let total_samples = u128::from(last_page_abgp.saturating_sub(first_page_abgp));
 
 			// Best case scenario
 			if total_samples > 0 {