RGBCube/serenity
1
Fork 0
mirror of https://github.com/RGBCube/serenity synced 2025-07-27 00:07:36 +00:00
Code Issues Activity Actions

LibAudio: Prevent multiple kinds of buffer overruns in FLAC picture load

Browse source 
The fuzzer found one heap buffer overflow here due to confusion between
u32* and u8* (the given size is for bytes, but we used it for 32-bit
elements, quadrupling it), and it looks like there's an opportunity for
several more. This commit modernizes the picture loader by using
String's built-in stream loader, and also adds several spec-compliance
checks: The MIME type must be ASCII in a specific range, and the picture
description must be UTF-8.
This commit is contained in:
kleines Filmröllchen 2023-06-27 20:26:39 +02:00 • committed by Jelle Raaijmakers
parent ab9c18c176
commit 9bece0d0da
2 changed files with 21 additions and 10 deletions
Download patch file Download diff file Expand all files Collapse all files

25
Userland/Libraries/LibAudio/FlacLoader.cpp
View file

@ -156,20 +156,28 @@ MaybeLoaderError FlacLoaderPlugin::load_picture(FlacRawMetadataBlock& block)
FixedMemoryStream memory_stream { block.data.bytes() }; FixedMemoryStream memory_stream { block.data.bytes() };
BigEndianInputBitStream picture_block_bytes { MaybeOwned<Stream>(memory_stream) }; BigEndianInputBitStream picture_block_bytes { MaybeOwned<Stream>(memory_stream) };
PictureData picture {}; PictureData picture;
picture.type = static_cast<ID3PictureType>(LOADER_TRY(picture_block_bytes.read_bits(32))); picture.type = static_cast<ID3PictureType>(LOADER_TRY(picture_block_bytes.read_bits(32)));
auto const mime_string_length = LOADER_TRY(picture_block_bytes.read_bits(32)); auto const mime_string_length = LOADER_TRY(picture_block_bytes.read_bits(32));
// Note: We are seeking before reading the value to ensure that we stayed inside buffer's size.
auto offset_before_seeking = memory_stream.offset(); auto offset_before_seeking = memory_stream.offset();
LOADER_TRY(memory_stream.seek(mime_string_length, SeekMode::FromCurrentPosition)); if (offset_before_seeking + mime_string_length >= block.data.size())
picture.mime_string = { block.data.bytes().data() + offset_before_seeking, (size_t)mime_string_length }; return LoaderError { LoaderError::Category::Format, LOADER_TRY(m_stream->tell()), "Picture MIME type exceeds available data" };
// "The MIME type string, in printable ASCII characters 0x20-0x7E."
picture.mime_string = LOADER_TRY(String::from_stream(memory_stream, mime_string_length));
for (auto code_point : picture.mime_string.code_points()) {
if (code_point < 0x20 || code_point > 0x7E)
return LoaderError { LoaderError::Category::Format, LOADER_TRY(m_stream->tell()), "Picture MIME type is not ASCII in range 0x20 - 0x7E" };
}
auto const description_string_length = LOADER_TRY(picture_block_bytes.read_bits(32)); auto const description_string_length = LOADER_TRY(picture_block_bytes.read_bits(32));
offset_before_seeking = memory_stream.offset(); offset_before_seeking = memory_stream.offset();
LOADER_TRY(memory_stream.seek(description_string_length, SeekMode::FromCurrentPosition)); if (offset_before_seeking + description_string_length >= block.data.size())
picture.description_string = Vector<u32> { Span<u32> { reinterpret_cast<u32*>(block.data.bytes().data() + offset_before_seeking), (size_t)description_string_length } }; return LoaderError { LoaderError::Category::Format, LOADER_TRY(m_stream->tell()), "Picture description exceeds available data" };
picture.description_string = LOADER_TRY(String::from_stream(memory_stream, description_string_length));
picture.width = LOADER_TRY(picture_block_bytes.read_bits(32)); picture.width = LOADER_TRY(picture_block_bytes.read_bits(32));
picture.height = LOADER_TRY(picture_block_bytes.read_bits(32)); picture.height = LOADER_TRY(picture_block_bytes.read_bits(32));
@ -179,8 +187,11 @@ MaybeLoaderError FlacLoaderPlugin::load_picture(FlacRawMetadataBlock& block)
auto const picture_size = LOADER_TRY(picture_block_bytes.read_bits(32)); auto const picture_size = LOADER_TRY(picture_block_bytes.read_bits(32));
offset_before_seeking = memory_stream.offset(); offset_before_seeking = memory_stream.offset();
if (offset_before_seeking + picture_size > block.data.size())
return LoaderError { LoaderError::Category::Format, static_cast<size_t>(TRY(m_stream->tell())), "Picture size exceeds available data" };
LOADER_TRY(memory_stream.seek(picture_size, SeekMode::FromCurrentPosition)); LOADER_TRY(memory_stream.seek(picture_size, SeekMode::FromCurrentPosition));
picture.data = Vector<u8> { Span<u8> { block.data.bytes().data() + offset_before_seeking, (size_t)picture_size } }; picture.data = Vector<u8> { block.data.bytes().slice(offset_before_seeking, picture_size) };
m_pictures.append(move(picture)); m_pictures.append(move(picture));

6
Userland/Libraries/LibAudio/GenericTypes.h
View file

@ -6,7 +6,7 @@
#pragma once #pragma once
#include <AK/DeprecatedString.h> #include <AK/String.h>
#include <AK/Vector.h> #include <AK/Vector.h>
namespace Audio { namespace Audio {
@ -40,8 +40,8 @@ enum class ID3PictureType : u32 {
// Note: This was first implemented for Flac but is compatible with ID3v2 // Note: This was first implemented for Flac but is compatible with ID3v2
struct PictureData { struct PictureData {
ID3PictureType type {}; ID3PictureType type {};
DeprecatedString mime_string {}; String mime_string {};
Vector<u32> description_string {}; String description_string {};
u32 width {}; u32 width {};
u32 height {}; u32 height {};