Skip to content

Commit

Permalink
fix: prevent panics from malformed id3 metadata
Browse files Browse the repository at this point in the history
* mllt overflow
* invalid frame id length
* decode content size underflow
* decode MLLT deviation overflow
* stream tag frame bytes underflow
  • Loading branch information
cdmurph32 committed Dec 13, 2024
1 parent ca57592 commit 89b12bc
Show file tree
Hide file tree
Showing 3 changed files with 144 additions and 5 deletions.
77 changes: 75 additions & 2 deletions src/stream/frame/content.rs
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,9 @@ impl<W: io::Write> Encoder<W> {
&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,
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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<u8> {
encoding.encode(text)
Expand Down Expand Up @@ -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");
}
}
}
57 changes: 55 additions & 2 deletions src/stream/frame/v4.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ pub fn decode(mut reader: impl io::Read) -> crate::Result<Option<(usize, Frame)>

let read_size = if flags.contains(Flags::DATA_LENGTH_INDICATOR) {
let _decompressed_size = unsynch::decode_u32(reader.read_u32::<BigEndian>()?);
content_size - 4
content_size.saturating_sub(4)
} else {
content_size
};
Expand Down Expand Up @@ -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::<BigEndian>(unsynch::encode_u32(
Expand All @@ -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");
}
}
}
15 changes: 14 additions & 1 deletion src/stream/tag.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
}
}

0 comments on commit 89b12bc

Please sign in to comment.