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

3 files changed, 97 insertions, 53 deletions

diff --git a/delta_performer.cc b/delta_performer.cc
index e6647497..d83bb536 100644
--- a/delta_performer.cc
+++ b/delta_performer.cc
@@ -440,10 +440,9 @@ DeltaPerformer::MetadataParseResult DeltaPerformer::ParsePayloadMetadata(
            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 @@ DeltaPerformer::MetadataParseResult DeltaPerformer::ParsePayloadMetadata(
         *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 @@ DeltaPerformer::MetadataParseResult DeltaPerformer::ParsePayloadMetadata(
   }
 
   // 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 @@ DeltaPerformer::MetadataParseResult DeltaPerformer::ParsePayloadMetadata(
 
   // 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 @@ bool DeltaPerformer::Write(const void* bytes, size_t count, ErrorCode *error) {
     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 @@ bool DeltaPerformer::GetPublicKeyFromResponse(base::FilePath *out_tmp_key) {
 }
 
 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;
+
+  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 (install_plan_->metadata_signature.empty()) {
+  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 @@ ErrorCode DeltaPerformer::ValidateMetadataSignature(
     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 @@ ErrorCode DeltaPerformer::ValidateMetadataSignature(
   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 @@ ErrorCode DeltaPerformer::ValidateMetadataSignature(
     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 @@ ErrorCode DeltaPerformer::VerifyPayload(
   // 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 4f9881c1..536dd839 100644
--- a/delta_performer.h
+++ b/delta_performer.h
@@ -185,6 +185,8 @@ class DeltaPerformer : public FileWriter {
  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 @@ class DeltaPerformer : public FileWriter {
   // 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 @@ class DeltaPerformer : public FileWriter {
   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 5373e097..8c857680 100644
--- a/delta_performer_unittest.cc
+++ b/delta_performer_unittest.cc
@@ -105,7 +105,8 @@ class DeltaPerformerTest : public ::testing::Test {
   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 @@ class DeltaPerformerTest : public ::testing::Test {
                                  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 @@ class DeltaPerformerTest : public ::testing::Test {
     // 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 @@ TEST_F(DeltaPerformerTest, FullPayloadWriteTest) {
   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 @@ TEST_F(DeltaPerformerTest, ReplaceOperationTest) {
   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 @@ TEST_F(DeltaPerformerTest, ReplaceBzOperationTest) {
   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 @@ TEST_F(DeltaPerformerTest, ReplaceXzOperationTest) {
   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 @@ TEST_F(DeltaPerformerTest, ZeroOperationTest) {
   vector<AnnotatedOperation> aops = {aop};
 
   brillo::Blob payload_data = GeneratePayload(brillo::Blob(), aops, false,
+                                              kChromeOSMajorPayloadVersion,
                                               kSourceMinorPayloadVersion);
 
   EXPECT_EQ(expected_data,
@@ -410,6 +415,7 @@ TEST_F(DeltaPerformerTest, SourceCopyOperationTest) {
   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 @@ TEST_F(DeltaPerformerTest, BrilloMetadataSignatureSizeTest) {
   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) {