update_engine: Check metadata and signature sizes

Check that the size of the metadata size and signature sizes are
smaller that the payload size. Without this check, the delta
performer writes X number of bytes to the buffer before validating
these values, and an attacker could provide a huge value which will
make update_engine crash.

BUG=chromium:1027166
TEST=fuzzer, unittest, install/unistall DLC on DUT
TEST=test_that -b $BOARD $IP autoupdate_EOL

Change-Id: Iad3a314efacbb1005fac37dd383a3f8852008f4b
Reviewed-on: https://chromium-review.googlesource.com/c/aosp/platform/system/update_engine/+/1976079
Commit-Queue: Andrew Lassalle <andrewlassalle@chromium.org>
Tested-by: Andrew Lassalle <andrewlassalle@chromium.org>
Reviewed-by: Amin Hassani <ahassani@chromium.org>
Auto-Submit: Andrew Lassalle <andrewlassalle@chromium.org>
diff --git a/payload_consumer/delta_performer.cc b/payload_consumer/delta_performer.cc
index ee5f38c..3263ff7 100644
--- a/payload_consumer/delta_performer.cc
+++ b/payload_consumer/delta_performer.cc
@@ -458,6 +458,21 @@
         return MetadataParseResult::kError;
       }
     }
+
+    // Check that the |metadata signature size_| and |metadata_size_| are not
+    // very big numbers. This is necessary since |update_engine| needs to write
+    // these values into the buffer before being able to use them, and if an
+    // attacker sets these values to a very big number, the buffer will overflow
+    // and |update_engine| will crash. A simple way of solving this is to check
+    // that the size of both values is smaller than the payload itself.
+    if (metadata_size_ + metadata_signature_size_ > payload_->size) {
+      LOG(ERROR) << "The size of the metadata_size(" << metadata_size_ << ")"
+                 << " or metadata signature(" << metadata_signature_size_ << ")"
+                 << " is greater than the size of the payload"
+                 << "(" << payload_->size << ")";
+      *error = ErrorCode::kDownloadInvalidMetadataSize;
+      return MetadataParseResult::kError;
+    }
   }
 
   // Now that we have validated the metadata size, we should wait for the full
diff --git a/payload_consumer/delta_performer_integration_test.cc b/payload_consumer/delta_performer_integration_test.cc
index 5f55739..f1a492b 100644
--- a/payload_consumer/delta_performer_integration_test.cc
+++ b/payload_consumer/delta_performer_integration_test.cc
@@ -713,7 +713,8 @@
   // Update the A image in place.
   InstallPlan* install_plan = &state->install_plan;
   install_plan->hash_checks_mandatory = hash_checks_mandatory;
