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;
}