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

Author: Serial-ATA

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 {