From 673bf5912068218539f524b6dfe74dfb38fe67b8 Mon Sep 17 00:00:00 2001 From: Calder Kitagawa Date: Fri, 27 Apr 2018 22:37:45 +0000 Subject: [Zucchini] Validate equivalences on load A follow-up to https://chromium-review.googlesource.com/c/chromium/src/+/1028575 This moves patch_apply logic to check bounds of an equivalences from the call site of GetNext() to an internal function in the patch_reader. This means the equivalence consumer can use the equivalences without checking anything to do with bounds. I have manually tested that this doesn't appear to break any existing valid patches and it appears to catch all the same errors change 1028575 fixed so I can safely reverse that change. BUG: 837096 Change-Id: I84ccd9e1493f32d16eace4dd8e67586f559220d3 Reviewed-on: https://chromium-review.googlesource.com/1028836 Commit-Queue: Calder Kitagawa Reviewed-by: Samuel Huang Cr-Commit-Position: refs/heads/master@{#554536} NOKEYCHECK=True GitOrigin-RevId: ce5642400b37f5ff2b0a1213522f984bca8a080a --- patch_read_write_unittest.cc | 100 ++++++++++++++++++++++++++++--------------- 1 file changed, 65 insertions(+), 35 deletions(-) (limited to 'patch_read_write_unittest.cc') diff --git a/patch_read_write_unittest.cc b/patch_read_write_unittest.cc index f6396d6..ed0a6ed 100644 --- a/patch_read_write_unittest.cc +++ b/patch_read_write_unittest.cc @@ -55,6 +55,52 @@ void TestSerialize(const ByteVector& expected, const T& value) { EXPECT_EQ(expected, buffer); } +ByteVector CreatePatchElement() { + return { + 0x01, 0, 0, 0, // old_offset + 0x03, 0, 0, 0, // new_offset + 0x51, 0, 0, 0, // old_length + 0x50, 0, 0, 0, // new_length + 'P', 'x', '8', '6', // EXE_TYPE_WIN32_X86 + + 1, 0, 0, 0, // src_skip size + 0x10, // src_skip content + 1, 0, 0, 0, // dst_skip size + 0x11, // dst_skip content + 1, 0, 0, 0, // copy_count size + 0x12, // copy_count content + + 1, 0, 0, 0, // extra_data size + 0x13, // extra_data content + + 1, 0, 0, 0, // raw_delta_skip size + 0x14, // raw_delta_skip content + 1, 0, 0, 0, // raw_delta_diff size + 0x15, // raw_delta_diff content + + 1, 0, 0, 0, // reference_delta size + 0x16, // reference_delta content + + 2, 0, 0, 0, // pool count + 0, // pool_tag + 1, 0, 0, 0, // extra_targets size + 0x17, // extra_targets content + 2, // pool_tag + 1, 0, 0, 0, // extra_targets size + 0x18, // extra_targets content + }; +} + +// Helper to mutate test |data| (e.g., from CreatePatchElement()) at |idx| from |from_val| (as +// sanity check) to |to_val|. +void ModifyByte(size_t idx, + uint8_t from_val, + uint8_t to_val, + std::vector* data) { + ASSERT_EQ(from_val, (*data)[idx]); + (*data)[idx] = to_val; +} + } // namespace bool operator==(const ByteVector& a, ConstBufferView b) { @@ -391,39 +437,7 @@ TEST(TargetSourceSinkTest, Normal) { } TEST(PatchElementTest, Normal) { - ByteVector data = { - 0x01, 0, 0, 0, // old_offset - 0x03, 0, 0, 0, // new_offset - 0x02, 0, 0, 0, // old_length - 0x04, 0, 0, 0, // new_length - 'P', 'x', '8', '6', // EXE_TYPE_WIN32_X86 - - 1, 0, 0, 0, // src_skip size - 0x10, // src_skip content - 1, 0, 0, 0, // dst_skip size - 0x11, // dst_skip content - 1, 0, 0, 0, // copy_count size - 0x12, // copy_count content - - 1, 0, 0, 0, // extra_data size - 0x13, // extra_data content - - 1, 0, 0, 0, // raw_delta_skip size - 0x14, // raw_delta_skip content - 1, 0, 0, 0, // raw_delta_diff size - 0x15, // raw_delta_diff content - - 1, 0, 0, 0, // reference_delta size - 0x16, // reference_delta content - - 2, 0, 0, 0, // pool count - 0, // pool_tag - 1, 0, 0, 0, // extra_targets size - 0x17, // extra_targets content - 2, // pool_tag - 1, 0, 0, 0, // extra_targets size - 0x18, // extra_targets content - }; + ByteVector data = CreatePatchElement(); PatchElementReader patch_element_reader = TestInitialize(&data); @@ -433,9 +447,9 @@ TEST(PatchElementTest, Normal) { EXPECT_EQ(kExeTypeWin32X86, element_match.old_element.exe_type); EXPECT_EQ(kExeTypeWin32X86, element_match.new_element.exe_type); EXPECT_EQ(0x1U, element_match.old_element.offset); - EXPECT_EQ(0x2U, element_match.old_element.size); + EXPECT_EQ(0x51U, element_match.old_element.size); EXPECT_EQ(0x3U, element_match.new_element.offset); - EXPECT_EQ(0x4U, element_match.new_element.size); + EXPECT_EQ(0x50U, element_match.new_element.size); EquivalenceSource equivalence_source = patch_element_reader.GetEquivalenceSource(); @@ -476,6 +490,22 @@ TEST(PatchElementTest, Normal) { TestSerialize(data, patch_element_writer); } +TEST(PatchElementTest, BadEquivalence) { + // If the "old" element is too small the test should fail. + { + ByteVector data = CreatePatchElement(); + ModifyByte(8, 0x51, 0x04, &data); // old_length (too small) + TestInvalidInitialize(&data); + } + + // If the "new" element is too small the test should fail. + { + ByteVector data = CreatePatchElement(); + ModifyByte(12, 0x50, 0x05, &data); // new_length (too small) + TestInvalidInitialize(&data); + } +} + TEST(EnsemblePatchTest, RawPatch) { ByteVector data = { 0x5A, 0x75, 0x63, 0x00, // magic -- cgit v1.2.3