From 94ebb2306bb3ec1ac2185c4c89a31e59fe365e57 Mon Sep 17 00:00:00 2001 From: Jonas Teuwen Date: Sat, 25 Jul 2026 14:40:35 +0200 Subject: [PATCH] chore: hardening GitOrigin-RevId: fab6fcbae2626becb9ad5f107924bcae1e96fb34 --- BUILD.bazel | 24 ++++ include/fastslide/core/tile_plan.h | 16 ++- include/fastslide/image.h | 7 ++ .../readers/isyntax/third_party/isyntax.h | 10 +- .../readers/simpletiff_decode_utils.h | 35 ++++++ include/fastslide/runtime/io/path_utils.h | 116 ++++++++++++++++++ package/versions.json | 16 +-- src/core/tile_plan_test.cpp | 33 +++++ src/image_test.cpp | 22 ++++ src/readers/isyntax/third_party/open.cpp | 13 ++ .../isyntax/third_party/xml_parser.cpp | 13 ++ .../isyntax/third_party/xml_semantics.cpp | 13 +- src/readers/mrxs/mrxs_data_reader.cpp | 20 +++ src/readers/mrxs/mrxs_metadata_loader.cpp | 22 ++++ src/readers/omezarr/omezarr.cpp | 10 +- src/readers/qptiff/metadata_parser.cpp | 30 ++++- src/runtime/decoders/bmp_decoder.cpp | 19 ++- src/runtime/decoders/bmp_decoder_test.cpp | 27 ++++ src/runtime/io/path_utils_test.cpp | 94 ++++++++++++++ src/runtime/tile_writer/paint_dispatch.cpp | 55 +++++++++ src/runtime/tile_writer_test.cpp | 27 ++++ tests/meson.build | 1 + 22 files changed, 601 insertions(+), 22 deletions(-) create mode 100644 include/fastslide/runtime/io/path_utils.h create mode 100644 src/runtime/io/path_utils_test.cpp diff --git a/BUILD.bazel b/BUILD.bazel index 88fc844..1ead2dd 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -499,6 +499,7 @@ cc_library( ":runtime_cache_interface", ":runtime_format_descriptor", ":runtime_lru_tile_cache", + ":runtime_path_utils", ":runtime_reader_registry", ":runtime_tile_writer", ], @@ -1414,6 +1415,18 @@ cc_library( ], ) +cc_library( + name = "runtime_path_utils", + hdrs = ["include/fastslide/runtime/io/path_utils.h"], + copts = WASM_COPTS, + includes = ["include"], + visibility = ["//visibility:public"], + deps = [ + "@aifocore-cpp//:result", + "@aifocore-cpp//:utilities-core", + ], +) + cc_library( name = "runtime_file_reader", srcs = ["src/runtime/io/file_reader.cpp"], @@ -1916,6 +1929,17 @@ cc_test( ], ) +cc_test( + name = "path_utils_test", + size = "small", + srcs = ["src/runtime/io/path_utils_test.cpp"], + deps = [ + ":runtime_path_utils", + "@googletest//:gtest", + "@googletest//:gtest_main", + ], +) + # Format tests cc_test( name = "aperio_metadata_parser_test", diff --git a/include/fastslide/core/tile_plan.h b/include/fastslide/core/tile_plan.h index 268339b..049d051 100644 --- a/include/fastslide/core/tile_plan.h +++ b/include/fastslide/core/tile_plan.h @@ -199,20 +199,32 @@ struct OutputSpec { }; /// @brief Convert a DataType to the closest OutputSpec::PixelFormat +/// +/// The tile sinks memcpy raw samples out of the decoded tile using the +/// canvas's sample width, so this mapping must preserve the source's bytes per +/// sample. `PixelFormat` has no signed members; the signed types therefore map +/// onto the unsigned member of the same width, which keeps the bit pattern +/// intact. Falling back to `kFloat32` instead would widen a 16-bit sample to +/// four bytes and make the sinks stride off the end of the tile. +/// /// @param dtype Source data type -/// @return Matching pixel format (kUInt8, kUInt16, kUInt32, or kFloat32) +/// @return Pixel format with the same width as @p dtype constexpr OutputSpec::PixelFormat ToOutputPixelFormat( fastslide::DataType dtype) { switch (dtype) { case fastslide::DataType::kUInt8: return OutputSpec::PixelFormat::kUInt8; case fastslide::DataType::kUInt16: + case fastslide::DataType::kInt16: return OutputSpec::PixelFormat::kUInt16; case fastslide::DataType::kUInt32: + case fastslide::DataType::kInt32: return OutputSpec::PixelFormat::kUInt32; - default: + case fastslide::DataType::kFloat32: + case fastslide::DataType::kFloat64: return OutputSpec::PixelFormat::kFloat32; } + return OutputSpec::PixelFormat::kUInt8; } /// @brief Complete tile reading plan diff --git a/include/fastslide/image.h b/include/fastslide/image.h index 0c44ba0..5aebb5c 100644 --- a/include/fastslide/image.h +++ b/include/fastslide/image.h @@ -89,6 +89,13 @@ constexpr DataType DataTypeFromSampleFormat(uint16_t bits_per_sample, case 3: // IEEE floating point. return bits_per_sample >= 64 ? DataType::kFloat64 : DataType::kFloat32; case 2: // Two's-complement signed integer. + // `DataType` has no signed 8-bit member. Map an 8-bit signed page onto + // kUInt8 rather than widening it to kInt16: the bit pattern survives and, + // more importantly, so does the one-byte sample width that the tile sinks + // assume when they copy out of the decoded strip. + if (bits_per_sample <= 8) { + return DataType::kUInt8; + } return bits_per_sample <= 16 ? DataType::kInt16 : DataType::kInt32; case 1: // Unsigned integer. default: diff --git a/include/fastslide/readers/isyntax/third_party/isyntax.h b/include/fastslide/readers/isyntax/third_party/isyntax.h index 9a7d49c..c39d1cb 100644 --- a/include/fastslide/readers/isyntax/third_party/isyntax.h +++ b/include/fastslide/readers/isyntax/third_party/isyntax.h @@ -38,6 +38,12 @@ typedef struct isyntax_xml_cpp_state_t isyntax_xml_cpp_state_t; #include "fastslide/readers/isyntax/third_party/third_party/yxml.h" +// Capacities of the inline arrays in `isyntax_image_t` and `isyntax_t`. Both +// are filled from counts parsed out of the file's XML header, so every writer +// must bound the count against these before indexing. +#define ISYNTAX_MAX_LEVELS 16 +#define ISYNTAX_MAX_IMAGES 16 + enum isyntax_image_type_enum { ISYNTAX_IMAGE_TYPE_NONE = 0, ISYNTAX_IMAGE_TYPE_MACROIMAGE = 1, @@ -305,7 +311,7 @@ typedef struct isyntax_image_t { int32_t offset_y; int32_t level_count; int32_t max_scale; - isyntax_level_t levels[16]; + isyntax_level_t levels[ISYNTAX_MAX_LEVELS]; int32_t compressor_version; bool compression_is_lossy; int32_t lossy_image_compression_ratio; @@ -376,7 +382,7 @@ typedef struct isyntax_t { isyntax_open_flags_t open_flags; int64_t filesize; file_handle_t file_handle; - isyntax_image_t images[16]; + isyntax_image_t images[ISYNTAX_MAX_IMAGES]; int32_t image_count; isyntax_block_header_template_t block_header_templates[64]; int32_t block_header_template_count; diff --git a/include/fastslide/readers/simpletiff_decode_utils.h b/include/fastslide/readers/simpletiff_decode_utils.h index afae7eb..77675d3 100644 --- a/include/fastslide/readers/simpletiff_decode_utils.h +++ b/include/fastslide/readers/simpletiff_decode_utils.h @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -176,6 +177,40 @@ inline aifocore::Result ReadTileOrStrip( decoded_w, decoded_h)); } + // `buffer` is sized from the actual payload, but the geometry we advertise + // below comes from the IFD. The two can disagree: a JPEG page declaring + // SamplesPerPixel=4 still decodes to three bytes per pixel, and an + // undersized LZW/Deflate payload inflates to fewer rows than declared. + // Consumers stride using the advertised geometry, so a buffer that does not + // back it becomes an out-of-bounds read. Reject the contradiction instead of + // guessing which side is right. + const size_t decoded_pixels = + static_cast(decoded_w) * static_cast(decoded_h); + const bool pixels_overflowed = + decoded_pixels / static_cast(decoded_h) != + static_cast(decoded_w); + if (pixels_overflowed || + decoded_pixels > std::numeric_limits::max() / bytes_per_pixel) { + return AIFOCORE_MAKE_STATUS( + aifocore::StatusCode::kInvalidArgument, + aifocore::fmt::format("Decoded geometry {}x{} at {} bytes/pixel " + "overflows for page {}", + decoded_w, decoded_h, bytes_per_pixel, + page_index)); + } + const size_t required_bytes = decoded_pixels * bytes_per_pixel; + if (buffer.size() < required_bytes) { + return AIFOCORE_MAKE_STATUS( + aifocore::StatusCode::kInvalidArgument, + aifocore::fmt::format( + "Page {} tile/strip {} decoded to {} bytes but its {}x{} geometry " + "at {} bytes/pixel (SamplesPerPixel={}, BitsPerSample={}) requires " + "{}", + page_index, tile_or_strip_index, buffer.size(), decoded_w, + decoded_h, bytes_per_pixel, page_header.samples_per_pixel, + page_header.bits_per_sample, required_bytes)); + } + return DecodedInterleavedView{ .data = std::span(buffer.data(), buffer.size()), .width = static_cast(decoded_w), diff --git a/include/fastslide/runtime/io/path_utils.h b/include/fastslide/runtime/io/path_utils.h new file mode 100644 index 0000000..5c7200d --- /dev/null +++ b/include/fastslide/runtime/io/path_utils.h @@ -0,0 +1,116 @@ +// Copyright 2026 Jonas Teuwen. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef AIFO_FASTSLIDE_INCLUDE_FASTSLIDE_RUNTIME_IO_PATH_UTILS_H_ +#define AIFO_FASTSLIDE_INCLUDE_FASTSLIDE_RUNTIME_IO_PATH_UTILS_H_ + +#include +#include + +#include "aifocore/status/result.h" +#include "aifocore/utilities/fmt.h" + +/** + * @file path_utils.h + * @brief Containment checks for filenames taken from slide files. + * + * Several formats are directory bundles whose manifest names the sibling files + * holding the pixel data: MIRAX reads `FILE_0`.. out of `Slidedat.ini`, + * OME-Zarr reads `datasets[].path` out of `zarr.json`. Those strings are + * attacker controlled, so joining them onto the bundle directory without + * checking lets a crafted slide address any file the process can open. + */ + +namespace fastslide { +namespace runtime { +namespace io { + +/// @brief Join `relative` onto `root` without allowing an escape from `root`. +/// +/// Rejects absolute paths and any `..` component, then resolves the result and +/// confirms it is still inside `root`. Resolution goes through +/// `std::filesystem::weakly_canonical`, so a symlink pointing out of the bundle +/// is rejected as well -- the check would otherwise pass on a path that +/// contains no `..` at all. +/// +/// @param root Bundle directory the result must stay within. +/// @param relative Path read out of the slide file. +/// @return Resolved absolute path, or `kInvalidArgument` if it escapes `root`. +[[nodiscard]] inline aifocore::Result +ResolveContainedPath(const std::filesystem::path& root, + const std::filesystem::path& relative) { + namespace fs = std::filesystem; + + if (relative.empty()) { + return AIFOCORE_MAKE_STATUS(aifocore::StatusCode::kInvalidArgument, + "Slide references an empty path"); + } + if (relative.is_absolute() || relative.has_root_name() || + relative.has_root_directory()) { + return AIFOCORE_MAKE_STATUS( + aifocore::StatusCode::kInvalidArgument, + aifocore::fmt::format("Slide references absolute path '{}'", + relative.string())); + } + for (const auto& component : relative) { + if (component == "..") { + return AIFOCORE_MAKE_STATUS( + aifocore::StatusCode::kInvalidArgument, + aifocore::fmt::format("Slide references parent directory in '{}'", + relative.string())); + } + } + + std::error_code err; + const fs::path resolved_root = fs::weakly_canonical(root, err); + if (err) { + return AIFOCORE_MAKE_STATUS( + aifocore::StatusCode::kInvalidArgument, + aifocore::fmt::format("Cannot resolve slide directory '{}': {}", + root.string(), err.message())); + } + const fs::path resolved = fs::weakly_canonical(root / relative, err); + if (err) { + return AIFOCORE_MAKE_STATUS( + aifocore::StatusCode::kInvalidArgument, + aifocore::fmt::format("Cannot resolve slide path '{}': {}", + relative.string(), err.message())); + } + + // Compare component-wise rather than as strings so a sibling directory whose + // name merely starts with the root's name is not mistaken for a child. + auto root_it = resolved_root.begin(); + auto resolved_it = resolved.begin(); + for (; root_it != resolved_root.end(); ++root_it, ++resolved_it) { + if (resolved_it == resolved.end() || *resolved_it != *root_it) { + return AIFOCORE_MAKE_STATUS( + aifocore::StatusCode::kInvalidArgument, + aifocore::fmt::format( + "Slide path '{}' resolves to '{}', outside the slide directory " + "'{}'", + relative.string(), resolved.string(), resolved_root.string())); + } + } + + return resolved; +} + +} // namespace io +} // namespace runtime + +using runtime::io::ResolveContainedPath; + +} // namespace fastslide + +#endif // AIFO_FASTSLIDE_INCLUDE_FASTSLIDE_RUNTIME_IO_PATH_UTILS_H_ diff --git a/package/versions.json b/package/versions.json index dc0646c..ddadbd4 100644 --- a/package/versions.json +++ b/package/versions.json @@ -1,10 +1,10 @@ { - "versions": [ - { - "display": "FastSlide", - "id": "fastslide", - "type": "primary", - "version": "0.8.1" - } - ] + "versions": [ + { + "display": "FastSlide", + "id": "fastslide", + "type": "primary", + "version": "0.8.1" + } + ] } diff --git a/src/core/tile_plan_test.cpp b/src/core/tile_plan_test.cpp index 9bcce73..3113bdf 100644 --- a/src/core/tile_plan_test.cpp +++ b/src/core/tile_plan_test.cpp @@ -553,5 +553,38 @@ TEST(BatchTilePlanTest, DeduplicatedOperations) { EXPECT_EQ(batch.GetUniqueOperations(), 1); // Only one unique tile } +/// @brief The output format must never be wider than the source data type. +/// +/// The tile sinks copy raw samples out of a decoded tile using the canvas's +/// sample width. +TEST(OutputPixelFormatTest, PreservesSampleWidth) { + const auto width_of = [](OutputSpec::PixelFormat format) -> size_t { + switch (format) { + case OutputSpec::PixelFormat::kUInt8: + return 1; + case OutputSpec::PixelFormat::kUInt16: + return 2; + case OutputSpec::PixelFormat::kUInt32: + case OutputSpec::PixelFormat::kFloat32: + return 4; + } + return 0; + }; + + for (const auto dtype : + {DataType::kUInt8, DataType::kUInt16, DataType::kInt16, + DataType::kUInt32, DataType::kInt32, DataType::kFloat32, + DataType::kFloat64}) { + EXPECT_LE(width_of(ToOutputPixelFormat(dtype)), GetDataTypeSize(dtype)) + << "output format wider than source for DataType " + << static_cast(dtype); + } + + EXPECT_EQ(ToOutputPixelFormat(DataType::kInt16), + OutputSpec::PixelFormat::kUInt16); + EXPECT_EQ(ToOutputPixelFormat(DataType::kInt32), + OutputSpec::PixelFormat::kUInt32); +} + } // namespace core } // namespace fastslide diff --git a/src/image_test.cpp b/src/image_test.cpp index 9661ac0..bc21a0c 100644 --- a/src/image_test.cpp +++ b/src/image_test.cpp @@ -306,4 +306,26 @@ TEST(DataTypeFromBitsPerSampleTest, MapsCommonBitDepths) { EXPECT_EQ(DataTypeFromBitsPerSample(32), DataType::kFloat32); } +/// The chosen type must keep the storage width of the page it describes; +/// otherwise the tile sinks stride a decoded strip by the wrong sample size. +TEST(DataTypeFromSampleFormatTest, PreservesStorageWidth) { + constexpr uint16_t kUnsigned = 1; + constexpr uint16_t kSigned = 2; + constexpr uint16_t kFloat = 3; + + for (const uint16_t bits : {uint16_t{8}, uint16_t{16}, uint16_t{32}}) { + for (const uint16_t format : {kUnsigned, kSigned, kFloat}) { + if (format == kFloat && bits < 32) { + continue; // No sub-32-bit IEEE format is defined. + } + EXPECT_EQ(GetDataTypeSize(DataTypeFromSampleFormat(bits, format)), + bits / 8u) + << "bits=" << bits << " sample_format=" << format; + } + } + + // Signed 8-bit has no exact DataType member; it must still stay one byte. + EXPECT_EQ(DataTypeFromSampleFormat(8, kSigned), DataType::kUInt8); +} + } // namespace fastslide diff --git a/src/readers/isyntax/third_party/open.cpp b/src/readers/isyntax/third_party/open.cpp index 2adc66d..db41103 100644 --- a/src/readers/isyntax/third_party/open.cpp +++ b/src/readers/isyntax/third_party/open.cpp @@ -36,6 +36,7 @@ #include "aifocore/platform/portability.h" #include "aifocore/status/result.h" +#include "aifocore/utilities/fmt.h" #include "fastslide/readers/isyntax/third_party/open_helpers.h" #include "fastslide/readers/isyntax/third_party/platform/common.h" #include "fastslide/readers/isyntax/third_party/seektable.h" @@ -205,6 +206,18 @@ aifocore::Result ReadXmlHeader(std::FILE* fp, isyntax_t* isyntax, aifocore::Status InitializeLevelGeometry(isyntax_t* isyntax, isyntax_image_t* wsi_image) { + // `level_count` comes from the XML header and indexes the fixed-capacity + // `wsi_image->levels` array in the loops below. The parser clamps it, so a + // value outside the representable range means the header contradicted + // itself; refuse the file rather than describing geometry we did not parse. + if (wsi_image->level_count < 1 || + wsi_image->level_count > ISYNTAX_MAX_LEVELS) { + return AIFOCORE_MAKE_STATUS( + aifocore::StatusCode::kInvalidArgument, + aifocore::fmt::format("iSyntax: level count {} outside supported " + "range 1..{}", + wsi_image->level_count, ISYNTAX_MAX_LEVELS)); + } isyntax::open::InitializeLevelGeometry(isyntax, wsi_image); return aifocore::Status(); } diff --git a/src/readers/isyntax/third_party/xml_parser.cpp b/src/readers/isyntax/third_party/xml_parser.cpp index 74123de..7db0d57 100644 --- a/src/readers/isyntax/third_party/xml_parser.cpp +++ b/src/readers/isyntax/third_party/xml_parser.cpp @@ -674,6 +674,19 @@ static bool HandleAttrEnd(isyntax_t* isyntax, yxml_t* x) { ASSERT(parser->attribute_index == 0); ASSERT(std::strcmp(x->attr, "ObjectType") == 0); if (std::strcmp(parser->attrbuf, "DPScannedImage") == 0) { + // Each DPScannedImage node claims the next slot of the fixed-capacity + // `isyntax->images` array. The header controls how many such nodes it + // declares, so without this bound it can write past the array. + if (isyntax->image_count >= ISYNTAX_MAX_IMAGES) { + SetXmlErrorOnce( + isyntax, + AIFOCORE_MAKE_STATUS( + aifocore::StatusCode::kResourceExhausted, + aifocore::fmt::format( + "iSyntax XML error: more than {} DPScannedImage objects", + ISYNTAX_MAX_IMAGES))); + return false; + } parser->current_image = isyntax->images + isyntax->image_count; parser->running_image_index = isyntax->image_count++; } diff --git a/src/readers/isyntax/third_party/xml_semantics.cpp b/src/readers/isyntax/third_party/xml_semantics.cpp index 6ebd9d6..7057237 100644 --- a/src/readers/isyntax/third_party/xml_semantics.cpp +++ b/src/readers/isyntax/third_party/xml_semantics.cpp @@ -18,6 +18,7 @@ #include "fastslide/readers/isyntax/third_party/xml_semantics.h" +#include #include #include #include @@ -449,10 +450,16 @@ void ParseDimensionRange(isyntax_t* isyntax, isyntax_image_t* image, case 2: break; // always 3 color channels case 3: { - image->level_count = range.numsteps; - image->max_scale = range.numsteps - 1; + // `numsteps` is attacker-controlled and indexes the fixed + // `image->levels` array, and is also used as a shift distance below. + // Bound it to the array capacity so neither can run out of range; the + // Status check in InitializeLevelGeometry rejects the file afterwards. + const int32_t level_count = + std::clamp(range.numsteps, 1, ISYNTAX_MAX_LEVELS); + image->level_count = level_count; + image->max_scale = level_count - 1; image->level0_padding = - (kPerLevelPadding << range.numsteps) - kPerLevelPadding; + (kPerLevelPadding << level_count) - kPerLevelPadding; image->width = image->width_including_padding - 2 * image->level0_padding; image->height = diff --git a/src/readers/mrxs/mrxs_data_reader.cpp b/src/readers/mrxs/mrxs_data_reader.cpp index cac6f98..f923bd4 100644 --- a/src/readers/mrxs/mrxs_data_reader.cpp +++ b/src/readers/mrxs/mrxs_data_reader.cpp @@ -138,10 +138,30 @@ aifocore::Result> MrxsDataReader::ReadData( aifocore::fmt::format("Invalid size: {}", size)); } + if (size > constants::kMaxTileSize) { + return AIFOCORE_MAKE_STATUS( + aifocore::StatusCode::kInvalidArgument, + aifocore::fmt::format("Requested read of {} bytes exceeds maximum {}", + size, constants::kMaxTileSize)); + } + // Open file FileReader file; AIFOCORE_ASSIGN_OR_RETURN(file, FileReader::Open(datafile_path, "rb")); + // Mirror the bounds check ReadTileData performs. Without it a crafted + // non-hierarchical index record can name any offset and length, and + // `ReadBytes` allocates before it discovers the file is shorter. + int64_t file_size; + AIFOCORE_ASSIGN_OR_RETURN(file_size, file.GetSize()); + if (offset > file_size - size) { + return AIFOCORE_MAKE_STATUS( + aifocore::StatusCode::kInvalidArgument, + aifocore::fmt::format( + "Read extends beyond file: offset={}, size={}, file_size={}", + offset, size, file_size)); + } + // Seek to offset AIFOCORE_RETURN_IF_ERROR(file.Seek(offset)); diff --git a/src/readers/mrxs/mrxs_metadata_loader.cpp b/src/readers/mrxs/mrxs_metadata_loader.cpp index bad691a..fe8c3d0 100644 --- a/src/readers/mrxs/mrxs_metadata_loader.cpp +++ b/src/readers/mrxs/mrxs_metadata_loader.cpp @@ -25,6 +25,7 @@ #include "fastslide/readers/mrxs/mrxs_ini_parser.h" #include "fastslide/readers/mrxs/mrxs_layer_parser.h" #include "fastslide/readers/mrxs/mrxs_position_reader.h" +#include "fastslide/runtime/io/path_utils.h" namespace fastslide { namespace mrxs { @@ -131,12 +132,33 @@ aifocore::Result ReadSlidedatIni(const fs::path& slidedat_path, return info; } +/// @brief Reject INI-supplied filenames that point outside the slide directory. +/// +/// `FILE_n` and `INDEXFILE` are joined onto the slide directory at several +/// points during reading, and their contents are returned to the caller as +/// tile or associated-image bytes. Validating them here, once, keeps every +/// downstream join safe. +aifocore::Status ValidateReferencedPaths(const fs::path& dirname, + const SlideDataInfo& info) { + for (const std::string& datafile : info.datafile_paths) { + AIFOCORE_RETURN_IF_ERROR( + runtime::io::ResolveContainedPath(dirname, datafile).status()); + } + if (!info.index_filename.empty()) { + AIFOCORE_RETURN_IF_ERROR( + runtime::io::ResolveContainedPath(dirname, info.index_filename) + .status()); + } + return aifocore::Status::OkStatus(); +} + } // namespace aifocore::Result MrxsMetadataLoader::Load( const fs::path& slidedat_path, const fs::path& dirname) { SlideDataInfo info; AIFOCORE_ASSIGN_OR_RETURN(info, ReadSlidedatIni(slidedat_path, dirname)); + AIFOCORE_RETURN_IF_ERROR(ValidateReferencedPaths(dirname, info)); // IMPORTANT: Camera positions MUST be loaded during initialization; // they are required for accurate tile positioning at read time. diff --git a/src/readers/omezarr/omezarr.cpp b/src/readers/omezarr/omezarr.cpp index 6871152..b80d3bc 100644 --- a/src/readers/omezarr/omezarr.cpp +++ b/src/readers/omezarr/omezarr.cpp @@ -35,6 +35,7 @@ #include "fastslide/readers/omezarr/omezarr_metadata.h" #include "fastslide/readers/omezarr/omezarr_plan_builder.h" #include "fastslide/readers/omezarr/omezarr_tile_executor.h" +#include "fastslide/runtime/io/path_utils.h" namespace fs = std::filesystem; @@ -214,8 +215,13 @@ aifocore::Status OmeZarrReader::LoadMetadata() { pyramid_.reserve(ngff_.datasets.size()); for (const auto& dataset : ngff_.datasets) { OmeZarrLevelInfo level; - level.array_dir = (root_dir_ / dataset.path).string(); - const fs::path array_json_path = fs::path(level.array_dir) / "zarr.json"; + // `dataset.path` comes straight out of the store's zarr.json, so it has to + // be confined to the store directory before it reaches the filesystem. + AIFOCORE_ASSIGN_OR_RETURN( + const fs::path array_dir, + runtime::io::ResolveContainedPath(root_dir_, dataset.path)); + level.array_dir = array_dir.string(); + const fs::path array_json_path = array_dir / "zarr.json"; AIFOCORE_ASSIGN_OR_RETURN(const std::string array_text, ReadFileToString(array_json_path)); AIFOCORE_ASSIGN_OR_RETURN( diff --git a/src/readers/qptiff/metadata_parser.cpp b/src/readers/qptiff/metadata_parser.cpp index 64dbd30..a01ec2f 100644 --- a/src/readers/qptiff/metadata_parser.cpp +++ b/src/readers/qptiff/metadata_parser.cpp @@ -15,8 +15,12 @@ #include "fastslide/readers/qptiff/metadata_parser.h" #include +#include #include +#include #include +#include +#include #include #include @@ -28,6 +32,27 @@ namespace fastslide { namespace formats { namespace qptiff { +namespace { + +/// @brief Parse an unsigned integer from untrusted XML text. +/// +/// `std::stoull` throws on both malformed and out-of-range input, which would +/// escape this translation unit and, through the C API, cross an `extern "C"` +/// boundary. Returns 0 for anything that is not a clean in-range integer, +/// matching the treatment of an absent element. +uint64_t ParseUInt64OrZero(std::string_view text) { + uint64_t value = 0; + const char* begin = text.data(); + const char* end = begin + text.size(); + const std::from_chars_result result = std::from_chars(begin, end, value); + if (result.ec != std::errc() || result.ptr != end) { + return 0; + } + return value; +} + +} // namespace + aifocore::Status QpTiffMetadataParser::ParseSlideMetadata( const std::string& xml_content, QpTiffSlideMetadata& metadata) { @@ -81,12 +106,11 @@ aifocore::Result QpTiffMetadataParser::ParseChannelInfo( // Extract exposure time std::string exposure_str = GetText(&root, "ExposureTime"); - channel.exposure_time = exposure_str.empty() ? 0 : std::stoull(exposure_str); + channel.exposure_time = ParseUInt64OrZero(exposure_str); // Extract signal units std::string signal_units_str = GetText(&root, "SignalUnits"); - channel.signal_units = - signal_units_str.empty() ? 0 : std::stoull(signal_units_str); + channel.signal_units = ParseUInt64OrZero(signal_units_str); // Extract and parse color std::string color_str = GetText(&root, "Color"); diff --git a/src/runtime/decoders/bmp_decoder.cpp b/src/runtime/decoders/bmp_decoder.cpp index 704986a..3e144cd 100644 --- a/src/runtime/decoders/bmp_decoder.cpp +++ b/src/runtime/decoders/bmp_decoder.cpp @@ -41,6 +41,7 @@ #include #include #include +#include #include #include "aifocore/status/result.h" @@ -176,8 +177,22 @@ aifocore::Result DecodeBmpToRgb( const int32_t height = std::abs(height_raw); const bool top_down = height_raw < 0; - const uint32_t row_stride_src = - ((static_cast(width) * 3U) + 3U) & ~3U; + // `biWidth` is a 32-bit field, so computing the 4-byte-aligned row stride in + // 32-bit arithmetic wraps once `width * 3` exceeds 2^32: a declared width of + // 0x55555556 collapses the stride to 4. That understates the pixel array, so + // the truncation check below would accept a ~58-byte file while the row loop + // still walks `width` pixels. Compute in 64-bit and reject anything that will + // not fit. With a correct stride the truncation check then bounds the whole + // pixel array by the input size, which also caps the output allocation -- + // BI_RGB is uncompressed, so it cannot expand. + const uint64_t row_stride_64 = + ((static_cast(width) * 3ULL) + 3ULL) & ~3ULL; + if (row_stride_64 > std::numeric_limits::max()) { + return AIFOCORE_MAKE_STATUS( + aifocore::StatusCode::kInvalidArgument, + aifocore::fmt::format("BMP row stride overflows for width {}", width)); + } + const uint32_t row_stride_src = static_cast(row_stride_64); const std::size_t pixel_array_bytes = static_cast(row_stride_src) * diff --git a/src/runtime/decoders/bmp_decoder_test.cpp b/src/runtime/decoders/bmp_decoder_test.cpp index aa0ed96..7f37acb 100644 --- a/src/runtime/decoders/bmp_decoder_test.cpp +++ b/src/runtime/decoders/bmp_decoder_test.cpp @@ -242,5 +242,32 @@ TEST(BmpDecoderTest, DecodesLargerImage) { } } +/// @brief A width whose 24 bpp row stride wraps 32 bits must be rejected. +/// +/// `width * 3` overflows for widths at or above 0x55555556. Computed in 32-bit +/// arithmetic the stride collapses to a handful of bytes, so the truncation +/// check would accept this 54-byte header while the row loop still walked +/// billions of pixels. +TEST(BmpDecoderTest, RejectsWidthWhoseRowStrideOverflows) { + std::vector bmp(54, 0); + bmp[0] = 'B'; + bmp[1] = 'M'; + const uint32_t data_offset = 54; + std::memcpy(&bmp[10], &data_offset, 4); + const uint32_t info_size = 40; + std::memcpy(&bmp[14], &info_size, 4); + const int32_t width = 0x55555556; // 3 * width == 0x100000002 (mod 2^32 = 2) + std::memcpy(&bmp[18], &width, 4); + const int32_t height = 1; + std::memcpy(&bmp[22], &height, 4); + const uint16_t planes = 1; + std::memcpy(&bmp[26], &planes, 2); + const uint16_t bpp = 24; + std::memcpy(&bmp[28], &bpp, 2); + + const auto out_or = DecodeBmpToRgb(bmp); + EXPECT_FALSE(out_or.ok()); +} + } // namespace } // namespace fastslide::runtime::decoders diff --git a/src/runtime/io/path_utils_test.cpp b/src/runtime/io/path_utils_test.cpp new file mode 100644 index 0000000..82f1307 --- /dev/null +++ b/src/runtime/io/path_utils_test.cpp @@ -0,0 +1,94 @@ +// Copyright 2026 Jonas Teuwen. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "fastslide/runtime/io/path_utils.h" + +#include +#include + +#include "gtest/gtest.h" + +namespace fastslide::runtime::io { +namespace { + +namespace fs = std::filesystem; + +/// @brief Scratch bundle directory plus an out-of-bundle sibling. +class ResolveContainedPathTest : public ::testing::Test { + protected: + void SetUp() override { + base_ = fs::temp_directory_path() / + fs::path("fastslide_path_utils_test_" + + std::to_string( + ::testing::UnitTest::GetInstance()->random_seed()) + + "_" + std::to_string(counter_++)); + bundle_ = base_ / "slide"; + fs::create_directories(bundle_ / "sub"); + fs::create_directories(base_ / "outside"); + + std::ofstream(bundle_ / "sub" / "data.dat") << "inside"; + std::ofstream(base_ / "outside" / "secret.dat") << "outside"; + } + + void TearDown() override { + std::error_code err; + fs::remove_all(base_, err); + } + + fs::path base_; + fs::path bundle_; + static int counter_; +}; + +int ResolveContainedPathTest::counter_ = 0; + +TEST_F(ResolveContainedPathTest, AcceptsPathInsideBundle) { + const auto resolved = ResolveContainedPath(bundle_, "sub/data.dat"); + ASSERT_TRUE(resolved.ok()) << resolved.status().message(); + EXPECT_EQ(resolved.value().filename(), "data.dat"); +} + +TEST_F(ResolveContainedPathTest, RejectsParentTraversal) { + EXPECT_FALSE(ResolveContainedPath(bundle_, "../outside/secret.dat").ok()); + EXPECT_FALSE(ResolveContainedPath(bundle_, "sub/../../outside/x").ok()); +} + +TEST_F(ResolveContainedPathTest, RejectsAbsolutePath) { + EXPECT_FALSE(ResolveContainedPath(bundle_, "/etc/passwd").ok()); +} + +TEST_F(ResolveContainedPathTest, RejectsEmptyPath) { + EXPECT_FALSE(ResolveContainedPath(bundle_, "").ok()); +} + +/// A symlink escapes without using any `..` component, so the textual check +/// alone would let it through. +TEST_F(ResolveContainedPathTest, RejectsSymlinkPointingOutsideBundle) { + std::error_code err; + fs::create_symlink(base_ / "outside", bundle_ / "link", err); + if (err) { + GTEST_SKIP() << "symlinks unavailable: " << err.message(); + } + EXPECT_FALSE(ResolveContainedPath(bundle_, "link/secret.dat").ok()); +} + +/// `slide_extra` shares a textual prefix with `slide` but is not inside it. +TEST_F(ResolveContainedPathTest, RejectsSiblingSharingNamePrefix) { + fs::create_directories(base_ / "slide_extra"); + std::ofstream(base_ / "slide_extra" / "f.dat") << "x"; + EXPECT_FALSE(ResolveContainedPath(bundle_, "../slide_extra/f.dat").ok()); +} + +} // namespace +} // namespace fastslide::runtime::io diff --git a/src/runtime/tile_writer/paint_dispatch.cpp b/src/runtime/tile_writer/paint_dispatch.cpp index e045bb3..62da0ce 100644 --- a/src/runtime/tile_writer/paint_dispatch.cpp +++ b/src/runtime/tile_writer/paint_dispatch.cpp @@ -14,16 +14,33 @@ #include "fastslide/runtime/tile_writer.h" +#include #include +#include #include #include #include "aifocore/status/result.h" +#include "aifocore/utilities/fmt.h" #include "fastslide/core/tile_plan.h" #include "fastslide/image.h" namespace fastslide::runtime { +namespace { + +/// @brief Multiply without wrapping. +/// @return False when the product would exceed `size_t`. +bool CheckedMul(size_t a, size_t b, size_t* out) { + if (a != 0 && b > std::numeric_limits::max() / a) { + return false; + } + *out = a * b; + return true; +} + +} // namespace + aifocore::Status Canvas::PaintTileLocked(const core::TileReadOp& op, std::span pixel_data, uint32_t tile_width, @@ -34,6 +51,44 @@ aifocore::Status Canvas::PaintTileLocked(const core::TileReadOp& op, "Canvas has null image pointer"); } + // The paint sinks below take a bare pointer and derive every source offset + // from the declared tile geometry, which originates in file metadata. If the + // decoded buffer is shorter than that geometry implies, they read past its + // end and the stale bytes land in the image handed back to the caller. + // Reconcile the two here, once, before any sink sees the pointer. + if (tile_channels == 0) { + return AIFOCORE_MAKE_STATUS(aifocore::StatusCode::kInvalidArgument, + "Tile declares zero channels"); + } + + // `CopyTilePlanar` strides the source as a single-channel plane; every other + // sink reads interleaved samples. + const bool planar_plane_source = + !use_rgb8_blending_ && !use_rgb16_copy_blending_ && + config_.planar_config == PlanarConfig::kSeparate; + const uint32_t source_channels = planar_plane_source ? 1U : tile_channels; + + size_t required = output_image_->GetBytesPerSample(); + const bool fits = CheckedMul(required, tile_width, &required) && + CheckedMul(required, tile_height, &required) && + CheckedMul(required, source_channels, &required); + if (!fits) { + return AIFOCORE_MAKE_STATUS( + aifocore::StatusCode::kInvalidArgument, + aifocore::fmt::format( + "Tile geometry {}x{}x{} overflows an address-space-sized buffer", + tile_width, tile_height, source_channels)); + } + if (pixel_data.size() < required) { + return AIFOCORE_MAKE_STATUS( + aifocore::StatusCode::kInvalidArgument, + aifocore::fmt::format( + "Tile buffer holds {} bytes but declared geometry {}x{}x{} at {} " + "bytes/sample requires {}", + pixel_data.size(), tile_width, tile_height, source_channels, + output_image_->GetBytesPerSample(), required)); + } + if (use_rgb8_blending_) { return PaintTileRgb8Blended(op, pixel_data, tile_width, tile_height, tile_channels); diff --git a/src/runtime/tile_writer_test.cpp b/src/runtime/tile_writer_test.cpp index 714c935..4ad0fd3 100644 --- a/src/runtime/tile_writer_test.cpp +++ b/src/runtime/tile_writer_test.cpp @@ -171,6 +171,33 @@ TEST(CanvasTest, PaintSingleTileRGB) { EXPECT_FALSE(output.Empty()); } +/// @brief A tile buffer shorter than its declared geometry must be refused. +/// +/// The paint sinks derive every source offset from the declared tile geometry, +/// which comes from file metadata. A malicious or corrupt slide can declare a +/// larger tile than it actually stores; without this check the sinks read past +/// the end of the decoded buffer and the stale bytes reach the caller. +TEST(CanvasTest, RejectsTileBufferShorterThanDeclaredGeometry) { + auto plan = CreateSimpleRGBPlan(256, 256); + Canvas canvas(plan); + + // Declare a 256x256x3 tile but supply one byte less than that needs. + std::vector truncated(256U * 256U * 3U - 1U, 0); + + const auto& op = plan.operations[0]; + const auto status = canvas.PaintTile(op, truncated, 256, 256, 3); + EXPECT_FALSE(status.ok()); +} + +TEST(CanvasTest, RejectsTileDeclaringZeroChannels) { + auto plan = CreateSimpleRGBPlan(256, 256); + Canvas canvas(plan); + + auto pixel_data = CreateTestPixelData(256, 256, 3); + const auto& op = plan.operations[0]; + EXPECT_FALSE(canvas.PaintTile(op, pixel_data, 256, 256, 0).ok()); +} + TEST(CanvasTest, PaintPartialTile) { auto plan = CreateSimpleRGBPlan(512, 512); Canvas canvas(plan); diff --git a/tests/meson.build b/tests/meson.build index c7f53c8..8cc2eec 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -28,6 +28,7 @@ if gtest_dep.found() 'lru_tile_cache_test' : '../src/runtime/lru_tile_cache_test.cpp', 'cache_interface_test' : '../src/runtime/cache_interface_test.cpp', 'binary_utils_test' : '../src/runtime/io/binary_utils_test.cpp', + 'path_utils_test' : '../src/runtime/io/path_utils_test.cpp', 'png_decoder_test' : '../src/runtime/decoders/png_decoder_test.cpp', 'bmp_decoder_test' : '../src/runtime/decoders/bmp_decoder_test.cpp', 'mrxs_index_reader_test' : '../src/readers/mrxs/mrxs_index_reader_test.cpp',