[Zucchini]: Stricter patch read

This enforces a stricter patch read in a few ways:
- Raw Deltas of 0 are forbidden. These are unused values which have no
  meaning and should not appear.
- Extra Data length must be equal to the size of the patch element
  minus the total length of equivalences.
- Element match headers must have regions of nonzero length.

This change also annotates a number of "unsafe" values read from the
patch that could be sources of error later if unchecked. It also adds
caveats about under what conditions emitted values are considered to
be safe/unsafe.

Bug: 837096
Change-Id: I1b7614d92b85c0a1546d8dccb7d371d28b2a4cd3
Reviewed-on: https://chromium-review.googlesource.com/1037186
Commit-Queue: Calder Kitagawa <ckitagawa@google.com>
Reviewed-by: Samuel Huang <huangs@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555396}
NOKEYCHECK=True
GitOrigin-RevId: 63f65c0a8b295b925c1dc92c7bed7625e7d99d46

1 files changed, 107 insertions, 45 deletions

diff --git a/patch_read_write_unittest.cc b/patch_read_write_unittest.cc
index ed0a6ed..681e3c2 100644
--- a/patch_read_write_unittest.cc
+++ b/patch_read_write_unittest.cc
@@ -60,13 +60,13 @@ ByteVector CreatePatchElement() {
       0x01, 0,   0,   0,    // old_offset
       0x03, 0,   0,   0,    // new_offset
       0x51, 0,   0,   0,    // old_length
-      0x50, 0,   0,   0,    // new_length
+      0x13, 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
+      0x00,               // dst_skip content
       1,    0,   0,   0,  // copy_count size
       0x12,               // copy_count content
 
@@ -91,8 +91,18 @@ ByteVector CreatePatchElement() {
   };
 }
 
-// Helper to mutate test |data| (e.g., from CreatePatchElement()) at |idx| from |from_val| (as
-// sanity check) to |to_val|.
+ByteVector CreateElementMatch() {
+  return {
+      0x01, 0,   0,   0,    // old_offset
+      0x03, 0,   0,   0,    // new_offset
+      0x02, 0,   0,   0,    // old_length
+      0x04, 0,   0,   0,    // new_length
+      'D',  'E', 'X', ' ',  // kExeTypeDex
+  };
+}
+
+// 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,
@@ -108,13 +118,7 @@ bool operator==(const ByteVector& a, ConstBufferView b) {
 }
 
 TEST(PatchTest, ParseSerializeElementMatch) {
-  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
-      'D',  'E', 'X', ' ',  // kExeTypeDex
-  };
+  ByteVector data = CreateElementMatch();
   BufferSource buffer_source(data.data(), data.size());
   ElementMatch element_match = {};
   EXPECT_TRUE(patch::ParseElementMatch(&buffer_source, &element_match));
@@ -141,6 +145,34 @@ TEST(PatchTest, ParseElementMatchTooSmall) {
   EXPECT_FALSE(patch::ParseElementMatch(&buffer_source, &element_match));
 }
 
