From 85c043ca69a7738293577bf03300e1700ab51565 Mon Sep 17 00:00:00 2001 From: Stefan Boberg Date: Fri, 20 Mar 2026 20:40:09 +0100 Subject: Add validation tests and harden legacy package parser - Add tests for ParsePackageMessage validation: truncated header, bad magic, attachment count overflow, truncated tables/data, local ref rejection, and CbPackageReader equivalents - Add tests for legacy::TryLoadCbPackage: empty input, zero-size binary rejection, null mapper handling, hash validation toggle - Harden legacy parser: reject zero-size binary fields that would desync the reader, replace ZEN_ASSERT(Mapper) with graceful failure, add optional ValidateHashes parameter for resolved attachment data - Rename test cases from "usonpackage.*" to "cbpackage.*" - Fix CbPackage.LocalRef test to pass kAllowLocalReferences --- src/zencore/compactbinarypackage.cpp | 120 ++++++++++++++++++++++++++++++++--- 1 file changed, 112 insertions(+), 8 deletions(-) (limited to 'src/zencore/compactbinarypackage.cpp') diff --git a/src/zencore/compactbinarypackage.cpp b/src/zencore/compactbinarypackage.cpp index 56a292ca6..76a18e9ed 100644 --- a/src/zencore/compactbinarypackage.cpp +++ b/src/zencore/compactbinarypackage.cpp @@ -684,14 +684,22 @@ namespace legacy { Writer.Save(Ar); } - bool TryLoadCbPackage(CbPackage& Package, IoBuffer InBuffer, BufferAllocator Allocator, CbPackage::AttachmentResolver* Mapper) + bool TryLoadCbPackage(CbPackage& Package, + IoBuffer InBuffer, + BufferAllocator Allocator, + CbPackage::AttachmentResolver* Mapper, + bool ValidateHashes) { BinaryReader Reader(InBuffer.Data(), InBuffer.Size()); - return TryLoadCbPackage(Package, Reader, Allocator, Mapper); + return TryLoadCbPackage(Package, Reader, Allocator, Mapper, ValidateHashes); } - bool TryLoadCbPackage(CbPackage& Package, BinaryReader& Reader, BufferAllocator Allocator, CbPackage::AttachmentResolver* Mapper) + bool TryLoadCbPackage(CbPackage& Package, + BinaryReader& Reader, + BufferAllocator Allocator, + CbPackage::AttachmentResolver* Mapper, + bool ValidateHashes) { Package = CbPackage(); for (;;) @@ -708,7 +716,11 @@ namespace legacy { if (ValueField.IsBinary()) { const MemoryView View = ValueField.AsBinaryView(); - if (View.GetSize() > 0) + if (View.GetSize() == 0) + { + return false; + } + else { SharedBuffer Buffer = SharedBuffer::MakeView(View, ValueField.GetOuterBuffer()).MakeOwned(); CbField HashField = LoadCompactBinary(Reader, Allocator); @@ -748,7 +760,11 @@ namespace legacy { { const IoHash Hash = ValueField.AsHash(); - ZEN_ASSERT(Mapper); + if (!Mapper) + { + return false; + } + if (SharedBuffer AttachmentData = (*Mapper)(Hash)) { IoHash RawHash; @@ -763,6 +779,10 @@ namespace legacy { } else { + if (ValidateHashes && IoHash::HashBuffer(AttachmentData) != Hash) + { + return false; + } const CbValidateError ValidationResult = ValidateCompactBinary(AttachmentData.GetView(), CbValidateMode::All); if (ValidationResult != CbValidateError::None) { @@ -807,7 +827,7 @@ usonpackage_forcelink() TEST_SUITE_BEGIN("core.compactbinarypackage"); -TEST_CASE("usonpackage") +TEST_CASE("cbpackage") { using namespace std::literals; @@ -997,7 +1017,7 @@ TEST_CASE("usonpackage") } } -TEST_CASE("usonpackage.serialization") +TEST_CASE("cbpackage.serialization") { using namespace std::literals; @@ -1303,7 +1323,7 @@ TEST_CASE("usonpackage.serialization") } } -TEST_CASE("usonpackage.invalidpackage") +TEST_CASE("cbpackage.invalidpackage") { const auto TestLoad = [](std::initializer_list RawData, BufferAllocator Allocator = UniqueBuffer::Alloc) { const MemoryView RawView = MakeMemoryView(RawData); @@ -1345,6 +1365,90 @@ TEST_CASE("usonpackage.invalidpackage") } } +TEST_CASE("cbpackage.legacy.invalidpackage") +{ + const auto TestLegacyLoad = [](std::initializer_list RawData) { + const MemoryView RawView = MakeMemoryView(RawData); + IoBuffer Buffer(IoBuffer::Wrap, const_cast(RawView.GetData()), RawView.GetSize()); + CbPackage Package; + CHECK_FALSE(legacy::TryLoadCbPackage(Package, Buffer, &UniqueBuffer::Alloc)); + }; + + SUBCASE("Empty") { TestLegacyLoad({}); } + + SUBCASE("Zero size binary rejects") + { + // A binary field with zero payload size should be rejected (would desync the reader) + BinaryWriter Writer; + CbWriter Cb; + Cb.AddBinary(MemoryView()); // zero-size binary + Cb.Save(Writer); + + IoBuffer Buffer(IoBuffer::Wrap, const_cast(MakeMemoryView(Writer).GetData()), MakeMemoryView(Writer).GetSize()); + CbPackage Package; + CHECK_FALSE(legacy::TryLoadCbPackage(Package, Buffer, &UniqueBuffer::Alloc)); + } +} + +TEST_CASE("cbpackage.legacy.hashresolution") +{ + // Build a valid legacy package with an object, then round-trip it + CbObjectWriter RootWriter; + RootWriter.AddString("name", "test"); + CbObject RootObject = RootWriter.Save(); + + CbAttachment ObjectAttach(RootObject); + + CbPackage OriginalPkg; + OriginalPkg.SetObject(RootObject); + OriginalPkg.AddAttachment(ObjectAttach); + + BinaryWriter Writer; + legacy::SaveCbPackage(OriginalPkg, Writer); + + IoBuffer Buffer(IoBuffer::Wrap, const_cast(MakeMemoryView(Writer).GetData()), MakeMemoryView(Writer).GetSize()); + CbPackage LoadedPkg; + CHECK(legacy::TryLoadCbPackage(LoadedPkg, Buffer, &UniqueBuffer::Alloc)); + + // The hash-only path requires a mapper — without one it should fail + CbWriter HashOnlyCb; + HashOnlyCb.AddHash(ObjectAttach.GetHash()); + HashOnlyCb.AddNull(); + BinaryWriter HashOnlyWriter; + HashOnlyCb.Save(HashOnlyWriter); + + IoBuffer HashOnlyBuffer(IoBuffer::Wrap, + const_cast(MakeMemoryView(HashOnlyWriter).GetData()), + MakeMemoryView(HashOnlyWriter).GetSize()); + CbPackage HashOnlyPkg; + CHECK_FALSE(legacy::TryLoadCbPackage(HashOnlyPkg, HashOnlyBuffer, &UniqueBuffer::Alloc, nullptr)); + + // With a mapper that returns valid data, it should succeed + CbPackage::AttachmentResolver Resolver = [&](const IoHash& Hash) -> SharedBuffer { + if (Hash == ObjectAttach.GetHash()) + { + return RootObject.GetBuffer(); + } + return {}; + }; + CHECK(legacy::TryLoadCbPackage(HashOnlyPkg, HashOnlyBuffer, &UniqueBuffer::Alloc, &Resolver)); + + // Build a different but structurally valid CbObject to use as mismatched data + CbObjectWriter DifferentWriter; + DifferentWriter.AddString("name", "different"); + CbObject DifferentObject = DifferentWriter.Save(); + + CbPackage::AttachmentResolver BadResolver = [&](const IoHash&) -> SharedBuffer { return DifferentObject.GetBuffer(); }; + CbPackage BadPkg; + + // With ValidateHashes enabled and a mapper that returns mismatched data, it should fail + CHECK_FALSE(legacy::TryLoadCbPackage(BadPkg, HashOnlyBuffer, &UniqueBuffer::Alloc, &BadResolver, /*ValidateHashes*/ true)); + + // Without ValidateHashes, the mismatched data is accepted (structure is still valid CB) + CbPackage UncheckedPkg; + CHECK(legacy::TryLoadCbPackage(UncheckedPkg, HashOnlyBuffer, &UniqueBuffer::Alloc, &BadResolver, /*ValidateHashes*/ false)); +} + TEST_SUITE_END(); #endif -- cgit v1.2.3