Verify metadata signature in major version 2.
Use metadata signature in payload version 2 if Omaha doesn't provide
metadata signature.
Bug: 23946683
TEST=unit test added.
Change-Id: I4f5e80019a8aeeaa4ff7daa82baa43a621c4ae98
diff --git a/delta_performer.cc b/delta_performer.cc
index e664749..d83bb53 100644
--- a/delta_performer.cc
+++ b/delta_performer.cc
@@ -440,10 +440,9 @@
kDeltaManifestSizeSize);
manifest_size_ = be64toh(manifest_size_); // switch big endian to host
- uint32_t metadata_signature_size = 0;
if (GetMajorVersion() == kBrilloMajorPayloadVersion) {
// Parse the metadata signature size.
- COMPILE_ASSERT(sizeof(metadata_signature_size) ==
+ COMPILE_ASSERT(sizeof(metadata_signature_size_) ==
kDeltaMetadataSignatureSizeSize,
metadata_signature_size_size_mismatch);
uint64_t metadata_signature_size_offset;
@@ -451,17 +450,17 @@
*error = ErrorCode::kError;
return kMetadataParseError;
}
- memcpy(&metadata_signature_size,
+ memcpy(&metadata_signature_size_,
&payload[metadata_signature_size_offset],
kDeltaMetadataSignatureSizeSize);
- metadata_signature_size = be32toh(metadata_signature_size);
+ metadata_signature_size_ = be32toh(metadata_signature_size_);
}
// If the metadata size is present in install plan, check for it immediately
// even before waiting for that many number of bytes to be downloaded in the
// payload. This will prevent any attack which relies on us downloading data
// beyond the expected metadata size.
- metadata_size_ = manifest_offset + manifest_size_ + metadata_signature_size;
+ metadata_size_ = manifest_offset + manifest_size_;
if (install_plan_->hash_checks_mandatory) {
if (install_plan_->metadata_size != metadata_size_) {
LOG(ERROR) << "Mandatory metadata size in Omaha response ("
@@ -474,8 +473,8 @@
}
// Now that we have validated the metadata size, we should wait for the full
- // metadata to be read in before we can parse it.
- if (payload.size() < metadata_size_)
+ // metadata and its signature (if exist) to be read in before we can parse it.
+ if (payload.size() < metadata_size_ + metadata_signature_size_)
return kMetadataParseInsufficientData;
// Log whether we validated the size or simply trusting what's in the payload
@@ -495,7 +494,7 @@
// We have the full metadata in |payload|. Verify its integrity
// and authenticity based on the information we have in Omaha response.
- *error = ValidateMetadataSignature(payload.data(), metadata_size_);
+ *error = ValidateMetadataSignature(payload);
if (*error != ErrorCode::kSuccess) {
if (install_plan_->hash_checks_mandatory) {
// The autoupdate_CatchBadSignatures test checks for this string
@@ -543,7 +542,7 @@
const bool do_read_header = !IsHeaderParsed();
CopyDataToBuffer(&c_bytes, &count,
(do_read_header ? kMaxPayloadHeaderSize :
- metadata_size_));
+ metadata_size_ + metadata_signature_size_));
MetadataParseResult result = ParsePayloadMetadata(buffer_, error);
if (result == kMetadataParseError)
@@ -1188,11 +1187,30 @@
}
ErrorCode DeltaPerformer::ValidateMetadataSignature(
- const void* metadata, uint64_t metadata_size) {
+ const brillo::Blob& payload) {
+ if (payload.size() < metadata_size_ + metadata_signature_size_)
+ return ErrorCode::kDownloadMetadataSignatureError;
- if (install_plan_->metadata_signature.empty()) {
+ brillo::Blob metadata_signature_blob, metadata_signature_protobuf_blob;
+ if (!install_plan_->metadata_signature.empty()) {
+ // Convert base64-encoded signature to raw bytes.
+ if (!brillo::data_encoding::Base64Decode(
+ install_plan_->metadata_signature, &metadata_signature_blob)) {
+ LOG(ERROR) << "Unable to decode base64 metadata signature: "
+ << install_plan_->metadata_signature;
+ return ErrorCode::kDownloadMetadataSignatureError;
+ }
+ } else if (major_payload_version_ == kBrilloMajorPayloadVersion) {
+ metadata_signature_protobuf_blob.assign(payload.begin() + metadata_size_,
+ payload.begin() + metadata_size_ +
+ metadata_signature_size_);
+ }
+
+ if (metadata_signature_blob.empty() &&
+ metadata_signature_protobuf_blob.empty()) {
if (install_plan_->hash_checks_mandatory) {
- LOG(ERROR) << "Missing mandatory metadata signature in Omaha response";
+ LOG(ERROR) << "Missing mandatory metadata signature in both Omaha "
+ << "response and payload.";
return ErrorCode::kDownloadMetadataSignatureMissingError;
}
@@ -1200,15 +1218,6 @@
return ErrorCode::kSuccess;
}
- // Convert base64-encoded signature to raw bytes.
- brillo::Blob metadata_signature;
- if (!brillo::data_encoding::Base64Decode(install_plan_->metadata_signature,
- &metadata_signature)) {
- LOG(ERROR) << "Unable to decode base64 metadata signature: "
- << install_plan_->metadata_signature;
- return ErrorCode::kDownloadMetadataSignatureError;
- }
-
// See if we should use the public RSA key in the Omaha response.
base::FilePath path_to_public_key(public_key_path_);
base::FilePath tmp_key;
@@ -1221,16 +1230,8 @@
LOG(INFO) << "Verifying metadata hash signature using public key: "
<< path_to_public_key.value();
- brillo::Blob expected_metadata_hash;
- if (!PayloadVerifier::GetRawHashFromSignature(metadata_signature,
- path_to_public_key.value(),
- &expected_metadata_hash)) {
- LOG(ERROR) << "Unable to compute expected hash from metadata signature";
- return ErrorCode::kDownloadMetadataSignatureError;
- }
-
OmahaHashCalculator metadata_hasher;
- metadata_hasher.Update(metadata, metadata_size);
+ metadata_hasher.Update(payload.data(), metadata_size_);
if (!metadata_hasher.Finalize()) {
LOG(ERROR) << "Unable to compute actual hash of manifest";
return ErrorCode::kDownloadMetadataSignatureVerificationError;
@@ -1243,12 +1244,28 @@
return ErrorCode::kDownloadMetadataSignatureVerificationError;
}
- if (calculated_metadata_hash != expected_metadata_hash) {
- LOG(ERROR) << "Manifest hash verification failed. Expected hash = ";
- utils::HexDumpVector(expected_metadata_hash);
- LOG(ERROR) << "Calculated hash = ";
- utils::HexDumpVector(calculated_metadata_hash);
- return ErrorCode::kDownloadMetadataSignatureMismatch;
+ if (!metadata_signature_blob.empty()) {
+ brillo::Blob expected_metadata_hash;
+ if (!PayloadVerifier::GetRawHashFromSignature(metadata_signature_blob,
+ path_to_public_key.value(),
+ &expected_metadata_hash)) {
+ LOG(ERROR) << "Unable to compute expected hash from metadata signature";
+ return ErrorCode::kDownloadMetadataSignatureError;
+ }
+ if (calculated_metadata_hash != expected_metadata_hash) {
+ LOG(ERROR) << "Manifest hash verification failed. Expected hash = ";
+ utils::HexDumpVector(expected_metadata_hash);
+ LOG(ERROR) << "Calculated hash = ";
+ utils::HexDumpVector(calculated_metadata_hash);
+ return ErrorCode::kDownloadMetadataSignatureMismatch;
+ }
+ } else {
+ if (!PayloadVerifier::VerifySignature(metadata_signature_protobuf_blob,
+ path_to_public_key.value(),
+ calculated_metadata_hash)) {
+ LOG(ERROR) << "Manifest hash verification failed.";
+ return ErrorCode::kDownloadMetadataSignatureMismatch;
+ }
}
// The autoupdate_CatchBadSignatures test checks for this string in
@@ -1397,7 +1414,8 @@
// Verifies the download size.
TEST_AND_RETURN_VAL(ErrorCode::kPayloadSizeMismatchError,
update_check_response_size ==
- metadata_size_ + buffer_offset_);
+ metadata_size_ + metadata_signature_size_ +
+ buffer_offset_);
// Verifies the payload hash.
const string& payload_hash_data = hash_calculator_.hash();
diff --git a/delta_performer.h b/delta_performer.h
index 4f9881c..536dd83 100644
--- a/delta_performer.h
+++ b/delta_performer.h
@@ -185,6 +185,8 @@
private:
friend class DeltaPerformerTest;
friend class DeltaPerformerIntegrationTest;
+ FRIEND_TEST(DeltaPerformerTest, BrilloMetadataSignatureSizeTest);
+ FRIEND_TEST(DeltaPerformerTest, BrilloVerifyMetadataSignatureTest);
FRIEND_TEST(DeltaPerformerTest, UsePublicKeyFromResponse);
// Parse and move the update instructions of all partitions into our local
@@ -228,16 +230,15 @@
// Returns ErrorCode::kSuccess on match or a suitable error code otherwise.
ErrorCode ValidateOperationHash(const InstallOperation& operation);
- // Interprets the given |protobuf| as a DeltaArchiveManifest protocol buffer
- // of the given protobuf_length and verifies that the signed hash of the
- // metadata matches what's specified in the install plan from Omaha.
- // Returns ErrorCode::kSuccess on match or a suitable error code otherwise.
- // This method must be called before any part of the |protobuf| is parsed
- // so that a man-in-the-middle attack on the SSL connection to the payload
- // server doesn't exploit any vulnerability in the code that parses the
- // protocol buffer.
- ErrorCode ValidateMetadataSignature(const void* protobuf,
- uint64_t protobuf_length);
+ // Given the |payload|, verifies that the signed hash of its metadata matches
+ // what's specified in the install plan from Omaha (if present) or the
+ // metadata signature in payload itself (if present). Returns
+ // ErrorCode::kSuccess on match or a suitable error code otherwise. This
+ // method must be called before any part of the metadata is parsed so that a
+ // man-in-the-middle attack on the SSL connection to the payload server
+ // doesn't exploit any vulnerability in the code that parses the protocol
+ // buffer.
+ ErrorCode ValidateMetadataSignature(const brillo::Blob& payload);
// Returns true on success.
bool PerformInstallOperation(const InstallOperation& operation);
@@ -304,6 +305,7 @@
bool manifest_valid_{false};
uint64_t metadata_size_{0};
uint64_t manifest_size_{0};
+ uint32_t metadata_signature_size_{0};
uint64_t major_payload_version_{0};
// Accumulated number of operations per partition. The i-th element is the
diff --git a/delta_performer_unittest.cc b/delta_performer_unittest.cc
index 5373e09..8c85768 100644
--- a/delta_performer_unittest.cc
+++ b/delta_performer_unittest.cc
@@ -105,7 +105,8 @@
brillo::Blob GeneratePayload(const brillo::Blob& blob_data,
const vector<AnnotatedOperation>& aops,
bool sign_payload,
- int32_t minor_version) {
+ uint64_t major_version,
+ uint32_t minor_version) {
string blob_path;
EXPECT_TRUE(utils::MakeTempFile("Blob-XXXXXX", &blob_path, nullptr));
ScopedPathUnlinker blob_unlinker(blob_path);
@@ -114,7 +115,7 @@
blob_data.size()));
PayloadGenerationConfig config;
- config.major_version = kChromeOSMajorPayloadVersion;
+ config.major_version = major_version;
config.minor_version = minor_version;
PayloadFile payload;
@@ -224,7 +225,7 @@
// Loads the payload and parses the manifest.
brillo::Blob payload = GeneratePayload(brillo::Blob(),
vector<AnnotatedOperation>(), sign_payload,
- kFullPayloadMinorVersion);
+ kChromeOSMajorPayloadVersion, kFullPayloadMinorVersion);
LOG(INFO) << "Payload size: " << payload.size();
@@ -310,7 +311,7 @@
aops.push_back(aop);
brillo::Blob payload_data = GeneratePayload(expected_data, aops, false,
- kFullPayloadMinorVersion);
+ kChromeOSMajorPayloadVersion, kFullPayloadMinorVersion);
EXPECT_EQ(expected_data, ApplyPayload(payload_data, "/dev/null"));
}
@@ -328,6 +329,7 @@
aops.push_back(aop);
brillo::Blob payload_data = GeneratePayload(expected_data, aops, false,
+ kChromeOSMajorPayloadVersion,
kSourceMinorPayloadVersion);
EXPECT_EQ(expected_data, ApplyPayload(payload_data, "/dev/null"));
@@ -349,6 +351,7 @@
aops.push_back(aop);
brillo::Blob payload_data = GeneratePayload(bz_data, aops, false,
+ kChromeOSMajorPayloadVersion,
kSourceMinorPayloadVersion);
EXPECT_EQ(expected_data, ApplyPayload(payload_data, "/dev/null"));
@@ -370,6 +373,7 @@
vector<AnnotatedOperation> aops = {aop};
brillo::Blob payload_data = GeneratePayload(xz_data, aops, false,
+ kChromeOSMajorPayloadVersion,
kSourceMinorPayloadVersion);
EXPECT_EQ(expected_data, ApplyPayload(payload_data, "/dev/null"));
@@ -392,6 +396,7 @@
vector<AnnotatedOperation> aops = {aop};
brillo::Blob payload_data = GeneratePayload(brillo::Blob(), aops, false,
+ kChromeOSMajorPayloadVersion,
kSourceMinorPayloadVersion);
EXPECT_EQ(expected_data,
@@ -410,6 +415,7 @@
aops.push_back(aop);
brillo::Blob payload_data = GeneratePayload(brillo::Blob(), aops, false,
+ kChromeOSMajorPayloadVersion,
kSourceMinorPayloadVersion);
string source_path;
EXPECT_TRUE(utils::MakeTempFile("Source-XXXXXX",
@@ -546,8 +552,26 @@
uint64_t manifest_offset;
EXPECT_TRUE(performer_.GetManifestOffset(&manifest_offset));
EXPECT_EQ(24, manifest_offset); // 4 + 8 + 8 + 4
- EXPECT_EQ(24 + manifest_size + metadata_signature_size,
- performer_.GetMetadataSize());
+ EXPECT_EQ(manifest_offset + manifest_size, performer_.GetMetadataSize());
+ EXPECT_EQ(metadata_signature_size, performer_.metadata_signature_size_);
+}
+
+TEST_F(DeltaPerformerTest, BrilloVerifyMetadataSignatureTest) {
+ SetSupportedMajorVersion(kBrilloMajorPayloadVersion);
+ brillo::Blob payload_data = GeneratePayload({}, {}, true,
+ kBrilloMajorPayloadVersion,
+ kSourceMinorPayloadVersion);
+ install_plan_.hash_checks_mandatory = true;
+ // Just set these value so that we can use ValidateMetadataSignature directly.
+ performer_.major_payload_version_ = kBrilloMajorPayloadVersion;
+ performer_.metadata_size_ = install_plan_.metadata_size;
+ uint64_t signature_length;
+ EXPECT_TRUE(PayloadSigner::SignatureBlobLength({kUnittestPrivateKeyPath},
+ &signature_length));
+ performer_.metadata_signature_size_ = signature_length;
+ performer_.set_public_key_path(kUnittestPublicKeyPath);
+ EXPECT_EQ(ErrorCode::kSuccess,
+ performer_.ValidateMetadataSignature(payload_data));
}
TEST_F(DeltaPerformerTest, BadDeltaMagicTest) {