+TEST(PatchTest, ParseElementMatchNoLength) {
+  // Set old_length to 0 to trigger an error.
+  {
+    ByteVector data = CreateElementMatch();
+    ModifyByte(8U, 0x02, 0x00, &data);  // Make the old_length = 0.
+    BufferSource buffer_source(data.data(), data.size());
+    ElementMatch element_match = {};
+    EXPECT_FALSE(patch::ParseElementMatch(&buffer_source, &element_match));
+  }
+  // Set new_length to 0 to trigger an error.
+  {
+    ByteVector data = CreateElementMatch();
+    ModifyByte(12U, 0x04, 0x00, &data);  // Make the new_length = 0.
+    BufferSource buffer_source(data.data(), data.size());
+    ElementMatch element_match = {};
+    EXPECT_FALSE(patch::ParseElementMatch(&buffer_source, &element_match));
+  }
+  // Set both new_length and old_length to 0 to trigger an error.
+  {
+    ByteVector data = CreateElementMatch();
+    ModifyByte(8U, 0x02, 0x00, &data);   // Make the old_length = 0.
+    ModifyByte(12U, 0x04, 0x00, &data);  // Make the new_length = 0.
+    BufferSource buffer_source(data.data(), data.size());
+    ElementMatch element_match = {};
+    EXPECT_FALSE(patch::ParseElementMatch(&buffer_source, &element_match));
+  }
+}
+
 TEST(PatchTest, ParseSerializeElementMatchExeMismatch) {
   ByteVector buffer(28);
   BufferSink buffer_sink(buffer.data(), buffer.size());
@@ -356,6 +388,18 @@ TEST(RawDeltaSinkSourceSinkTest, Normal) {
   TestSerialize(data, raw_delta_sink);
 }
 
+TEST(RawDeltaSourceSinkTest, InvalidContent) {
+  ByteVector data = {
+      2, 0, 0, 0,  // raw_delta_skip size
+      1, 3,        // raw_delta_skip content
+      2, 0, 0, 0,  // raw_delta_diff size
+      0, 4,        // raw_delta_diff content
+  };
+  RawDeltaSource raw_delta_source = TestInitialize<RawDeltaSource>(&data);
+  EXPECT_FALSE(raw_delta_source.GetNext());
+  EXPECT_FALSE(raw_delta_source.Done());
+}
+
 TEST(ReferenceDeltaSourceSinkTest, Empty) {
   ByteVector data = {
       0, 0, 0, 0,  // reference_delta size
@@ -449,12 +493,12 @@ TEST(PatchElementTest, Normal) {
   EXPECT_EQ(0x1U, element_match.old_element.offset);
   EXPECT_EQ(0x51U, element_match.old_element.size);
   EXPECT_EQ(0x3U, element_match.new_element.offset);
-  EXPECT_EQ(0x50U, element_match.new_element.size);
+  EXPECT_EQ(0x13U, element_match.new_element.size);
 
   EquivalenceSource equivalence_source =
       patch_element_reader.GetEquivalenceSource();
   EXPECT_EQ(ByteVector({0x10}), equivalence_source.src_skip());
-  EXPECT_EQ(ByteVector({0x11}), equivalence_source.dst_skip());
+  EXPECT_EQ(ByteVector({0x00}), equivalence_source.dst_skip());
   EXPECT_EQ(ByteVector({0x12}), equivalence_source.copy_count());
 
   ExtraDataSource extra_data_source = patch_element_reader.GetExtraDataSource();
@@ -481,7 +525,7 @@ TEST(PatchElementTest, Normal) {
   PatchElementWriter patch_element_writer(element_match);
 
   patch_element_writer.SetEquivalenceSink(
-      EquivalenceSink({0x10}, {0x11}, {0x12}));
+      EquivalenceSink({0x10}, {0x00}, {0x12}));
   patch_element_writer.SetExtraDataSink(ExtraDataSink({0x13}));
   patch_element_writer.SetRawDeltaSink(RawDeltaSink({0x14}, {0x15}));
   patch_element_writer.SetReferenceDeltaSink(ReferenceDeltaSink({0x16}));
@@ -501,7 +545,23 @@ TEST(PatchElementTest, BadEquivalence) {
   // If the "new" element is too small the test should fail.
   {
     ByteVector data = CreatePatchElement();
-    ModifyByte(12, 0x50, 0x05, &data);  // new_length (too small)
+    ModifyByte(12, 0x13, 0x05, &data);  // new_length (too small)
+    TestInvalidInitialize<PatchElementReader>(&data);
+  }
+}
+
+TEST(PatchElementTest, WrongExtraData) {
+  // Make "new" too large so there is insufficient extra data to cover the
+  // image.
+  {
+    ByteVector data = CreatePatchElement();
+    ModifyByte(12, 0x13, 0x14, &data);  // new_length (too large)
+    TestInvalidInitialize<PatchElementReader>(&data);
+  }
+  // Make "new" too small so there is too much extra data.
+  {
+    ByteVector data = CreatePatchElement();
+    ModifyByte(12, 0x13, 0x14, &data);  // new_length (too small)
     TestInvalidInitialize<PatchElementReader>(&data);
   }
 }
@@ -511,24 +571,25 @@ TEST(EnsemblePatchTest, RawPatch) {
       0x5A, 0x75, 0x63, 0x00,  // magic
       0x10, 0x32, 0x54, 0x76,  // old_size
       0x00, 0x11, 0x22, 0x33,  // old_crc
-      0x98, 0xBA, 0xDC, 0xFE,  // new_size
+      0x01, 0,    0,    0,     // new_size
       0x44, 0x55, 0x66, 0x77,  // new_crc
 
       1,    0,    0,    0,  // number of element
 
-      0x01, 0,    0,    0,     // old_offset
-      0x00, 0,    0,    0,     // new_offset
-      0x02, 0,    0,    0,     // old_length
-      0x98, 0xBA, 0xDC, 0xFE,  // new_length
-      'P',  'x',  '8',  '6',   // EXE_TYPE_WIN32_X86
-      0,    0,    0,    0,     // src_skip size
-      0,    0,    0,    0,     // dst_skip size
-      0,    0,    0,    0,     // copy_count size
-      0,    0,    0,    0,     // extra_data size
-      0,    0,    0,    0,     // raw_delta_skip size
-      0,    0,    0,    0,     // raw_delta_diff size
-      0,    0,    0,    0,     // reference_delta size
-      0,    0,    0,    0,     // pool count
+      0x01, 0,    0,    0,    // old_offset
+      0x00, 0,    0,    0,    // new_offset
+      0x02, 0,    0,    0,    // old_length
+      0x01, 0,    0,    0,    // new_length
+      'P',  'x',  '8',  '6',  // EXE_TYPE_WIN32_X86
+      0,    0,    0,    0,    // src_skip size
+      0,    0,    0,    0,    // dst_skip size
+      0,    0,    0,    0,    // copy_count size
+      0x01, 0,    0,    0,    // extra_data size
+      0x04,                   // extra_data content
+      0,    0,    0,    0,    // raw_delta_skip size
+      0,    0,    0,    0,    // raw_delta_diff size
+      0,    0,    0,    0,    // reference_delta size
+      0,    0,    0,    0,    // pool count
   };
 
   EnsemblePatchReader ensemble_patch_reader =
@@ -538,7 +599,7 @@ TEST(EnsemblePatchTest, RawPatch) {
   EXPECT_EQ(PatchHeader::kMagic, header.magic);
   EXPECT_EQ(0x76543210U, header.old_size);
   EXPECT_EQ(0x33221100U, header.old_crc);
-  EXPECT_EQ(0xFEDCBA98U, header.new_size);
+  EXPECT_EQ(0x01U, header.new_size);
   EXPECT_EQ(0x77665544U, header.new_crc);
 
   const std::vector<PatchElementReader>& elements =
@@ -548,7 +609,7 @@ TEST(EnsemblePatchTest, RawPatch) {
   EnsemblePatchWriter ensemble_patch_writer(header);
   PatchElementWriter patch_element_writer(elements[0].element_match());
   patch_element_writer.SetEquivalenceSink({});
-  patch_element_writer.SetExtraDataSink({});
+  patch_element_writer.SetExtraDataSink(ExtraDataSink({0x04}));
   patch_element_writer.SetRawDeltaSink({});
   patch_element_writer.SetReferenceDeltaSink({});
   ensemble_patch_writer.AddElement(std::move(patch_element_writer));
@@ -574,7 +635,8 @@ TEST(EnsemblePatchTest, CheckFile) {
       0,    0,    0,    0,    // src_skip size
       0,    0,    0,    0,    // dst_skip size
       0,    0,    0,    0,    // copy_count size
-      0,    0,    0,    0,    // extra_data size
+      0x03, 0,    0,    0,    // extra_data size
+      'A',  'B',  'C',        // extra_data content
       0,    0,    0,    0,    // raw_delta_skip size
       0,    0,    0,    0,    // raw_delta_diff size
       0,    0,    0,    0,    // reference_delta size
@@ -606,19 +668,19 @@ TEST(EnsemblePatchTest, InvalidMagic) {
 
       1,    0,    0,    0,  // number of element
 
-      0x01, 0,    0,    0,  // old_offset
-      0x00, 0,    0,    0,  // new_offset
-      0x02, 0,    0,    0,  // old_length
-      0x03, 0,    0,    0,  // new_length
-      1,    0,    0,    0,  // EXE_TYPE_WIN32_X86
-      0,    0,    0,    0,  // src_skip size
-      0,    0,    0,    0,  // dst_skip size
-      0,    0,    0,    0,  // copy_count size
-      0,    0,    0,    0,  // extra_data size
-      0,    0,    0,    0,  // raw_delta_skip size
-      0,    0,    0,    0,  // raw_delta_diff size
-      0,    0,    0,    0,  // reference_delta size
-      0,    0,    0,    0,  // pool count
+      0x01, 0,    0,    0,    // old_offset
+      0x00, 0,    0,    0,    // new_offset
+      0x02, 0,    0,    0,    // old_length
+      0x03, 0,    0,    0,    // new_length
+      'P',  'x',  '8',  '6',  // EXE_TYPE_WIN32_X86
+      0,    0,    0,    0,    // src_skip size
+      0,    0,    0,    0,    // dst_skip size
+      0,    0,    0,    0,    // copy_count size
+      0,    0,    0,    0,    // extra_data size
+      0,    0,    0,    0,    // raw_delta_skip size
+      0,    0,    0,    0,    // raw_delta_diff size
+      0,    0,    0,    0,    // reference_delta size
+      0,    0,    0,    0,    // pool count
   };
 
   TestInvalidInitialize<EnsemblePatchReader>(&data);