-  install_plan->payloads = {{.metadata_size = state->metadata_size,
+  install_plan->payloads = {{.size = state->delta.size(),
+                             .metadata_size = state->metadata_size,
                              .type = (full_kernel && full_rootfs)
                                          ? InstallPayloadType::kFull
                                          : InstallPayloadType::kDelta}};
diff --git a/payload_consumer/delta_performer_unittest.cc b/payload_consumer/delta_performer_unittest.cc
index 0671eca..47cb0e7 100644
--- a/payload_consumer/delta_performer_unittest.cc
+++ b/payload_consumer/delta_performer_unittest.cc
@@ -43,6 +43,7 @@
 #include "update_engine/payload_consumer/fake_file_descriptor.h"
 #include "update_engine/payload_consumer/mock_download_action.h"
 #include "update_engine/payload_consumer/payload_constants.h"
+#include "update_engine/payload_consumer/payload_metadata.h"
 #include "update_engine/payload_generator/bzip.h"
 #include "update_engine/payload_generator/extent_ranges.h"
 #include "update_engine/payload_generator/payload_file.h"
@@ -272,6 +273,7 @@
     test_utils::ScopedTempFile new_part("Partition-XXXXXX");
     EXPECT_TRUE(test_utils::WriteFileVector(new_part.path(), target_data));
 
+    payload_.size = payload_data.size();
     // We installed the operations only in the rootfs partition, but the
     // delta performer needs to access all the partitions.
     fake_boot_control_.SetPartitionDevice(
@@ -308,6 +310,7 @@
     EXPECT_TRUE(performer_.Write(&version, 8));
 
     payload_.metadata_size = expected_metadata_size;
+    payload_.size = actual_metadata_size + 1;
     ErrorCode error_code;
     // When filling in size in manifest, exclude the size of the 24-byte header.
     uint64_t size_in_manifest = htobe64(actual_metadata_size - 24);
@@ -325,7 +328,7 @@
     EXPECT_LT(performer_.Close(), 0);
   }
 
-  // Generates a valid delta file but tests the delta performer by suppling
+  // Generates a valid delta file but tests the delta performer by supplying
   // different metadata signatures as per metadata_signature_test flag and
   // sees if the result of the parsing are as per hash_checks_mandatory flag.
   void DoMetadataSignatureTest(MetadataSignatureTest metadata_signature_test,
@@ -338,6 +341,7 @@
                                            kBrilloMajorPayloadVersion,
                                            kFullPayloadMinorVersion);
 
+    payload_.size = payload.size();
     LOG(INFO) << "Payload size: " << payload.size();
 
     install_plan_.hash_checks_mandatory = hash_checks_mandatory;
@@ -866,15 +870,27 @@
   EXPECT_TRUE(performer_.Write(kDeltaMagic, sizeof(kDeltaMagic)));
 
   uint64_t major_version = htobe64(kBrilloMajorPayloadVersion);
-  EXPECT_TRUE(performer_.Write(&major_version, 8));
+  EXPECT_TRUE(
+      performer_.Write(&major_version, PayloadMetadata::kDeltaVersionSize));
 
   uint64_t manifest_size = rand_r(&seed) % 256;
-  uint64_t manifest_size_be = htobe64(manifest_size);
-  EXPECT_TRUE(performer_.Write(&manifest_size_be, 8));
-
   uint32_t metadata_signature_size = rand_r(&seed) % 256;
+
+  // The payload size has to be bigger than the |metadata_size| and
+  // |metadata_signature_size|
+  payload_.size = PayloadMetadata::kDeltaManifestSizeOffset +
+                  PayloadMetadata::kDeltaManifestSizeSize +
+                  PayloadMetadata::kDeltaMetadataSignatureSizeSize +
+                  manifest_size + metadata_signature_size + 1;
+
+  uint64_t manifest_size_be = htobe64(manifest_size);
+  EXPECT_TRUE(performer_.Write(&manifest_size_be,
+                               PayloadMetadata::kDeltaManifestSizeSize));
+
   uint32_t metadata_signature_size_be = htobe32(metadata_signature_size);
-  EXPECT_TRUE(performer_.Write(&metadata_signature_size_be, 4));
+  EXPECT_TRUE(
+      performer_.Write(&metadata_signature_size_be,
+                       PayloadMetadata::kDeltaMetadataSignatureSizeSize));
 
   EXPECT_LT(performer_.Close(), 0);
 
@@ -884,11 +900,75 @@
   EXPECT_EQ(metadata_signature_size, performer_.metadata_signature_size_);
 }
 
+TEST_F(DeltaPerformerTest, BrilloMetadataSizeNOKTest) {
+  unsigned int seed = time(nullptr);
+  EXPECT_TRUE(performer_.Write(kDeltaMagic, sizeof(kDeltaMagic)));
+
+  uint64_t major_version = htobe64(kBrilloMajorPayloadVersion);
+  EXPECT_TRUE(
+      performer_.Write(&major_version, PayloadMetadata::kDeltaVersionSize));
+
+  uint64_t manifest_size = UINT64_MAX - 600;  // Subtract to avoid wrap around.
+  uint64_t manifest_offset = PayloadMetadata::kDeltaManifestSizeOffset +
+                             PayloadMetadata::kDeltaManifestSizeSize +
+                             PayloadMetadata::kDeltaMetadataSignatureSizeSize;
+  payload_.metadata_size = manifest_offset + manifest_size;
+  uint32_t metadata_signature_size = rand_r(&seed) % 256;
+
+  // The payload size is greater than the payload header but smaller than
+  // |metadata_signature_size| + |metadata_size|
+  payload_.size = manifest_offset + metadata_signature_size + 1;
+
+  uint64_t manifest_size_be = htobe64(manifest_size);
+  EXPECT_TRUE(performer_.Write(&manifest_size_be,
+                               PayloadMetadata::kDeltaManifestSizeSize));
+  uint32_t metadata_signature_size_be = htobe32(metadata_signature_size);
+
+  ErrorCode error;
+  EXPECT_FALSE(
+      performer_.Write(&metadata_signature_size_be,
+                       PayloadMetadata::kDeltaMetadataSignatureSizeSize + 1,
+                       &error));
+
+  EXPECT_EQ(ErrorCode::kDownloadInvalidMetadataSize, error);
+}
+
+TEST_F(DeltaPerformerTest, BrilloMetadataSignatureSizeNOKTest) {
+  unsigned int seed = time(nullptr);
+  EXPECT_TRUE(performer_.Write(kDeltaMagic, sizeof(kDeltaMagic)));
+
+  uint64_t major_version = htobe64(kBrilloMajorPayloadVersion);
+  EXPECT_TRUE(
+      performer_.Write(&major_version, PayloadMetadata::kDeltaVersionSize));
+
+  uint64_t manifest_size = rand_r(&seed) % 256;
+  // Subtract from UINT32_MAX to avoid wrap around.
+  uint32_t metadata_signature_size = UINT32_MAX - 600;
+
+  // The payload size is greater than |manifest_size| but smaller than
+  // |metadata_signature_size|
+  payload_.size = manifest_size + 1;
+
+  uint64_t manifest_size_be = htobe64(manifest_size);
+  EXPECT_TRUE(performer_.Write(&manifest_size_be,
+                               PayloadMetadata::kDeltaManifestSizeSize));
+
+  uint32_t metadata_signature_size_be = htobe32(metadata_signature_size);
+  ErrorCode error;
+  EXPECT_FALSE(
+      performer_.Write(&metadata_signature_size_be,
+                       PayloadMetadata::kDeltaMetadataSignatureSizeSize + 1,
+                       &error));
+
+  EXPECT_EQ(ErrorCode::kDownloadInvalidMetadataSize, error);
+}
+
 TEST_F(DeltaPerformerTest, BrilloParsePayloadMetadataTest) {
   brillo::Blob payload_data = GeneratePayload(
       {}, {}, true, kBrilloMajorPayloadVersion, kSourceMinorPayloadVersion);
   install_plan_.hash_checks_mandatory = true;
   performer_.set_public_key_path(GetBuildArtifactsPath(kUnittestPublicKeyPath));
+  payload_.size = payload_data.size();
   ErrorCode error;
   EXPECT_EQ(MetadataParseResult::kSuccess,
             performer_.ParsePayloadMetadata(payload_data, &error));
diff --git a/payload_consumer/payload_metadata.cc b/payload_consumer/payload_metadata.cc
index 4d8ee7b..b83001a 100644
--- a/payload_consumer/payload_metadata.cc
+++ b/payload_consumer/payload_metadata.cc
@@ -92,6 +92,7 @@
   metadata_size_ = manifest_offset + manifest_size_;
   if (metadata_size_ < manifest_size_) {
     // Overflow detected.
+    LOG(ERROR) << "Overflow detected on manifest size.";
     *error = ErrorCode::kDownloadInvalidMetadataSize;
     return MetadataParseResult::kError;
   }
@@ -108,6 +109,7 @@
 
   if (metadata_size_ + metadata_signature_size_ < metadata_size_) {
     // Overflow detected.
+    LOG(ERROR) << "Overflow detected on metadata and signature size.";
     *error = ErrorCode::kDownloadInvalidMetadataSize;
     return MetadataParseResult::kError;
   }